[RFA] Add Windows x64 SEH unwinder

[Tristan Gingold]

On Jun 15, 2012, at 8:06 PM, Joel Brobecker wrote:

>> 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.

Ok, will do it.

> 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.

[...]

>> +/* 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?

No, this is simply part of the specs; but there are only a few patterns to consider.

[...]

>> +      /* 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.

Ok.

[...]

>> +  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.

I added a comment about that.

[...]

>> +  /* 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...

Yes, yet another xfer object will do it...

>> +      if (unwind_info & 1)
> 
> ?

I have adjust the comment about this (may not be in the second version just posted).

>> +	  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 *".

I fixed all of them (at least in my current tree).


Thank you for your extensive review,

Tristan