From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29887 invoked by alias); 5 Dec 2012 20:49:50 -0000 Received: (qmail 29877 invoked by uid 22791); 5 Dec 2012 20:49:48 -0000 X-SWARE-Spam-Status: No, hits=-6.5 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_SPAMHAUS_DROP,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,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; Wed, 05 Dec 2012 20:49:43 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id qB5KngKP012440 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 5 Dec 2012 15:49:43 -0500 Received: from barimba (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id qB5KnesF002626 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Wed, 5 Dec 2012 15:49:41 -0500 From: Tom Tromey To: Phil Muldoon Cc: "gdb-patches\@sourceware.org" Subject: Re: [patch][python] 5 of 5 - Frame filter documentation changes References: <50B8C35F.9040000@redhat.com> Date: Wed, 05 Dec 2012 20:49:00 -0000 In-Reply-To: <50B8C35F.9040000@redhat.com> (Phil Muldoon's message of "Fri, 30 Nov 2012 14:31:59 +0000") Message-ID: <877gowrzq3.fsf@fleche.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain 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/msg00091.txt.bz2 >>>>> "Phil" == Phil Muldoon 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