Re: [patch][python] 1 of 5 - Frame filter Python C code changes.

[Tom Tromey <tromey at redhat dot com>]

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