Re: [RFC] Support for calling overloaded C++ functions from Python

[Phil Muldoon <pmuldoon at redhat dot com>]

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