From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18048 invoked by alias); 5 Dec 2012 17:03:45 -0000 Received: (qmail 18028 invoked by uid 22791); 5 Dec 2012 17:03:43 -0000 X-SWARE-Spam-Status: No, hits=-5.7 required=5.0 tests=AWL,BAYES_00,KAM_STOCKGEN,KHOP_RCVD_UNTRUST,KHOP_SPAMHAUS_DROP,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS,TW_RG 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 17:03:09 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id qB5H39XR009074 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 5 Dec 2012 12:03:09 -0500 Received: from barimba (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id qB5H35hP021706 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Wed, 5 Dec 2012 12:03:06 -0500 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: <50B8C322.8050602@redhat.com> Date: Wed, 05 Dec 2012 17:03:00 -0000 In-Reply-To: <50B8C322.8050602@redhat.com> (Phil Muldoon's message of "Fri, 30 Nov 2012 14:30:58 +0000") Message-ID: <87mwxstos6.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/msg00075.txt.bz2 >>>>> "Phil" == Phil Muldoon writes: Hi. Lots of notes on this one. Phil> diff --git a/gdb/python/lib/gdb/BaseFrameWrapper.py b/gdb/python/lib/gdb/BaseFrameWrapper.py Phil> + If the result of frame filters running means we have one gdb.Frame Phil> + wrapped by multiple frame wrappers, all sub-classed from Phil> + BaseFrameWrapper: Phil> + Phil> + Wrapper1(Wrapper2(BaseFrameWrapper(gdb.Frame))) I think this notation is a bit weird. Phil> + @staticmethod Phil> + def is_limited_frame(frame): Probably should start with an underscore. Phil> + def __init__(self, base): Phil> + super(BaseFrameWrapper, self).__init__(base) Phil> + self.base = base Perhaps a leading underscore for 'base'. However -- the superclass sets 'self.frame'. If that is considered API, then you don't need 'base' at all. And if it isn't considered API, it should be renamed. Phil> +class BaseSymValueWrapper(): Probably should be BaseSymValueWrapper(object) for Python 3. Same for FrameVars. Phil> + """A container class conforming to the Symbol/Value interface Phil> + which holds frame locals or frame arguments.""" Phil> + def __init__(self, symbol, value): Phil> + self.sym = symbol Phil> + self.val = value As far as I can tell this is always instantiated with value == None. That seems wrong. 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 not frame local variables, return None.""" Typo: should be "there are no". [ fetch_frame_locals ] Phil> + if len(lvars) == 0: Phil> + return None It is curious that this is needed. Will whatever code eventually handles display of this do something different for an empty list? Phil> + def fetch_frame_args(self): Phil> + """Public utility method to fetch frame argument for the Phil> + stored frame. Frame arguments are the only type fetched. If Phil> + there are no frame arguments variables, return None.""" I think "frame argument" - not plural. Phil> + def get_value(self, sym, block): Phil> + """Public utility method to fetch a value from a symbol.""" What calls this? Phil> +class FrameIterator(object): [...] Phil> + def next (self): Phil> + """__next__ implementation. I think just "next", not "__next__". Phil> + self.frame = result.older () No space before parens in Python. It is hard to remember. Phil> +class FrameWrapper (object): Phil> + """Interface for a Frame Wrapper.""" If this is just for documentation, how about we just put it all in the docs and delete the class? Then we can rename "BaseFrameWrapper". I find the BaseFrameWrapper / FrameWrapper distinction a bit confusing. Narrowly speaking, this class is misnamed anyhow, since it doesn't actually wrap anything. Phil> +def _parse_arg(cmd_name, arg): Phil> + """ Internal worker function to take an argument and return a Phil> + tuple of arguments. ... Phil> + object_list = argv[0] Phil> + argument = argv[1] Phil> + Phil> + return(object_list, argument) It seems like you can just return argv here. Phil> +def _get_priority(filter_item): Phil> + """ Internal worker function to return the frame-filter's priority. It seems strange to have both _get_sort_priority and _get_priority. Similarly elsewhere. Phil> +def _set_priority(filter_item, priority): Phil> + """ Internal worker function to set the frame-filter's priority. Phil> + Phil> + Arguments: Phil> + filter_item: An object conforming to the frame filter Phil> + interface. Phil> + priority: The priority to assign as an integer. Phil> + Phil> + Raises: Phil> + gdb.GdbError: When the priority attribute has not been Phil> + implemented. Phil> + """ Phil> + Phil> + if hasattr(filter_item, "priority"): Phil> + filter_item.priority = priority Phil> + else: Phil> + raise gdb.GdbError("Cannot find class attribute 'priority'") Why check for priority before setting it? It seems simpler to just set it. Similarly elsewhere. Phil> + if hasattr(filter_item, "enabled"): Phil> + return filter_item.enabled Phil> + else: Phil> + raise gdb.GdbError("Cannot find class attribute 'enabled'") What is wrong with just fetching it and letting Python throw the exception? Phil> +def _return_list(name): Phil> + """ Internal Worker function to return the frame filter Phil> + dictionary, depending on the name supplied as an argument. If the Phil> + name is not "global" or "progspace", it is assumed to name an Phil> + object-file. Phil> + Phil> + Arguments: Phil> + name: The name of the list, as specified by GDB user commands. Phil> + Phil> + Returns: Phil> + A dictionary object. Phil> + Phil> + Raises: Phil> + gdb.GdbError: A dictionary of that name cannot be found. Phil> + """ Phil> + Phil> + if name == "global": Extra space. Phil> + sorted_frame_filters = copy.copy(all_filters) Phil> + sorted_frame_filters.sort(key = _get_sort_priority, Phil> + reverse = True) I think for Python 3 we recently switched to using 'sorted' instead of the in-place sort. Hmm, that patch doesn't seem to have gone in. Phil> + for ff in sorted_list: Phil> + frame_iterator = ff[1].filter(frame_iterator) I don't understand the "[1]" here. If the 'invoke' function is meant to be used from outside the new commands, it should probably not be in gdb.command. Phil> +# GDB Commands. Phil> +class SetFilterPrefixCmd(gdb.Command): I think this probably needs some help text. Phil> +class ShowFilterPrefixCmd(gdb.Command): Likewise. Phil> +class InfoFrameFilter(gdb.Command): Phil> + """GDB command to list all registered frame-filters. Starting the text with "GDB command" is redundant. Phil> + def __init__(self): This should be the first method. Phil> + def print_list(self, title, filter_list): Phil> + """Print a list.""" Just remove this text. It isn't useful. Phil> + def invoke(self, arg, from_tty): Phil> + """GDB calls this to perform the command.""" Likewise. Phil> +# Internal enable/disable functions. Phil> + Phil> +def do_enable_frame_filter_1(frame_filters, name, flag): This just has a single caller. I don't think it needs to be separate. Phil> + DICTIONARY is the name of the frame filter dictionary on which to Phil> + operate. Named dictionaries are: "global" for the global frame Phil> + filter dictionary, "progspace" for the program space's framefilter Phil> + dictionary. If either of these two are not specified, the Phil> + dictionary name is assumed to be the name of the object-file name. It would be nice if these commands had completion methods that did the right thing. Phil> + object_list = argv[0] Phil> + name = argv[1] Phil> + priority = argv[2] Phil> + return(object_list, name, priority) I think you can just return argv here. Phil> + def _set_filter_priority_1(self, frame_filters, name, priority): I don't think this function is needed. Phil> + object_list = argv[0] Phil> + name = argv[1] Phil> + return (object_list, name) Just return argv. I probably missed some instances of this pattern. Phil> +def register_frame_filter_commands(): Phil> + """Call from a top level script to install the frame-filter commands.""" Phil> + InfoFrameFilter() Phil> + SetFilterPrefixCmd() Phil> + ShowFilterPrefixCmd() Phil> + EnableFrameFilter() Phil> + DisableFrameFilter() Phil> + SetFrameFilterPriority() Phil> + ShowFrameFilterPriority() Phil> + Phil> +register_frame_filter_commands() No need to create a function just to call it once. Phil> + OBJ is the Python object to extract the values from. **NAME is a s/**NAME/NAME Similarly elsewhere. Phil> + pass-through argument where the name of the symbol will be written. Phil> + **NAME is allocated in this function, but the caller is responsible You probably mean *NAME here. **NAME is a single character. Phil> +static int Phil> +extract_sym (PyObject *obj, char **name, struct symbol **sym, Phil> + const struct language_defn **language) [...] Phil> + *language = current_language; Why current_language and not python_language? Phil> + PyErr_SetString (PyExc_RuntimeError, Phil> + _("Unexpected value. Expecting a " \ Don't need a backslash here. Phil> + PyErr_SetString (PyExc_RuntimeError, Phil> + _("Mandatory function 'symbol' not " \ Phil> + "implemented.")); On the whole it seems simpler to just try to call the method and let Python handle the failure, since you have to handle that case anyway. Phil> +static int Phil> +mi_should_print (struct symbol *sym, const char *type) This is only ever called with constant strings for the second argument. It is better to use an enum in this situation. Phil> +static int Phil> +py_print_type (struct ui_out *out, struct value *val) [...] Phil> + if (except.reason > 0) Phil> + { Phil> + PyErr_SetString (PyExc_RuntimeError, Phil> + except.message); Use gdbpy_convert_exception here. This should be fixed at each TRY_CATCH. Phil> +/* Helper function which outputs a value name to value field in a This reads strangely. Phil> + stream. OUT is the ui-out structure the value will be output too, s/too/to/ Phil> +static int Phil> +py_print_value (struct ui_out *out, struct value *val, Phil> + struct value_print_options opts, Phil> + int mi_print_type, Why is mi_print_type 'int' and not enum print_values? Phil> + /* MI disallows different value types against different options the Phil> + client passes, so test type against option. For CLI print all Phil> + values. */ This comment reads strangely. Phil> + if (ui_out_is_mi_like_p (out)) Phil> + { Phil> + struct type *type; Phil> + Phil> + type = check_typedef (value_type (val)); Phil> + if (mi_print_type == PRINT_ALL_VALUES) Phil> + should_print = 1; Phil> + else if (mi_print_type == PRINT_SIMPLE_VALUES Phil> + && TYPE_CODE (type) != TYPE_CODE_ARRAY Phil> + && TYPE_CODE (type) != TYPE_CODE_STRUCT Phil> + && TYPE_CODE (type) != TYPE_CODE_UNION) Phil> + should_print = 1; Phil> + } Rather than making this all conditional, you could make the API simpler to understand by having the CLI always pass PRINT_ALL_VALUES. Phil> +/* Helper function to call a Python method and extract an iterator Phil> + from the result, error checking for Python exception and returns Phil> + that are not iterators. FILTER is the Python object to call, and Phil> + FUNC is the name of the method. Returns a PyObject, or NULL on Phil> + error with the appropriate exception set. This function can return Phil> + an iterator, or None. */ Phil> + Phil> +static PyObject * Phil> +get_py_iter_from_func (PyObject *filter, char *func) Why not const char *? Phil> + if (! PyIter_Check (result)) Phil> + { Phil> + PyErr_Format (PyExc_RuntimeError, Phil> + _(" %s function must " \ Phil> + "return an iterator."), func); Phil> + Py_DECREF (result); Phil> + return NULL; Phil> + } Phil> + else Phil> + { Phil> + PyObject *iterator = PyObject_GetIter (result); I think it doesn't really make sense to both do PyIter_Check and then call PyObject_GetIter. Generally these patches specify the API to work on iterators. But it seems to me that it is more flexible to work on iterables instead. That way code could return an iterator, or an array or tuple, depending on what is convenient to that code -- more Pythonic IMO. This would require fixing up the docs in various places. In this spot at least it would mean not calling PyIter_Check. Phil> + if (! iterator) Phil> + return NULL; Phil> + else Phil> + return iterator; This can be just 'return iterator' Phil> +/* Helper function to output a single frame argument and value to an Phil> + output stream. This function will account for entry values if the Phil> + FV parameter is populated, the frame argument has entry values Phil> + associated with them, and the appropriate "set entry-value" Phil> + options are set. Will output in CLI or MI like format depending Phil> + on the type of output stream detected. OUT is the output stream, Phil> + SYM_NAME is the name of the symbol. If SYM_NAME is populated then Phil> + it must have an accompanying value in the parameter FV. FA is a Phil> + frame argument structure. If FA is populated, both SYM_NAME and Phil> + FV are ignored. OPTS contains the value printing options, Phil> + MI_PRINT_TYPE is an enumerator to the value types that will be Phil> + printed if the output is MI. PRINT_MI_ARGS indicates whether to Phil> + output the ARGS="1" field in MI output. */ Phil> +static int Phil> +py_print_single_arg (struct ui_out *out, Missing blank line. Phil> + char *sym_name, Why not const char *? Phil> + struct value_print_options opts, Why by-value and not 'const struct value_print_options *opts'? I missed it earlier but the same applies to py_print_value. Phil> + int mi_print_type, Phil> + int print_mi_args_flag, Phil> + const char *cli_print_frame_args_type, cli_print_frame_args_type isn't documented in the function comment. The comment refers to print_mi_args, not print_mi_args_flag. This seems like a lot of duplication to me. Why not use a single enum for all cases? That would be a lot simpler. It is fine to introduce a new enum just for this code. This applies to many of the functions in this file. Also these should have the proper enum type and not be ints. Phil> + struct cleanup *inner_cleanup = Phil> + make_cleanup (null_cleanup, NULL); Phil> + Phil> + if (fa) Phil> + { Phil> + language = language_def (SYMBOL_LANGUAGE (fa->sym)); Phil> + val = fa->val; Phil> + } Phil> + else Phil> + val = fv; Phil> + Phil> + /* MI has varying rules for tuples, but generally if there is only Phil> + one element in each item in the list, do not start a tuple. */ Phil> + if (print_mi_args_flag || mi_print_type != PRINT_NO_VALUES) Phil> + { Phil> + inner_cleanup = Phil> + make_cleanup_ui_out_tuple_begin_end (out, Phil> + NULL); It isn't good to assign to the same cleanup variable like this. You can just ignore the return value from this call though. I think the name 'inner_cleanup' is odd here, considering that there is no outer scope. Phil> + /* If frame argument is populated, check for entry-values and the Phil> + entry value options. */ Phil> + if (fa) Phil> + { Phil> + struct ui_file *stb; Phil> + Phil> + stb = mem_fileopen (); Phil> + Phil> + fprintf_symbol_filtered (stb, SYMBOL_PRINT_NAME (fa->sym), Phil> + SYMBOL_LANGUAGE (fa->sym), Phil> + DMGL_PARAMS | DMGL_ANSI); Phil> + if (fa->entry_kind == print_entry_values_compact) Phil> + { Phil> + fputs_filtered ("=", stb); Phil> + Phil> + fprintf_symbol_filtered (stb, SYMBOL_PRINT_NAME (fa->sym), Phil> + SYMBOL_LANGUAGE (fa->sym), Phil> + DMGL_PARAMS | DMGL_ANSI); Phil> + } Phil> + if (fa->entry_kind == print_entry_values_only Phil> + || fa->entry_kind == print_entry_values_compact) Phil> + { Phil> + fputs_filtered ("@entry", stb); Phil> + } Phil> + ui_out_field_stream (out, "name", stb); Phil> + ui_file_delete (stb); Phil> + } Phil> + else Phil> + /* Otherwise, just output the name. */ Phil> + ui_out_field_string (out, "name", sym_name); If similar code appears elsewhere in gdb, how about refactoring so it can be shared? This seems a bit questionable given that it calls mem_fileopen but never makes a cleanup for it. Phil> + /* For MI print the type. */ Phil> + if (ui_out_is_mi_like_p (out) Phil> + && mi_print_type == PRINT_SIMPLE_VALUES) It seems weird to only do this for PRINT_SIMPLE_VALUES. Phil> + else Phil> + if (PyErr_Occurred ()) Merge into one line. Phil> +static int Phil> +enumerate_locals (PyObject *iter, Phil> + struct ui_out *out, Phil> + int mi_print_type, Phil> + int indent, Phil> + int print_mi_args_flag, Phil> + struct frame_info *frame) This seems to share a lot of code with enumerate_args. Can't they be merged? These functions call a bunch of ui_out functions outside of TRY_CATCH. I think that isn't ok -- CLI backtraces are filtered, meaning that it is possible for the user to type "q" at the page break, and this results in a gdb exception being thrown. Phil> + make_cleanup_ui_out_list_begin_end (out, "locals"); Phil> + if (locals_iter != Py_None) Phil> + if (! enumerate_locals (locals_iter, out, mi_print_type, Phil> + indent, 0, frame)) Phil> + goto locals_error; In cases like this, does it really make sense to emit the "locals" field even when it is empty? Is this what MI ordinarily does? Phil> +static hashval_t Phil> +hash_printed_frame_entry (const void *data) Phil> +{ Phil> + const struct frame_info *frame = data; Phil> + Phil> + return htab_hash_pointer (frame); Phil> +} Phil> + Phil> +/* Equality function for the printed hash. */ Phil> + Phil> +static int Phil> +eq_printed_frame_entry (const void *a, const void *b) Phil> +{ Phil> + const struct frame_info *ea = a; Phil> + const struct frame_info *eb = b; Phil> + Phil> + return ea == eb; You don't need these, you can use htab_hash_pointer and htab_eq_pointer instead. Phil> + gdbarch = get_frame_arch (frame); In general you can't call a gdb function outside of TRY_CATCH. I think it is reasonably dangerous to deviate from this rule. Phil> + Py_DECREF (result); Phil> + } Phil> + else Phil> + { Phil> + PyErr_SetString (PyExc_RuntimeError, Phil> + _("'inferior_frame' API must be implemented.")); Phil> + goto error; Phil> + } This is another situation where you should just unconditionally call the method. You only need to check for the method if the code should adapt to its absence; if it is required, just call it and let Python report a failure if it is not there. Phil> + level = frame_relative_level (frame); Unprotected call. Phil> + char buffer[10]; Phil> + sprintf (buffer, "%d", level); Phil> + ui_out_spaces (out, strlen (buffer) + 2); Blank line between declarations and code. Also we're trying to avoid sprintf now; use xnsprintf. Phil> + ui_out_field_fmt_int (out, 2, ui_left, "level", Phil> + level); I wonder though if the above should use ui_out_field_skip instead of ui_out_spaces. Or perhaps both if just the former doesn't result in the correct output. Phil> + func = xstrdup (dup); Phil> + annotate_frame_function_name (); Phil> + ui_out_field_string (out, "func", func); Phil> + xfree (func); Phil> + Phil> + } Extra blank line. Also I think the name 'dup' here is misnomer. It isn't actually a dup of anything. And, there doesn't seem to be a reason to duplicate 'dup' just to print it and then free it. Why not just print it directly? This occurs elsewhere as well. 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. */ Phil> + if (! ui_out_is_mi_like_p (out)) Phil> + { Phil> + annotate_frame_end (); Phil> + ui_out_text (out, "\n"); ui_out_text is a no-op for MI. It is usually cleaner to simply always call it. It doesn't need a comment, either. I think the same applies to annotations. Occurs in other spots too. Phil> + int success = py_print_frame (item, print_level, Phil> + print_frame_info, Phil> + print_args, Phil> + print_locals, Phil> + mi_print_type, Phil> + cli_print_frame_args_type, Phil> + out, indent, Phil> + levels_printed); Phil> + Phil> + if (success == 0 && PyErr_Occurred ()) Phil> + { Phil> + Py_DECREF (item); Phil> + goto error; Phil> + } Can you have success == 0 but no error? Why not return 1 in that case? The py_print_frame doc comment doesn't explain its return values. Oh, I see it returns a PY_BT_ constant. But then a check against 0 is confusing, it should check against the constants. Phil> +/* Helper function to initiate frame filter invocation at starting Phil> + frame FRAME. */ Phil> +static PyObject * Missing blank line. Phil> +bootstrap_python_frame_filters (struct frame_info *frame) Phil> +{ Phil> + Phil> + PyObject *module, *sort_func, *iterable, *frame_obj; Extra blank line. Phil> + module = PyImport_ImportModule ("gdb.command.frame_filters"); Yeah, definitely should be a different module. I think 'invoke' should be renamed, too; and documented. It is nice for things like this if there is an obvious way from Python to accomplish the same thing that gdb does internally. We didn't think of this for value pretty printers (though you can still do it), but I'd like us to consider it for new additions. Phil> +/* Public and dispatch function for frame filters. This is the only Not sure about that first sentence. Phil> + publicly exported function in this file. FRAME is the source Phil> + frame to start frame-filter invocation. FLAGS is an integer Phil> + holding the flags for printing. The following elements of the Phil> + FRAME_FILTER_FLAGS enum denotes makeup of FLAGS: PRINT_LEVEL is a Phil> + flag indicating whether to print the frame's relative level in the Phil> + output. PRINT_FRAME_INFO is a flag that indicates whether this Phil> + function should print the frame information, PRINT_ARGS is a flag Phil> + that indicates whether to print frame arguments, and PRINT_LOCALS, Phil> + likewise, with frame local variables. MI_PRINT_ARGS_TYPE is an Phil> + enum from MI that indicates which values types to print. This Phil> + parameter is ignored if the output is detected to be CLI. Phil> + CLI_PRINT_FRAME_ARGS_TYPE likewise is a an element of what value Phil> + types to print from CLI. OUT is the output stream to print, and Phil> + COUNT is a delimiter (required for MI slices). */ I think COUNT is just a count. Phil> + int success = 0; I think this is probably initialized improperly. Phil> + /* If iterable is None, then there are not any frame filters Extra space before "If". This throws off the indentation of the rest of the comment. Phil> + make_cleanup_py_decref (iterable); Phil> + if (iterable == Py_None) Phil> + { Phil> + do_cleanups (cleanups); Phil> + return PY_BT_NO_FILTERS; Better I think to set 'success' and goto done. Phil> + /* Is it an iterator */ End with a period & two spaces. Phil> + if PyIter_Check (iterable) I think it really should be an iterable, not an iterator. So, remove the 'if' and call PyObject_GetIter unconditionally. Phil> + levels_printed = htab_create (20, Phil> + hash_printed_frame_entry, Phil> + eq_printed_frame_entry, Phil> + NULL); You're creating cleanups for everything except this. It doesn't really matter, since this code is not exposed to gdb exceptions, but I wonder why. Simpler to make it all uniform. Phil> + while ((item = PyIter_Next (iterator)) && count--) If 'count-- == 0' then you will leak 'item'. Phil> + success = py_print_frame (item, print_level, Extra space after the '='. Phil> + /* Do not exit on error printing the frame, continue with Phil> + other frames. */ Phil> + if (success == PY_BT_ERROR && PyErr_Occurred ()) Checking PyErr_Occurred makes me think that the return results could use a cleanup. I think PY_BT_ERROR should mean that an error definitely occurred, so a second check isn't needed. Phil> + gdbpy_print_stack (); It appears this function will never return PY_BT_ERROR if frame printing fails. Phil> + else Phil> + { Phil> + Py_DECREF (iterable); Phil> + error (_("Frame filter must support iteration protocol.")); Why an exception here but not for anything else? Phil> + self->frame_filters = PyDict_New (); Phil> + if (!self->frame_filters) Phil> + { Phil> + Py_DECREF (self->printers); I think this will double-free, due to the destructor. Phil> + Py_DECREF (self->printers); Phil> + Py_DECREF (self->frame_filters); Likewise. Phil> +PyObject * Phil> +objfpy_get_frame_filters (PyObject *o, void *ignore) Needs a comment. Phil> +static int Phil> +objfpy_set_frame_filters (PyObject *o, PyObject *filters, void *ignore) Likewise. Phil> + object->frame_filters = PyDict_New (); Phil> + if (!object->frame_filters) Phil> + { Phil> + Py_DECREF (object->printers); Double free. Phil> + Py_DECREF (object->printers); Phil> + Py_DECREF (object->frame_filters); Likewise. Phil> + self->frame_filters = PyDict_New (); Phil> + if (!self->frame_filters) Phil> + { Phil> + Py_DECREF (self->printers); Likewise. Phil> + Py_DECREF (self->printers); Phil> + Py_DECREF (self->frame_filters); Likewise. Phil> +PyObject * Phil> +pspy_get_frame_filters (PyObject *o, void *ignore) Needs a comment. Phil> +static int Phil> +pspy_set_frame_filters (PyObject *o, PyObject *frame, void *ignore) Likewise. Phil> + object->frame_filters = PyDict_New (); Phil> + if (!object->frame_filters) Phil> + { Phil> + Py_DECREF (object->printers); Double free. Phil> + Py_DECREF (object->printers); Phil> + Py_DECREF (object->frame_filters); Likewise. Phil> + Phil> set_program_space_data (pspace, pspy_pspace_data_key, object); Extra blank line. Phil> +/* This is a cleanup function which decrements the refcount on a Phil> + Python object. This function accounts appropriately for NULL Phil> + references. */ Phil> + Phil> +static void Phil> +py_xdecref (void *p) Phil> +{ Phil> + PyObject *py = p; Phil> + Phil> + /* Note that we need the extra braces in this 'if' to avoid a Phil> + warning from gcc. */ Phil> + if (py) Phil> + { Phil> + Py_XDECREF (py); You don't need the 'if'. Py_XDECREF has one already. Phil> + return make_cleanup (py_xdecref, (void *) py); You don't need the cast. 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; This should be an enum and then various 'int' types in the code changed as well. Tom