From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11225 invoked by alias); 19 Aug 2013 16:45:06 -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 11214 invoked by uid 89); 19 Aug 2013 16:45:06 -0000 X-Spam-SWARE-Status: No, score=-7.5 required=5.0 tests=AWL,BAYES_00,KAM_STOCKGEN,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 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; Mon, 19 Aug 2013 16:45:05 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r7JGj3tL001602 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 19 Aug 2013 12:45:04 -0400 Received: from localhost.localdomain (ovpn-112-34.ams2.redhat.com [10.36.112.34]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r7JGj2qk000778; Mon, 19 Aug 2013 12:45:02 -0400 Message-ID: <52124B8D.6010609@redhat.com> Date: Mon, 19 Aug 2013 16:45:00 -0000 From: Phil Muldoon MIME-Version: 1.0 To: Tom Tromey , "gdb-patches@sourceware.org" Subject: Re: [patch] [python] Fix Python 3 build and testsuite issues References: <521230C8.2040803@redhat.com> <878uzxlkl1.fsf@fleche.redhat.com> In-Reply-To: <878uzxlkl1.fsf@fleche.redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-08/txt/msg00513.txt.bz2 On 19/08/13 17:19, Tom Tromey wrote: >>>>>> "Phil" == Phil Muldoon writes: > > Phil> This patch fixes up both current build and test-failures for Python 3 > Phil> in GDB. Most of the errors were mine, as introduced with the frame > Phil> filter patchset. But while I was there I fixed up the other failures > Phil> too. > > Thanks, Phil. > > Phil> --- a/gdb/python/lib/gdb/FrameDecorator.py > Phil> +++ b/gdb/python/lib/gdb/FrameDecorator.py > Phil> @@ -236,12 +236,13 @@ class FrameVars(object): > Phil> # SYM may be a string instead of a symbol in the case of > Phil> # synthetic local arguments or locals. If that is the case, > Phil> # always fetch. > Phil> - if isinstance(sym, basestring): > Phil> - return True > Phil> + if isinstance(sym, gdb.Symbol): > Phil> + sym_type = sym.addr_class > > Phil> - sym_type = sym.addr_class > Phil> + return self.symbol_class.get(sym_type, False) > Phil> + else: > Phil> + return True > > This seems like a semantic change to me. > > Previously it checked for a string type and all other types fell through. > This means you could pass in a "symbol-like" class and get a certain > behavior thanks to duck-typing. > > Now, it checks specifically for gdb.Symbol and treats other types as the > "string" case. > > Perhaps a hasattr check is more appropriate. Yes, I agree. The duck-typing argument did not occur to me. Basically, I reversed the check as Python does not have basestring in Python 3.x so it seemed simpler to reverse the condition. But, of course, if they pass in some object that looks like a gdb.Symbol - but isn't - it will fail. I believe we do say in the documentation that this has to be a "gdb.Symbol or a string". But I like your comment better - there is no reason to force it to a strict gdb.Symbol type. > FWIW I couldn't see any way in-tree that this method could be called > with a non-symbol-like argument. But perhaps we're concerned about > out-of-tree callers. The branch occurs as we allow synthetic locals and arguments (ie, ones that do not exist in the inferior). > Phil> diff --git a/gdb/python/lib/gdb/frames.py b/gdb/python/lib/gdb/frames.py > [...] > > Phil> + iterable = None > > This new variable is never used. Oops apologies. > Phil> + # If all dictionaries are wanted in the case of "all" we > Phil> + # cannot return a combined dictionary as keys() may clash in > Phil> + # between different dictionaries. As we just want all the frame > Phil> + # filters to enable/disable them all, just return the combined > Phil> + # items() as a chained iterator of dictionary values. > Phil> + if name == "all": > Phil> + glob = gdb.frame_filters.values() > Phil> + prog = gdb.current_progspace().frame_filters.values() > Phil> + return_iter = itertools.chain(glob, prog) > Phil> + for objfile in gdb.objfiles(): > Phil> + return_iter = itertools.chain(return_iter, objfile.frame_filters.values()) > Phil> + > Phil> + return return_iter > > I don't understand why this code block was moved. Just a mistake, not sure why I did not notice it. > Phil> # Get a sorted list of frame filters. > Phil> sorted_list = _sort_list() > Phil> - > Phil> - # Check to see if there are any frame-filters. If not, just > Phil> - # return None and let default backtrace printing occur. > Phil> - if len(sorted_list) == 0: > Phil> - return None > Phil> + filter_count = 0 > > Phil> frame_iterator = FrameIterator(frame) > > Phil> - # Apply a basic frame decorator to all gdb.Frames. This unifies the > Phil> - # interface. > Phil> - frame_iterator = itertools.imap(FrameDecorator, frame_iterator) > Phil> + # Apply a basic frame decorator to all gdb.Frames. This unifies > Phil> + # the interface. Python 3.x moved the itertools.imap > Phil> + # functionality to map(), so check if it is available. > Phil> + if hasattr(itertools,"imap"): > Phil> + frame_iterator = itertools.imap(FrameDecorator, frame_iterator) > Phil> + else: > Phil> + frame_iterator = map(FrameDecorator, frame_iterator) > > Phil> for ff in sorted_list: > Phil> + filter_count = filter_count + 1 > Phil> frame_iterator = ff.filter(frame_iterator) > > Phil> + # If there are no filters, return None. > Phil> + if filter_count == 0: > Phil> + return None > > This approach means constructing extra objects in the case where no > frame filters apply. > > I think it is simpler and more efficient to keep the early exit and > either have _sort_list return a list, or do: > > sorted_list = list(_sort_list()) I avoided this because I did not want to convert an iterable to a list (the new dict.keys/values/items returns a lightweight iterator instead of a list.) My thoughts were that creating an imap or map like object would be cheaper than creating a list from a lightweight iterator. > Phil> if (py_func != NULL) > Phil> { > Phil> - const char *function = NULL; > Phil> + char *function = NULL; > > Phil> if (gdbpy_is_string (py_func)) > Phil> { > Phil> - function = PyString_AsString (py_func); > Phil> + function = python_string_to_host_string (py_func); > > Phil> if (function == NULL) > Phil> { > Phil> @@ -1188,7 +1188,8 @@ py_print_frame (PyObject *filter, int flags, enum py_frame_args args_type, > > Phil> msymbol = lookup_minimal_symbol_by_pc (addr); > Phil> if (msymbol.minsym != NULL) > Phil> - function = SYMBOL_PRINT_NAME (msymbol.minsym); > Phil> + /* Duplicate to maintain clean-up consistency. */ > Phil> + function = xstrdup (SYMBOL_PRINT_NAME (msymbol.minsym)); > > I think it is arguably better, and certainly more efficient, to make an > explicit cleanup on the one branch where it is needed. Cleanups are > already in use in this function so no added logic is needed. And, this > lets us keep "function" const, which is clearer in a large function. I chose this for consistency in dealing with the function name cleanup. I have no strong objections, but the paradigm of duplicating a string in one branch where other branches allocate a new string is already entrenched in the Python code. Though I do take your point in that this is dealing with symbols and is likely to be often traversed code. I'll fix it up. You mentioned it applies elsewhere in the patchset? The only similar path is for the filename, but that always creates a newly allocated string from python_string_to_host_string. There is no other newly-allocated/referenced pointer split in the code path in this patch? Cheers, Phil