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 F3B153858412 for ; Mon, 24 Oct 2022 17:22:57 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org F3B153858412 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=1666632177; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=sp0JadZzImNNHmsesF4CnN/iPq2DMwY3VHilm97+uy4=; b=QI9l1dGlb43IiqXwYJ4l6NUvAx0UG14VOIBUDm+ZaD02t3J7ZNQC01+XhRY6jvGz8FN5fg yu74XxXoJ8cg0fP6pubgteuhj0yBdH4aN9PiF2XE1CgPq3klhGGOn/DZm9PgWFQjEz1ZBr DkRsnOa6y6C1qjwNnvCH1MPT0WXtiJM= Received: from mail-qv1-f72.google.com (mail-qv1-f72.google.com [209.85.219.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-351-ks0ldgP5NiWQa85QGYdA1A-1; Mon, 24 Oct 2022 13:22:56 -0400 X-MC-Unique: ks0ldgP5NiWQa85QGYdA1A-1 Received: by mail-qv1-f72.google.com with SMTP id g1-20020ad45101000000b004bb5eb9913fso2615738qvp.16 for ; Mon, 24 Oct 2022 10:22:56 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:references :in-reply-to:subject:to:from:x-gm-message-state:from:to:cc:subject :date:message-id:reply-to; bh=xS7Cg+ooptyWNa7kK81T9Hq23tELdbZpnZ/DGtAZunM=; b=KpVgCNLvrP9KUSXj0At9N8ZgGXHOijtISP4xaSGEjYyOmxggPnRX5+zA/VymOeBTcg KCeSe4fCAbGAnLXtsNWmFJsTa62YOBOgxLFwZmiUwgdwh69v5PF/OXo9sLmXWNgqBamN MW2GfuSvtI82cujWh8fKccb4/cvTiRZlhrBYmZRQxEv32+TRPxfs12SEGLiZKFjwEhBs /z4cJAF7vS1b/f/RkvY1bx1OA4NwwzlyXPAqqt+3h2uM+EOt9eTsY74j+RulZLQoD6qN jz6bb8XZGmY6aX70ACCV0pbHIOoL6ulzetEdFmYhvZu58Rv5OMyvOnRTcOuQYUCKxJp9 /Thw== X-Gm-Message-State: ACrzQf1lfo76nBBdMP38zk9xnY7EKpc1ZL5D9Z0eeLfoNnf83iG/kl2l PAR6FzygyIBcWFnwLvzHWGu0RZeU/Ph3SI0uLTFdjBc5VnIoGhwIsDkcupWt5jpBIjzEuAfHweF vdPmwEQv3tCa9M7VTeeKTHA== X-Received: by 2002:a37:42d2:0:b0:6ef:241:f7ad with SMTP id p201-20020a3742d2000000b006ef0241f7admr16807970qka.38.1666632175224; Mon, 24 Oct 2022 10:22:55 -0700 (PDT) X-Google-Smtp-Source: AMsMyM51DW4hPnuDZnH8m2ctPeR1hOUppXNqCks/TWGo42JCLzE08CtvpUvEOp8A8i1wZ+Z/oY653Q== X-Received: by 2002:a37:42d2:0:b0:6ef:241:f7ad with SMTP id p201-20020a3742d2000000b006ef0241f7admr16807947qka.38.1666632174696; Mon, 24 Oct 2022 10:22:54 -0700 (PDT) Received: from localhost ([31.111.84.238]) by smtp.gmail.com with ESMTPSA id f11-20020a05620a408b00b006eeb51bb33dsm331514qko.78.2022.10.24.10.22.54 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 24 Oct 2022 10:22:54 -0700 (PDT) From: Andrew Burgess To: Simon Marchi , gdb-patches@sourceware.org Subject: Re: [PATCH] gdb/python: avoid throwing an exception over libopcodes code In-Reply-To: <499ecf26-0f57-2376-a617-9b6214319b4a@simark.ca> References: <20221024125016.2823862-1-aburgess@redhat.com> <499ecf26-0f57-2376-a617-9b6214319b4a@simark.ca> Date: Mon, 24 Oct 2022 18:22:52 +0100 Message-ID: <87r0yx6sr7.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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: Simon Marchi writes: > On 10/24/22 08:50, Andrew Burgess via Gdb-patches wrote: >> Bug gdb/29712 identifies a problem with the Python disassembler API. >> In some cases GDB will try to throw an exception through the >> libopcodes disassembler code, however, not all targets include >> exception unwind information when compiling C code, for targets that >> don't include this information GDB will terminate when trying to pass >> the exception through libopcodes. >>=20 >> To explain what GDB is trying to do, consider the following trivial >> use of the Python disassembler API: >>=20 >> class ExampleDisassembler(gdb.disassembler.Disassembler): >>=20 >> class MyInfo(gdb.disassembler.DisassembleInfo): >> def __init__(self, info): >> super().__init__(info) >>=20 >> def read_memory(self, length, offset): >> return super().read_memory(length, offset) >>=20 >> def __init__(self): >> super().__init__("ExampleDisassembler") >>=20 >> def __call__(self, info): >> info =3D self.MyInfo(info) >> return gdb.disassembler.builtin_disassemble(info) >>=20 >> This disassembler doesn't add any value, it defers back to GDB to do >> all the actual work, but it serves to allow us to discuss the problem. >>=20 >> The problem occurs when a Python exception is raised by the >> MyInfo.read_memory method. The MyInfo.read_memory method is called >> from the C++ function gdbpy_disassembler::read_memory_func. The C++ >> stack at the point this function is called looks like this: >>=20 >> #0 gdbpy_disassembler::read_memory_func (memaddr=3D4198805, buff=3D0x= 7fff9ab9d2a8 "\220=D3=B9\232\377\177", len=3D1, info=3D0x7fff9ab9d558) at .= ./../src/gdb/python/py-disasm.c:510 >> #1 0x000000000104ba06 in fetch_data (info=3D0x7fff9ab9d558, addr=3D0x= 7fff9ab9d2a9 "=D3=B9\232\377\177") at ../../src/opcodes/i386-dis.c:305 >> #2 0x000000000104badb in ckprefix (ins=3D0x7fff9ab9d100) at ../../src= /opcodes/i386-dis.c:8571 >> #3 0x000000000104e28e in print_insn (pc=3D4198805, info=3D0x7fff9ab9d= 558, intel_syntax=3D-1) at ../../src/opcodes/i386-dis.c:9548 >> #4 0x000000000104f4d4 in print_insn_i386 (pc=3D4198805, info=3D0x7fff= 9ab9d558) at ../../src/opcodes/i386-dis.c:9949 >> #5 0x00000000004fa7ea in default_print_insn (memaddr=3D4198805, info= =3D0x7fff9ab9d558) at ../../src/gdb/arch-utils.c:1033 >> #6 0x000000000094fe5e in i386_print_insn (pc=3D4198805, info=3D0x7fff= 9ab9d558) at ../../src/gdb/i386-tdep.c:4072 >> #7 0x0000000000503d49 in gdbarch_print_insn (gdbarch=3D0x5335560, vma= =3D4198805, info=3D0x7fff9ab9d558) at ../../src/gdb/gdbarch.c:3351 >> #8 0x0000000000bcc8c6 in disasmpy_builtin_disassemble (self=3D0x7f2ab= 07f54d0, args=3D0x7f2ab0789790, kw=3D0x0) at ../../src/gdb/python/py-disasm= .c:324 >>=20 >> ### ... snip lots of frames as we pass through Python itself ... >>=20 >> #22 0x0000000000bcd860 in gdbpy_print_insn (gdbarch=3D0x5335560, memad= dr=3D0x401195, info=3D0x7fff9ab9e3c8) at ../../src/gdb/python/py-disasm.c:7= 83 >> #23 0x00000000008995a5 in ext_lang_print_insn (gdbarch=3D0x5335560, ad= dress=3D0x401195, info=3D0x7fff9ab9e3c8) at ../../src/gdb/extension.c:939 >> #24 0x0000000000741aaa in gdb_print_insn_1 (gdbarch=3D0x5335560, vma= =3D0x401195, info=3D0x7fff9ab9e3c8) at ../../src/gdb/disasm.c:1078 >> #25 0x0000000000741bab in gdb_disassembler::print_insn (this=3D0x7fff9= ab9e3c0, memaddr=3D0x401195, branch_delay_insns=3D0x0) at ../../src/gdb/dis= asm.c:1101 >>=20 >> So gdbpy_disassembler::read_memory_func is called from the libopcodes >> disassembler to read memory, this C++ function then calls into user >> supplied Python code to do the work. >>=20 >> If the user supplied Python code raises an gdb.MemoryError exception >> indicating the memory read failed, this this is fine. The C++ code >> converts this exception back into a return value that libopcodes can >> understand, and returns to libopcodes. >>=20 >> However, if the user supplied Python code raises some other exception, >> what we want is for this exception to propagate through GDB and appear >> as if raised by the call to gdb.disassembler.builtin_disassemble. To >> achieve this, when gdbpy_disassembler::read_memory_func spots an >> unknown Python exception, we must pass the information about this >> exception from frame #0 to frame #8 in the above backtrace. Frame #8 >> is the C++ implementation of gdb.disassembler.builtin_disassemble, and >> so it is this function that we want to re-raise the unknown Python >> exception, so the user can, if they want, catch the exception in their >> code. >>=20 >> The previous mechanism by which the exception was passed was to pack >> the details of the Python exception into a C++ exception, then throw >> the exception from frame #0, and catch the exception in frame #8, >> unpack the details of the Python exception, and re-raise it. >>=20 >> However, this relies on the exception passing through frames #1 to #7, >> some of which are in libopcodes, which is C code, and so, might not be >> compiled with exception support. > > Can we make gdbpy_disassembler::read_memory_func "noexcept", so that if > an exception escapes that function, regardless of the architecture, > there will be an apparent GDB crash? Yes! This was my first thought once I spotted this bug. So, what I originally tried was this, in gdb/disasm.h, I applied this: diff --git a/gdb/disasm.h b/gdb/disasm.h index b7d16744c20..7a217dfad12 100644 --- a/gdb/disasm.h +++ b/gdb/disasm.h @@ -52,7 +52,9 @@ struct gdb_disassemble_info protected: =20 /* Types for the function callbacks within m_di. */ - using read_memory_ftype =3D decltype (disassemble_info::read_memory_func= ); + using read_memory_ftype + =3D int (*) (bfd_vma, bfd_byte *, unsigned int, + struct disassemble_info *) noexcept; using memory_error_ftype =3D decltype (disassemble_info::memory_error_fu= nc); using print_address_ftype =3D decltype (disassemble_info::print_address_= func); using fprintf_ftype =3D decltype (disassemble_info::fprintf_func); What I was trying to do was require that _every_ read memory function that we have in GDB _must_ be noexcept. This makes sense I think - the same issue I ran into here could just as easily crop up elsewhere if we tried to throw an exception. Unfortunately, the above doesn't build, I run into this warning/error: In file included from ../../src/gdb/disasm.c:25: ../../src/gdb/disasm.h: In constructor =E2=80=98gdb_printing_disassembler= ::gdb_printing_disassembler(gdbarch*, ui_file*, gdb_disassemble_info::read_= memory_ftype, gdb_disassemble_info::memory_error_ftype, gdb_disassemble_inf= o::print_address_ftype)=E2=80=99: ../../src/gdb/disasm.h:116:3: error: mangled name for =E2=80=98gdb_printi= ng_disassembler::gdb_printing_disassembler(gdbarch*, ui_file*, gdb_disassem= ble_info::read_memory_ftype, gdb_disassemble_info::memory_error_ftype, gdb_= disassemble_info::print_address_ftype)=E2=80=99 will change in C++17 becaus= e the exception specification is part of a function type [-Werror=3Dnoexcep= t-type] 116 | gdb_printing_disassembler (struct gdbarch *gdbarch, | ^~~~~~~~~~~~~~~~~~~~~~~~~ At this point I figured I couldn't use noexcept in the API like this, so gave up. But I think this was a mistake. I just tried, and turns out I can add noexcept to gdbpy_disassembler::read_memory_func, and everything compiles fine, which, thinking about it more makes perfect sense. The noexcept function is more restrictive, so passing it to a function pointer type that doesn't include noexcept can't hurt. What I'd like to do though it create a single patch that adds noexcept to all the disassembler callbacks throughout GDB in one go. I'll reply to this message once I have that patch ready, but if it's OK, I'll leave that as a separate change to the original patch? > >>=20 >> This commit proposes an alternative solution that does not rely on >> throwing a C++ exception. >>=20 >> When we spot an unhandled Python exception in frame #0, we will store >> the details of this exception within the gdbpy_disassembler object >> currently in use. Then we return to libopcodes a value indicating >> that the memory_read failed. >>=20 >> libopcodes will now continue to disassemble as though that memory read >> failed (with one special case described below), then, when we >> eventually return to disasmpy_builtin_disassemble we check to see if >> there is an exception stored in the gdbpy_disassembler object. If >> there is then this exception can immediately be installed, and then we >> return back to Python, when the user will be able to catch the >> exception. >>=20 >> There is one extra change in gdbpy_disassembler::read_memory_func. >> After the first call that results in an exception being stored on the >> gdbpy_disassembler object, any future calls to the ::read_memory_func >> function will immediately return as if the read failed. This avoids >> any additional calls into user supplied Python code. >>=20 >> My thinking here is that should the first call fail with some unknown >> error, GDB should not keep trying with any additional calls. This >> maintains the illusion that the exception raised from >> MyInfo.read_memory is immediately raised by >> gdb.disassembler.builtin_disassemble. I have no tests for this change >> though - to trigger this issue would rely on a libopcodes disassembler >> that will try to read further memory even after the first failed >> read. I'm not aware of any such disassembler that currently does >> this, but that doesn't mean such a disassembler couldn't exist in the >> future. > > I don't really understand the need for this change. The > read_memory_func interface should mostly be stateless, if you try to > read an invalid address, you get an error. If you then try to read a > valid address, I don't see why that wouldn't work. If the error/exception that occurs the first time is a gdb.MemoryError then I agree with you completely. The question is about any other exception type. Imagine this totally made up nonsense example: class MyInfo(gdb.disassembler.DisassembleInfo): def __init__(self, info): super().__init__(info) def read_memory(self, length, offset): if read_resource_from_somewhere_remote(): return super().read_memory(length, offset) else: raise RemoteResourceUnavailableError() If the disassembler tries to read memory, and for some reason the read_resource_from_somewhere_remote() call fails, we raise a RemoteResourceUnavailableError exception. My expectation (and my desire for the API) is that the RemoteResourceUnavailableError will _immediately_ be re-raised by the corresponding builtin_disassemble call. Without the line you are commenting on though, it is possible that the disassembler will repeatedly call the read_memory method, and so repeatedly raise the RemoteResourceUnavailableError error. I don't know if I'm doing a good job of arguing my position :/ does the above make any sense? > > Apart from that design decision, the code itself looks good to me. > >> @@ -138,6 +160,23 @@ struct gdbpy_disassembler : public gdb_printing_dis= assembler >> memory source object then a pointer to the object is placed in her= e, >> otherwise, this field is nullptr. */ >> PyObject *m_memory_source; >> + >> + /* Move the exception EX into this disassembler object. */ >> + void store_exception (gdbpy_err_fetch &&ex) >> + { >> + if (!m_stored_exception.has_value ()) >> + m_stored_exception.emplace (std::move (ex)); > > When would m_stored_exception already have a value? I'm thinking this > should be an assertion instead: > > gdb_assert (!m_stored_exception.has_value ()); Good spot. This was originally added before I added the: if (dis->has_stored_exception ()) return -1; guard to gdbpy_disassembler::read_memory_func that we are discussing above, and ensured only the first Python exception was captured. Then I started worrying about whether we should even allow the function to be run multiple times, added the guard ... and never updated this. I'll change this to an assert as you suggest. > >> @@ -513,7 +555,8 @@ gdbpy_disassembler::read_memory_func (bfd_vma memadd= r, gdb_byte *buff, >> =09 exception and throw it, this will then be caught in >> =09 disasmpy_builtin_disassemble, at which point the exception will be >> =09 restored. */ >> - throw gdbpy_err_fetch (); >> + dis->store_exception (std::move (gdbpy_err_fetch ())); > > I believe these std::moves are unnecessary. The result of > gdbpy_err_fetch() is already a temporary that will be moved from. Ah, OK. I'll remove these then. Thanks. Updated patch below has the assert, and less std::move. I'll follow up with an extra patch adding the noexcept keywords. Thanks, Andrew --- commit f8a254c1d1c14d2b6b934e300e5b48158f15f23b Author: Andrew Burgess Date: Fri Oct 21 16:20:58 2022 +0100 gdb/python: avoid throwing an exception over libopcodes code =20 Bug gdb/29712 identifies a problem with the Python disassembler API. In some cases GDB will try to throw an exception through the libopcodes disassembler code, however, not all targets include exception unwind information when compiling C code, for targets that don't include this information GDB will terminate when trying to pass the exception through libopcodes. =20 To explain what GDB is trying to do, consider the following trivial use of the Python disassembler API: =20 class ExampleDisassembler(gdb.disassembler.Disassembler): =20 class MyInfo(gdb.disassembler.DisassembleInfo): def __init__(self, info): super().__init__(info) =20 def read_memory(self, length, offset): return super().read_memory(length, offset) =20 def __init__(self): super().__init__("ExampleDisassembler") =20 def __call__(self, info): info =3D self.MyInfo(info) return gdb.disassembler.builtin_disassemble(info) =20 This disassembler doesn't add any value, it defers back to GDB to do all the actual work, but it serves to allow us to discuss the problem. =20 The problem occurs when a Python exception is raised by the MyInfo.read_memory method. The MyInfo.read_memory method is called from the C++ function gdbpy_disassembler::read_memory_func. The C++ stack at the point this function is called looks like this: =20 #0 gdbpy_disassembler::read_memory_func (memaddr=3D4198805, buff=3D0= x7fff9ab9d2a8 "\220=D3=B9\232\377\177", len=3D1, info=3D0x7fff9ab9d558) at = ../../src/gdb/python/py-disasm.c:510 #1 0x000000000104ba06 in fetch_data (info=3D0x7fff9ab9d558, addr=3D0= x7fff9ab9d2a9 "=D3=B9\232\377\177") at ../../src/opcodes/i386-dis.c:305 #2 0x000000000104badb in ckprefix (ins=3D0x7fff9ab9d100) at ../../sr= c/opcodes/i386-dis.c:8571 #3 0x000000000104e28e in print_insn (pc=3D4198805, info=3D0x7fff9ab9= d558, intel_syntax=3D-1) at ../../src/opcodes/i386-dis.c:9548 #4 0x000000000104f4d4 in print_insn_i386 (pc=3D4198805, info=3D0x7ff= f9ab9d558) at ../../src/opcodes/i386-dis.c:9949 #5 0x00000000004fa7ea in default_print_insn (memaddr=3D4198805, info= =3D0x7fff9ab9d558) at ../../src/gdb/arch-utils.c:1033 #6 0x000000000094fe5e in i386_print_insn (pc=3D4198805, info=3D0x7ff= f9ab9d558) at ../../src/gdb/i386-tdep.c:4072 #7 0x0000000000503d49 in gdbarch_print_insn (gdbarch=3D0x5335560, vm= a=3D4198805, info=3D0x7fff9ab9d558) at ../../src/gdb/gdbarch.c:3351 #8 0x0000000000bcc8c6 in disasmpy_builtin_disassemble (self=3D0x7f2a= b07f54d0, args=3D0x7f2ab0789790, kw=3D0x0) at ../../src/gdb/python/py-disas= m.c:324 =20 ### ... snip lots of frames as we pass through Python itself ... =20 #22 0x0000000000bcd860 in gdbpy_print_insn (gdbarch=3D0x5335560, mema= ddr=3D0x401195, info=3D0x7fff9ab9e3c8) at ../../src/gdb/python/py-disasm.c:= 783 #23 0x00000000008995a5 in ext_lang_print_insn (gdbarch=3D0x5335560, a= ddress=3D0x401195, info=3D0x7fff9ab9e3c8) at ../../src/gdb/extension.c:939 #24 0x0000000000741aaa in gdb_print_insn_1 (gdbarch=3D0x5335560, vma= =3D0x401195, info=3D0x7fff9ab9e3c8) at ../../src/gdb/disasm.c:1078 #25 0x0000000000741bab in gdb_disassembler::print_insn (this=3D0x7fff= 9ab9e3c0, memaddr=3D0x401195, branch_delay_insns=3D0x0) at ../../src/gdb/di= sasm.c:1101 =20 So gdbpy_disassembler::read_memory_func is called from the libopcodes disassembler to read memory, this C++ function then calls into user supplied Python code to do the work. =20 If the user supplied Python code raises an gdb.MemoryError exception indicating the memory read failed, this this is fine. The C++ code converts this exception back into a return value that libopcodes can understand, and returns to libopcodes. =20 However, if the user supplied Python code raises some other exception, what we want is for this exception to propagate through GDB and appear as if raised by the call to gdb.disassembler.builtin_disassemble. To achieve this, when gdbpy_disassembler::read_memory_func spots an unknown Python exception, we must pass the information about this exception from frame #0 to frame #8 in the above backtrace. Frame #8 is the C++ implementation of gdb.disassembler.builtin_disassemble, and so it is this function that we want to re-raise the unknown Python exception, so the user can, if they want, catch the exception in their code. =20 The previous mechanism by which the exception was passed was to pack the details of the Python exception into a C++ exception, then throw the exception from frame #0, and catch the exception in frame #8, unpack the details of the Python exception, and re-raise it. =20 However, this relies on the exception passing through frames #1 to #7, some of which are in libopcodes, which is C code, and so, might not be compiled with exception support. =20 This commit proposes an alternative solution that does not rely on throwing a C++ exception. =20 When we spot an unhandled Python exception in frame #0, we will store the details of this exception within the gdbpy_disassembler object currently in use. Then we return to libopcodes a value indicating that the memory_read failed. =20 libopcodes will now continue to disassemble as though that memory read failed (with one special case described below), then, when we eventually return to disasmpy_builtin_disassemble we check to see if there is an exception stored in the gdbpy_disassembler object. If there is then this exception can immediately be installed, and then we return back to Python, when the user will be able to catch the exception. =20 There is one extra change in gdbpy_disassembler::read_memory_func. After the first call that results in an exception being stored on the gdbpy_disassembler object, any future calls to the ::read_memory_func function will immediately return as if the read failed. This avoids any additional calls into user supplied Python code. =20 My thinking here is that should the first call fail with some unknown error, GDB should not keep trying with any additional calls. This maintains the illusion that the exception raised from MyInfo.read_memory is immediately raised by gdb.disassembler.builtin_disassemble. I have no tests for this change though - to trigger this issue would rely on a libopcodes disassembler that will try to read further memory even after the first failed read. I'm not aware of any such disassembler that currently does this, but that doesn't mean such a disassembler couldn't exist in the future. =20 With this change in place the gdb.python/py-disasm.exp test should now pass on AArch64. =20 Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=3D29712 diff --git a/gdb/python/py-disasm.c b/gdb/python/py-disasm.c index c37452fcf72..1d460997831 100644 --- a/gdb/python/py-disasm.c +++ b/gdb/python/py-disasm.c @@ -122,6 +122,28 @@ struct gdbpy_disassembler : public gdb_printing_disass= embler return m_string_file.release (); } =20 + /* If there is a Python exception stored in this disassembler then + restore it (i.e. set the PyErr_* state), clear the exception within + this disassembler, and return true. There must be no current + exception set (i.e. !PyErr_Occurred()) when this function is called, + as any such exception might get lost. + + Otherwise, there is no exception stored in this disassembler, return + false. */ + bool restore_exception () + { + gdb_assert (!PyErr_Occurred ()); + if (m_stored_exception.has_value ()) + { +=09gdbpy_err_fetch ex =3D std::move (*m_stored_exception); +=09m_stored_exception.reset (); +=09ex.restore (); +=09return true; + } + + return false; + } + private: =20 /* Where the disassembler result is written. */ @@ -138,6 +160,25 @@ struct gdbpy_disassembler : public gdb_printing_disass= embler memory source object then a pointer to the object is placed in here, otherwise, this field is nullptr. */ PyObject *m_memory_source; + + /* Move the exception EX into this disassembler object. */ + void store_exception (gdbpy_err_fetch &&ex) + { + /* The only calls to store_exception are from read_memory_func, which + will return early if there's already an exception stored. */ + gdb_assert (!m_stored_exception.has_value ()); + m_stored_exception.emplace (std::move (ex)); + } + + /* Return true if there is an exception stored in this disassembler. */ + bool has_stored_exception () const + { + return m_stored_exception.has_value (); + } + + /* Store a single exception. This is used to pass Python exceptions bac= k + from ::memory_read to disasmpy_builtin_disassemble. */ + gdb::optional m_stored_exception; }; =20 /* Return true if OBJ is still valid, otherwise, return false. A valid OB= J @@ -288,20 +329,15 @@ disasmpy_builtin_disassemble (PyObject *self, PyObjec= t *args, PyObject *kw) the disassembled instruction, or -1 if there was a memory-error encountered while disassembling. See below more more details on handling of -1 return value. */ - int length; - try - { - length =3D gdbarch_print_insn (disasm_info->gdbarch, disasm_info->ad= dress, + int length =3D gdbarch_print_insn (disasm_info->gdbarch, disasm_info->ad= dress, =09=09=09=09 disassembler.disasm_info ()); - } - catch (gdbpy_err_fetch &pyerr) - { - /* Reinstall the Python exception held in PYERR. This clears to -=09 pointers held in PYERR, hence the need to catch as a non-const -=09 reference. */ - pyerr.restore (); - return nullptr; - } + + /* It is possible that, while calling a user overridden memory read + function, a Python exception was raised that couldn't be + translated into a standard memory-error. In this case the first such + exception is stored in the disassembler and restored here. */ + if (disassembler.restore_exception ()) + return nullptr; =20 if (length =3D=3D -1) { @@ -483,6 +519,14 @@ gdbpy_disassembler::read_memory_func (bfd_vma memaddr,= gdb_byte *buff, =3D static_cast (info->application_data); disasm_info_object *obj =3D dis->py_disasm_info (); =20 + /* If a previous read attempt resulted in an exception, then we don't + allow any further reads to succeed. We only do this check for the + read_memory_func as this is the only one the user can hook into, + thus, this check prevents us calling back into user code if a + previous call has already thrown an error. */ + if (dis->has_stored_exception ()) + return -1; + /* The DisassembleInfo.read_memory method expects an offset from the address stored within the DisassembleInfo object; calculate that offset here. */ @@ -513,7 +557,8 @@ gdbpy_disassembler::read_memory_func (bfd_vma memaddr, = gdb_byte *buff, =09 exception and throw it, this will then be caught in =09 disasmpy_builtin_disassemble, at which point the exception will be =09 restored. */ - throw gdbpy_err_fetch (); + dis->store_exception (gdbpy_err_fetch ()); + return -1; } =20 /* Convert the result to a buffer. */ @@ -523,7 +568,8 @@ gdbpy_disassembler::read_memory_func (bfd_vma memaddr, = gdb_byte *buff, { PyErr_Format (PyExc_TypeError, =09=09 _("Result from read_memory is not a buffer")); - throw gdbpy_err_fetch (); + dis->store_exception (gdbpy_err_fetch ()); + return -1; } =20 /* Wrap PY_BUFF so that it is cleaned up correctly at the end of this @@ -536,7 +582,8 @@ gdbpy_disassembler::read_memory_func (bfd_vma memaddr, = gdb_byte *buff, PyErr_Format (PyExc_ValueError, =09=09 _("Buffer returned from read_memory is sized %d instead of the e= xpected %d"), =09=09 py_buff.len, len); - throw gdbpy_err_fetch (); + dis->store_exception (gdbpy_err_fetch ()); + return -1; } =20 /* Copy the data out of the Python buffer and return success. */