This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA] Add Windows x64 SEH unwinder
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