Re: [python] [patch] PR python/13331

[Phil Muldoon <pmuldoon@redhat.com>]

Paul Koning <paulkoning@comcast.net> writes:

>> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-owner@sourceware.org] On Behalf Of Tom Tromey
>> Subject: Re: [python] [patch] PR python/13331
>> 
>>>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:
>> 
>> Phil> 2011-10-24  Phil Muldoon  <pmuldoon@redhat.com>
>> Phil>         PR python/13331
>> Phil> 	* python/py-function.c (fnpy_call): Check 'args' is not NULL.
>> Phil> 	(convert_values_to_python): Return on Python tuple allocation
>> Phil> 	failure.
>> 
>> Phil> +  if (! result)
>> Phil> +    return NULL;
>> 
>> I think the function should have a single return convention.
>> One easy way to do that would be to remove the 'error' call and have it return NULL when value_to_value_object fails.
>
> I think the patch is not quite right because it goes against the
> Python rule for exception handling.  A NULL function result is the
> normal way to indicate an error, but associated with returning NULL is
> that in the spot where the error is first detected you set the
> exception status.  

The exception is set by the PyTuple_New.  We purely propagate it, until
it is handled.

> And in addition, anywhere else NULL is propagated (with any necessary
> cleanup) but the exception status is *not* overwritten.

In this patch, and in this functionality it is not.  The Python
exception in the caller gets converted to a GDB error and the exception
cleared.



>> ...
>> Index: python/py-function.c
>> ===================================================================
>> RCS file: /cvs/src/src/gdb/python/py-function.c,v
>> retrieving revision 1.10
>> diff -u -r1.10 py-function.c
>> --- python/py-function.c	5 Aug 2011 14:24:10 -0000	1.10
>> +++ python/py-function.c	24 Oct 2011 13:15:38 -0000
>> @@ -38,6 +38,9 @@
>> {
>>   int i;
>>   PyObject *result = PyTuple_New (argc);
>> +  
>> +  if (! result)
>> +    return NULL;
>> 
>>   for (i = 0; i < argc; ++i)
>>     {
>
> In this loop there is an example of what I mentioned: if
> value_to_value_object returns NULL, exception status has been set, and
> that should not be overwritten here.  On the other hand, there is
> missing cleanup: any objects added to the tuple before the failing
> call need to have a DECREF done on them.

I'm not sure what you mean by this.  If result fails, the args check in
the calling function will fail, the exception converted to a gdb_error,
the exception bits cleared and the call will fail. The loop will not be
executed.  It is also my understanding that PyTuple_SetItem steals a
reference to the object being added.  In this case, there is only one
reference to the object, so the logic here works.  When the tuple is
garbage collected, the reference count for each object will
automatically be decremented and therefore garbage collected too.

>
>
>> @@ -59,24 +62,35 @@
>> 	   void *cookie, int argc, struct value **argv)  {
>>   struct value *value = NULL;
>> -  PyObject *result, *callable, *args;
>> +  /* 'result' must be set to NULL, this initially indicates whether
>> +     the function was called, or not.  */  PyObject *result = NULL;  
>> + PyObject *callable, *args;
>>   struct cleanup *cleanup;
>> 
>>   cleanup = ensure_python_env (gdbarch, language);
>> 
>>   args = convert_values_to_python (argc, argv);
>> +  /* convert_values_to_python can return NULL on error.  If we
>> +     encounter this, do not call the function, but allow the Python ->
>> +     error code conversion below to deal with the Python exception.
>> +     Note, that this is different if the function simply does not
>> +     have arguments.  */
>> 
>> -  callable = PyObject_GetAttrString ((PyObject *) cookie, "invoke");
>> -  if (! callable)
>> +  if (args)
>>     {
>> +      callable = PyObject_GetAttrString ((PyObject *) cookie, "invoke");
>> +      if (! callable)
>> +	{
>> +	  Py_DECREF (args);
>> +	  error (_("No method named 'invoke' in object."));
>
> That should just be a cleanup and return NULL -- the
> PyObject_GetAttrString has set the exception.

Well yes and no, it is seemly expedient here to short-circuit the
process and convert it to a gdb error directly.  The error is more
informative than the error from GetAttrString -- that is there is no
"invoke" function.  This is something we (as in GDB), want to enforce in
our API

Cheers,

Phil