Re: [PATCH v3] Add Guile frame unwinder interface

[Andy Wingo <wingo@igalia.com>]

Hi :)

[-asmundak, as he probably doesn't care about Guile API things]

A couple quick replies.  ACK to all the other things.

On Tue 10 Mar 2015 18:48, Doug Evans <dje@google.com> writes:

>  > +@deffn {Scheme Procedure} make-frame-unwinder name procedure @
>  > +       @r{[}#:priority priority@r{]} @r{[}#:enabled? boolean@r{]} @
>  > +       @r{[}#:objfile objfile@r{]} @r{[}#:progspace progspace@r{]}
>
> Instead of both #:objfile and #:progspace I'd rather just provide
> say, #:locus or #:scope or some such.

Let's do #:scope then.  Note that I copied this infrastructure from the
frame filter patch, which should also be ready for review :)  I'll apply
corresponding changes there and update both patches.

>  > +            add-frame-unwinder!
>  > +            remove-frame-unwinder!
>
> As a user who can't remember names very well, consistent naming
> is welcome. There is a (relatively) consistent naming thus far:
> register-breakpoint!, delete-breakpoint!, register-parameter!,
> register-command!.
>
> -> register-frame-unwinder! and delete-frame-unwinder!
>
> But I can quibble over the names too :-).
> "delete-breakpoint!" doesn't really delete it so much as remove it
> from gdb's tables. So I could go with remove-frame-unwinder!,
> add remove-breakpoint! and deprecate delete-breakpoint!.
>
> Plus it's nice to have names that go together: If we're going to have
> register-foo, the intuitive opposite is unregister-foo.
> But I'm ok with register-foo and remove-foo.

How about register-frame-unwinder! and unregister-frame-unwinder!.  It
matches the frame-unwinder-registered? predicate.  "Add" and "remove"
already raised the question of "to what?", and in that regard "register"
might be a bit clearer.

> Now's a good a time as any to lay down how things get deprecated in
> the Guile API.  We could have a guile/lib/gdb/deprecated.scm file that
> defined register-foo! (and others) or some such. As for when to delete
> them, I'm ok with anything <= 5 years.

Sure.  Or, we leave the interfaces where they are and just issue
deprecation warnings when they are used.

>
>  > +            enable-frame-unwinder!
>  > +            disable-frame-unwinder!
>
> Similarly, there is set-breakpoint-enabled!.
> -> set-frame-unwinder-enabled!

Uf, that's terrible :)  In Guile the convention is mostly
set-TYPE-FIELD!, and here TYPE is "frame-unwinder" and FIELD is
"enabled?", so the usual production is "set-frame-unwinder-enabled?!".
Really.  It's ugly but it does avoid the ambiguous reading of
"set-frame-unwinder-enabled!" as "set the frame unwinder to be enabled".

Actually it's so ugly that historically set-foo-enabled?! sometimes gets
replaced by more verby forms (cf in Guile's NEWS, `set-batch-mode?!'
replaced by `ensure-batch-mode!').

There is also the issue that without a good story yet on how to control
enabled/disabled status from the console, a common thing a user might
want to do would be to say "%&^%*&%@@@!  It's not working, so let's try
again with all the unwinders disabled."  It's much easier to do:

  (gdb) guile (for-each disable-frame-unwinder! (all-frame-unwinders))

than to

  (gdb) guile (for-each (lambda (uw) (set-frame-unwinder-enabled! uw #f)) (all-frame-unwinders))

(Should enabling / disabling unwinders invalidate the frame cache, I
wonder?)

What's the right way out of here? :)  I can change, it doesn't matter
much, but I wanted to argue a bit for the status quo.

What about adding a set-frame-unwinder-enabled! setter for consistency
and also keeping enable-frame-unwinder! / disable-frame-unwinder!.  I
don't know, none of the options are perfect :)

>  > +/* (set-ephemeral-frame-id! ephemeral-frame stack-address
>  > +                            [code-address [special-address]])
>  > +
>  > +   Set the frame ID on this ephemeral frame.  */
>  > +
>  > +static SCM
>  > +uwscm_set_ephemeral_frame_id_x (SCM ephemeral_frame, SCM sp, SCM ip,
>  > +				SCM special)
>
> I'm not sold on exporting the term "special" into the API.
> It conveys no information to the reader. "How special?" "Special in what way?"
> Internally we can call it whatever we like, but we should have a
> better name in the published API.
> I realize we don't want to pick an architecture-specific name,
> but as data for the discussion, is this only used by ia64?

It's "special" all through the internal API, of course.  However it does
appear to only be called by ia64-hpux-tdep.c and ia64-tdep.c.

We could simply not support "special" in the Guile and/or Python APIs,
for now at least.  I hope to live a long life and, when I die, to look
back in satisfaction that I never worked on an IA64 system :-))

Thanks for the review, will update this and the frame filter patch.

Andy