This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v4] Add Guile frame unwinder interface
- From: Doug Evans <xdje42 at gmail dot com>
- To: Andy Wingo <wingo at igalia dot com>
- Cc: gdb-patches at sourceware dot org, Pedro Alves <palves at redhat dot com>
- Date: Sun, 26 Apr 2015 22:01:02 -0700
- Subject: Re: [PATCH v4] Add Guile frame unwinder interface
- Authentication-results: sourceware.org; auth=none
- References: <87bnjtc4o4 dot fsf at igalia dot com> <21777 dot 57403 dot 725615 dot 308934 at ruffy2 dot mtv dot corp dot google dot com> <871tjtf4td dot fsf at igalia dot com>
Andy Wingo <wingo@igalia.com> writes:
> Hi!
Hi.
When we last left our story ...
> I have a new version of this patch that I'll post as a separate thread,
> addressing all the issues except a couple that I comment below.
>
> On Tue 24 Mar 2015 23:07, Doug Evans <dje@google.com> writes:
>
>> > +@deffn {Scheme Procedure} unwind-info-add-saved-register! unwind-info register value
>> > +Set the saved value of a register in a ephemeral frame.
>> > +
>> > +@var{register} names a register, and should be a string, for example
>> > +@samp{rip}. @var{value} is the register value, as a @value{GDBN}
>>
>> To be more universal let's replace "rip" here with "pc".
>
> I kept in "rip" because user regs like "pc" don't work on pending
> frames, and I didn't want to mislead the reader that user regs would
> work. (OK, "pc" is probably a user reg only on some architectures.)
> WDYT? Still change to "pc"?
"rip" is amd64-specific, and we don't really distinguish "rip" from "pc".
They're different, technically (one's a "real" register, ref: amd64-tdep.c,
and the other is a "user" reg - ref: user-regs.c), but if they're not
equivalent I think we've got problems, and not just in the unwinders.
I guess I need to understand better why "rip" works whereas "pc" doesn't work.
>> > +Any register whose value is not recorded as saved via
>> > +@code{unwind-info-add-saved-register!} will be marked as ``not saved''
>> > +in the outer frame.
>>
>> "outer frame" here is confusing.
>> I'd have thought this would read "``not saved'' in the frame".
>> [with the frame?]
>
> Let's say you are unwinding pending frame 1, which will be frame-1. You
> add some saved registers to it. Those registers will become the values
> of (frame-read-register (frame-older frame-1) "foo"). If a register
> isn't added to the set, then the frame-read-register on the older frame
> will return "not saved". Is that clear? It's a bit confusing but I
> think it reflects the problem space...
It's the "outer" that's confusing, not the function's purpose.
I often come into a manual at the point that describes
the function I'm interested in. If I read the introductory text I can
infer that "outer" here equates to the pending frame we are building,
but when I imagine reading this text "standalone" (so to speak) I wonder
if the reader might think "outer to the pending frame being constructed?".
I dunno.
I think this would be clearer:
"... will be marked as ``not saved'' in the pending frame being unwound."
Or one could say both:
"... will be marked as ``not saved'' in the outer frame (the pending
frame being unwound)."
Or some such.
>> > +(define* (make-frame-unwinder name procedure #:key (priority 20) (enabled? #t))
>>
>> In addition to removing priority (for now), let's remove enabled? as
>> a parameter here. The user can disable the unwinder before registering it.
>
> I kept it in, as noted in the patch set header of the next mail, because
> that's more like frame filters which indeed have a priority and also
> have an enabled flag. Please let me know again if this bugs you and
> I'll take them both out.
But there isn't a real reason for having it there either.
But whatever, this isn't important enough.
Re: priority: Ok.
>> > +(define* (register-frame-unwinder! unwinder #:key scope)
>>
>> Guile question: Does scope default to #f?
>
> Yep.
>
>> > +gdb_test_no_output "guile (enable-frame-unwinder! \"Synthetic\")" \
>> > + "enable synthetic unwinder"
>> > +
>> > +gdb_breakpoint "f2"
>> > +gdb_continue_to_breakpoint "breakpoint at f2"
>> > +
>> > +gdb_test "bt 10" " f2 .*cabba9e5 .*cabba9e5 .*"
>>
>> IWBN to test that unwinding handles unwinder enabling.
>> E.g., try a backtrace with and without an unwinder enabled.
>>
>> This brings up an issue I haven't seen discussed before.
>> Apologies if I missed it.
>> Should enabling/disabling/registering/removing an unwinder
>> invalidate the frame cache? Seems like it.
>
> I can't do it here for the reason you mention. I have added something
> that does a bt from before the enable!, and also doesn't have a trailing
> .*. A followup perhaps?
I guess I'd have to see what happens if I do a backtrace with an
unwinder enabled, then disable it, and then do another backtrace
and see if the result of the second backtrace is identical to a
backtrace where the unwinder was originally disabled.
If your patch has a test for this, great.
And if the test works without invalidating the frame cache
I'd be curious to know why.
[Easy enough to check on my own of course, and will if I get time.]
>> > +(define (synthetic-unwinder frame)
>> > + ;; Increment the stack pointer, set IP to 0xdeadbeef
>> > + (let* ((this-pc (ephemeral-frame-read-register frame "rip"))
>> > + (this-sp (ephemeral-frame-read-register frame "rsp"))
>>
>> rip,rsp are architecture-specific names but the .exp file
>> doesn't check for amd64. IWBN if the test wasn't architecture-specific.
>> Will "pc" and "sp" work here?
>
> They won't currently. Should I look into making user regs work or
> somehow mark this test as architecture-specific?
While fetching "pc" does take a different code path than "rip", it
does end up getting mapped to "rip". Plus if they're not equivalent
using this feature across different architectures
is going to be a pain. Users generally just always type "pc" without
having to care what the spelling is of the real h/w register.
Why doesn't pc,sp work here?
I can imagine it not working on arches that don't set the pc,sp regnums,
but that's just a guess.