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] Fix Python 3 build and testsuite issues


On 19/08/13 17:19, Tom Tromey wrote:
>>>>>> "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.

Yes, I agree. The duck-typing argument did not occur to me.
Basically, I reversed the check as Python does not have basestring in
Python 3.x so it seemed simpler to reverse the condition.  But, of
course, if they pass in some object that looks like a gdb.Symbol - but
isn't - it will fail.  I believe we do say in the documentation that
this has to be a "gdb.Symbol or a string".  But I like your comment
better - there is no reason to force it to a strict gdb.Symbol type.


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

The branch occurs as we allow synthetic locals and arguments (ie, ones
that do not exist in the inferior).

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

Oops apologies.

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

Just a mistake, not sure why I did not notice it.

> 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())

I avoided this because I did not want to convert an iterable to a list
(the new dict.keys/values/items returns a lightweight iterator instead
of a list.)  My thoughts were that creating an imap or map like object
would be cheaper than creating a list from a lightweight iterator.


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

I chose this for consistency in dealing with the function name cleanup.  I
have no strong objections, but the paradigm of duplicating a string in
one branch where other branches allocate a new string is already 
entrenched in the Python code.

Though I do take your point in that this is dealing with symbols and
is likely to be often traversed code.  I'll fix it up.  You mentioned
it applies elsewhere in the patchset? The only similar path is for the
filename, but that always creates a newly allocated string from
python_string_to_host_string.  There is no other
newly-allocated/referenced pointer split in the code path in this
patch?

Cheers,

Phil



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