Re: [PATCH] Add Guile frame filter interface

[Doug Evans <xdje42 at gmail dot com>]

On Wed, Mar 4, 2015 at 5:07 AM, Andy Wingo <wingo@igalia.com> wrote:
> Hi!
>
> Thanks for the review, and sorry for the delay in responding.  With
> extension languages so much is possible that it's hard to know what's
> the right thing to do!  Comments inline, will send the new patch.

Sorry as well.
The time I have available to work on Guile and random GDB stuff
can be sporadic.

> On Sun 22 Feb 2015 22:53, Doug Evans <xdje42@gmail.com> writes:
>
>> We've been putting off some of the details for python/scheme interaction.
>
> Yes.  I propose we address the command issue in the other thread:
>
>   http://thread.gmane.org/gmane.comp.gdb.patches/105225
>
> So as you mention it's probably fine in this patch to just provide the
> API and punt the command question down the road.
>
>>> +@deffn {Scheme Procedure} add-frame-filter! name filter @
>>> +       @r{[}#:priority priority@r{]} @
>>> +       @r{[}#:objfile objfile@r{]} @r{[}#:progspace progspace@r{]}
>>> +Register the frame filter procedure @var{filter} with @value{GDBN}.
>>> +@var{filter} should be a function of one argument, taking a SRFI-41
>>> +stream of annotated frames and returning a possibily modified stream
>>> +of annotated frames.  The filter is identified by @var{name}, which
>>> +should be unique among all known filters.
>>
>> I sometimes worry about the word "should". :-)
>> It implies other possibilities are not necessarily wrong.
>> How about "must be unique"?
>
> Sure.  I assume you are you OK with things of the form "@var{filter}
> should be a function of one argument"?  Seems clear to be but YMMV
> obviously; please do point out weirdnesses in the updated patch.

Well, TBH I'm not opposed to it, but s/should/shall/
would be an improvement IMO (and obviously YMMV as well).

>> Also,
>> GDB can debug multiple programs, and one can imagine different
>> binaries using slightly different versions of the same library,
>> and thus filters for each version of the library could be loaded.
>> I wouldn't require them to have different names, the names just
>> have to be different within their respective sets (global, progspace
>> or objfile).  So maybe something like "... must be unique within its
>> registered scope"?
>> That will complicate the implementation a bit, but I don't want
>> the implementation to drive this aspect of the API.
>
> Interesting.  So what if you add a global filter "foo", do you have to
> check that the filter name is unique in all progspaces and in all
> objfiles?  It's easy and sensible to check "up" the scope chain but not
> so nice to check "down".  We could do it though.  Or, we could instead
> allow references to filters to be disambiguated by mentioning the
> specific objfile or progspace... dunno.

On the python side, such things are indeed qualified by their scope
so there is no conflict. Consistency here would be nice.
The python xmethod support uses the term locus,
IWBN to have a consistent term here too.
"locus/loci" has the nice attribute that it currently isn't used
for anything else. I don't have as strong an opinion on which
term is used as I do on using the term consistently.

>>> +@deffn {Scheme Procedure} all-frame-filters
>>> +Return a list of the names of all frame filters.
>>> +@end deffn
>>> +
>>> +@deffn {Scheme Procedure} remove-frame-filter! name
>>> +@deffnx {Scheme Procedure} enable-frame-filter! name
>>> +@deffnx {Scheme Procedure} disable-frame-filter! name
>>> +Remove, enable, or disable a frame filter, respectively.  @var{name}
>>> +should correspond to the name of a filter previously added with
>>> +@code{add-frame-filter!}.  If no such filter is found, an error is
>>> +signalled.
>>> +@end deffn
>>
>> We'll want a way to pretty-print all filters with their attributes
>> (scope, name, enabled, priority).
>> Thus can all-frame-filters return a list of objects instead of names?
>
> Both you and Ludovic have mentioned this.  I wanted to avoid it because
> I don't really like the state in which a filter can exist but not be
> registered, possibly even contradicting the already-existing "enabled"
> flag... for example with breakpoints you can't set the stop function on
> a breakpoint before you register it.  That seems silly to me, is
> probably a bug, but bugs like this fall out of this object-centered
> approach.  But OK, I will see what I can do :)

I guess this is a case where I'm not as bothered by such "bugs".

>> Things are more manageable if progspace and objfile frame-filters
>> are kept with that object, instead of just one global list that
>> has everything. E.g., all-frame-filters should probably return *all*
>> frame filters, e.g., including all progspaces and all objfiles of all
>> progspaces, but users are generally only interested in the current
>> progspace, and all objfiles of the current progspace.
>
> Hmm.  Should we assert then that the objfile/progspace is valid when a
> filter is installed on it?  I would guess so.

I think so.

> In general I would think that the final form of this implementation
> depends quite a bit on the answers to the python/guile command
> integration question.  Given that, for now I would like to go for a
> pure-scheme solution, so that any possible future
> integration/abstraction would be easier to make just by touching the
> Python side, then I could update Guile to follow suit.  WDYT?  I will
> see about pruning invalid filters for invalidated objfiles/progspaces
> from the filter set.

I don't see the cost difference being that great.
I could go with a pure Guile implementation AS LONG AS
it could be later rewritten to be consistent with
everything else. If there isn't a compelling reason
to be inconsistent, I'd rather not be.
And if there are barriers to being consistent,
what can we do to remove them?
[IOW, can we add something that makes a more pure
Guile implementation possible, and yet still be
consistent in form?]

>>> +When a command is executed from @value{GDBN} that is compatible with
>>> +frame filters, @value{GDBN} selects all filters registered in the
>>> +current progspace, filters for all objfiles, and filters with no
>>
>> all objfiles of the current progspace
>
> ACK.
>
>> reannotate-frame! has a trailing ! but the text implies no side effects.
>> E.g., "return a new ..." and "that inherits ..."
>> I don't currently have a strong opinion on whether to modify the
>> ann argument or construct a new frame.
>
> Whoops!  Accidental.  Will remove the !.  Constructing a new object
> harder to get wrong, so if you still don't have a strong opinion there
> I'll keep the functional style.
>
>>> +Annotated frames may also have @dfn{child frames}.  By default, no
>>> +frame has a child frame, but filters may reorganize the frame stream
>>> +into a stream of frame trees, by populating the child list.  Of
>>> +course, such a reorganization is ultimately cosmetic, as it doesn't
>>> +alter the stack of frames seen by @value{GDBN} and navigable by the
>>> +user, for example by using the @code{frame} command.  Still, nesting
>>> +frames may lead to a more understandable presentation of a backtrace.
>>
>> The Python API uses "elided" for child frames (right?).
>
> Yes.  To me this is a silly name -- the frames are not actually elided,
> they are presentationally (not semantically) nested, and the "elided"
> set can also introduce new frames.  The MI interface calls this set
> "children", and rightly so.
>
>> Plus the Python API uses "Frame Decorator" whereas we're using
>> "Annotated Frame". I don't mind the differences if they're warranted,
>> but it's taking me a bit of time to map back and forth, and if it's
>> taking me a bit of time I'm sure it'll take some users more time.
>> Thus if we keep this naming we need to provide additional text
>> to map the names back to their Python cousins.
>
> ACK.  I guess the word "decorate" has more meaning to Python users,
> given Python's decorators, but "redecorate-frame",
> "decorated-frame-address", etc doesn't sound bad to me either.  Probably
> best to stick with the Python names then.
>
>>> +@subsubsection Writing a Frame Filter in Guile
>>> +@cindex writing a frame filter
>>> +
>>> +The simplest kind of frame filter just takes the incoming stream of
>>> +frames and produces an identical stream of values.  For example:
>>> +
>>> +@example
>>> +(use-modules (gdb frames))
>>
>> (use-modules (srfi srfi-41))
>
> About this!  I realized at some point that SRFI-41 was only added to
> Guile in version 2.0.9.  It's the best streams interface we have, but
> that's too new for GDB.

TBH, I don't mind limiting new features to newer version of Guile.

> Sometimes you can get around these things by dynamically including
> features depending on what's available at runtime, but like a
> header-only C++ library, you can't use these dlopen-style tricks on
> streams because most of the (srfi srfi-41) exports are macros, and so
> embed their code when they are used.
>
> Options:
>
>   1. GDB ships a srfi-41 module if the Guile doesn't have it
>
>   2. GDB uses the old (ice-9 streams) module always
>
>   3. GDB uses the old (ice-9 streams) module conditionally
>
>   4. We don't use streams, we use GDB iterators or something
>
>   5. We expose only the "decorator" interface and not the "filter"
>      interface if GDB was compiled without srfi-41 support, and
>      internally use (ice-9 streams).
>
>   6. We could require a new version of Guile.
>
> All of the options are bad, but some are worse :)  I don't like (1), as
> that's not really GDB's job.  Conditional stream implementations (3) is
> terrible, because how can you find out which one your GDB has?  (2)
> would be OK but not great.  GDB iterators (4) are something of a nasty
> interface IMO but that's just me :)  But documenting filters and not
> having them as in (5) is bad too.  And finally although 2.0.9 was
> released in April 2013, that still prevents many common users from
> having Guile support.
>
> It seems to me that (2) is the least bad option, and Ludovic agrees.
> (ice-9 streams) streams aren't terrible; they are like the ones in SICP.
> They have a problem called "oddness", which you can read about here if
> you are interested:

Fine by me.

>   "How to Add Laziness to a Strict Language Without Even Being Odd"
>   Philip Wadler, Walid Taha, David Macqueen
>   http://citeseerx.ist.psu.edu/viewdoc/summary?doi=10.1.1.35.9427
>
> Anyway.  In Guile 2.2 we'll see about reimplementing our (ice-9 streams)
> in terms of the newer SRFI-41, so they will be interoperable, but in the
> GDB manual we'll have to mention some of these details, sadly.
>
>>> +@example
>>> +(add-frame-filter! "identity" identity-frame-filter)
>>> +@end example
>>> +
>>> +Now our filter will run each time a backtrace is printed, or in
>>> +general for any GDB command that uses the frame filter interface.
>>
>> Assuming python doesn't "win".
>> This includes errors, if a python filter gets an error guile filters
>> aren't tried. For compatibility's sake, python always wins.
>> We'll need to document this here (or near here), somehow.
>
> ACK.  I don't see any nice way for Python and Guile to interact, so if
> Python decides to filter frames, it will win.
>
>>> +(define (alias-annotator ann)
>>> +  (let* ((name (annotated-frame-function-name ann))
>>> +         (alias (assoc-ref *function-name-aliases* name)))
>>> +    (if alias
>>> +        (reannotate-frame ann #:function-name
>>> +                              (string-append "[" alias "] " name))
>>> +        ann)))
>>> +
>>> +(add-frame-annotator! "alias-annotator" alias-annotator)
>>> +@end smallexample
>>> +
>>> +A backtrace with this annotator in place produces:
>>> +
>>> +@smallexample
>>> +#19 [...] in vm_debug_engine ([...]) at vm-engine.c:806
>>> +#20 [...] in scm_call_n ([...]) at vm.c:1258
>>> +#21 [...] in [primitive-eval] scm_primitive_eval ([...]) at eval.c:656
>>> +#22 [...] in scm_eval ([...]) at eval.c:690
>>> +#23 [...] in scm_shell ([...]) at script.c:454
>>> +@end smallexample
>>
>> Can annotators be implemented on top of filters?
>> If so, IWBN to have just one low level entity.
>
> Yes, good idea.  You could define this function:
>
>   (define (annotator->filter annotate)
>     (lambda (stream)
>       (stream-map annotate stream)))
>
> I think it could still make sense to have annotators as a concept,
> because they are simpler to reason about.

Sure.

>> How would this example look if it were implemented with filters
>> instead of annotators?
>
> With (ice-9 streams) this would look like:
>
>  (define (alias-filter stream)
>    (stream-map
>     (lambda (ann)
>       (let* ((name (annotated-frame-function-name ann))
>              (alias (assoc-ref *function-name-aliases* name)))
>         (if alias
>             (reannotate-frame ann #:function-name
>                               (string-append "[" alias "] " name))
>             ann)))
>     stream))
>
> which is not so bad I guess.
>
>>> +(define (remove-from-priority-list priority-list name)
>>> +  (remove (lambda (x) (equal? (&name x) name)) priority-list))
>>
>> When an objfile goes away we'll want all its frame-filters to go away.
>> [E.g., imagine a frame-filter registered with an objfile that is
>> dlopen'd and later dlclose'd.]
>> Either that, or the code has to do its own garbage collection of
>> invalid objfiles. One nice thing about recording frame-filters
>> with the objfile is that such garbage collection comes for free.
>
> ACK.
>
>>> +    (define* (add! name entry #:key objfile progspace (priority 20))
>>
>> I couldn't find what the default priority is for python filters.
>> 20 is fine by me, but it would be good to be consistent.
>
> Seems to be zero, from get_priority() in frames.py:
>
>     # Do not fail here, as the sort will fail.  If a filter has not
>     # (incorrectly) set a priority, set it to zero.
>     return getattr(filter_item, "priority", 0)
>
> FWIW all the examples in the manual are 100.
>
>>> +/* Called by lib/gdb/frames.scm.  */
>>
>> Blank line between function command and function definition.
>> Here and throughout.
>
> Between comment and definition, I assume.  ACK.
>
>>> +#define BEGIN_DYNWIND_AND_CATCH_GDB_EXCEPTIONS()                        \
>>
>> move to guile-internal.h
>
> ACK
>
>>> +  do {                                                                  \
>>> +    volatile struct gdb_exception __except;                             \
>>
>> __anything is reserved for the compiler.
>> dynwind_except or some such is probably fine.
>> Similarly with __cleanups.
>
> ACK
>
>>> +  } while (0)
>>> +
>>
>> extra blank line
>
> ACK
>
>>> +/* Helper function to extract a symbol, a name, a language definition,
>>> +   and a value from ITEM, which is an element of a Scheme "arguments" or
>>> +   "locals" list.
>>> +
>>> +   ITEM will either be a pair of a string and a value, a pair of a
>>> +   symbol and a value, or just a symbol.  NAME is a pass-through
>>> +   argument where the name of the symbol will be written.  NAME is
>>> +   allocated in this function, and a cleanup handler is registered if
>>> +   needed.  SYM is a pass-through argument where the symbol will be
>>> +   written.  If the name is a string and not a symbol, SYM will be set
>>> +   to NULL.  LANGUAGE is also a pass-through argument denoting the
>>> +   language attributed to the symbol.  In the case of SYM being NULL,
>>> +   this will be set to the current language.  Finally, VALUE will be set
>>> +   to the unwrapped GDB value, if ITEM is a pair, and otherwise
>>> +   NULL.  */
>>> +static void
>>> +extract_sym_and_value (SCM item, const char **name, struct symbol **sym,
>>> +                       const struct language_defn **language,
>>> +                       struct value **value, struct gdbarch *gdbarch)
>>> +{
>>> +  if (scm_is_pair (item))
>>> +    {
>>> +      SCM symbol_scm = scm_car (item), value_scm = scm_cdr (item);
>>> +      SCM exception = SCM_BOOL_F;
>>> +
>>> +      if (scm_is_string (symbol_scm))
>>> +        {
>>> +          *name = gdbscm_scm_to_host_string (symbol_scm, NULL,
>>> +                                         &exception);
>>> +          if (!*name)
>>> +            gdbscm_throw (exception);
>>> +          make_cleanup (xfree, name);
>>> +
>>> +          *sym = NULL;
>>> +          *language = current_language;
>>> +        }
>>> +      else
>>> +        {
>>> +          *sym = syscm_get_valid_symbol_arg_unsafe (symbol_scm,
>>> +                                                GDBSCM_ARG_NONE,
>>> +                                                    "print-frame");
>>> +          *name = SYMBOL_PRINT_NAME (*sym);
>>> +
>>> +          if (language_mode == language_mode_auto)
>>> +            *language = language_def (SYMBOL_LANGUAGE (*sym));
>>> +          else
>>> +            *language = current_language;
>>
>> Any reason for overriding the language recorded with the symbol
>> even if language_mode is not auto?
>
> I have no idea.  Copied this from py-framefilter.c:99, which says in its
> comment:
>
>       /* If a symbol is specified attempt to determine the language
>          from the symbol.  If mode is not "auto", then the language
>          has been explicitly set, use that.  */
>
> The "If a symbol is specified" I think is true at that point.  So, no
> idea basically.

Ah.
I suppose that's in keeping with the purpose of language_mode.

>>> +      if (!*value)
>>
>> gdb style rules require this to be written as *value == NULL.
>> ref: https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#NULL_Is_Not_Zero
>
> ACK
>
>>> +  switch (SYMBOL_CLASS (sym))
>>> +    {
>>> +    default:
>>> +    case LOC_UNDEF:     /* catches errors        */
>>
>> I'm not aware of a gdb convention for aligning "*/", but it's not used
>> much in gdb, and at any rate in this case the comments don't really
>> add any value, so let's remove them.
>> [I realize this is cut-n-paste from py-framefilter.c.]
>
> ACK
>
>
>>> +    case LOC_CONST:     /* constant              */
>>> +    case LOC_TYPEDEF:   /* local typedef         */
>>> +    case LOC_LABEL:     /* local label           */
>>> +    case LOC_BLOCK:     /* local function        */
>>> +    case LOC_CONST_BYTES:       /* loc. byte seq.        */
>>> +    case LOC_UNRESOLVED:        /* unresolved static     */
>>> +    case LOC_OPTIMIZED_OUT:     /* optimized out         */
>>> +      print_me = 0;
>>> +      break;
>>
>> Since this is the default, simplify this and remove the above cases,
>> except the default case if you want to keep it.
>> Also, I don't think I've seen many cases of default appearing at the
>> front of the case list, we generally put it at the end.
>
> ACK
>
>>> +      struct ui_file *stb = mem_fileopen ();
>>
>> blank line after locals
>> [not my favorite of our conventions, but that's what it is]
>
> ACK
>
>>> +      else if (args_type == MI_PRINT_SIMPLE_VALUES
>>> +               && TYPE_CODE (type) != TYPE_CODE_ARRAY
>>> +               && TYPE_CODE (type) != TYPE_CODE_STRUCT
>>> +               && TYPE_CODE (type) != TYPE_CODE_UNION)
>>
>> we should have a predicate to abstract away what MI_PRINT_SIMPLE_VALUES
>> means. Maybe even better would be a function that also took args_type as
>> a parameter. No need to add that with this patch set.
>> Bonus points for doing so though. :-)
>
> ACK :)
>
>>> +                         const struct language_defn *language)
>>
>> Mixed kinds of indentation (spaces vs tabs+spaces).
>> General gdb convention is to use tabs+spaces.
>
> Will reindent the file.
>
>>> +      /*  MI has varying rules for tuples, but generally if there is
>>
>> Extra space after /*.
>> I know this is cut-n-paste from py-framefilter.c, but you've
>> reformatted it anyway.
>
> ACK
>
>>> +          gdbscm_print_single_arg (out, NULL, &entryarg, NULL, &opts,
>>> +                                   args_type, print_args_field, NULL);
>>> +        }
>>> +    }
>>> +
>>
>> extra blank line
>
> ACK
>
>>> +  inferior = current_inferior();
>>
>> space before (
>
> ACK