From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22935 invoked by alias); 1 Dec 2012 10:59:41 -0000 Received: (qmail 22919 invoked by uid 22791); 1 Dec 2012 10:59:40 -0000 X-SWARE-Spam-Status: No, hits=-4.5 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,RCVD_IN_DNSWL_NONE,RCVD_IN_HOSTKARMA_NO,SPF_SOFTFAIL X-Spam-Check-By: sourceware.org Received: from mtaout20.012.net.il (HELO mtaout20.012.net.il) (80.179.55.166) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sat, 01 Dec 2012 10:59:08 +0000 Received: from conversion-daemon.a-mtaout20.012.net.il by a-mtaout20.012.net.il (HyperSendmail v2007.08) id <0MEC00500MH3S000@a-mtaout20.012.net.il> for gdb-patches@sourceware.org; Sat, 01 Dec 2012 12:59:06 +0200 (IST) Received: from HOME-C4E4A596F7 ([87.69.4.28]) by a-mtaout20.012.net.il (HyperSendmail v2007.08) with ESMTPA id <0MEC005KBMIH3LC0@a-mtaout20.012.net.il>; Sat, 01 Dec 2012 12:59:06 +0200 (IST) Date: Sat, 01 Dec 2012 10:59:00 -0000 From: Eli Zaretskii Subject: Re: [patch][python] 5 of 5 - Frame filter documentation changes In-reply-to: <50B8C35F.9040000@redhat.com> To: Phil Muldoon Cc: gdb-patches@sourceware.org Reply-to: Eli Zaretskii Message-id: <83boeet4wh.fsf@gnu.org> References: <50B8C35F.9040000@redhat.com> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2012-12/txt/msg00000.txt.bz2 > Date: Fri, 30 Nov 2012 14:31:59 +0000 > From: Phil Muldoon > > This patch/email address documentation changes for Python Frame > Filters. Thanks. What about NEWS? > +@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". 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. > +@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). > +@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. > +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. > +@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"? > This iterator must contains the frames that ^^^^^^^^^^^^^ Either "must contain" or "contains" without "must". > +It is the frame filter task to also filter out the elided frames from ^^^^^^ "filter's" > +the source iterator. This will avoid the frame being printed twice. ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ "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. > +@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? > +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. > +@defun FrameWrapper.frame_locals () > + > +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. Likewise here: a description of what the symbol and the value express would be beneficial. > +@node Writing a Frame Filter/Wrapper > +@subsubsection Writing a Frame Filter and Wrapper > +@cindex Writing a Frame Filter/Wrapper Lower-case letters in index entries, please. > +The Python dictionary @code{gdb.frame_filters} contains key/object > +pairings that compromise a frame filter. These frame filters must ^^^^^^^^^^ I assume you meant "comprise". > +Auto-loading}). The two other areas where frame filter dictionaries > +can be found are: @code{gdb.Progspace} which contains a > +@code{frame_filters} dictionary attribute, and each @code{gdb.Objfile} > +object which also contain a @code{frame_filters} dictionary attribute. ^^^^^^^ "contains" > + > +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. > +@value{GDBN} then prunes any frame filter where the @code{enabled} > +attribute is set to @code{False}. "any frame filters whose @code{enabled} attribute is @code{False}." > +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? > 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. > +@subsubheading Implementing a frame filter If you take my suggestion above, this subsubheading will become unnecessary. > +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? > +@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.) > +(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? > +(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. > +@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? > +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.