gdb/python: rework how the disassembler API reads the result object

This commit is a refactor ahead of the next change which will make
disassembler styling available through the Python API.

Unfortunately, in order to make the styling support available, I think
the easiest solution is to make a very small change to the existing
API.

The current API relies on returning a DisassemblerResult object to
represent each disassembled instruction.  Currently GDB allows the
DisassemblerResult class to be sub-classed, which could mean that a
user tries to override the various attributes that exist on the
DisassemblerResult object.

This commit removes this ability, effectively making the
DisassemblerResult class final.

Though this is a change to the existing API, I'm hoping this isn't
going to cause too many issues:

  - The Python disassembler API was only added in the previous release
    of GDB, so I don't expect it to be widely used yet, and

  - It's not clear to me why a user would need to sub-class the
    DisassemblerResult type, I allowed it in the original patch
    because at the time I couldn't see any reason to NOT allow it.

Having prevented sub-classing I can now rework the tail end of the
gdbpy_print_insn function; instead of pulling the results out of the
DisassemblerResult object by calling back into Python, I now cast the
Python object back to its C++ type (disasm_result_object), and access
the fields directly from there.  In later commits I will be reworking
the disasm_result_object type in order to hold information about the
styled disassembler output.

The tests that dealt with sub-classing DisassemblerResult have been
removed, and a new test that confirms that DisassemblerResult can't be
sub-classed has been added.

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

diff --git a/gdb/NEWS b/gdb/NEWS
index 6aa0d5171f26e165bd12d0ee5d7e68d0918c7c59..ca164257126f94b5088588dfeea94733e5f58b60 100644 (file)
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -170,6 +170,9 @@ info main
      (program-counter) values, and can be used as the frame-id when
      calling gdb.PendingFrame.create_unwind_info.
 
+  ** It is now no longer possible to sub-class the
+     gdb.disassembler.DisassemblerResult type.
+
 *** Changes in GDB 13
 
 * MI version 1 is deprecated, and will be removed in GDB 14.

diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 111359106562d03f47265cf2c4928bce85398081..a906c168373cc35a759c36e9703a426618a788c1 100644 (file)
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -7032,6 +7032,8 @@ instance of this class should be returned from
 @w{@code{Disassembler.__call__}} (@pxref{Disassembler Class}) if an
 instruction was successfully disassembled.
 
+It is not possible to sub-class the @code{DisassemblerResult} class.
+
 The @code{DisassemblerResult} class has the following properties and
 methods:
 

diff --git a/gdb/python/py-disasm.c b/gdb/python/py-disasm.c
index 9c281f6e089cd312e10a9d03dd590f2168c62ae3..f246a09301452a21b88d189f12f5fb86a51e455a 100644 (file)
--- a/gdb/python/py-disasm.c
+++ b/gdb/python/py-disasm.c
@@ -904,43 +904,14 @@ gdbpy_print_insn (struct gdbarch *gdbarch, CORE_ADDR memaddr,
       return gdb::optional<int> (-1);
     }
 
-  /* The call into Python neither raised an exception, or returned None.
-     Check to see if the result looks valid.  */
-  gdbpy_ref<> length_obj (PyObject_GetAttrString (result.get (), "length"));
-  if (length_obj == nullptr)
-    {
-      gdbpy_print_stack ();
-      return gdb::optional<int> (-1);
-    }
-
-  gdbpy_ref<> string_obj (PyObject_GetAttrString (result.get (), "string"));
-  if (string_obj == nullptr)
-    {
-      gdbpy_print_stack ();
-      return gdb::optional<int> (-1);
-    }
-  if (!gdbpy_is_string (string_obj.get ()))
-    {
-      PyErr_SetString (PyExc_TypeError, _("String attribute is not a string."));
-      gdbpy_print_stack ();
-      return gdb::optional<int> (-1);
-    }
-
-  gdb::unique_xmalloc_ptr<char> string
-    = gdbpy_obj_to_string (string_obj.get ());
-  if (string == nullptr)
-    {
-      gdbpy_print_stack ();
-      return gdb::optional<int> (-1);
-    }
-
-  long length;
-  if (!gdb_py_int_as_long (length_obj.get (), &length))
-    {
-      gdbpy_print_stack ();
-      return gdb::optional<int> (-1);
-    }
-
+  /* The result from the Python disassembler has the correct type.  Convert
+     this back to the underlying C++ object and read the state directly
+     from this object.  */
+  struct disasm_result_object *result_obj
+    = (struct disasm_result_object *) result.get ();
+
+  /* Validate the length of the disassembled instruction.  */
+  long length = result_obj->length;
   long max_insn_length = (gdbarch_max_insn_length_p (gdbarch) ?
                          gdbarch_max_insn_length (gdbarch) : INT_MAX);
   if (length <= 0)
@@ -961,7 +932,10 @@ gdbpy_print_insn (struct gdbarch *gdbarch, CORE_ADDR memaddr,
       return gdb::optional<int> (-1);
     }
 
-  if (strlen (string.get ()) == 0)
+  /* Validate the text of the disassembled instruction.  */
+  gdb_assert (result_obj->content != nullptr);
+  std::string string (std::move (result_obj->content->release ()));
+  if (strlen (string.c_str ()) == 0)
     {
       PyErr_SetString (PyExc_ValueError,
                       _("String attribute must not be empty."));
@@ -971,7 +945,7 @@ gdbpy_print_insn (struct gdbarch *gdbarch, CORE_ADDR memaddr,
 
   /* Print the disassembled instruction back to core GDB, and return the
      length of the disassembled instruction.  */
-  info->fprintf_func (info->stream, "%s", string.get ());
+  info->fprintf_func (info->stream, "%s", string.c_str ());
   return gdb::optional<int> (length);
 }
 
@@ -1159,7 +1133,7 @@ PyTypeObject disasm_result_object_type = {
   0,                                           /*tp_getattro*/
   0,                                           /*tp_setattro*/
   0,                                           /*tp_as_buffer*/
-  Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE,    /*tp_flags*/
+  Py_TPFLAGS_DEFAULT,                          /*tp_flags*/
   "GDB object, representing a disassembler result",    /* tp_doc */
   0,                                           /* tp_traverse */
   0,                                           /* tp_clear */

diff --git a/gdb/testsuite/gdb.python/py-disasm.exp b/gdb/testsuite/gdb.python/py-disasm.exp
index 2550e60111e092f9d0277bd04ed03db65a742b6d..5cbf02fc9fe1267c1a86e3be76df82d134335d4d 100644 (file)
--- a/gdb/testsuite/gdb.python/py-disasm.exp
+++ b/gdb/testsuite/gdb.python/py-disasm.exp
@@ -96,9 +96,7 @@ set test_plans \
         [list "ReadMemoryCaughtRuntimeErrorDisassembler" "${addr_pattern}${nop}\r\n.*"] \
         [list "MemorySourceNotABufferDisassembler" "${addr_pattern}Python Exception <class 'TypeError'>: Result from read_memory is not a buffer\r\n\r\n${unknown_error_pattern}"] \
         [list "MemorySourceBufferTooLongDisassembler" "${addr_pattern}Python Exception <class 'ValueError'>: Buffer returned from read_memory is sized $decimal instead of the expected $decimal\r\n\r\n${unknown_error_pattern}"] \
-        [list "ResultOfWrongType" "${addr_pattern}Python Exception <class 'TypeError'>: Result is not a DisassemblerResult.\r\n.*"] \
-        [list "ResultWithInvalidLength" "${addr_pattern}Python Exception <class 'ValueError'>: Invalid length attribute: length must be greater than 0.\r\n.*"] \
-        [list "ResultWithInvalidString" "${addr_pattern}Python Exception <class 'ValueError'>: String attribute must not be empty.\r\n.*"]]
+        [list "ResultOfWrongType" "${addr_pattern}Python Exception <class 'TypeError'>: Result is not a DisassemblerResult.\r\n.*"]]
 
 # Now execute each test plan.
 foreach plan $test_plans {
@@ -217,3 +215,14 @@ with_test_prefix "Bad DisassembleInfo creation" {
             "RuntimeError: DisassembleInfo is no longer valid\\." \
             "Error while executing Python code\\."]
 }
+
+# Test that we can't inherit from the DisassemblerResult class.
+gdb_test_multiline "Sub-class a breakpoint" \
+    "python" "" \
+    "class InvalidResultType(gdb.disassembler.DisassemblerResult):" "" \
+    "   def __init__(self):" "" \
+    "     pass" "" \
+    "end" \
+    [multi_line \
+        "TypeError: type 'gdb\\.disassembler\\.DisassemblerResult' is not an acceptable base type" \
+        "Error while executing Python code\\."]

diff --git a/gdb/testsuite/gdb.python/py-disasm.py b/gdb/testsuite/gdb.python/py-disasm.py
index 435a3bf53391f1ff1b2eb3bc9c49255f5e1b7243..17a7e752935b1386dcc1f08371715ae8468710b0 100644 (file)
--- a/gdb/testsuite/gdb.python/py-disasm.py
+++ b/gdb/testsuite/gdb.python/py-disasm.py
@@ -274,43 +274,6 @@ class ResultOfWrongType(TestDisassembler):
         return self.Blah(1, "ABC")
 
 
-class ResultWrapper(gdb.disassembler.DisassemblerResult):
-    def __init__(self, length, string, length_x=None, string_x=None):
-        super().__init__(length, string)
-        if length_x is None:
-            self.__length = length
-        else:
-            self.__length = length_x
-        if string_x is None:
-            self.__string = string
-        else:
-            self.__string = string_x
-
-    @property
-    def length(self):
-        return self.__length
-
-    @property
-    def string(self):
-        return self.__string
-
-
-class ResultWithInvalidLength(TestDisassembler):
-    """Return a result object with an invalid length."""
-
-    def disassemble(self, info):
-        result = gdb.disassembler.builtin_disassemble(info)
-        return ResultWrapper(result.length, result.string, 0)
-
-
-class ResultWithInvalidString(TestDisassembler):
-    """Return a result object with an empty string."""
-
-    def disassemble(self, info):
-        result = gdb.disassembler.builtin_disassemble(info)
-        return ResultWrapper(result.length, result.string, None, "")
-
-
 class TaggingDisassembler(TestDisassembler):
     """A simple disassembler that just tags the output."""