From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31951 invoked by alias); 19 Aug 2013 16:19:26 -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 31942 invoked by uid 89); 19 Aug 2013 16:19:26 -0000 X-Spam-SWARE-Status: No, score=-6.2 required=5.0 tests=AWL,BAYES_00,KAM_STOCKGEN,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:19:25 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r7JGJO6O022145 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 19 Aug 2013 12:19:24 -0400 Received: from barimba (ovpn-113-142.phx2.redhat.com [10.3.113.142]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id r7JGJMTj029059 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Mon, 19 Aug 2013 12:19:23 -0400 From: Tom Tromey To: Phil Muldoon Cc: "gdb-patches\@sourceware.org" Subject: Re: [patch] [python] Fix Python 3 build and testsuite issues References: <521230C8.2040803@redhat.com> Date: Mon, 19 Aug 2013 16:19:00 -0000 In-Reply-To: <521230C8.2040803@redhat.com> (Phil Muldoon's message of "Mon, 19 Aug 2013 15:50:48 +0100") Message-ID: <878uzxlkl1.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-08/txt/msg00512.txt.bz2 >>>>> "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. 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. 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. 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. 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()) 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. This applies elsewhere as well. Tom