This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On 12/05/2012 05:03 PM, Tom Tromey wrote:
>>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:
> 
> Hi.  Lots of notes on this one.

Thanks for the review.  I will fix the issues you highlighted and
submit a patch in the new year.  I will be out of the office until the
first week of January, so I will not be able to reply in the next two
weeks.

 
> 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.


I was just trying to show a case where you have frame wrappers,
wrapping other frame wrappers, etc, until you get to BaseFrameWrapper
which wraps a gdb.Frame.  What notation should I use to illustrate
this?


> 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.

In most cases, value will be None.  The typical case is if symbol is a
gdb.Symbol, and value is None, then GDB will read that value for you.
The documentation patch gives all of the different combinations of
assignment for these.

> 
> 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?

If this function returns None, then GDB will not print any frame
locals.  It can probably be written differently in the Python code.
But fetch_frame_locals has to return a None, or an iterator
 
> 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?

Nothing, but I just left it there if the user wishes to use it.  I can
remove it.  Fetching a local/frame value has some caveats to it, so I
decided to leave it in there as a signpost.  I can remove it.

> 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.

This was something you requested when we were talking about it earlier
this year ;)

This is pretty much just the equivalent of a Java interface.  But I
have no strong feelings on this either way.
 

> It seems strange to have both _get_sort_priority and _get_priority.
> Similarly elsewhere.

This was because the Python sort/filter functions (in invoke) stop if
it encounters an error.  So if there is a badly implemented filter
(say one that does not have a "priority" attribute) then all frame
filters will not run.  This was to try and make the system more
robust.


> 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.


Sure, but I think if a class does not have an attribute which is
mandatory, we should not just set it blindly.

> 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?

This was a case again, of trying to make the exception more relevant
to the user where a frame filter is badly implemented.  I have no
strong feelings here either.


> 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.

Sure I will take a look at it when it goes in
 
> Phil> +    for ff in sorted_list:
> Phil> +        frame_iterator = ff[1].filter(frame_iterator)
> 
> I don't understand the "[1]" here.

keys() from a dictionary returns a tuple of key and value pairings.
This just calls the value side (ie, the filter itself)

> 
> If the 'invoke' function is meant to be used from outside the new
> commands, it should probably not be in gdb.command.

It was easier to write "invoke" in Python than in C, and yes it is
only meant for GDB to call.  Good point on the gdb.Command issue
though.

> 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.

(and others), this was a case of copying and pasting code from the
info pretty printers command.  Mea Culpa, will fix.
 

> 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.

Completion of the printer name?

> 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?

I guess I am not clear on the difference between the two?

> 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?

Typo.
 
> 
> 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.

I am not sure what you mean here with CLI, as this is an MI case?
 
> 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 *?

Typo/Oops.
 
> 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.

I vaguely remember discussing with you, but I cannot remember the
outcome.  Still, no objections  from me.

> Phil> +		     char *sym_name,
> 
> Why not const char *?

Oops, and to all others too. 

> 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.

I can't remember why, but I do not see a reason why, either. Will fix.
 
> 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.

Because CLI and MI use those enums internally, and each print values
differently depending on type.  So I get those values from MI/CLI.
They provide them, I did not create them.  But I still have to account
for their instructions to match existing CLI/MI expectations of output.
 
> 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.

That's just an error on my part, ignore the "inner_cleanup = " on the
second instance.  The second instance is to deal with an oddity of
tuples in MI.  Usually if there is only one item in a list, then a
tuple is not created.  Except in one command, -stack-list-variables I
think, which always creates a tuple regardless.  I will write more on
this in another email.

> 
> 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.

That's a bug/memory leak.

> 
> 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.

Depending what option is passed, sometimes type is printed sometimes
not.  You can see for yourself with the -stack-list-* commands, and
the value type options specified.  Personally, I would always print
the type, but I cannot, to stay compatible.  Or I could be completely
wrong ;)  But I do recall this being the case, and if I remember
correctly the testsuite accounts for non-frame-filter output too.
 

> 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.

Ok, thanks.

> 
> 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?

Yes.

-stack-list-locals --all-values
^done,locals=[]

> 
> 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.

I generally followed the case in the stack.c frame printing which "bt"
uses:

     /* Do this regardless of SOURCE because we don't have any source
         to list for this frame.  */
      if (print_level)
        {
          ui_out_text (uiout, "#");
          ui_out_field_fmt_int (uiout, 2, ui_left, "level",
				frame_relative_level (frame));
        }

There is no alternative to that "if".

> 
> 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?

Error on my part.

> 
> 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.

No this is an error on my part, as this is a recursive call.  I
probably forgot to update it when I added the PY_BT_ constants.
 
> 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.

It was very much simpler to write "invoke" in Python, than in C.  In
fact where I could write something in Python, I did.  Most of the C
code is just printing the frames.

> 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.

This was a late addition to track frames printed and is simply a
blimp.  I think I did not create a cleanup for this as it will always
fall through to the free.
 
> I think this will double-free, due to the destructor.
> 
> Phil> +	  Py_DECREF (self->printers);
> Phil> +	  Py_DECREF (self->frame_filters);
> 
> Likewise.

Yeah, I forgot that decref'ing self, calls the object's custom dealloc
method if it has one, and these do.  Already fixed this locally.

> 
> 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.

Ditto, above.
 
> Phil> +PyObject *
> Phil> +pspy_get_frame_filters (PyObject *o, void *ignore)
> 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.

Ditto.
 
Thanks

Phil


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]