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.

1 files changed, 34 insertions, 5 deletions

diff --git a/gdb/disasm.c b/gdb/disasm.c
index c045dfc..4d1ee68 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;
 }