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

Message ID 20241121124714.419946-6-jan.vrany@labware.com
State New
Headers
Series Add Python "JIT" API |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-arm warning Skipped upon request
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 warning Skipped upon request

Commit Message

Jan Vrany Nov. 21, 2024, 12:46 p.m. UTC
  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(+)
  

Comments

Eli Zaretskii Nov. 21, 2024, 1:34 p.m. UTC | #1
> From: Jan Vrany <jan.vrany@labware.com>
> CC: Jan Vrany <jan.vrany@labware.com>,
> 	Eli Zaretskii <eliz@gnu.org>
> Date: Thu, 21 Nov 2024 12:46:58 +0000
> 
> 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(+)

The documentation parts are okay, but...

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

...please use @dots{} instead of literal "..." in both cases here,
when you push.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>
  
Jan Vrany Nov. 21, 2024, 2:06 p.m. UTC | #2
On Thu, 2024-11-21 at 15:34 +0200, Eli Zaretskii wrote:
> > From: Jan Vrany <jan.vrany@labware.com>
> > CC: Jan Vrany <jan.vrany@labware.com>,
> > 	Eli Zaretskii <eliz@gnu.org>
> > Date: Thu, 21 Nov 2024 12:46:58 +0000
> > 
> > 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(+)
> 
> The documentation parts are okay, but...
> 
> > +@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
> 
> ...please use @dots{} instead of literal "..." in both cases here,
> when you push.
> 

Thanks! I have fixed this in my local working version
and it will part of next version. Same for the extra "to"
in the other commit (
https://sourceware.org/pipermail/gdb-patches/2024-November/213479.html
)

Jan

> Reviewed-By: Eli Zaretskii <eliz@gnu.org>
>
  
Andrew Burgess Dec. 9, 2024, 2:56 p.m. UTC | #3
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?

> +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?

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

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?

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

> +
> +      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)

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

> +	      return nullptr;
> +	    }
> +	}
> +    }
> +
> +  try
> +    {
> +      type = lookup_function_type_with_arguments (
> +	       type, param_types.size (), param_types.data ());

Please wrap lines before the '(', not immediately after.

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
  
Eli Zaretskii Dec. 9, 2024, 3:27 p.m. UTC | #4
> From: Andrew Burgess <aburgess@redhat.com>
> Cc: Jan Vrany <jan.vrany@labware.com>, Eli Zaretskii <eliz@gnu.org>
> Date: Mon, 09 Dec 2024 14:56:03 +0000
> 
> Jan Vrany <jan.vrany@labware.com> writes:
> 
> > +@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?

Yes.  Sorry for missing that.
  
Jan Vrany Dec. 10, 2024, 10:02 p.m. UTC | #5
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
>
  
Tom Tromey Dec. 19, 2024, 7:55 p.m. UTC | #6
>>>>> "Jan" == Jan Vrany <jan.vrany@labware.com> writes:

Jan> +  std::vector<struct type *> param_types (PySequence_Length (args));
Jan> +
Jan> +  for (int i = 0; i < PySequence_Length (args); i++)
Jan> +    {

PySequence_Length can supposedly fail.  I think it would be better to
hoist these calls into a variable and then check / return NULL.

Jan> +      type = lookup_function_type_with_arguments (
Jan> +	       type, param_types.size (), param_types.data ());

I think this can lead to obscure crashes.

The issue is that in gdb there is a hidden rule: an objfile-specific
type may only refer to other types from that objfile or to arch-specific
types.  Arch-specific types may only refer to other arch-specific types.

These rules are used to manage the lifetimes of type objects.  A
cross-objfile reference can cause a crash if one of the objfiles, but
not the other one, is deleted.  This can happen in a number of ways, for
example an inferior dlclose.

One idea to solve this is to decouple types from objfiles and have a
"type GC".  This is kind of tricky because some types refer to symbols
though.

Documenting and enforcing the rule in the Python layer would be another
way forward.

Tom
  
Tom Tromey Dec. 19, 2024, 7:57 p.m. UTC | #7
Andrew> Additionally, instead of using `None` for the special value, I wonder if
Andrew> it would be nicer to use an integer constant.  I'm not sure exactly what
Andrew> we'd call it, but something like this:

Andrew>   (gdb) python func_t = int_t.function(int_t, gdb.VARARGS)

Andrew> This might be more self-documenting, than using `None`.  What do you
Andrew> think?

It may be more Pythonic to use a keyword argument:

    int_t.function(type, type, vararg=True)

Tom
  
Jan Vrany Dec. 19, 2024, 11:14 p.m. UTC | #8
On Thu, 2024-12-19 at 12:55 -0700, Tom Tromey wrote:
> > > > > > "Jan" == Jan Vrany <jan.vrany@labware.com> writes:
> 
> Jan> +  std::vector<struct type *> param_types (PySequence_Length
> (args));
> Jan> +
> Jan> +  for (int i = 0; i < PySequence_Length (args); i++)
> Jan> +    {
> 
> PySequence_Length can supposedly fail.  I think it would be better to
> hoist these calls into a variable and then check / return NULL.
> 
> Jan> +      type = lookup_function_type_with_arguments (
> Jan> +	       type, param_types.size (), param_types.data ());
> 
> I think this can lead to obscure crashes.
> 
> The issue is that in gdb there is a hidden rule: an objfile-specific
> type may only refer to other types from that objfile or to arch-
> specific
> types.  Arch-specific types may only refer to other arch-specific
> types.
> 
> These rules are used to manage the lifetimes of type objects.  A
> cross-objfile reference can cause a crash if one of the objfiles, but
> not the other one, is deleted.  This can happen in a number of ways,
> for
> example an inferior dlclose.

Ah, thanks! I was not aware of this!
> 
> One idea to solve this is to decouple types from objfiles and have a
> "type GC".  This is kind of tricky because some types refer to
> symbols
> though.
> 
> Documenting and enforcing the rule in the Python layer would be
> another
> way forward.

I'll do the latter: document and enforce the rules. I think this 
will be okay for all my usecases - at the very least.

Thanks!
Jan


> 
> Tom
>
  

Patch

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
+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.
+@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);
+
+      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])
+	    {
+	      PyErr_Format (PyExc_TypeError,
+			    _("Argument at index %d is not a gdb.Type "
+			      "object."), i);
+	      return nullptr;
+	    }
+	}
+    }
+
+  try
+    {
+      type = lookup_function_type_with_arguments (
+	       type, param_types.size (), param_types.data ());
+    }
+  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