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

[Tom Tromey <tromey at redhat dot com>]

>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:

Phil> +A frame filter works by applying actions to an iterator that is passed
Phil> +to that frame filter as a parameter.  Typically, frame filters utilize
Phil> +tools such as the Python's @code{itertools} module to modify the
Phil> +iterator.  If the frame filter modifies the iterator, it returns that
Phil> +modified iterator, otherwise it returns the original iterator
Phil> +unmodified.

I think this is a somewhat confusing phrasing.  The iterator is not
really modified; instead, the filter takes one iterator as an argument
and then returns an iterator (possibly even the same one).

Phil> +@defvar FrameFilter.name
Phil> +The @code{name} attribute must be Python string which contains the
Phil> +name of the filter displayed by @value{GDBN} (@pxref{Managing Frame
Phil> +Filters}).  This attribute may contain any combination of letters,
Phil> +numbers and spaces.  Care should be taken to ensure that it is unique.
Phil> +This attribute is mandatory.

Do we really want to allow spaces?
Won't that mess with argument parsing and/or command installation?

If we really want this then there should be tests for it.
(I didn't go back to see if there were already some...)

Phil> +@node Frame Wrapper API
Phil> +@subsubsection Wrapping and Decorating Frames.
Phil> +@cindex Frame Wrapper API
Phil> +
Phil> +Frame wrappers are sister objects to frame filters (@pxref{Frame
Phil> +Filters API}).  Frame wrappers are applied by a frame filter and can
Phil> +only be used in conjunction with frame filters.

I tend to think we should have a name for the generic object other than
FrameWrapper.

Also I think maybe we should explain why we didn't just use the exact
same API as supplied by gdb.Frame.

Phil> +A frame wrapper object works on a single frame, but a frame wrapper
Phil> +object can be applied to multiple frames.

This reads weirdly.

Phil> +@value{GDBN} already contains a frame wrapper called
Phil> +@code{BaseFrameWrapper}.  This contains substantial amounts of
Phil> +boilerplate code to print the content of frames.

It doesn't really print... and I think this should mention gdb.Frame
explicitly, rather than "frames".

Phil> +override the methods needed.  The Python code for
Phil> +@code{BaseFrameWrapper} can be found in
Phil> +@file{@var{data-directory}/python/gdb}

I think if people really need to read the source then we probably should
write more documentation.  Let's just drop this line.

Phil> +@defun FrameWrapper.elided ()
[...]
Phil> +The @code{elide} function must return an iterator that conforms to the
Phil> +Python iterator protocol.

Let's let elided return any iterable, not just an iterator.

Phil> +are being elided wrapped in a suitable frame wrapper.  If there are no
Phil> +frames being elided in this frame wrapper, this method must return a
Phil> +Python @code{None}.

I think just "return @code{None}", here and elsewhere.

Phil> +@defun FrameWrapper.frame_args ()
Phil> +
Phil> +This method must return an  iterator that conforms to the Python

Extra space after "an".

I think allowing any iterable here would be good.

Phil> +Even if the @code{frame_args} method returns only a single object, it
Phil> +must be wrapped in an iterator.

This can be dropped.

Phil> +@defun FrameWrapper.frame_locals ()

Since frame_args and frame_locals are both very similar, I think their
text should be shared somehow.  E.g., duplicating all the text about the
returned objects seems difficult to maintain.

Phil> +Each frame filter object in these dictionaries is passed a single
Phil> +Python iterator argument and should return a Python iterator.  Each
Phil> +frame filter object must conform to the frame filter interface
Phil> +definition (@pxref{Frame Filters API}).  The iterator returned by the
Phil> +frame filter must contain only a collection of frame wrappers
Phil> +(@pxref{Frame Wrapper API}), conforming to the frame wrapper interface
Phil> +definition.

I'd say drop the first and last sentences and leave just the middle one.
I think the frame filter API is sufficiently described elsewhere.

Phil> +When a command is executed from @value{GDBN} that is compatible with
Phil> +frame filters, @value{GDBN} combines the @code{global},
Phil> +@code{gdb.Progspace} and all @code{gdb.ObjFile} dictionaries
Phil> +currently loaded.  All of the @code{gdb.Objfile} dictionaries are

One of these "objfile"s is capitalized incorrectly.

Phil> +@smallexample
Phil> +        gdb.frame_filters [self.name] = self
Phil> +@end smallexample
Phil> +
Phil> +As noted earlier, @code{gdb.frame_filters} is a dictionary that is
Phil> +initialized in the @code{gdb} module when @value{GDBN} starts.  In
Phil> +this example, the frame filter only wishes to register with the
Phil> +@code{global} dictionary.

It would be good to mention that in general it is preferable not to
register filters globally.  I'm not sure if the value pretty-printing
text mentions this, but worth a look to see what it says.

Phil> +frame filters should avoid is unwinding the stack if possible.  To
Phil> +search every frame to determine if it is inlined ahead of time may be
Phil> +too expensive at the filtering step.  The frame filter cannot know how
Phil> +many frames it has to iterate over, and it would have to iterate
Phil> +through them all.  This ends up duplicating effort as @value{GDBN}
Phil> +performs this iteration when it prints the frames.

While I think we do need to mention that lazy iteration is very
important, I don't think this is the reason -- gdb only prints the
frames that the frame iterator supplies it, so there is no double
iteration.  The issue is that eagerly unwinding the stack may just be
expensive, as stacks can get very deep.

Phil> +In this example decision making can be deferred to the printing step.
Phil> +As each frame is printed, the frame wrapper can examine each frame in
Phil> +turn when @value{GDBN} iterates.  From a performance viewpoint, this
Phil> +is the most appropriate decision to make.  A backtrace from large or
Phil> +complex programs can constitute many thousands of frames.  Also, if
Phil> +there are many frame filters unwinding the stack during filtering, it
Phil> +can substantially delay the printing of the backtrace which will
Phil> +result in large memory usage, and a poor user experience.

I guess you said that too :)
So part of that previous paragraph can just be zapped, I think.

Phil> +    def function (self):
Phil> +        frame = self.inferior_frame()
Phil> +        name = str(frame.name())
Phil> +        function = str(frame.function())
Phil> +
Phil> +        if frame.type() == gdb.INLINE_FRAME:
Phil> +            name = name + " [inlined from "+ function +"]"

Does this really work?
It seems like it shouldn't.

Phil> +    def next(self):
Phil> +        frame = next(self.input_iterator)
Phil> +        if frame.inferior_frame().type() != gdb.INLINE_FRAME:
Phil> +            return frame
Phil> +
Phil> +        eliding_frame = next(self.input_iterator)
Phil> +        return ElidingFrameWrapper(eliding_frame, [frame])

This works, I think, since it probably isn't possible for gdb to make an
INLINE_FRAME that doesn't have a next frame.

However, this is maybe not a good way to write it, on the theory that
people will cut-and-paste it, and then wind up with incorrect code --
normally such code has to check for the StopIteration case explicitly,
to avoid dropping the outermost frame.

Phil> +@node Managing Frame Filters
Phil> +@subsubsection Management of Frame Filters.
Phil> +@cindex Managing Frame Filters

This node documents user commands, so I think it should go somewhere
else -- somewhere in the "Stack" chapter.

Phil> +global frame-filters:
Phil> +  Priority  Enabled  Name
Phil> +  ========  =======  ====
Phil> +  1000      No       Capital Primary Function Filter

I forgot to mention it before, but it is odd that this table uses "==="
separators, when other tables printed by gdb do not.

Phil> +This feature is currently (as of @value{GDBN} 7.6) experimental, and
Phil> +may work differently in future versions of @value{GDBN}.
 
I'd rather we not say this.

Tom