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

[Paul Koning <paulkoning@comcast.net>]

> 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.  And in addition, anywhere else NULL is propagated (with any necessary cleanup) but the exception status is *not* overwritten.

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


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

	paul