Re: [PATCH v4] Add Guile frame unwinder interface

[Andy Wingo <wingo at igalia dot com>]

Hi!

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"?

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

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

>  > +(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?

>  > +(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?

Thanks for your time, Doug!

Andy