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

[Phil Muldoon <pmuldoon at redhat dot com>]

On 12/05/2012 08:49 PM, Tom Tromey wrote:
>>>>>> "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).

I will reword this.
 
> 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...)

As long as the argument (in this case, the name with spaces) is
enclosed in quotes no.  There are tests for it, I believe.


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

What do you suggest for a name?  A frame wrapper is essentially a
decorator onto top of a gdb.Frame, and gdb.Frames are immutable.

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

It just stating that a Frame Wrapper decorates one frame, but one
frame wrapper object can wrap multiple frames.  Not sure how to word
it differently.  For example, a Frame Wrapper that capitalizes
function names.  That is one frame wrapper written in Python.  It
works on the frame it is wrapping.  But this frame wrapper can be
applied to as many frames as you like, where it will capitalize each
function according to the content of the frame.


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

Ok, will do.

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

I was thinking in the case of the user overriding BaseFrameWrapper as
we suggest they do.  It is something I would consider a handy base
reference.  They may want to slightly alter how frame_locals are
printed, but might need a primer on how they are handled in the base
object.  Anyway, no strong opinions here.  I will remove it if you
feel I should.
 
> 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.

Ok, was wondering how to do this across two method declarations in
texinfo.  There did not seem a straightforward way.
 
> 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.

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


I think there is a case of dual iteration?  If you unwind the entire
stack in the frame filter section looking for inlined frames, then
when frame filters have completed, GDB starts printing from the top of
that iterator again.

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

Sure ;)
 
> 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.


Yes, name() returns the inlined frame name, while function() returns
the actual frame where the inlined frame was inlined into.  We talked
a little about this on IRC, and we may think name() is buggy.  But not
sure if we can change that now.
 
> 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.

I really struggled with the examples as writing a short, yet
meaningful example in the scope of a manual is really difficult.  I am
taking suggestions/patches for better! ;)

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

Sure, in the backtraces section?
 
> 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.

Ok will remove.
 
> Phil> +This feature is currently (as of @value{GDBN} 7.6) experimental, and
> Phil> +may work differently in future versions of @value{GDBN}.

As I mentioned to Eli, I included it as the pretty printing Python
support does, and I thought that it might be modus operandi for these
things.  I will remove it.

Thanks,

Phil