From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21768 invoked by alias); 22 Mar 2013 17:15:00 -0000 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 Received: (qmail 21755 invoked by uid 89); 22 Mar 2013 17:14:51 -0000 X-Spam-SWARE-Status: No, score=-8.1 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.1 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Fri, 22 Mar 2013 17:14:48 +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 r2MHEk5Z018201 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 22 Mar 2013 13:14:47 -0400 Received: from barimba (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id r2MHEjCh017678 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Fri, 22 Mar 2013 13:14:45 -0400 From: Tom Tromey To: Phil Muldoon Cc: "gdb-patches\@sourceware.org" Subject: Re: [patch][python] 5 of 5 - Frame filter documentation changes References: <513E573C.7010502@redhat.com> Date: Fri, 22 Mar 2013 19:54:00 -0000 In-Reply-To: <513E573C.7010502@redhat.com> (Phil Muldoon's message of "Mon, 11 Mar 2013 22:14:20 +0000") Message-ID: <87k3ozxsoq.fsf@fleche.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-SW-Source: 2013-03/txt/msg00857.txt.bz2 >>>>> "Phil" == Phil Muldoon writes: Phil> This patch/email address documentation changes for Python Frame Phil> Filters. Phil> * Python scripting Phil> +** Frame filters and frame decorators have been added. Phil> + Phil> ** Vectors can be created with gdb.Type.vector. The addition isn't indented properly. You'll need to tweak this for trunk anyhow, since you'll be starting a new "Python scripting" entry for 7.7. Phil> +@node Frame Filter Management Phil> +@section Management of Frame Filters. Phil> +@cindex managing frame filters Phil> + Phil> +There are several commands available within @value{GDBN} to manage Phil> +frame filters, detailed here. I think this should first describe what a frame filter does, in a way that ordinary users (not Python-writing users) will understand. Phil> +@kindex set python frame-filter priority Phil> +@item set python frame-filter priority @var{filter-dictionary} @var{filter-name} @var{priority} Seeing this in the manual made me wonder why "python" appears here. I don't think it matters to users. It seems like it is more to type, without a corresponding benefit. What do you think of dropping the "python" sub-command? Phil> +* Frame Filter API:: Filtering Frames. Phil> +* Frame Decorator API:: Decorating Frames. Phil> +* Writing a Frame Filter/Decorator:: Writing a Frame Filter and Decorator. I think this node would be just as clear, and would look better, if it were just named "Writing a Frame Filter". Phil> +@node Frame Filter API Phil> +@subsubsection Filtering Frames. Phil> +@cindex frame filters api Phil> +to the @code{priority} attribute in each filter. Once the Phil> +dictionaries are combined, pruned and sorted, @value{GDBN} then wraps Phil> +all frames in the call-stack with a @code{FrameDecorator} object, and Phil> +calls each filter in order. The input to the first frame filter will I think it should say: Once the dictionaries are combined, pruned and sorted, @value{GDBN} creates an iterator which wraps each frame in the call stack in a @code{FrameDecorator} object, and calls each filter in order. That is, mention the iterator explicitly. The current text makes it sound as though the stack is pre-emptively unwound, but that isn't true, and it is important that developers understand this. Phil> +@defun FrameDecorator.elided () I think these methods should mention 'self'. That is what we did in the pretty-printing API documentation. Phil> +The @code{elided} method groups frames together in a hierarchical Phil> +system. An example would be an interpreter call that occurs over many Phil> +frames but might be better represented as a group of frames distinct Phil> +from the other frames. I think the latter part of this is too vague. How about - An example would be an interpreter, where multiple low-level frames make up a single call in the interpreted language. In this example, the frame filter would elide the low-level frames and present a single high-level frame, representing the call in the interpreted language, to the user. Phil> +frame decorator. If there are no frames being elided in this frame Phil> +decorator, this method must return @code{None}. What happens if it returns an empty iterable? If that is ok, how about ", this method may return an empty iterable, or @code{None}"? Phil> +@defun FrameDecorator.frame_args () Phil> +@anchor{frame_args} Phil> + Phil> +This method must return an iterable or @code{None}. This iterable Phil> +must contain objects that implement two methods, described here. What if it returns an empty iterable instead? Phil> +The object must implement an @code{argument} method which takes no Phil> +parameters and must return a @code{gdb.Symbol} (@pxref{Symbols In Phil> +Python}), or a Python string. I think it should say "a single @code{self} parameter" rather than "no parameters". Likewise for the other method. Phil> +Even if the @code{frame_args} method returns only a single object, it Phil> +must be iterable. I would zap this sentence. Phil> +Even if the @code{frame_locals} method returns only a single object, it Phil> +must be iterable. Here too. Phil> +The second step is registering the frame filter with the dictionary or Phil> +dictionaries that the frame filter has interest in. I think this paragraph should xref to "Python Auto-loading" at some point. Phil> The global dictionary is always Phil> +present, and so will the frame filters registered in it. This reads weirdly. Phil> +well worth reflecting upon when designing a frame filter. An issue Phil> +that frame filters should avoid is unwinding the stack if possible. I think the laziness issue should be pointed out in the main API documentation somewhere. It's very important; it is fine to repeat it here, but it should also be somewhere other than in an example. Phil> +@smallexample Phil> +class InlinedFrameDecorator(FrameDecorator): Phil> + Phil> + def __init__(self, fobj): Phil> + super(InlinedFrameDecorator, self).__init__(fobj) Phil> + self.fobj = fobj self.fobj is never used. FrameDecorator also stores this in an attribute, "base". Should that attribute just be declared public? And if not, shouldn't it be renamed to start with a "_"? 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 +"]" This actually violates the FrameDecorator guidelines, since it bypasses self.base to get the name from the inferior_frame. It seems like it should call self.base.name() instead. And, it should have a comment explaining why it needs to use inferior_frame to call function. I'm a little surprised that calling the inferior frame's "function" does what you want here. That seems like a bug TBH. Phil> +This example approaches the issue with the @code{elided} method. This Phil> +example is quite long, but very simplistic. It is out-of-scope for I would say, s/simplistic/simple/. Phil> + try: Phil> + eliding_frame = next(self.input_iterator) Phil> + except StopIteration: Phil> + return frame Phil> + return ElidingFrameDecorator(eliding_frame, [frame]) What if there are multiple inline frames in a row? Wouldn't you want to elide all of them? That will make the example trickier though. Tom