This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFA] Add Windows x64 SEH unwinder


> This patch adds an unwinder that reads these data.  The main advantage
> is that gdb is now able to unwind through code compiled with other
> compilers (and through system libraries).

I see you beat me to this task, and you didn't even tell me you started
:-). Thanks for doing it!

> I haven't run the gdb testsuite on Windows x64 (I don't know if this
> is doable), but I have manually tested it on a few executables,
> including gdb itself.

It would be good if you could test those changes against AdaCore's
testsuite, which we know runs well on x64. It's not as complete as
the official testsuite, but will already test quite a bit, and will
definitely be better than nothing.

Because you prepend the unwinder, I think that this unwinder will
take precedence over the DWARF unwinder (right?), and so it's
important we get a minimum amount of confidence before we apply it
(hence the testing suggestion above).  We are also trying to get ready
to branch 7.5, and I don't think I would feel all that comfortable
applying this now.

> 2012-06-15  Tristan Gingold  <gingold@adacore.com>
> 
> 	* amd64-windows-tdep.c (struct x64_frame_cache): Declare.
> 	(x64_w2gdb_regnum): New array.
> 	(x64_frame_decode_epilogue,	x64_frame_decode_insns)
> 	(x64_frame_cache, x64_frame_prev_register, x64_frame_this_id): New
> 	functions.
> 	(x64_frame_unwind): New variable.
> 	(amd64_windows_init_abi): Register this unwinder.

Some comments below. Apologies in advance for being anal on certain
things...

> +struct x64_frame_cache
> +{
> +  CORE_ADDR image_base;		/* ImageBase for the module.  */
> +  CORE_ADDR start_address;	/* Function start rva.  */

The more usual style for the comments is to place them on their own
line, just before the field itself. I know the style you chose is
acceptable because you were able to make them fit within the line.
But I'd rather have them follow my suggested style, because we can
then extend some of them to multi-line comments if necessary.

> +/* Try to recognize and decode an epilogue sequence.  */
> +
> +static int
> +x64_frame_decode_epilogue (struct frame_info *this_frame,
> +			   struct x64_frame_cache *cache)

I am amazed that one would still need to do manual epilogue
detection and associated by-hand instruction scanning when
using any unwind info. But I assume the info is not complete
enough and there is no other way?

> +{
> +  /* Not in a prologue, so maybe in an epilogue.  For the rules, cf
> +     http://msdn.microsoft.com/en-us/library/tawsa7cb.aspx
> +     Furthermore, according to RtlVirtualUnwind, the complete list of
> +     epilog marker is:
> +     - ret			[c3]
> +     - ret n			[c2 imm16]
> +     - rep ret			[f3 c3]
> +     - jmp imm8 | imm32	        [eb rel8] or [e9 rel32]
> +     - jmp qword ptr imm32                 - not handled
> +     - rex jmp reg		[4X ff eY]
> +     I would add:
> +     -  pop reg                 [41 58-5f] or [58-5f]

(can we align the text in square brackets?)

> +     We don't care about the instruction that deallocate the frame:
                                                 deallocates
> +     if it hasn't been executed, we can safely decode the insns,
> +     if it has been executed, the following epilog decoding will
> +     work.
> +  */

Can you join the comment-closing marker with the last line of the
comment?

> +	  pc = pc + 2 + (signed char)rel8;

space after closing parens, even for casts...

> +      /* Allow the user to break this loop.  */
> +      if (quit_flag)
> +	return 0;

Why not use the QUIT macro? I assume that you want to avoid
the call to the "quit" function, and return 0 instead.  But
that seems very odd to me, and maybe even wrong, because you'll
be continuing the unwind even though the user pressed ctrl-c,
with unpredictable results.

> +  for (j = 0; ; j++)
> +    {
> +      struct external_pex64_unwind_info ex_ui;
> +      gdb_byte insns[2 * 256];

I am a little concerned about this magic number. Can you explain
how you came to it? The multiplication seems to suggest that there
is a logic behind it.

> +	  /* According to msdn:
> +	     If an FP reg is used, then any unwind code taking an offset must
> +	     only be used the the FP reg is established in the prolog.  */
                          ^^^^^^^
                          (guess: after?)

> +      /* Allow the user to break this loop.  */
> +      if (quit_flag)
> +	return;

Same comment as above.

> +static struct x64_frame_cache *
> +x64_frame_cache (struct frame_info *this_frame, void **this_cache)

Function documentation missing.  This 

> +  /* Find the entry.
> +     Note: it might be a better idea to ask directly to the kernel.  This
> +     will handle dynamically added entries (for JIT engines).  */

I am fine with the current approach, but this makes me wondering
how we would do that, and why we are not doing it? Asking the kernel
from a tdep file means that we'd have to define another xfer object
kind, so that's the first obstacle...

> +      if (unwind_info & 1)

?

> +	  if (target_read_memory (image_base + (unwind_info & ~1),
> +				  (char *) &d, sizeof (d)) != 0)
                                  ^^^^^^^^ (gdb_byte *)

I am realizing I might have missed other cases of casting to
"char *".

> +static struct value *
> +x64_frame_prev_register (struct frame_info *this_frame,
> +			 void **this_cache, int regnum)

Missing function comment.

> +static void
> +x64_frame_this_id (struct frame_info *this_frame, void **this_cache,
> +		   struct frame_id *this_id)

Same...

All in all, I'd say, pretty nice!
-- 
Joel


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]