From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2951 invoked by alias); 21 Mar 2013 20:55:31 -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 2878 invoked by uid 89); 21 Mar 2013 20:55:19 -0000 X-Spam-SWARE-Status: No, score=-7.3 required=5.0 tests=AWL,BAYES_00,KAM_STOCKGEN,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,SPF_HELO_PASS,TW_BJ,TW_JF 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; Thu, 21 Mar 2013 20:55:15 +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 r2LKtD6R030946 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 21 Mar 2013 16:55:14 -0400 Received: from barimba (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r2LKtAPW024852 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Thu, 21 Mar 2013 16:55:11 -0400 From: Tom Tromey To: Phil Muldoon Cc: "gdb-patches\@sourceware.org" Subject: Re: [patch][python] 1 of 5 - Frame filter Python C code changes. References: <513E56EC.2050802@redhat.com> Date: Thu, 21 Mar 2013 20:55:00 -0000 In-Reply-To: <513E56EC.2050802@redhat.com> (Phil Muldoon's message of "Mon, 11 Mar 2013 22:13:00 +0000") Message-ID: <87vc8kzd5d.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/msg00815.txt.bz2 Phil> This email and patch covers the python/ changes for Python Frame Filters. Thanks. On irc you mentioned that performance wasn't great. I'm wondering if you are working on the frame stash fix. I think it would be good for that to go in first, so that there isn't a window where frame filter performance is bad. Phil> diff --git a/gdb/python/lib/gdb/FrameDecorator.py b/gdb/python/lib/gdb/FrameDecorator.py Phil> + @staticmethod Phil> + def _is_limited_frame(frame): Phil> + """Internal utility to determine if the frame is special or Phil> + limited.""" Phil> + sal = frame.find_sal() Phil> + Phil> + if (not sal.symtab or not sal.symtab.filename Phil> + or frame == gdb.DUMMY_FRAME Phil> + or frame == gdb.SIGTRAMP_FRAME): These should compare 'frame.type()'. Phil> + def function(self): Phil> + """ Return the name of the frame's function, first determining Phil> + if it is a special frame. If not, try to determine filename Phil> + from GDB's frame internal function API. Finally, if a name Phil> + cannot be determined return the address.""" Phil> + Phil> + if not isinstance(self.base, gdb.Frame): Phil> + if hasattr(self.base, "function"): Phil> + return self.base.function() If self.base doesn't have "function", is falling through the right thing to do? It seemed odd to me. If it is correct, I suggest a comment. Alternatively, why the check for gdb.Frame? Nothing else does this. Phil> + if func == None: Phil> + unknown = format(" 0x%08x in" % pc) Extra space. Also this seems weird to me. Why not return None and let whatever prints the frame deal with this detail? Phil> + if (not sal.symtab or not sal.symtab.filename): Extra parens. Phil> + return { Phil> + gdb.SYMBOL_LOC_STATIC: True, Phil> + gdb.SYMBOL_LOC_REGISTER: True, Phil> + gdb.SYMBOL_LOC_ARG: True, Phil> + gdb.SYMBOL_LOC_REF_ARG: True, Phil> + gdb.SYMBOL_LOC_LOCAL: True, Phil> + gdb.SYMBOL_LOC_REGPARM_ADDR: True, Phil> + gdb.SYMBOL_LOC_COMPUTED: True Phil> + }.get(sym_type, False) Does this make a new hash each time through? (I don't know.) If so let me suggest using a global frozenset instead. Phil> + def fetch_frame_locals(self): Phil> + """Public utility method to fetch frame local variables for Phil> + the stored frame. Frame arguments are not fetched. If there Phil> + are no frame local variables, return an empty list.""" Phil> + lvars = [] Phil> + try: Phil> + block = self.frame.block() Phil> + except: Phil> + return None Phil> + Phil> + for sym in block: Phil> + if sym.is_argument: Phil> + continue; Phil> + if self.fetch_b(sym): Phil> + lvars.append(SymValueWrapper(sym, None)) Phil> + Phil> + return lvars FrameDecorator.frame_locals claims to return an iterator but this returns an array. I think it is better to update the docs to say it returns an iterable. This probably applies elsewhere too. This could return a real iterator rather than an array. That might conceivably matter for frames with many variables. Doesn't this need to traverse the blocks upward until it finds the function's outermost block? Phil> + def fetch_frame_args(self): Doesn't this one also need to find the function's outermost block? A test case with a breakpoint inside a nested block would show this. Phil> + def get_value(self, sym, block): Is this function ever used? Phil> + """Public utility method to fetch a value from a symbol.""" Phil> + if len(sym.linkage_name): Phil> + nsym, is_field_of_this = gdb.lookup_symbol(sym.linkage_name, block) Phil> + if nsym != None: Phil> + if nsym.addr_class != gdb.SYMBOL_LOC_REGISTER: Phil> + sym = nsym I wonder if this is really needed. Phil> + if len(sorted_frame_filters) == 0: Phil> + print " No frame filters registered." I think we have to use print(...) now, for Python 3 compatibility. Phil> + if argv[0] == "all" and argc > 1: Phil> + raise gdb.GdbError(cmd_name + " with 'all' " \ Phil> + "you may not specify a filter.") It seems like there should be a ":" in the output after the command name. Phil> + if all_flag == True: Phil> + filter_locations = ["all", "global","progspace"] Missing space after the second comma. Phil> + else: Phil> + filter_locations = ["global","progspace"] Space. Phil> + Phil> + # If we only have one completion, complete it and return it. Phil> + if len(flist) == 1: Phil> + flist[0] = flist[0][len(text)-len(word):] Phil> + Phil> + # Otherwise, return an empty list, or a list of frame filter I don't understand why the one completion branch chops up the string, but the other branch doesn't. Phil> + # dictioanries that the previous filter operation returned. Typo, "dictionaries". Phil> + def complete(self,text,word): Spaces. There are more of these. Phil> +class SetFrameFilterPriority(gdb.Command): Phil> + """GDB command to set the priority of the specified frame-filter. Phil> + Phil> + Usage: set python frame-filter priority dictionary name priority I think the variable parts should be in all caps here. I found the duplication of "priority" confusing... Using caps here is a GNU convention, so I think at least all the user-facing doc strings should be updated. Phil> + PRIORITY is the new priority to set the frame filter. I think this rewording. Phil> + try: Phil> + self._set_filter_priority(command_tuple) Phil> + except gdb.GdbError as e: Phil> + # Print the error, instead of raising it. Phil> + gdb.write(e.message+"\n") I think a GdbError should already be handled this way. See (info "(gdb) Exception Handling"). Phil> +import gdb.command.frame_filters as ffc I think the 'command' package should be a leaf. If you're importing things from there, it is a sign that they should be in this module instead (gdb.frames). Also, I think we have a rule against renaming on import like this. Phil> +def execute_frame_filters(frame, frame_low, frame_high): If this is called by the gdb core, there should at least be a comment to that effect. Phil> + frame_high: The high range of the slice. If this is a negative Phil> + integer then it indicates all frames until the end of the Phil> + stack from frame_low. This disagrees with the code: Phil> + # -1 for frame_high means until the end of the backtrace. Set to Phil> + # None if that is the case, to indicate to itertools.islice to Phil> + # slice to the end of the iterator. Phil> + if frame_high == -1: Phil> + frame_high = None I think one or the other should change. Phil> + else: Phil> + # The end argument for islice is a count, not a position, so Phil> + # add one as frames start at zero. Phil> + frame_high = frame_high + 1; I think it isn't a "count", since otherwise you'd have to subtract frame_low. Phil> +typedef enum mi_print_types Phil> +{ Phil> + MI_PRINT_ALL, Phil> + MI_PRINT_ARGS, Phil> + MI_PRINT_LOCALS Phil> +} mi_print_types; I don't think this is ever used as a typedef, so I suggest dropping the "typedef" part. I didn't see any uses of MI_PRINT_ALL. Phil> + /* For 'symbol' callback, the function can return a symbol or a Phil> + string. */ Phil> + if (PyString_Check (result)) I think you want gdbpy_is_string here. Phil> + *language = python_language; I have a suspicion that this can cause weird behavior. Shouldn't each frame carry its language with it? At least to some extent? Or else could you explain it more? If it is really ok, then it deserves a comment. Phil> + if (language_mode == language_mode_auto) Phil> + *language = language_def (SYMBOL_LANGUAGE (*sym)); Phil> + else Phil> + *language = current_language; Why current_language and not python_language here? If this is correct then why not the same above? Phil> + } Phil> + Phil> + return 1; This function returns PY_BT_ERROR in other places. It should use PY_BT_ values for all the returns. PY_BT_* should be in an enum and that should be used instead of int. I will note it again at that spot. Phil> +static int Phil> +extract_value (PyObject *obj, struct value **value) This is another int / PY_BT_ function: Phil> + return PY_BT_ERROR; [...] Phil> + return 1; There are more of these, like py_print_type and py_print_value. Phil> + Phil> +static int Phil> +mi_should_print (struct symbol *sym, enum mi_print_types type) Needs an intro comment. Phil> +static int Phil> +py_print_value (struct ui_out *out, struct value *val, Phil> + const struct value_print_options *opts, Phil> + enum py_frame_args args_type, Phil> + const struct language_defn *language) [...] Phil> + type = check_typedef (value_type (val)); It seems like this has to be wrapped in a TRY_CATCH. Isn't one of the callers in a Python context? And if not, there's no need to bother with TRY_CATCH and exception conversion elsewhere in this function. Phil> + else Phil> + if (args_type != MI_PRINT_NO_VALUES) Phil> + should_print = 1; Wrong formatting here, should be "else if ...". Phil> +static PyObject * Phil> +get_py_iter_from_func (PyObject *filter, char *func) Phil> +{ Phil> + PyObject *result = PyObject_CallMethod (filter, func, NULL); Phil> + Phil> + if (result) New style, if (result != NULL) Phil> + { Phil> + if (result == Py_None) Phil> + { Phil> + Py_DECREF (result); Phil> + Py_RETURN_NONE; Phil> + } You can just 'return result' here and avoid the reference counting business. Phil> + if (fa) fa != NULL There's a few of these... unfortunately the coding standard changed between your patch submissions. Phil> + if (! success) I think this should be checking against PY_BT_ERROR. And likewise elsewhere. Phil> + if (sym && ui_out_is_mi_like_p (out) && ! mi_should_print (sym, MI_PRINT_ARGS)) Phil> + continue; 'sym != NULL', line is too long, and I think the "continue" leaks sym_name. Phil> + TRY_CATCH (except, RETURN_MASK_ALL) Phil> + { Phil> + read_frame_arg (sym, frame, &arg, &entryarg); Phil> + } Phil> + if (except.reason > 0) I think this should be except.reason < 0. This occurs in a number of places. Phil> + if (arg.entry_kind != print_entry_values_only) Phil> + py_print_single_arg (out, NULL, &arg, NULL, &opts, Phil> + args_type, print_args_field, NULL); What if py_print_single_arg returns PY_BT_ERROR? Phil> + py_print_single_arg (out, NULL, &entryarg, NULL, &opts, Phil> + args_type, print_args_field, NULL); Likewise. Phil> + { Phil> + /* If the object has provided a value, we just print that. */ Phil> + if (val) Phil> + py_print_single_arg (out, sym_name, NULL, val, &opts, Phil> + args_type, print_args_field, language); The "!= NULL" thing, plus the error checking thing. There are many more instances of the missing NULL check. Phil> + if (item) Phil> + { Phil> + TRY_CATCH (except, RETURN_MASK_ALL) Phil> + { Phil> + ui_out_text (out, ", "); Phil> + } Phil> + if (except.reason > 0) Phil> + { Phil> + gdbpy_convert_exception (except); Phil> + goto error; This can leak 'item' on exception. Phil> + TRY_CATCH (except, RETURN_MASK_ALL) Phil> + { Phil> + annotate_arg_end (); Phil> + } Phil> + if (except.reason > 0) Phil> + { Phil> + gdbpy_convert_exception (except); Phil> + goto error; Phil> + } Here too. Phil> +static int Phil> +enumerate_locals (PyObject *iter, Phil> + struct ui_out *out, Phil> + int indent, Phil> + enum py_frame_args args_type, Phil> + int print_args_field, Phil> + struct frame_info *frame) This sure has a lot of overlap with enumerate_args. Phil> + if (sym && ui_out_is_mi_like_p (out) && ! mi_should_print (sym, MI_PRINT_LOCALS)) Phil> + continue; Same issues as above. Phil> + TRY_CATCH (except, RETURN_MASK_ALL) Phil> + { Phil> + ui_out_field_string (out, "name", sym_name); Phil> + xfree (sym_name); This will leak sym_name if ui_out_field_string throws. Phil> + if (args_type != MI_PRINT_NO_VALUES) Phil> + { Phil> + py_print_value (out, val, &opts, args_type, language); Phil> + } Error checking. Phil> + do_cleanups (cleanups); Phil> + } I think this is calling do_cleanups in the loop, but initializing it outside the loop. This should crash if there is more than one local. Phil> +static int Phil> +py_print_frame (PyObject *filter, int flags, enum py_frame_args args_type, Phil> + struct ui_out *out, int indent, htab_t levels_printed) Phil> + PyObject *result = PyObject_CallMethod (filter, "function", NULL); This shadows a variable named 'result' in the outer scope. I'd prefer to avoid shadowing. This occurs in a few spots. You could rename the outer variable if you prefer. Phil> + slot = (struct frame_info **) htab_find_slot (levels_printed, Phil> + frame, INSERT); [...] Phil> + if (*slot != NULL && (*slot) == frame) I think just *slot == frame suffices here. Can we really have FRAME==NULL? If we can then I think this code won't really work right, since I think you can't store a NULL into an htab. Phil> + line = PyLong_AsLong (result); Extra space. Phil> + /* For MI we need to deal with the "children" list population of Phil> + elided frames, so if MI output detected do not send newline. */ MI ignores ui_out_text, so you don't need the check. Anyway the next thing printed is the locals, so this comment seems wrong. Phil> + /* Finally recursively print elided frames, if any. */ Phil> + elided = get_py_iter_from_func (filter, "elided"); Phil> + if (! elided) Phil> + goto error; I didn't notice this before, but get_py_iter_from_func doesn't check to see if the object has the indicated method. The other spots that call PyObject_CallMethod do this check. Phil> + if (elided != Py_None) Phil> + { Phil> + PyObject *item; Phil> + Phil> + make_cleanup_py_decref (elided); This leaks elided in the case where it is Py_None. You can just hoist the cleanup. Phil> + while ((item = PyIter_Next (elided))) There is no error check after the loop exits, but there should be. Phil> + { Phil> + int success = py_print_frame (item, flags, args_type, out, indent, Phil> + levels_printed); Extra space after "=". Phil> + TRY_CATCH (except, RETURN_MASK_ALL) Phil> + { Phil> + /* In MI now we can signal the end. */ Phil> + if (ui_out_is_mi_like_p (out)) Phil> + ui_out_text (out, "\n"); Phil> + } Phil> + if (except.reason > 0) Phil> + { Phil> + gdbpy_convert_exception (except); Phil> + goto error; Phil> + } I don't think this does anything. See mi-out.c:mi_text. Phil> +static PyObject * Phil> +bootstrap_python_frame_filters (struct frame_info *frame, int Phil> + frame_low, int frame_high) Wrong spot for a line break, should be before the "int". Phil> + PyObject *module, *sort_func, *iterable, *frame_obj, *iterator, Phil> + *py_frame_low, *py_frame_high; I'd prefer a ";" at the end of the first line and "PyObject" to be repeated. Phil> + frame_obj = frame_info_to_frame_object (frame); Phil> + if (! frame_obj) Phil> + return NULL; This leaves a dangling cleanup. It should be 'goto error'. Phil> + if (iterable != Py_None) Phil> + { Phil> + iterator = PyObject_GetIter (iterable); Phil> + Py_DECREF (iterable); Phil> + } Phil> + else Phil> + Py_RETURN_NONE; This leaves a dangling ref in the Py_None case. You have to always decref iterable. Phil> +int apply_frame_filter (struct frame_info *frame, int flags, Line break after the first "int". Phil> + while ((item = PyIter_Next (iterable))) This loop needs an error check after it. Phil> @@ -91,6 +101,7 @@ objfpy_new (PyTypeObject *type, PyObject *args, PyObject *keywords) Phil> return (PyObject *) self; Phil> } Phil> + Phil> PyObject * Phil> objfpy_get_printers (PyObject *o, void *ignore) Phil> { Spurious newline addition. Phil> +int apply_frame_filter (struct frame_info *frame, int flags, Newline after first "int". Phil> +/* Python frame-filter status returns constants. */ Phil> +static const int PY_BT_ERROR = 0; Phil> +static const int PY_BT_COMPLETED = 1; Phil> +static const int PY_BT_NO_FILTERS = 2; I think this should be an enum. Phil> + MI_PRINT_NO_VALUES = PRINT_NO_VALUES, Surprising that this is different from CLI_NO_VALUES. Phil> + MI_PRINT_ALL_VALUES = PRINT_ALL_VALUES, This one doesn't seem to be any different from the CLI variant. If so, please merge them. We may not be able to tame the insanity but we can reduce it a little bit... :) Phil> + /* NO_OP for commands that do not print frame arguments */ Phil> + NO_VALUES I didn't see this one used. It seems horrible to have 3 options for "no values". Phil> +} py_frame_args; I think the typedef can be dropped. AFAICT the whole patch uses the enum tag. thanks, Tom