From: Andrew Burgess Date: Sat, 6 Nov 2021 11:25:12 +0000 (+0000) Subject: gdb: use try/catch around a gdb_disassembler::print_insn call X-Git-Url: https://git.libre-soc.org/?a=commitdiff_plain;h=a5d8391846052cde835015c894237c799089c8cd;p=binutils-gdb.git gdb: use try/catch around a gdb_disassembler::print_insn call While investigating some disassembler problems I ran into this case; GDB compiled on a 32-bit arm target, with --enable-targets=all. Then in GDB: (gdb) set architecture i386 (gdb) disassemble 0x0,+4 unknown disassembler error (error = -1) This is interesting because it shows a case where the libopcodes disassembler is returning -1 without first calling the memory_error_func callback. Indeed, the return from libopcodes happens from this code snippet in i386-dis.c in the print_insn function: if (address_mode == mode_64bit && sizeof (bfd_vma) < 8) { (*info->fprintf_func) (info->stream, _("64-bit address is disabled")); return -1; } Notice how, prior to the return the disassembler tries to print a helpful message out, but GDB doesn't print this message. The reason this message goes missing is the call stack, it looks like this: gdb_pretty_print_disassembler::pretty_print_insn gdb_disassembler::print_insn gdbarch_print_insn ... i386-dis.c:print_insn When i386-dis.c:print_insn returns -1 this is handled in gdb_disassembler::print_insn, where an exception is thrown. However, the actual printing of the disassembler output is done in gdb_pretty_print_disassembler::pretty_print_insn, and is only done if an exception is not thrown. In this commit I change this. The pretty_print_insn now uses try/catch around the call to gdb_disassembler::print_insn, if we catch an error then we first print any pending output in the instruction buffer, before rethrowing the exception. As a result, even if an exception is thrown we still print any pending disassembler output to the screen; in the above case the helpful message will now be shown. Before my patch we might expect to see this output: (gdb) disassemble 0x0,+4 Dump of assembler code from 0x0 to 0x4: 0x0000000000000000: unknown disassembler error (error = -1) (gdb) But now we see this: (gdb) disassemble 0x0,+4 Dump of assembler code from 0x0 to 0x4: 0x0000000000000000: 64-bit address is disabled unknown disassembler error (error = -1) If the disassembler returns -1 without printing a helpful message then we would still expect a change in output, something like: (gdb) disassemble 0x0,+4 Dump of assembler code from 0x0 to 0x4: 0x0000000000000000: unknown disassembler error (error = -1) Which I think is still acceptable, though at this point I think a strong case can be made that this is a disassembler bug (not printing anything, but still returning -1). Notice however, that the error message is always printed on a new line now. This is also true for the memory error case, where before we might see this: (gdb) disassemble 0x0,+4 Dump of assembler code from 0x0 to 0x4: 0x00000000: Cannot access memory at address 0x0 We now get this: (gdb) disassemble 0x0,+4 Dump of assembler code from 0x0 to 0x4: 0x00000000: Cannot access memory at address 0x0 For me, I'm happy to accept this change, having the error on a line by itself, rather than just appended to the end of the previous line, seems like an improvement, but I'm aware others might feel differently, so I'd appreciate any feedback. --- diff --git a/gdb/disasm.c b/gdb/disasm.c index c045dfc94a6..4d1ee6893f5 100644 --- a/gdb/disasm.c +++ b/gdb/disasm.c @@ -270,8 +270,40 @@ gdb_pretty_print_disassembler::pretty_print_insn (const struct disasm_insn *insn else m_uiout->text (":\t"); + /* Clear the buffer into which we will disassemble the instruction. */ m_insn_stb.clear (); + /* A helper function to write the M_INSN_STB buffer, followed by a + newline. This can be called in a couple of situations. */ + auto write_out_insn_buffer = [&] () + { + m_uiout->field_stream ("inst", m_insn_stb); + m_uiout->text ("\n"); + }; + + try + { + /* Now we can disassemble the instruction. If the disassembler + returns a negative value this indicates an error and is handled + within the print_insn call, resulting in an exception being + thrown. Returning zero makes no sense, as this indicates we + disassembled something successfully, but it was something of no + size? */ + size = m_di.print_insn (pc); + gdb_assert (size > 0); + } + catch (const gdb_exception &ex) + { + /* An exception was thrown while disassembling the instruction. + However, the disassembler might still have written something + out, so ensure that we flush the instruction buffer before + rethrowing the exception. We can't perform this write from an + 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; + } + if (flags & DISASSEMBLY_RAW_INSN) { CORE_ADDR end_pc; @@ -282,7 +314,6 @@ gdb_pretty_print_disassembler::pretty_print_insn (const struct disasm_insn *insn write them out in a single go for the MI. */ m_opcode_stb.clear (); - size = m_di.print_insn (pc); end_pc = pc + size; for (;pc < end_pc; ++pc) @@ -295,12 +326,10 @@ gdb_pretty_print_disassembler::pretty_print_insn (const struct disasm_insn *insn m_uiout->field_stream ("opcodes", m_opcode_stb); m_uiout->text ("\t"); } - else - size = m_di.print_insn (pc); - m_uiout->field_stream ("inst", m_insn_stb); + /* Disassembly was a success, write out the instruction buffer. */ + write_out_insn_buffer (); } - m_uiout->text ("\n"); return size; }