From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25799 invoked by alias); 7 Oct 2014 13:35:55 -0000 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 Received: (qmail 25789 invoked by uid 89); 7 Oct 2014 13:35:54 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.7 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Tue, 07 Oct 2014 13:35:52 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s97DZon6024285 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Tue, 7 Oct 2014 09:35:50 -0400 Received: from localhost.localdomain (ovpn-112-17.ams2.redhat.com [10.36.112.17]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s97DZmWu032633; Tue, 7 Oct 2014 09:35:49 -0400 Message-ID: <5433EC34.2050801@redhat.com> Date: Tue, 07 Oct 2014 13:35:00 -0000 From: Phil Muldoon MIME-Version: 1.0 To: gdb-patches@sourceware.org CC: dancol@dancol.org Subject: Re: [RFC] Support for calling overloaded C++ functions from Python References: <54330374.8000302@dancol.org> In-Reply-To: <54330374.8000302@dancol.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2014-10/txt/msg00109.txt.bz2 On 06/10/14 22:02, Daniel Colascione wrote: > This patch adds support for more naturally calling overloaded C++ > functions from Python extensions. It adds a new top-level function > exposing find_overload_match to Python, then uses this facility to > implement object-specific call methods that take function names and > argument lists, automatically resolving overloads based on type information. Thanks. Problematically we already have an inferior function call facility for Python values. From the manual: https://sourceware.org/gdb/current/onlinedocs/gdb/Values-From-Inferior.html "A gdb.Value that represents a function can be executed via inferior function call. Any arguments provided to the call must match the function's prototype, and must be provided in the order specified by that prototype. For example, some_val is a gdb.Value instance representing a function that takes two integers as arguments. To execute this function, call it like so: result = some_val (10,20) Any values returned from a function call will be stored as a gdb.Value." While I do not object to your patch (I think it is great), I do think we need to either clarify in the manual the difference between "calling" a value in Python, and using the .call() function. I notice you have not written a documentation patch, so maybe it can be clarified sufficiently there. (Also needs testsuite patch). An alternative is to somehow merge the two approaches so that they can be supported seamlessly. Personally I prefer the latter, but as this just an RFC I'd like to hear your feedback. > +++ b/gdb/python/lib/gdb/__init__.py > @@ -110,6 +110,18 @@ def auto_load_packages(): > > auto_load_packages() > > +def call(fn_name, *args): > + "Call function fn_name with args, performing overload resolution." > + fn_symbol, is_member = lookup_symbol(fn_name) > + if fn_symbol is None: > + # No symbol for this value, so just look it up globally and > + # call it as a value. > + fn = parse_and_eval(fn_name) > + return fn(*args) > + > + # Otherwise, try to resolve the C++ overload set > + find_overload(fn_name, *args, symbol = fn_symbol)(*args) > + I think this should be defined elsewhere other than __init__.py. > +/* Implementation of gdb.find_overload(). */ > + > +struct find_overload_arguments { > + int side_effects; > + int adl; > + int full; > + int both; > + struct value *this_value; > + struct symbol *this_symbol; > +}; Can you please comment each member of the struct? Especially as the names are short and have little meaning/context in the names. > + if (foargs->this_symbol == NULL) > + { > + PyErr_SetString (PyExc_TypeError, "invalid symbol"); > + return -1; > + } > + } Documentation nit: all communication to the user should go through the _() function where possible (for internationalization) and also be capitalized and punctuated. For this and other strings, please. > +static PyObject * > +gdbpy_find_overload_1 (PyObject *self, PyObject *args, PyObject *kw) > +{ > + struct value **vargs; > + Py_ssize_t nargs, i, varg_nr, nr_vargs; > + int badness; > + char *name = NULL; > + struct find_overload_arguments foargs; > + enum oload_search_type search_method; > + PyObject* pyresult; > + > + struct value *result_value; > + struct symbol *result_symbol; > + int result_staticp; > + > + memset (&foargs, 0, sizeof (foargs)); > + foargs.adl = 1; > + foargs.side_effects = 1; > + > + nargs = PyTuple_Size (args); > + if (nargs == -1) > + return NULL; > + > + if (nargs == 0) > + { > + PyErr_SetString (PyExc_TypeError, "no function given"); > + return NULL; > + } > + > + if (PyTuple_GET_ITEM (args, 0) != Py_None) > + { > + name = python_string_to_host_string (PyTuple_GET_ITEM (args, 0)); > + if (name == NULL) > + return NULL; > + > + make_cleanup (xfree, name); > + } > + > + if (parse_find_overload_kw_arguments (kw, &foargs) == -1) > + return NULL; I think you should call do_cleanups at some point in this function. (You registered a cleanup earlier). Even though the cleanup will likely happen on a later call, it is good practice to cleanup explicitly what you add to the cleanup stack before you exit the function. While some functions do not call do_cleanups in GDB (knowing that the cleanup will happen later, they are rare and always need a specialized reason for it.) If you want to make sure only your cleanups are executed, stash the return value of the first cleanup call in a variable, and call do_cleanups (yourvar). If the call to do_cleanups is conditional somehow, you can register a null_cleanup and just call it unconditionally. > + > + nr_vargs = nargs - 1; > + if (foargs.this_value != NULL) > + nr_vargs += 1; > + vargs = alloca (sizeof (*vargs) * nr_vargs); > + varg_nr = 0; This snippet needs a comment imo. As "this_value" is vaguely named and not commented above, I am not sure what you are checking. I think you are checking if a value exists in the structure, and if so adjusting the size. But it would nice to be explicit here. > + if (foargs.this_value != NULL) > + vargs[varg_nr++] = foargs.this_value; > + > + for (i = 1; i < nargs; ++i) > + { > + PyObject *parg = PyTuple_GET_ITEM (args, i); I'm not sure why you are using the non-argument checking version of this Python function? > + struct value *varg = convert_value_from_python (parg); > + if (varg == NULL) > + return NULL; I think at this point you still have a cleanup registered but are not calling do_cleanups(). This and for other "return" statement after the cleanup registration. > + > + vargs[varg_nr++] = varg; > + } > + > + if (foargs.both) > + { > + search_method = BOTH; > + if (foargs.this_value == NULL) > + { > + PyErr_SetString (PyExc_TypeError, > + "with both=True, object must be set"); > + return NULL; > + } > + } > + else if (foargs.this_value && foargs.this_symbol) > + search_method = BOTH; > + else if (foargs.this_value) > + search_method = METHOD; > + else > + search_method = NON_METHOD; > + > + result_value = NULL; > + result_symbol = NULL; > + result_staticp = 0; > + > + badness = find_overload_match (vargs, nr_vargs, I think we should expose the return value of this function to Python as one of three constants and make it available after this function is called. Otherwise we hide information from the user. I notice you incorporate it as part of BuildTuple so it should be just adding some constants. Something like: gdb.GOOD_MATCH gdb.COERCIONS_APPLIED gdb.INCOMPATIBLE Though when coercions are applied, GDB will print a warning message, a scripted use of this function would not catch this caveat. > + if (!foargs.full && result_symbol != NULL) > + { > + if (symbol_read_needs_frame (result_symbol)) > + { > + PyErr_SetString (PyExc_RuntimeError, "need frame?!"); Nit, I would just write here "Symbol %s needs a frame to be read." or something to that effect. ;) > + if (foargs.full) { The "{" needs its own line. This and others. > + PyObject* pyobject; PyObject *someothername. Maybe return_object. Also nit with the placement of the *. > + > + if (foargs.this_value == NULL) > + { > + pyobject = Py_None; > + Py_INCREF (pyobject); > + } > + else > + { > + pyobject = value_to_value_object (foargs.this_value); > + if (pyobject == NULL) > + return NULL; > + } > + > + make_cleanup_py_decref (pyresult); > + return Py_BuildValue ("(iOOO)", > + badness, > + pyobject, > + pyresult, > + result_staticp ? Py_True : Py_False); > + } > + > + return pyresult; I think Py_BuildValue always returns a new reference. If that is the case, I think you are leaking a reference count to the "pyobject" variable. > + > +PyObject * > +gdbpy_find_overload (PyObject *self, PyObject *args, PyObject *kw) > +{ > + volatile struct gdb_exception except; > + PyObject *result = NULL; > + > + TRY_CATCH (except, RETURN_MASK_ALL) > + { > + struct cleanup *cleanup = make_cleanup_value_free_to_mark > (value_mark ()); Ah I think I see what you did with the cleanups in gdbpy_find_overload_1 now. Exporting a cleanup like this and freeing it from a mark seems much more complicated than it needs be. A better approach would be the cleanup strategy I mentioned above. It makes the function more self contained, and able to not have to rely on another function to cleanup the mess it made. > - if (TYPE_CODE (ftype) != TYPE_CODE_FUNC) > + /* call_function_by_hand will fail if our function happens not to be > + callable, but it considers integers callable, treating them as > + function addresses. Allowing any integer to be callable from > + Python seems silly, however. */ > + if (TYPE_CODE (ftype) == TYPE_CODE_INT) I don't agree. A gdb.Value in Python could be a pointer pointing to a function address. Anyway, this falls under an API guarantee in that it has been in GDB for several releases. We cannot just yank it now. We are currently discussing a deprecation framework though. > +/* Convenient wrapper around find_overload. */ > + > +static PyObject * > +valpy_call_method_1 (PyObject *self, PyObject *args, PyObject *kw) > +{ > + PyObject* function = NULL; > + PyObject* value_args = NULL; > + Py_ssize_t nargs, i; > + struct value *self_value = ((value_object *) self)->value; > + struct type *self_type; > + > + self_type = check_typedef (value_type (self_value)); > + > + if (TYPE_CODE (self_type) == TYPE_CODE_CLASS || > + TYPE_CODE (self_type) == TYPE_CODE_STRUCT || > + TYPE_CODE (self_type) == TYPE_CODE_REF) > + { > + /* Calls of member functions need `this' to be an address, but > + we have a value. Get the address here. */ > + self = valpy_get_address (self, NULL); > + if (self == NULL) > + return NULL; > + > + make_cleanup_py_decref (self); Same arguments with cleanups here. I'd rather see them called in the function at exit. We often use local gotos if the exit of a function is conditional according to some value. > + value_args = PyTuple_New (nargs); > + if (value_args == NULL) > + return NULL; > + > + make_cleanup_py_decref (value_args); > + Py_INCREF (self); Why do you need to increment the reference count of "self" here? > +static PyObject * > +valpy_call_method (PyObject *self, PyObject *args, PyObject *kw) > +{ > + volatile struct gdb_exception except; > + PyObject *result = NULL; > + > + TRY_CATCH (except, RETURN_MASK_ALL) > + { > + struct cleanup *cleanup = make_cleanup_value_free_to_mark > (value_mark ()); Same argument for cleanups as above? Cheers Phil