Re: [PATCH v4] Add Guile frame unwinder interface

[Doug Evans <xdje42 at gmail 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.