Re: [PATCH] Allow nested python pretty printers.

[Andrew Oakley <andrew at ado dot is-a-geek dot net>]

On 17/08/11 10:56, Phil Muldoon wrote:
> andrew@ado.is-a-geek.net writes:
> 
>> From: Andrew Oakley <andrew@ado.is-a-geek.net>
>>
>> I don't think this is quite ready to commit - MI support needs to be added and
>> documentation needs to be updated.  I thought it was sensible to post it now
>> though so I can deal with any feedback and so that others don't find the need
>> to implement the same thing again.
> 
> Understood.  If you want comments on a work-in-progress, (I believe) you
> should submit it to gdb-patches anyway, with a [RFC] tag.

Fair enough, I'll do that in future.

>> By allowing the children iterator of pretty printers to return more
>> pretty printers multi levels of display can be created.
> 
> I think this idea is ok, but I do not think renaming functions ad-hoc is
> ok.
> 
>> - try_convert_value_from_python trys to convert a python PyObject to a
>>   gdb struct value like convert_value_from_python but does not raise an
>>   error if the PyObject type was not valid.
> 
> I think if you want to do this, you should have
> try_convert_value_from_python wrap convert_value_from_python and deal
> with the exception internally, instead of renaming the function.

I don't really regard this as a rename - there is still a function with
the same name, signature and behaviour as the old function.  I'll come
back to this later on.

>> - run_pretty_printer performs all of the steps to print a value given a
> 
> The name change seems gratuitous.
> 
>> -/* Helper for apply_val_pretty_printer that formats children of the
>> -   printer, if any exist.  If is_py_none is true, then nothing has
>> -   been printed by to_string, and format output accordingly. */
>> +/* Helper for apply_val_pretty_printer that runs a pretty printer,
>> +   including formattin of the children if any exist.  */
> 
> Typo formattin.
> 
>>  static void
>> -print_children (PyObject *printer, const char *hint,
>> -		struct ui_file *stream, int recurse,
>> -		const struct value_print_options *options,
>> -		const struct language_defn *language,
>> -		int is_py_none)
>> +run_pretty_printer (PyObject *printer, struct ui_file *stream, int recurse,
>> +		    const struct value_print_options *options,
>> +		    const struct language_defn *language,
>> +		    struct gdbarch *gdbarch)
> 
> I'm not sure I agree with name change.  I don't want to bike-shed this
> but "print_children" is a legitimate atomic operation that should remain
> as its own function.  I would suggest architecting another function to
> call the relevant pieces if we detect nested printers.

Thats fine by me, I'll do that.  Because print_children was a static
function that was only used in one place I didn't think it would matter
(the rename happened because it now does more than just print the children).

>>    if (!iter)
>> @@ -634,15 +645,33 @@ print_children (PyObject *printer, const char *hint,
>>  	}
>>        else
>>  	{
>> -	  struct value *value = convert_value_from_python (py_v);
>> +	  struct value *value;
>>  
>> -	  if (value == NULL)
>> +	  if (try_convert_value_from_python (py_v, &value))
>> +	    {
>> +	      if (value == NULL)
>> +		{
>> +		  gdbpy_print_stack ();
>> +		  error (_("Error while executing Python code."));
>> +		}
> 
> I'm not sure what his change  gives us?  You still raise an exception,
> if value = NULL? Which is what the old code did.

Yes, if the type was OK for converting to a struct value then the
behaviour is unchanged.  The real difference is that we now know when
the type was not OK (so we can try doing something else).

> 
>> +	      else
>> +		common_val_print (value, stream, recurse + 1, options,
>> +				  language);
>> +	    }
>> +	  else if (PyObject_HasAttr (py_v, gdbpy_to_string_cst))
>> +	    {
>> +	      /* have another pretty printer to run */
>> +	      run_pretty_printer (py_v, stream, recurse + 1, options, language,
>> +	      			  gdbarch);
>> +	    }
> 
> So, in this case, if try_convert_value_to_python returns false, one
> assumes there is a nested printer?  Can printers nest other printer
> ad infinitum? What happens if the nested printer returns another nested
> printer? It's not clear here what the implications are.

You are correct.  If the value could not be handled any other way and it
has a to_string member then we assume it is a pretty printer.  This has
been discussed previously on the list in the thread "Python API - pretty
printing complex types".  This should become more clear when I do the
documentation update.

With regards infinite loops of pretty printers you are correct that we
will get stuck.  This isn't a regression - it has always been possible
to construct cases like this.  Perhaps I need to stick a QUIT somewhere
so you can interrupt it though.

>>  /* Try to convert a Python value to a gdb value.  If the value cannot
>> -   be converted, set a Python exception and return NULL.  Returns a
>> -   reference to a new value on the all_values chain.  */
>> -
>> -struct value *
>> -convert_value_from_python (PyObject *obj)
>> +   be converted because it was of the incorrect type return 0.  If the value
>> +   was of the correct type set value to a reference to a new value on the
>> +   all_values chain and returns 1.  If a Python exception occurs attempting to
>> +   convert the value then value is set to NULL and 1 is returned */
>> +int
>> +try_convert_value_from_python (PyObject *obj, struct value **value)
>>  {
>> -  struct value *value = NULL; /* -Wall */
>> +  int typeok = 1;
> 
> As I noted, I really don't like this change.  I think you should create
> a new function that wraps this one and discards the exception rather
> than altering this.  Essentially the opposite of what you have done.

There main reason why I wanted to not throw the exception at all rather
than throwing it and catching it again is so I can tell why the error
occured - was the value of the wrong type (so we didn't even attempt to
do a conversion) or did the conversion itself fail?  I also didn't
really want to have to work out what the nasty macros + setjmp/longjmp
code was doing in GDB.

To be honest what I really wanted to be able to do is say "is this
python value convertible to a gdb.Value" and then only actually do the
conversion if it was.  The problem with that is we would get duplication
of code between the type checking function and the function that
actually did the conversion.

Is it possible to check what exception has been raised in GDB?  Do you
still think this would be better the other way round?

>> +/* Try to convert a Python value to a gdb value.  If the value cannot
>> +   be converted, set a Python exception and return NULL.  Returns a
>> +   reference to a new value on the all_values chain.  */
>> +struct value *
>> +convert_value_from_python (PyObject *obj)
>> +{
>> +  struct value * value;
>> +  if (!try_convert_value_from_python(obj, &value))
>> +    PyErr_Format (PyExc_TypeError,
>> +		  _("Could not convert Python object: %s."),
>> +		  PyString_AsString (PyObject_Str (obj)));
>>    return value;
> 
> So this would be try_convert_value_from_python  that just discarded the
> exception.  But there are deeper problems.  In this case, if there
> really is an error converting a value, in your code you replace it with
> a generic exception.  I think the old code is wrong in this too.  I
> think if there is an exception, we should not mask it.  I guess we could
> mask particular types given your case.

try_convert_value_from_python didn't discard an exception - it didn't
even try to convert the value if it returns 0.

There are essentially two failure modes for
try_convert_value_from_python - if it returns 0 then the given value was
not of the correct type and it made no attempt to convert it, if it
returns 1 but value is set to NULL then it was of the correct type but
the conversion failed (and a python exception has been raised).  The
comment above try_convert_value_from_python explains this.

The gdb (rather than python) error state does indeed seem to be reset to
be a generic error even though we could have started with a gdb error
and turned it into a python KeyboardInterrupt or RuntimeError.  I think
the code probably should be more careful about this, but again it isn't
a regression in behaviour.

Thanks for reviewing my patch, apologies if I sound like I'm just trying
to be argumentative in places (thats not the intention).


-- 
Andrew Oakley