gdb/python: missing PyObject_IsTrue error check in py-arch.c

Building on the previous two commits, I was auditing our uses of
PyObject_IsTrue looking for places where we were missing an error
check.

The gdb.Architecture.integer_type() function takes a 'signed' argument
which should be a 'bool', and the docs do say:

  If SIGNED is not specified, it defaults to 'True'.  If SIGNED is
  'False', the returned type will be unsigned.

Currently we use PyObject_IsTrue, but we are missing an error check.

To fix this I've tightened the code to enforce the bool requirement at
the point that the arguments are parsed.  With that done I can remove
the call to PyObject_IsTrue and instead compare to Py_True directly,
the object in question will always be a PyBool_Type.

However, we were testing that passing a non-bool argument for 'signed'
is treated as Py_False, this was added with this commit:

  commit 90fe61ced1c9aa4afb263326e336330d15603fbf
  Date:   Mon Nov 29 13:53:06 2021 +0000

      gdb/python: don't use the 'p' format for parsing args

which is when the PyObject_IsTrue call was added.  Given that the docs
do seem pretty clear that only True or False are suitable argument
values, my proposal is that we just remove these tests and instead
test that any non-bool argument value for 'signed' gives a TypeError.

This is a breaking change to the Python API, however, my hope is that
this is such a edge case that it will not cause too many problem.
I've added a NEWS entry to highlight this change though.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>
Approved-By: Tom Tromey <tom@tromey.com>

diff --git a/gdb/NEWS b/gdb/NEWS
index 20837bb7b1795e0803056949e3ab880b6e1ba0f2..b96ac5e6b79f11fe58fa7001807eaedfd23e329b 100644 (file)
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -69,6 +69,9 @@
   ** New class gdb.missing_objfile.MissingObjfileHandler which can be
      sub-classed to create handlers for missing objfiles.
 
+  ** The 'signed' argument to gdb.Architecture.integer_type() will no
+     longer accept non-bool types.
+
 * Debugger Adapter Protocol changes
 
   ** The "scopes" request will now return a scope holding global

diff --git a/gdb/python/py-arch.c b/gdb/python/py-arch.c
index d73d7fc48ed483dbe3a08c9d1b0a6b95ffd49cb4..f7e35a4aa2bd6c336db00260698c03d9db92bbc0 100644 (file)
--- a/gdb/python/py-arch.c
+++ b/gdb/python/py-arch.c
@@ -269,15 +269,16 @@ archpy_integer_type (PyObject *self, PyObject *args, PyObject *kw)
 {
   static const char *keywords[] = { "size", "signed", NULL };
   int size;
-  PyObject *is_signed_obj = nullptr;
+  PyObject *is_signed_obj = Py_True;
 
-  if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "i|O", keywords,
-                                       &size, &is_signed_obj))
+  if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "i|O!", keywords,
+                                       &size,
+                                       &PyBool_Type, &is_signed_obj))
     return nullptr;
 
   /* Assume signed by default.  */
-  bool is_signed = (is_signed_obj == nullptr
-                   || PyObject_IsTrue (is_signed_obj));
+  gdb_assert (PyBool_Check (is_signed_obj));
+  bool is_signed = is_signed_obj == Py_True;
 
   struct gdbarch *gdbarch;
   ARCHPY_REQUIRE_VALID (self, gdbarch);

diff --git a/gdb/testsuite/gdb.python/py-arch.exp b/gdb/testsuite/gdb.python/py-arch.exp
index aef4186d0259f3301c9edac842550c2fc185d529..14802ec80a34a7cd75e9fb3edeab4e36160f83bd 100644 (file)
--- a/gdb/testsuite/gdb.python/py-arch.exp
+++ b/gdb/testsuite/gdb.python/py-arch.exp
@@ -70,9 +70,7 @@ if { ![is_address_zero_readable] } {
 foreach size {0 1 2 3 4 8 16} {
     foreach sign_data {{"" True} \
                           {", True" True} \
-                          {", False" False} \
-                          {", None" False} \
-                          {", \"blah\"" True}} {
+                          {", False" False}} {
        set sign [lindex $sign_data 0]
        # GDB's 0 bit type is always signed.
        if { $size == 0 } {
@@ -94,6 +92,18 @@ gdb_test "python arch.integer_type(95)" \
     ".*ValueError.* no integer type of that size is available.*" \
     "call integer_type with invalid size"
 
+foreach_with_prefix test_data { {None None} \
+                                   {"\"blah\"" str} \
+                                   {1 int} } {
+    set bad_sign [lindex $test_data 0]
+    set bad_type [lindex $test_data 1]
+    gdb_test "python arch.integer_type(8, $bad_sign)" \
+       [multi_line \
+            "Python Exception <class 'TypeError'>: argument 2 must be bool, not $bad_type" \
+            "Error occurred in Python: argument 2 must be bool, not $bad_type"] \
+       "check 'signed' argument can handle non-bool type $bad_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 []