From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id B4127385842B for ; Tue, 4 Apr 2023 08:21:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org B4127385842B Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1680596480; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Cj4/Aga101fMKmoCwBCTvqGe3SPK+5TaniKbQqjW7Wg=; b=LD3DUNT1/VkrC+m0OfJBk2l3FFY1wuQIVvY2hAzl5aN9PiU3+RVl8K4YpGYWMprWoQHBaA a3pUTVz6lKbVIGDKg1nZFZ5M3SziyBlqNfHoQcv4qIaJiTaljjqFYvGFKr2d6qm1ffr1Le b27NRKjvXAj7vf+xnYLP9bl9AS4j6fQ= Received: from mail-qt1-f198.google.com (mail-qt1-f198.google.com [209.85.160.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-425-2vWerwRhMauY7olSAJ4tXA-1; Tue, 04 Apr 2023 04:21:19 -0400 X-MC-Unique: 2vWerwRhMauY7olSAJ4tXA-1 Received: by mail-qt1-f198.google.com with SMTP id u1-20020a05622a198100b003e12a0467easo21539260qtc.11 for ; Tue, 04 Apr 2023 01:21:19 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1680596478; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=Cj4/Aga101fMKmoCwBCTvqGe3SPK+5TaniKbQqjW7Wg=; b=JO4IVYP3vvNYnE3iijoJVt5ji7+SgWPMrHTgfntnzpDB9KlJNutnS+4tdLJ5B4nuzT VtferD9mRm5/q6LmSGuSBvzp08LUbm7Y1ratSKaHh/UDGtzArkgKqU+o9Nz8epzmUdMI lO5F7h7g+SGEhFfm3NAAZWMOmKMSV2EEd7Dn50OCW2wBTH0Jng2LXTngs6U50RKOkNTL EsKRXMVM2rbUQJTxrepg9STXQnXyiOwB5WGgALeY+tblIBUR0dX2lMDjxePQwXVALe9U Rwpw3b3EFaWU+7BtFz7TqHwB6N9J+visW97wV71wKnYeKUUl/xBBVkxjGChb/DsjthmM n6Dw== X-Gm-Message-State: AAQBX9dhmgsd7onxVgo1p/nGLYB3y3IeuvVIxq2DRccpbZ3a9Ez0dhRU SspNyGn1mhRvA0c0FHKK2JIlXYmSusYesmdjpWFKBH2YNzqUAA90PJpF/s5jv6RyVx1/cR9uFb9 0yONIGJQu7Maso0DcBJJqx433LIW3EOS7PSqWdKe8+0iBaRK0+9tcRUYg2C6kWJuvALyLPiBxVO WnbhK5uw== X-Received: by 2002:ac8:7e82:0:b0:3e3:9958:5fe8 with SMTP id w2-20020ac87e82000000b003e399585fe8mr2009771qtj.42.1680596478584; Tue, 04 Apr 2023 01:21:18 -0700 (PDT) X-Google-Smtp-Source: AKy350YgcZl/FVL5Nt7b4t83/dvtSs2gEUNkzx/e9j9cO0LFzLDpTfOOhl9g8Q+H4AXiOI10yy6B7Q== X-Received: by 2002:ac8:7e82:0:b0:3e3:9958:5fe8 with SMTP id w2-20020ac87e82000000b003e399585fe8mr2009748qtj.42.1680596478202; Tue, 04 Apr 2023 01:21:18 -0700 (PDT) Received: from localhost (95.72.115.87.dyn.plus.net. [87.115.72.95]) by smtp.gmail.com with ESMTPSA id m124-20020a375882000000b0073b8745fd39sm3407606qkb.110.2023.04.04.01.21.17 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 04 Apr 2023 01:21:18 -0700 (PDT) From: Andrew Burgess To: gdb-patches@sourceware.org Cc: Andrew Burgess Subject: [PATCH 4/5] gdb/python: rework how the disassembler API reads the result object Date: Tue, 4 Apr 2023 09:21:06 +0100 Message-Id: <05065dd898c6efc263bde5697e0c11130b99c902.1680596378.git.aburgess@redhat.com> X-Mailer: git-send-email 2.25.4 In-Reply-To: References: MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="US-ASCII"; x-default=true X-Spam-Status: No, score=-11.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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. --- 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(-) diff --git a/gdb/NEWS b/gdb/NEWS index 10a1a70fa52..d1726842299 100644 --- a/gdb/NEWS +++ b/gdb/NEWS @@ -152,6 +152,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 a350260559f..b0615171363 100644 --- a/gdb/doc/python.texi +++ b/gdb/doc/python.texi @@ -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: diff --git a/gdb/python/py-disasm.c b/gdb/python/py-disasm.c index c61fe70c26b..4b33e20fd41 100644 --- a/gdb/python/py-disasm.c +++ b/gdb/python/py-disasm.c @@ -901,43 +901,14 @@ gdbpy_print_insn (struct gdbarch *gdbarch, CORE_ADDR memaddr, return gdb::optional (-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 (-1); - } - - gdbpy_ref<> string_obj (PyObject_GetAttrString (result.get (), "string")); - if (string_obj == nullptr) - { - gdbpy_print_stack (); - return gdb::optional (-1); - } - if (!gdbpy_is_string (string_obj.get ())) - { - PyErr_SetString (PyExc_TypeError, _("String attribute is not a string.")); - gdbpy_print_stack (); - return gdb::optional (-1); - } - - gdb::unique_xmalloc_ptr string - = gdbpy_obj_to_string (string_obj.get ()); - if (string == nullptr) - { - gdbpy_print_stack (); - return gdb::optional (-1); - } - - long length; - if (!gdb_py_int_as_long (length_obj.get (), &length)) - { - gdbpy_print_stack (); - return gdb::optional (-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) @@ -958,7 +929,10 @@ gdbpy_print_insn (struct gdbarch *gdbarch, CORE_ADDR memaddr, return gdb::optional (-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.")); @@ -968,7 +942,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 (length); } @@ -1152,7 +1126,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 66ed39f048b..faa58b0b918 100644 --- 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 : Result from read_memory is not a buffer\r\n\r\n${unknown_error_pattern}"] \ [list "MemorySourceBufferTooLongDisassembler" "${addr_pattern}Python Exception : 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 : Result is not a DisassemblerResult.\r\n.*"] \ - [list "ResultWithInvalidLength" "${addr_pattern}Python Exception : Invalid length attribute: length must be greater than 0.\r\n.*"] \ - [list "ResultWithInvalidString" "${addr_pattern}Python Exception : String attribute must not be empty.\r\n.*"]] + [list "ResultOfWrongType" "${addr_pattern}Python Exception : 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 435a3bf5339..17a7e752935 100644 --- 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.""" -- 2.25.4