[PATCHv3,1/2] gdb/python: rework how the disassembler API reads the result object
Commit Message
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>
---
gdb/NEWS | 3 ++
gdb/doc/python.texi | 2 +
gdb/python/py-disasm.c | 54 +++++++-------------------
gdb/testsuite/gdb.python/py-disasm.exp | 15 +++++--
gdb/testsuite/gdb.python/py-disasm.py | 37 ------------------
5 files changed, 31 insertions(+), 80 deletions(-)
@@ -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.
@@ -7032,6 +7032,8 @@
@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:
@@ -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 */
@@ -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\\."]
@@ -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."""