diff mbox series

[RFC,v2,04/21] gdb/python: add void_type () method to gdb.Architecture object

Message ID 20241121124714.419946-5-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 architecture objects that
returns a void type for that architecture.

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                  |  4 ++++
 gdb/python/py-arch.c                 | 16 ++++++++++++++++
 gdb/testsuite/gdb.python/py-arch.exp |  4 ++++
 4 files changed, 27 insertions(+)

Comments

Eli Zaretskii Nov. 21, 2024, 1:32 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:57 +0000
> 
> This commit adds a new method to Python architecture objects that
> returns a void type for that architecture.
> 
> 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                  |  4 ++++
>  gdb/python/py-arch.c                 | 16 ++++++++++++++++
>  gdb/testsuite/gdb.python/py-arch.exp |  4 ++++
>  4 files changed, 27 insertions(+)

OK for the documentation part, thanks.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>
Andrew Burgess Dec. 9, 2024, 11:43 a.m. UTC | #2 
Jan Vrany <jan.vrany@labware.com> writes:

> This commit adds a new method to Python architecture objects that
> returns a void type for that architecture.
>
> This will be useful later to create types for function symbols created
> using Python extension code.

Jan,

Thanks for this.  Two minor nits, see below...

>
> Reviewed-By: Eli Zaretskii <eliz@gnu.org>
> ---
>  gdb/NEWS                             |  3 +++
>  gdb/doc/python.texi                  |  4 ++++
>  gdb/python/py-arch.c                 | 16 ++++++++++++++++
>  gdb/testsuite/gdb.python/py-arch.exp |  4 ++++
>  4 files changed, 27 insertions(+)
>
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 1ea9fcc65b9..765d14a1ae4 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -88,6 +88,9 @@
>  
>    ** Added gdb.Symbol.domain.  Contains the domain of the symbol.
>  
> +  ** Added gdb.Architecture.void_type. Returns a gdb.Type representing "void"
> +     type for that architecture.
> +
>  * 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 09e374700a4..dac5115a5f8 100644
> --- a/gdb/doc/python.texi
> +++ b/gdb/doc/python.texi
> @@ -7090,6 +7090,10 @@ If the indicated type cannot be found, this function will throw a
>  @code{ValueError} exception.
>  @end defun
>  
> +@defun Architecture.void_type ()
> +This function returns a void type.
> +@end defun
> +
>  @anchor{gdbpy_architecture_registers}
>  @defun Architecture.registers (@r{[} reggroup @r{]})
>  Return a @code{gdb.RegisterDescriptorIterator} (@pxref{Registers In
> diff --git a/gdb/python/py-arch.c b/gdb/python/py-arch.c
> index f7e35a4aa2b..6aed133eedd 100644
> --- a/gdb/python/py-arch.c
> +++ b/gdb/python/py-arch.c
> @@ -318,6 +318,18 @@ archpy_integer_type (PyObject *self, PyObject *args, PyObject *kw)
>    return type_to_type_object (type);
>  }
>  
> +/* Implementation of gdb.void_type.  */
> +static PyObject *
> +archpy_void_type (PyObject *self, PyObject *args)
> +{
> +  struct gdbarch *gdbarch;
> +  ARCHPY_REQUIRE_VALID (self, gdbarch);
> +
> +  builtin_type (gdbarch);

I don't think this line is needed.

> +
> +  return type_to_type_object (builtin_type (gdbarch)->builtin_void);
> +}
> +
>  /* __repr__ implementation for gdb.Architecture.  */
>  
>  static PyObject *
> @@ -383,6 +395,10 @@ END_PC." },
>      "integer_type (size [, signed]) -> type\n\
>  Return an integer Type corresponding to the given bitsize and signed-ness.\n\
>  If not specified, the type defaults to signed." },
> +  { "void_type", (PyCFunction) archpy_void_type,
> +    METH_NOARGS,
> +    "void_type () -> type\n\
> +Return an void Type." },

Should be: 'Return a void Type.'

With these two fixed:

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew


>    { "registers", (PyCFunction) archpy_registers,
>      METH_VARARGS | METH_KEYWORDS,
>      "registers ([ group-name ]) -> Iterator.\n\
> diff --git a/gdb/testsuite/gdb.python/py-arch.exp b/gdb/testsuite/gdb.python/py-arch.exp
> index 14802ec80a3..c76fc778117 100644
> --- a/gdb/testsuite/gdb.python/py-arch.exp
> +++ b/gdb/testsuite/gdb.python/py-arch.exp
> @@ -104,6 +104,10 @@ foreach_with_prefix test_data { {None None} \
>  	"check 'signed' argument can handle non-bool type $bad_type"
>  }
>  
> +gdb_test "python print(arch.void_type())" \
> +    "void" \
> +    "get void type"
> +
>  # Test for gdb.architecture_names().  First we're going to grab the
>  # complete list of architecture names using the 'complete' command.
>  set arch_names []
> -- 
> 2.45.2
Tom Tromey Dec. 19, 2024, 7:47 p.m. UTC | #3 
>>>>> "Jan" == Jan Vrany <jan.vrany@labware.com> writes:

Jan> +@defun Architecture.void_type ()
Jan> +This function returns a void type.
Jan> +@end defun

I was wondering if we really want a function for this.
In many cases we've preferred properties instead.

Of course the gdb API is already pretty inconsistent so perhaps one more
oddity isn't important.

Also it is sort of weird to think that the void type can be
architecture-specific.  But maybe this is to distinguish different
pointer sizes.  Perhaps the docs should note this?

Jan> +static PyObject *
Jan> +archpy_void_type (PyObject *self, PyObject *args)
Jan> +{
Jan> +  struct gdbarch *gdbarch;
Jan> +  ARCHPY_REQUIRE_VALID (self, gdbarch);
Jan> +
Jan> +  builtin_type (gdbarch);

I think this line is extraneous.

Tom
Jan Vrany Dec. 19, 2024, 11:07 p.m. UTC | #4 
On Thu, 2024-12-19 at 12:47 -0700, Tom Tromey wrote:
> > > > > > "Jan" == Jan Vrany <jan.vrany@labware.com> writes:
> 
> Jan> +@defun Architecture.void_type ()
> Jan> +This function returns a void type.
> Jan> +@end defun
> 
> I was wondering if we really want a function for this.
> In many cases we've preferred properties instead.
> 
> Of course the gdb API is already pretty inconsistent so perhaps one
> more
> oddity isn't important.

Yeah, the existing API seems inconsistent. My reasoning here was
that the other type-creating APIs in gdb.Arch is function so I made
this one too to be consistent. 

> 
> Also it is sort of weird to think that the void type can be
> architecture-specific.  But maybe this is to distinguish different
> pointer sizes.  

Yes, but at the C++ level it looks that one needs to somehow get
references to gdbarch anyway (because we need result of builtin_type
(gdbarch)). So this is either made explicit in the API by putting it
a method (property) into gdb.Arch object or implict by using 
current_inferior()->arch() somewhere down the road. I personally prefer
the former. 

Also later (not in this series though, to keep it as small as possible)
we'll
also need access to floating point types, booleans, maybe decfloats - 
all of this architecture-specific. So from Python API consistency
POV void would be an outlier.

That being said, if you prefer something like gdb.Type.void(), I'm
happy to change it. My understanding of GDB internals is incomplete at
best. 

> Perhaps the docs should note this?

Sorry, I'm not sure what do you mean. Looking at the code, there is
really only one instance of struct type * that represents void. 

Thanks!

Jan

> 
> Jan> +static PyObject *
> Jan> +archpy_void_type (PyObject *self, PyObject *args)
> Jan> +{
> Jan> +  struct gdbarch *gdbarch;
> Jan> +  ARCHPY_REQUIRE_VALID (self, gdbarch);
> Jan> +
> Jan> +  builtin_type (gdbarch);
> 
> I think this line is extraneous.
> 
> Tom
>
Tom Tromey Dec. 20, 2024, 4:16 p.m. UTC | #5 
>>>>> "Jan" == Jan Vraný <Jan.Vrany@labware.com> writes:

>> Perhaps the docs should note this?

Jan> Sorry, I'm not sure what do you mean. Looking at the code, there is
Jan> really only one instance of struct type * that represents void. 

Oh ok.  Just ignore this bit then.

Tom
diff mbox series

Patch

diff --git a/gdb/NEWS b/gdb/NEWS
index 1ea9fcc65b9..765d14a1ae4 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -88,6 +88,9 @@ 
 
   ** Added gdb.Symbol.domain.  Contains the domain of the symbol.
 
+  ** Added gdb.Architecture.void_type. Returns a gdb.Type representing "void"
+     type for that architecture.
+
 * 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 09e374700a4..dac5115a5f8 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -7090,6 +7090,10 @@  If the indicated type cannot be found, this function will throw a
 @code{ValueError} exception.
 @end defun
 
+@defun Architecture.void_type ()
+This function returns a void type.
+@end defun
+
 @anchor{gdbpy_architecture_registers}
 @defun Architecture.registers (@r{[} reggroup @r{]})
 Return a @code{gdb.RegisterDescriptorIterator} (@pxref{Registers In
diff --git a/gdb/python/py-arch.c b/gdb/python/py-arch.c
index f7e35a4aa2b..6aed133eedd 100644
--- a/gdb/python/py-arch.c
+++ b/gdb/python/py-arch.c
@@ -318,6 +318,18 @@  archpy_integer_type (PyObject *self, PyObject *args, PyObject *kw)
   return type_to_type_object (type);
 }
 
+/* Implementation of gdb.void_type.  */
+static PyObject *
+archpy_void_type (PyObject *self, PyObject *args)
+{
+  struct gdbarch *gdbarch;
+  ARCHPY_REQUIRE_VALID (self, gdbarch);
+
+  builtin_type (gdbarch);
+
+  return type_to_type_object (builtin_type (gdbarch)->builtin_void);
+}
+
 /* __repr__ implementation for gdb.Architecture.  */
 
 static PyObject *
@@ -383,6 +395,10 @@  END_PC." },
     "integer_type (size [, signed]) -> type\n\
 Return an integer Type corresponding to the given bitsize and signed-ness.\n\
 If not specified, the type defaults to signed." },
+  { "void_type", (PyCFunction) archpy_void_type,
+    METH_NOARGS,
+    "void_type () -> type\n\
+Return an void Type." },
   { "registers", (PyCFunction) archpy_registers,
     METH_VARARGS | METH_KEYWORDS,
     "registers ([ group-name ]) -> Iterator.\n\
diff --git a/gdb/testsuite/gdb.python/py-arch.exp b/gdb/testsuite/gdb.python/py-arch.exp
index 14802ec80a3..c76fc778117 100644
--- a/gdb/testsuite/gdb.python/py-arch.exp
+++ b/gdb/testsuite/gdb.python/py-arch.exp
@@ -104,6 +104,10 @@  foreach_with_prefix test_data { {None None} \
 	"check 'signed' argument can handle non-bool type $bad_type"
 }
 
+gdb_test "python print(arch.void_type())" \
+    "void" \
+    "get void type"
+
 # Test for gdb.architecture_names().  First we're going to grab the
 # complete list of architecture names using the 'complete' command.
 set arch_names []