From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27933 invoked by alias); 7 Dec 2012 14:35:02 -0000 Received: (qmail 27632 invoked by uid 22791); 7 Dec 2012 14:35:01 -0000 X-SWARE-Spam-Status: No, hits=-6.3 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,SARE_TOWRITE,SPF_HELO_PASS X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 07 Dec 2012 14:34:54 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id qB7EYqgE030821 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 7 Dec 2012 09:34:52 -0500 Received: from localhost.localdomain (ovpn-116-39.ams2.redhat.com [10.36.116.39]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id qB7EYpTF028922; Fri, 7 Dec 2012 09:34:51 -0500 Message-ID: <50C1FE8A.20909@redhat.com> Date: Fri, 07 Dec 2012 14:35:00 -0000 From: Phil Muldoon MIME-Version: 1.0 To: Eli Zaretskii CC: gdb-patches@sourceware.org Subject: Re: [patch][python] 5 of 5 - Frame filter documentation changes References: <50B8C35F.9040000@redhat.com> <83boeet4wh.fsf@gnu.org> In-Reply-To: <83boeet4wh.fsf@gnu.org> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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/msg00170.txt.bz2 On 12/01/2012 10:58 AM, Eli Zaretskii wrote: >> 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? 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