Re: [patch] [python] Fix Python 3 build and testsuite issues

[Tom Tromey <tromey at redhat dot com>]

>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> 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