Re: [patch][python] 5 of 5 - Frame filter documentation changes

[Phil Muldoon <pmuldoon at redhat dot com>]

On 12/01/2012 10:58 AM, Eli Zaretskii wrote:
>> Date: Fri, 30 Nov 2012 14:31:59 +0000
>> From: Phil Muldoon <pmuldoon@redhat.com>
>>
>> This patch/email address documentation changes for Python Frame
>> Filters.
> 
> Thanks.
> 
> What about NEWS?

Oops, will include in next patch.

> 
>> +@item backtrace raw
>> +@itemx bt raw
>> +@itemx bt raw @var{n}
>> +@itemx bt raw -@var{n}
>> +@itemx bt raw full
>> +@itemx bt raw full @var{n}
>> +@itemx bt raw full -@var{n}
>> +Do not run Python frame filters on this backtrace.  @xref{Frame
>> +Filters API}, for more information.  This is only relevant when
>> +@value{GDBN} has been configured with @code{Python} support.
> 
> I'd prefer 'no-filters' or some such, since 'raw' has no immediate
> relation to "frame filters".

Yeah this was something I brought up in the first email (0 of 5).  I
have no strong preference in naming for the CLI as long as it is not a
pain to type.  "no-filters" still seems long too me.
 
> I suggest to mention here that frame filters can be disabled, with a
> cross-reference to where commands to disable them are described.
> Also, I think we should have a CLI commands to enable and disable them
> all, because otherwise I see no simple way of getting
> backward-compatible behavior.

I did not add a command, because I was trying to be consistent in
functionality with other Python GDB elements.  For example, there is
no way to disable pretty printers globally.  I have no objection to
adding such a command, though.
 
>> +@node Frame Filters API
>> +@subsubsection Filtering and Wrapping Frames.
>> +@cindex Frame Filter/Wrappers API
> 
> There's a separate node about frame wrapper API, so I think this
> @cindex entry should be modified accordingly.  Also, please start
> index entries with a lower-case letter, unless caps are absolutely
> necessary (as in proper names).

Ok, thanks.

> 
>> +@value{GDBN}.  Frame filters may only work on the wrapping iterator.
>> +This preserves data integrity within @value{GDBN}.
> 
> Not sure the reader will understand what you mean by "wrapping
> iterator" here.  Perhaps explain or give a cross-reference.

Actually wrapping can go, and we can just let iterator remain, perhaps
"Frame filters may only work on a Python iterator".
 
>> +takes a Python iterator, and returns a Python iterator.  For further
>> +information on frame filters see, @ref{Writing a Frame
>> +Filter/Wrapper}.                ^
> 
> No need for this comma.

Thanks, and to other straight-forward comments.
 
>> +@node Frame Wrapper API
>> +@subsubsection Wrapping and Decorating Frames.
>> +@cindex Frame Wrapper API
> 
> Index entries should start with a lower-case letter.
> 
>> +@defun FrameWrapper.elided ()
>> +
>> +The @code{elided} method groups frames together in a hierarchical
>> +system.  An example would be an interpreter call that occurs over many
>> +frames but might be better represented as a group of frames distinct
>> +from the other frames.
>> +
>> +The @code{elide} function must return an iterator that conforms to the
>> +Python iterator protocol.
> 
> "elide" or "elided"?

I don't care, but this function returns frames that have been elided
so it made sense to me.  The eliding itself is done in the frame
filter or in another Python object.
 
> "This will avoid printing the frame twice."
> 
>> +@defun FrameWrapper.filename ()
>> +
>> +This method returns the filename associated with this frame.
>> +
>> +This method must return a Python string containing the filename, and
>> +optionally, the path to the filename of the frame, or a Python
>> +@code{None}.
> 
> So does this method return one string or two, when it uses the
> optional behavior?  IOW, it is not clear whether "and" means another
> output or the contents of a single output string.


Badly worded.  This should always return the filename and path.
 

>> +@defun FrameWrapper.frame_args ()
>> +
>> +This method must return an  iterator that conforms to the Python
>> +iterator protocol, or a Python @code{None}.  This iterator must
>> +contain objects that implement two methods, described here.
>> +
>> +The object must implement an @code{argument} method which takes no
>> +parameters and must return a @code{gdb.Symbol} or a Python string.  It
>> +must also implement a @code{value} method which takes no parameters
>> +and which must return a @code{gdb.Value}, a Python value, or
>> +@code{None}.  If the @code{value} method returns a Python @code{None},
>> +and the @code{argument} method returns a @code{gdb.Symbol},
>> +@value{GDBN} will look-up and print the value of the @code{gdb.Symbol}
>> +automatically.
> 
> I would suggest to describe what are the symbol and the value here.
> Maybe the reader will be able to guess that, but why expect them to
> guess?

I really struggled with this section.  As value and symbol are not
simple things to describe in the scope of this function definition.  I
guess prefacing with a simple description is fine, but to my mind they
will always be inadequate.  I took the notion that if a GDB Python
scriptwriter is writing a frame filter, the concept of symbols and values
should be assumed to be known.

> 
>> +A brief example:
>> +
>> +@smallexample
>> +class SymValueWrapper ():
>> +
>> +    def __init__(self, symbol, value):
>> +        self.sym = symbol
>> +        self.val = value
>> +
>> +    def value (self):
>> +        return self.val
>> +
>> +    def symbol (self):
>> +
>> +        return self.sym
> 
> Is this example (which AFAIU simply shows what the default of None
> will do) really useful?  A useful example should show some non-trivial
> use of the facility.

This was another thing I struggled with.  In this section I just
wanted to show the make-up of an object that would implement that
interface.  The actual printing of symbols and values is much more
complex.  There is ready-made code for this in "BaseFrameWrapper",
which I reference elsewhere.  But for example, not all values are
printed in GDB backtrace, and the accountancy for these would go well
beyond the scope of a @smallexample.

>> +
>> +Each frame filter object in these dictionaries is passed a single
>> +Python iterator argument and should return a Python iterator.  Each
>> +frame filter object must conform to the frame filter interface
>> +definition (@pxref{Frame Filters API}).  The iterator returned by the
>> +frame filter must contain only a collection of frame wrappers
>> +(@pxref{Frame Wrapper API}), conforming to the frame wrapper interface
>> +definition.
>> +
>> +When a command is executed from @value{GDBN} that is compatible with
>> +frame filters, @value{GDBN} combines the @code{global},
>> +@code{gdb.Progspace} and all @code{gdb.ObjFile} dictionaries
>> +currently loaded.  All of the @code{gdb.Objfile} dictionaries are
>> +combined as several frames and thus object files might be in use.
>            ^                 ^                     ^
> I think you need commas in the indicated places, or else this sentence
> can be misinterpreted.  Also I'd insert "several" before "object", for
> the same reason.  That's assuming I understood correctly what you
> meant in that sentence.

Ok, will recompose.

>> +according to the @code{priority} attribute in each filter.  Once the
>> +dictionaries are combined, sorted and pruned, @value{GDBN} then wraps
>                               ^^^^^^^^^^^^^^^^^
> "pruned and sorted" (as this is the order you just described).
> 
>> +all frames in the call-stack with a @code{BaseFrameWrapper} object,
>> +and calls each filter in priority order.
> 
> The "priority" part should not be here, since the filters are already
> sorted, right?

Yes, will remove.

> 
>>                                         The input to the first frame
>> +filter will be an initial iterator wrapping a collection of
>> +@code{BaseFrameWrapper} objects.  The output from the previous filter
>> +will always be the input to the next filter, and so on.
> 
> I would suggest to move all the text of this node up to here to the
> "Frame Filters API" section.  This text provides an overview of how
> filters and wrappers work, which IMO is sorely missed before you dive
> into describing the API itself.  Having this overview there will allow
> the reader to better understand what you describe in the correct
> context.  In any case, the text up to here has almost nothing to do
> with "writing a filter/wrapper", which is the subject of this section.


What I tried to do with this functionality is provide four sections
that stood independently.  Frame Filters API and Frame Wrappers API
which were API references.  These would form a library of the API
calls.  But writing a frame filter also implies knowledge of frame
wrappers.  And putting this in the Frame Filters API seemed the wrong
place.  It's an API section, not a description of how frame filters
and frame wrappers work together. So my opinion was that API sections
should only reference API like content.  So I decided to write a
cookbook like section. This section, with suitable cross-references,
brought the knowledge stored in the two API sections and approached
the problem of how to write a Frame Filter and Frame Wrapper with an
example focused approach.  I would still like to maintain this
approach if I can.  (The last section I have not talked about here is
Managing Frame Filters, with GDB commands).


>> +The first step is attribute creation and assignment:
>> +
>> +@smallexample
>> +        self.name = "Foo"
>> +        self.priority = 100
>> +        self.enabled = True
>> +@end smallexample
>> +
>> +The second step is registering the frame filter with the dictionary or
>> +dictionaries that the frame filter has interest in:
>> +
>> +@smallexample
>> +        gdb.frame_filters [self.name] = self
>> +@end smallexample
> 
> Instead of repeating the code fragments, how about adding comments to
> the example that explain what its various parts do?

I don't have a strong opinion, but repeating the very small code
fragments seemed more convenient than having to constantly scroll up
and reference the example when reading the describing text.
 
>> +@item disable frame-filter @var{filter-dictionary} @var{filter-name}
>> +Disable a frame filter in the dictionary matching
>> +@var{filter-dictionary} and @var{filter-name}.
>> +@var{filter-dictionary} may be @code{global}, @code{progspace} or the
>> +name of the object file where the frame filter dictionary resides.
>> +@var{filter-name} is the name of the frame filter.  A disabled
>> +frame-filter is not deleted, it may be enabled again later.
> 
> It would be good to say here how to disable all filters at once.  (I
> hope there is such a way.)

>From the CLI there is no way, see above comment at beginning of this
email.

 
>> +(gdb) info frame-filter
>> +
>> +global frame-filters:
>> +  Priority  Enabled  Name
>> +  ========  =======  ====
>> +  1000      No       Capital Primary Function Filter
>> +  100       Yes      Reverse
>> +progspace /build/test frame-filters:
>> +  Priority  Enabled  Name
>> +  ========  =======  ====
>> +  100       Yes      Progspace Filter 1
>> +objfile /build/test frame-filters:
>> +  Priority  Enabled  Name
>> +  ========  =======  ====
>> +  999       Yes      Build Test Program Filter
> 
> Wouldn't it be better to display all the filters in a single table,
> adding the dictionary as another column?

I made this output to be compatible with "info pretty-printers", which
also outputs them in this way.  They use a similar
global/objfile/progspace approach.

>> +(gdb) disable frame-filter ``/build/test'' ``Build Test Program Filter''
> 
> Why the quotes? they were never mentioned before.  And in any case,
> use ".." here, not ``..'', as the conversion is not done in @example,
> AFAIR.

Quotes are needed so that options with spaces are correctly parsed
(like the name).  Quotes are not needed for the first "/build/test"
option.  I will remove those.  EMACS is quite insistent on placing
those quotes in @smallexample.  In the PDF output I think they are
output correctly, but I will fix.

 
>> +@subheading The @code{-enable-frame-filters} Command
>> +@findex -enable-frame-filters
>> +
>> +@smallexample
>> +-enable-frame-filters
>> +@end smallexample
>> +
>> +@value{GDBN} allows Python-based frame filters to affect the output of
>> +the MI commands relating to stack traces.  As there is no way to
>> +implement this in a fully backward-compatible way, a front end must
>> +request that this functionality be enabled.
>> +
>> +Once enabled, this feature cannot be disabled.
> 
> So in CLI, each filter can be enabled and disabled individually, but
> in MI they can only be enabled en masse, and cannot be disabled?  is
> that a good idea?

No.  In MI they first have to be enabled globally.  This is because
for elided frames we add a "children" field, and that may be
inconsistent with existing front-ends.  We have no machine parsing
obligations with the CLI.  Each MI command can disable frame filters
individually with the "--no-frame-filters" option when they are
globally set to be "on".

> 
>> +This feature is currently (as of @value{GDBN} 7.6) experimental, and
>> +may work differently in future versions of @value{GDBN}.
> 
> Do we have to have this version-specific note in the manual?  They are
> maintenance burden in the long run.

I added this as the pretty printers feature also has a similar
statement.  I can remove it.

Cheers,

Phil