Bug 29712 - [gdb, aarch64] UNRESOLVED: gdb.python/py-disasm.exp: global_disassembler=ReadMemoryGdbErrorDisassembler: disassemble test
Summary: [gdb, aarch64] UNRESOLVED: gdb.python/py-disasm.exp: global_disassembler=Read...
Status: RESOLVED FIXED
Alias: None
Product: gdb
Classification: Unclassified
Component: python (show other bugs)
Version: HEAD
: P2 normal
Target Milestone: 13.1
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-10-21 10:47 UTC by Tom de Vries
Modified: 2022-12-02 16:39 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments
Possible patch for this problem (2.96 KB, patch)
2022-10-21 20:54 UTC, Andrew Burgess
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tom de Vries 2022-10-21 10:47:53 UTC
On aarch64-linux, I run into:
...
(gdb) PASS: gdb.python/py-disasm.exp: global_disassembler=ReadMemoryGdbErrorDisassembler: python add_global_disassembler(ReadMemoryGdbErrorDisassembler)
disassemble test^M
Dump of assembler code for function test:^M
   0x00000000004005e4 <+0>:     nop^M
terminate called after throwing an instance of 'gdbpy_err_fetch'^M
^M
^M
Fatal signal: Aborted^M
----- Backtrace -----^M
0x55dc4f gdb_internal_backtrace_1^M
        /home/tdevries/gdb/src/gdb/bt-utils.c:122^M
0x55dd0f _Z22gdb_internal_backtracev^M
        /home/tdevries/gdb/src/gdb/bt-utils.c:168^M
0x739383 handle_fatal_signal^M
        /home/tdevries/gdb/src/gdb/event-top.c:964^M
0xffffbe4eb7cb ???^M
0xffffbda48500 ???^M
0xffffbda498a3 ???^M
0xffffbdc74173 ???^M
0xffffbdc71c9b ???^M
0xffffbdc71cff ???^M
0xffffbdc71fe3 ???^M
0x8f1f0f _ZN18gdbpy_disassembler16read_memory_funcEmPhjP16disassemble_info^M
        /home/tdevries/gdb/src/gdb/python/py-disasm.c:516^M
0xbd8753 print_insn_aarch64^M
        /home/tdevries/gdb/src/opcodes/aarch64-dis.c:3982^M
---------------------^M
A fatal error internal to GDB has been detected, further^M
debugging is not possible.  GDB will now terminate.^M
^M
This is a bug, please report it.  For instructions, see:^M
<https://www.gnu.org/software/gdb/bugs/>.^M
^M
ERROR: GDB process no longer exists
GDB process exited with wait status 20369 exp48 0 0 CHILDKILLED SIGABRT SIGABRT
UNRESOLVED: gdb.python/py-disasm.exp: global_disassembler=ReadMemoryGdbErrorDisassembler: disassemble test
...
Comment 1 Tom de Vries 2022-10-21 13:09:38 UTC
I tried to reproduce this using:
...
$ ./build/gdb/gdb -q -batch -ex "set trace-commands on" -x outputs/gdb.python/py-disasm/gdb.in.1
...
but didn't manage because some disasm commands fail, so I tried to prefix all disasm commands using ignore-errors ( https://sourceware.org/pipermail/gdb-patches/2021-May/178990.html ), but found that that still stopped script execution.

I was able to repair this by doing:
...
diff --git a/gdb/disasm.c b/gdb/disasm.c
index de44aac3263..b1ba119db9e 100644
--- a/gdb/disasm.c
+++ b/gdb/disasm.c
@@ -454,7 +454,7 @@ gdb_pretty_print_disassembler::pretty_print_insn (const struct disasm_insn *insn
           object destructor as the write itself might throw an exception
           if the pager kicks in, and the user selects quit.  */
        write_out_insn_buffer ();
-       throw ex;
+       throw;
       }
 
     if ((flags & (DISASSEMBLY_RAW_INSN | DISASSEMBLY_RAW_BYTES)) != 0)
...

It's possible that the ignore-errors code should try harder to catch errors, but this change doesn't look wrong.
Comment 2 Tom de Vries 2022-10-21 13:43:26 UTC
Minimal gdb.in to reproduce:
...
file /home/tdevries/gdb/build/gdb/testsuite/outputs/gdb.python/py-disasm/py-disasm
start
source /home/tdevries/gdb/build/gdb/testsuite/outputs/gdb.python/py-disasm/py-disasm.py
break 22
continue
print/x $pc
python current_pc = 0x4005e8
python remove_all_python_disassemblers()
python add_global_disassembler(ReadMemoryGdbErrorDisassembler)
disassemble test
...
Comment 3 Tom de Vries 2022-10-21 14:11:24 UTC
Well, this is weird.  I apply this patch:
...
diff --git a/gdb/python/py-disasm.c b/gdb/python/py-disasm.c
index c37452fcf72..8d684a98eb9 100644
--- a/gdb/python/py-disasm.c
+++ b/gdb/python/py-disasm.c
@@ -294,12 +294,12 @@ disasmpy_builtin_disassemble (PyObject *self, PyObject *args, PyObject *kw)
       length = gdbarch_print_insn (disasm_info->gdbarch, disasm_info->address,
                                   disassembler.disasm_info ());
     }
-  catch (gdbpy_err_fetch &pyerr)
+  catch (...)
     {
       /* Reinstall the Python exception held in PYERR.  This clears to
         pointers held in PYERR, hence the need to catch as a non-const
         reference.  */
-      pyerr.restore ();
+      //pyerr.restore ();
       return nullptr;
     }
 
...
and do a catch throw, and a bt to see the callstack at the point of the throw:
...
(gdb) bt
#0  0x0000fffff7782f9c in __cxa_throw () from /usr/lib64/libstdc++.so.6
#1  0x00000000008f1e1c in gdbpy_disassembler::read_memory_func (memaddr=4195816, 
    buff=0xffffffffd1d8 "\370\353d", len=4, info=0xffffffffd350)
    at /home/tdevries/gdb/src/gdb/python/py-disasm.c:516
#2  0x0000000000bd8660 in print_insn_aarch64 (pc=4195816, info=0xffffffffd350)
    at /home/tdevries/gdb/src/opcodes/aarch64-dis.c:3982
#3  0x00000000004a8ad8 in default_print_insn (memaddr=4195816, info=0xffffffffd350)
    at /home/tdevries/gdb/src/gdb/arch-utils.c:1033
#4  0x0000000000429b5c in aarch64_gdb_print_insn (memaddr=4195816, info=0xffffffffd350)
    at /home/tdevries/gdb/src/gdb/aarch64-tdep.c:2235
#5  0x00000000004b36a8 in gdbarch_print_insn (gdbarch=0x165d810, vma=4195816, 
    info=0xffffffffd350) at /home/tdevries/gdb/src/gdb/gdbarch.c:3351
#6  0x00000000008f163c in disasmpy_builtin_disassemble (
    self=<module at remote 0xfffff40678b8>, args=(<MyInfo at remote 0xffffdec57048>,), 
    kw=0x0) at /home/tdevries/gdb/src/gdb/python/py-disasm.c:294
...

It looks clear that the exception thrown should be caught by the catch clause in  disasmpy_builtin_disassemble, but it doesn't.

Maybe -fexceptions missing on the opcodes file or some such.
Comment 4 Tom de Vries 2022-10-21 14:18:43 UTC
(In reply to Tom de Vries from comment #3)
> Maybe -fexceptions missing on the opcodes file or some such.

Yep, this patch is sufficient to make the test-case pass on aarch64:
...
diff --git a/opcodes/Makefile.in b/opcodes/Makefile.in
index fe4539d6097..03efe198461 100644
--- a/opcodes/Makefile.in
+++ b/opcodes/Makefile.in
@@ -206,11 +206,11 @@ depcomp = $(SHELL) $(top_srcdir)/../depcomp
 am__depfiles_maybe = depfiles
 am__mv = mv -f
 COMPILE = $(CC) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(AM_CPPFLAGS) \
-       $(CPPFLAGS) $(AM_CFLAGS) $(CFLAGS)
+       $(CPPFLAGS) $(AM_CFLAGS) $(CFLAGS) -fexceptions
 LTCOMPILE = $(LIBTOOL) $(AM_V_lt) --tag=CC $(AM_LIBTOOLFLAGS) \
        $(LIBTOOLFLAGS) --mode=compile $(CC) $(DEFS) \
        $(DEFAULT_INCLUDES) $(INCLUDES) $(AM_CPPFLAGS) $(CPPFLAGS) \
-       $(AM_CFLAGS) $(CFLAGS)
+       $(AM_CFLAGS) $(CFLAGS) -fexceptions
 AM_V_CC = $(am__v_CC_@AM_V@)
 am__v_CC_ = $(am__v_CC_@AM_DEFAULT_V@)
 am__v_CC_0 = @echo "  CC      " $@;
...
Comment 5 Andrew Burgess 2022-10-21 20:54:51 UTC
Created attachment 14411 [details]
Possible patch for this problem

Here's my proposal for fixing this issue.  This patch avoids the throw/catch over libopcode.  I've tested this on aarch64 and it seems to fix the issue.  I'm still running the full testsuite.
Comment 6 Tom de Vries 2022-10-22 04:31:37 UTC
(In reply to Andrew Burgess from comment #5)
> Created attachment 14411 [details]
> Possible patch for this problem
> 
> Here's my proposal for fixing this issue.  This patch avoids the throw/catch
> over libopcode.  I've tested this on aarch64 and it seems to fix the issue. 
> I'm still running the full testsuite.

Thanks.

I'll submit the patch from comment 1.
Comment 7 Tom de Vries 2022-10-24 09:17:33 UTC
(In reply to Tom de Vries from comment #6)
> (In reply to Andrew Burgess from comment #5)
> > Created attachment 14411 [details]
> > Possible patch for this problem
> > 
> > Here's my proposal for fixing this issue.  This patch avoids the throw/catch
> > over libopcode.  I've tested this on aarch64 and it seems to fix the issue. 
> > I'm still running the full testsuite.
> 
> Thanks.
> 
> I'll submit the patch from comment 1.

https://sourceware.org/pipermail/gdb-patches/2022-October/193067.html
Comment 8 Andrew Burgess 2022-10-24 12:51:37 UTC
And I posted my part here:

https://sourceware.org/pipermail/gdb-patches/2022-October/193079.html
Comment 9 Joel Brobecker 2022-10-31 18:17:51 UTC
Andrew requested that this be included in the GDB 13 release, and it seems like a good idea, so marking the Target Milestone to 13.1.
Comment 10 Luis Machado 2022-11-02 07:12:16 UTC
Thanks for addressing this.
Comment 11 Sourceware Commits 2022-11-28 19:24:22 UTC
The master branch has been updated by Andrew Burgess <aburgess@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=65639fcc54226c0621d4312efac702c92ddde324

commit 65639fcc54226c0621d4312efac702c92ddde324
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Fri Oct 21 16:20:58 2022 +0100

    gdb/python: avoid throwing an exception over libopcodes code
    
    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 is fine.  The C++ code
    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.
    
    With this change in place the gdb.python/py-disasm.exp test should now
    pass on AArch64.
    
    Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29712
    
    Approved-By: Simon Marchi <simon.marchi@efficios.com>
Comment 12 Sourceware Commits 2022-11-28 19:24:27 UTC
The master branch has been updated by Andrew Burgess <aburgess@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=8eb7d135e32ad6b7cdcfeffe486b195058206cdb

commit 8eb7d135e32ad6b7cdcfeffe486b195058206cdb
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Mon Oct 24 18:35:41 2022 +0100

    gdb/disasm: mark functions passed to the disassembler noexcept
    
    While working on another patch, Simon pointed out that GDB could be
    improved by marking the functions passed to the disassembler as
    noexcept.
    
      https://sourceware.org/pipermail/gdb-patches/2022-October/193084.html
    
    The reason this is important is the on some hosts, libopcodes, being C
    code, will not be compiled with support for handling exceptions.  As
    such, an attempt to throw an exception over libopcodes code will cause
    GDB to terminate.
    
    See bug gdb/29712 for an example of when this happened.
    
    In this commit all the functions that are passed to the disassembler,
    and which might be used as callbacks by libopcodes are marked
    noexcept.
    
    Ideally, I would have liked to change these typedefs:
    
      using read_memory_ftype = decltype (disassemble_info::read_memory_func);
      using memory_error_ftype = decltype (disassemble_info::memory_error_func);
      using print_address_ftype = decltype (disassemble_info::print_address_func);
      using fprintf_ftype = decltype (disassemble_info::fprintf_func);
      using fprintf_styled_ftype = decltype (disassemble_info::fprintf_styled_func);
    
    which are declared in disasm.h, as including the noexcept keyword.
    However, when I tried this, I ran into this warning/error:
    
      In file included from ../../src/gdb/disasm.c:25:
      ../../src/gdb/disasm.h: In constructor âgdb_printing_disassembler::gdb_printing_disassembler(gdbarch*, ui_file*, gdb_disassemble_info::read_memory_ftype, gdb_disassemble_info::memory_error_ftype, gdb_disassemble_info::print_address_ftype)â:
      ../../src/gdb/disasm.h:116:3: error: mangled name for âgdb_printing_disassembler::gdb_printing_disassembler(gdbarch*, ui_file*, gdb_disassemble_info::read_memory_ftype, gdb_disassemble_info::memory_error_ftype, gdb_disassemble_info::print_address_ftype)â will change in C++17 because the exception specification is part of a function type [-Werror=noexcept-type]
        116 |   gdb_printing_disassembler (struct gdbarch *gdbarch,
            |   ^~~~~~~~~~~~~~~~~~~~~~~~~
    
    So I've left that change out.  This does mean that if somebody adds a
    new use of the disassembler classes in the future, and forgets to mark
    the callbacks as noexcept, this will compile fine.  We'll just have to
    manually check for that during review.
Comment 13 Tom Tromey 2022-12-01 18:10:01 UTC
Is this fixed now?
Comment 14 Andrew Burgess 2022-12-02 16:39:56 UTC
Thanks Tom.  Yes this is fixed, I just forgot to close the bug.