[RFC v2 05/21] gdb/python: add function () method to gdb.Type object

[Jan Vraný]

On Mon, 2024-12-09 at 14:56 +0000, Andrew Burgess wrote:
> Jan Vrany <jan.vrany@labware.com> writes:
> 
> > This commit adds a new method to Python type objects that returns
> > possibly new function type returning that type. Parameter types can
> > be specified too.
> > 
> > This will be useful later to create types for function symbols
> > created
> > using Python extension code.
> > 
> > Reviewed-By: Eli Zaretskii <eliz@gnu.org>
> > ---
> >  gdb/NEWS                             |  3 ++
> >  gdb/doc/python.texi                  |  8 +++++
> >  gdb/python/py-type.c                 | 54
> > ++++++++++++++++++++++++++++
> >  gdb/testsuite/gdb.python/py-type.exp | 20 +++++++++++
> >  4 files changed, 85 insertions(+)
> > 
> > diff --git a/gdb/NEWS b/gdb/NEWS
> > index 765d14a1ae4..3d208744103 100644
> > --- a/gdb/NEWS
> > +++ b/gdb/NEWS
> > @@ -91,6 +91,9 @@
> >    ** Added gdb.Architecture.void_type. Returns a gdb.Type
> > representing "void"
> >       type for that architecture.
> >  
> > +  ** Added gdb.Type.function.  Returns a new gdb.Type representing
> > a function
> > +     returning that type.  Parameter types can be specified too.
> > +
> >  * Debugger Adapter Protocol changes
> >  
> >    ** The "scopes" request will now return a scope holding global
> > diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
> > index dac5115a5f8..272b51d32c5 100644
> > --- a/gdb/doc/python.texi
> > +++ b/gdb/doc/python.texi
> > @@ -1574,6 +1574,14 @@ Return a new @code{gdb.Type} object which
> > represents a pointer to this
> >  type.
> >  @end defun
> >  
> > +@defun Type.function (@r{[}param_type...@r{]})
> > +Return a new @code{gdb.Type} object which represents a type of
> > function
> > +returning this type.  @code{param_type...} arguments specify
> > parameter
> 
> Shouldn't this be: @var{param_type...} instead of @code?  The
> `param_type' isn't an actual piece of code, but a reference to a
> placeholder in the function template above?

Thanks. Will fix it next submission. 

> 
> > +types.  Use @code{None} as last parameter type to create a vararg
> > function
> > +type.  When invoked with single @code{None} argument or with no
> > arguments at
> > +all it creates a vararg function taking zero or more parameters.
> 
> How do we create a function which takes no arguments then?  I think
> the
> single `None` case is different from the empty list case, right?

Yes, you're right. It should be and it is. I've got fooled by the
printing behaviour you pointed out below. 

> 
> With this patch applied I tried this:
> 
> (gdb) python arch = gdb.selected_inferior().architecture()
> (gdb) python int_t = arch.integer_type(8, True)
> (gdb) python func_t = int_t.function()
> (gdb) python print(func_t)
> int8_t ()
> (gdb) 
> 
> Which looks like a function that takes no arguments.  So then I
> tried:
> 
> (gdb) python func_t = int_t.function(None)
> (gdb) python print(func_t)
> int8_t ()
> (gdb) 
> 
> Which looks like I go the same.  

Yes, both *look* the same but are not. To double check I added
gdb.Type.has_varargs (returning type::has_varargs()) and it 
returned False in first case and True in second. 

I'll fix the documentation. 


> So I checked
> lookup_function_type_with_arguments, and indeed, a trailing nullptr
> does
> make this a vararg function.  It feels like we should probably be
> printing the `...` in the type signature.  I guess this is a bug
> somewhere, likely because C doesn't support varargs without a named
> argument first, so we've never tried to print such a thing before. 
> It
> would be nice to get this fixed (likely not in this patch though),
> but
> at the very least we should create a new sourceware bug to track this
> issue.

I'm not sure if it's a bug or feature. I single-stepped through the
printing machinery and the execution ended up in c_type_print_args. 
There's a comment saying:

      /* Print out a trailing ellipsis for varargs functions.  Ignore
	 TYPE_VARARGS if the function has no named arguments; that
	 represents unprototyped (K&R style) C functions.  */

But reading that method I'm even more confused. Shall
gdb.Type.function() make the type "prototyped", that is, call
type::set_is_prototyped(true)? Anyways, I will probably open a bug
as suggested. 


> 
> Additionally, instead of using `None` for the special value, I wonder
> if
> it would be nicer to use an integer constant.  I'm not sure exactly
> what
> we'd call it, but something like this:
> 
>   (gdb) python func_t = int_t.function(int_t, gdb.VARARGS)
> 
> This might be more self-documenting, than using `None`.  What do you
> think?

Yes, I like it. Maybe using object() as the value and not int.

> 
> > +@end defun
> > +
> >  @defun Type.strip_typedefs ()
> >  Return a new @code{gdb.Type} that represents the real type,
> >  after removing all layers of typedefs.
> > diff --git a/gdb/python/py-type.c b/gdb/python/py-type.c
> > index 284960a3a87..348889dddd8 100644
> > --- a/gdb/python/py-type.c
> > +++ b/gdb/python/py-type.c
> > @@ -774,6 +774,57 @@ typy_unqualified (PyObject *self, PyObject
> > *args)
> >    return type_to_type_object (type);
> >  }
> >  
> > +/* Return a function type. */
> > +static PyObject *
> > +typy_function (PyObject *self, PyObject *args)
> > +{
> > +  struct type *type = ((type_object *) self)->type;
> > +
> > +  gdb_assert (PySequence_Check (args));
> > +
> > +  std::vector<struct type *> param_types (PySequence_Length
> > (args));
> > +
> > +  for (int i = 0; i < PySequence_Length (args); i++)
> > +    {
> > +      PyObject *param_type_obj = PySequence_GetItem (args, i);
> 
> PySequence_GetItem can return nullptr on error, you need to check for
> this case.

OK. 

> 
> > +
> > +      if (param_type_obj == Py_None)
> > +	{
> > +	  param_types[i] = nullptr;
> > +	  if (i != (PySequence_Length (args) - 1))
> > +	    {
> > +	      PyErr_Format (PyExc_ValueError,
> > +			    _("Argument at index %d is None but
> > None can "
> > +			      "only be the last type."), i);
> > +	      return nullptr;
> > +	    }
> > +	}
> > +      else
> > +	{
> > +	  param_types[i] = type_object_to_type (param_type_obj);
> > +	  if (!param_types[i])
> 
> Should be: if (param_types[i] == nullptr)
> 
OK. 

> > +	    {
> > +	      PyErr_Format (PyExc_TypeError,
> > +			    _("Argument at index %d is not a
> > gdb.Type "
> > +			      "object."), i);
> 
> This error could be improved with something like:
> 
>       PyErr_Format (PyExc_TypeError,
> 		    _("Argument at index %d is %s, not a gdb.Type "
> 		      "object."), i, Py_TYPE (param_type_obj)-
> >tp_name);
> 
> The builtin Python errors related to e.g. function argument
> processing,
> tend to show the incorrect type, and I think it helps.  And it's
> pretty
> easy to do.

Good idea, will do. 

> 
> > +	      return nullptr;
> > +	    }
> > +	}
> > +    }
> > +
> > +  try
> > +    {
> > +      type = lookup_function_type_with_arguments (
> > +	       type, param_types.size (), param_types.data ());
> 
> Please wrap lines before the '(', not immediately after.
> 
> 
OK. I saw it written like this elsewhere in the code. 

Thanks for looking into this! 

Jan

> Thanks,
> Andrew
> 
> > +    }
> > +  catch (const gdb_exception &except)
> > +    {
> > +      return gdbpy_handle_gdb_exception (nullptr, except);
> > +    }
> > +
> > +  return type_to_type_object (type);
> > +}
> > +
> >  /* Return the size of the type represented by SELF, in bytes.  */
> >  static PyObject *
> >  typy_get_sizeof (PyObject *self, void *closure)
> > @@ -1641,6 +1692,9 @@ Return the type of a template argument." },
> >    { "unqualified", typy_unqualified, METH_NOARGS,
> >      "unqualified () -> Type\n\
> >  Return a variant of this type without const or volatile
> > attributes." },
> > +  { "function", typy_function, METH_VARARGS,
> > +    "function () -> Type\n\
> > +Return a function type returning value of this type." },
> >    { "values", typy_values, METH_NOARGS,
> >      "values () -> list\n\
> >  Return a list holding all the fields of this type.\n\
> > diff --git a/gdb/testsuite/gdb.python/py-type.exp
> > b/gdb/testsuite/gdb.python/py-type.exp
> > index 7e469c93c35..783261e6f4a 100644
> > --- a/gdb/testsuite/gdb.python/py-type.exp
> > +++ b/gdb/testsuite/gdb.python/py-type.exp
> > @@ -365,6 +365,26 @@ if { [build_inferior "${binfile}" "c"] == 0 }
> > {
> >    gdb_test "python print(gdb.lookup_type('int').optimized_out())"
> > \
> >        "<optimized out>"
> >  
> > +  gdb_test_no_output "python int_t = gdb.lookup_type('int')"
> > +
> > +  gdb_test "python print(repr(int_t.function()))" \
> > +      "<gdb.Type code=TYPE_CODE_FUNC name=int \\(\\)>"
> > +
> > +  gdb_test "python print(repr(int_t.function(int_t, int_t,
> > int_t)))" \
> > +      "<gdb.Type code=TYPE_CODE_FUNC name=int \\(int, int,
> > int\\)>"
> > +
> > +  gdb_test "python print(repr(int_t.function(int_t, None)))" \
> > +      "<gdb.Type code=TYPE_CODE_FUNC name=int \\(int, ...\\)>"
> > +
> > +  gdb_test "python print(repr(int_t.function(None)))" \
> > +      "<gdb.Type code=TYPE_CODE_FUNC name=int \\(\\)>"
> > +
> > +  gdb_test "python print(repr(int_t.function(123)))" \
> > +      "TypeError.*:.*"
> > +
> > +   gdb_test "python print(repr(int_t.function(int_t, None,
> > int_t)))" \
> > +      "ValueError.*:.*"
> > +
> >    set sint [get_sizeof int 0]
> >    gdb_test "python
> > print(gdb.parse_and_eval('aligncheck').type.alignof)" \
> >        $sint
> > -- 
> > 2.45.2
>