From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15729 invoked by alias); 24 Oct 2011 15:52:15 -0000 Received: (qmail 15719 invoked by uid 22791); 24 Oct 2011 15:52:14 -0000 X-SWARE-Spam-Status: No, hits=-6.8 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,SPF_HELO_PASS X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 24 Oct 2011 15:51:57 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p9OFpu6d009249 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Mon, 24 Oct 2011 11:51:56 -0400 Received: from localhost.localdomain (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id p9OFps7J027220; Mon, 24 Oct 2011 11:51:55 -0400 From: Phil Muldoon To: Paul Koning Cc: gdb-patches@sourceware.org, Tom Tromey Subject: Re: [python] [patch] PR python/13331 References: <09787EF419216C41A903FD14EE5506DD030CE5E15C@AUSX7MCPC103.AMER.DELL.COM> <966B9848-BA1E-4777-BCE3-FD57AEA6E989@comcast.net> Reply-to: pmuldoon@redhat.com X-URL: http://www.redhat.com Date: Mon, 24 Oct 2011 16:00:00 -0000 In-Reply-To: <966B9848-BA1E-4777-BCE3-FD57AEA6E989@comcast.net> (Paul Koning's message of "Mon, 24 Oct 2011 11:31:07 -0400") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2011-10/txt/msg00641.txt.bz2 Paul Koning 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 writes: >> >> Phil> 2011-10-24 Phil Muldoon >> 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