Re: [PATCHv2 1/3] gdb/python: avoid throwing an exception over libopcodes code

[Simon Marchi <simark@simark.ca>]

On 10/27/22 11:38, Andrew Burgess 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.
> 
> To explain what GDB is trying to do, consider the following trivial
> use of the Python disassembler API:
> 
>   class ExampleDisassembler(gdb.disassembler.Disassembler):
> 
>       class MyInfo(gdb.disassembler.DisassembleInfo):
>           def __init__(self, info):
>               super().__init__(info)
> 
>           def read_memory(self, length, offset):
>               return super().read_memory(length, offset)
> 
>       def __init__(self):
>           super().__init__("ExampleDisassembler")
> 
>       def __call__(self, info):
>           info = self.MyInfo(info)
>           return gdb.disassembler.builtin_disassemble(info)
> 
> 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.
> 
> 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:
> 
>   #0  gdbpy_disassembler::read_memory_func (memaddr=4198805, buff=0x7fff9ab9d2a8 "\220ӹ\232\377\177", len=1, info=0x7fff9ab9d558) at ../../src/gdb/python/py-disasm.c:510
>   #1  0x000000000104ba06 in fetch_data (info=0x7fff9ab9d558, addr=0x7fff9ab9d2a9 "ӹ\232\377\177") at ../../src/opcodes/i386-dis.c:305
>   #2  0x000000000104badb in ckprefix (ins=0x7fff9ab9d100) at ../../src/opcodes/i386-dis.c:8571
>   #3  0x000000000104e28e in print_insn (pc=4198805, info=0x7fff9ab9d558, intel_syntax=-1) at ../../src/opcodes/i386-dis.c:9548
>   #4  0x000000000104f4d4 in print_insn_i386 (pc=4198805, info=0x7fff9ab9d558) at ../../src/opcodes/i386-dis.c:9949
>   #5  0x00000000004fa7ea in default_print_insn (memaddr=4198805, info=0x7fff9ab9d558) at ../../src/gdb/arch-utils.c:1033
>   #6  0x000000000094fe5e in i386_print_insn (pc=4198805, info=0x7fff9ab9d558) at ../../src/gdb/i386-tdep.c:4072
>   #7  0x0000000000503d49 in gdbarch_print_insn (gdbarch=0x5335560, vma=4198805, info=0x7fff9ab9d558) at ../../src/gdb/gdbarch.c:3351
>   #8  0x0000000000bcc8c6 in disasmpy_builtin_disassemble (self=0x7f2ab07f54d0, args=0x7f2ab0789790, kw=0x0) at ../../src/gdb/python/py-disasm.c:324
> 
>   ### ... snip lots of frames as we pass through Python itself ...
> 
>   #22 0x0000000000bcd860 in gdbpy_print_insn (gdbarch=0x5335560, memaddr=0x401195, info=0x7fff9ab9e3c8) at ../../src/gdb/python/py-disasm.c:783
>   #23 0x00000000008995a5 in ext_lang_print_insn (gdbarch=0x5335560, address=0x401195, info=0x7fff9ab9e3c8) at ../../src/gdb/extension.c:939
>   #24 0x0000000000741aaa in gdb_print_insn_1 (gdbarch=0x5335560, vma=0x401195, info=0x7fff9ab9e3c8) at ../../src/gdb/disasm.c:1078
>   #25 0x0000000000741bab in gdb_disassembler::print_insn (this=0x7fff9ab9e3c0, memaddr=0x401195, branch_delay_insns=0x0) at ../../src/gdb/disasm.c:1101
> 
> 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.
> 
> If the user supplied Python code raises an gdb.MemoryError exception
> indicating the memory read failed, this this is fine.  The C++ code

"this this"

> converts this exception back into a return value that libopcodes can
> understand, and returns to libopcodes.
> 
> 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.
> 
> 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.
> 
> 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.
> 
> This commit proposes an alternative solution that does not rely on
> throwing a C++ exception.
> 
> 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.
> 
> 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.
> 
> 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.
> 
> 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.

After understanding this, I'm fine with the idea.   You can add:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon