[PATCH 0/2] Further disassembler error handling changes

[PATCH 0/2] Further disassembler error handling changes

[Andrew Burgess]

Luis pointed out that some of my recent disassembler changes had
caused some tests to start failing on 32-bit ARM builds of GDB.

The first commit in this series should fix these regressions.

The second commit goes further, and, I think, improves the error
reporting from the disassembler.

I've been struggling to test on 32-bit ARM, my local board is very
unstable.  I'm reasonably sure this series should fix the regressions,
but it would be great if someone (Luis?) could confirm this has fixed
the failures seen on 32-bit ARM for the
gdb.base/all-architectures*.exp tests.

All feedback welcome,

Thanks,
Andrew


---

Andrew Burgess (2):
  gdb: fix disassembler regressions for 32-bit arm
  gdb: use SCOPE_EXIT to write out disassembler instructions

 gdb/disasm.c                                  | 30 +++++++++++++++----
 .../gdb.base/all-architectures.exp.tcl        | 30 ++++++++++++++-----
 2 files changed, 46 insertions(+), 14 deletions(-)

-- 
2.25.4

[PATCH 1/2] gdb: fix disassembler regressions for 32-bit arm

[Andrew Burgess]

After this commit:

  commit 76b43c9b5c2b275cbf4f927bfc25984410cb5dd5
  Date:   Tue Oct 5 15:10:12 2021 +0100

      gdb: improve error reporting from the disassembler

We started seeing FAILs in the gdb.base/all-architectures*.exp tests,
when running on a 32-bit ARM target, though I suspect running on any
target that compiles such that bfd_vma is 32-bits would also trigger
the failures.

The problem is that the test is expected GDB's disassembler to print
an error like this:

  Cannot access memory at address 0x0

However, after the above commit we see an error like:

  unknown disassembler error (error = -1)

The reason for this is this code in opcodes/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;
    }

This code effectively disallows us from ever disassembling 64-bit x86
code if we compiled GDB with a 32-bit bfd_vma.  Notice we return
-1 (indicating a failure to disassemble), but never call the
memory_error_func callback.

Prior to the above commit GDB, when it received the -1 return value
would assume that a memory error had occurred and just print whatever
value happened to be in the memory error address variable, the default
value of 0 just happened to be fine because the test had asked GDB to
do this 'disassemble 0x0,+4'.

If we instead change the test to do 'disassemble 0x100,+4' then GDB
would (previously) have still reported:

  Cannot access memory at address 0x0

which makes far less sense.

In this commit I propose to fix this issue by changing the test to
accept either the "Cannot access memory ..." string, or the newer
"unknown disassembler error ..." string.  With this change done the
test now passes.

However, there is one weakness with this strategy; if GDB broke such
that we _always_ reported "unknown disassembler error ..." we would
never notice.  This clearly would be bad.  To avoid this issue I have
adjusted the all-architectures*.exp tests so that, when we disassemble
for the default architecture (the one selected by "auto") we _only_
expect to get the "Cannot access memory ..." error string.

[ Note: In an ideal world we should be able to disassemble any
  architecture at all times.  There's no reason why the 64-bit x86
  disassembler requires a 64-bit bfd_vma, other than the code happens
  to be written that way.  We could rewrite the disassemble to not
  have this requirement, but, I don't plan to do that any time soon. ]

Further, I have changed the all-architectures*.exp test so that we now
disassemble at address 0x100, this should avoid us being able to pass
by printing a default address of 0x0.  I did originally change the
address we disassembled at to 0x4, however, some architectures,
e.g. ia64, have a default instruction alignment that is greater than
4, so would still round down to 0x0.  I could have just picked 0x8 as
an address, but I figured that 0x100 was likely to satisfy most
architectures alignment requirements.
---
 .../gdb.base/all-architectures.exp.tcl        | 30 ++++++++++++++-----
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/gdb/testsuite/gdb.base/all-architectures.exp.tcl b/gdb/testsuite/gdb.base/all-architectures.exp.tcl
index 967aa108665..c247d13e581 100644
--- a/gdb/testsuite/gdb.base/all-architectures.exp.tcl
+++ b/gdb/testsuite/gdb.base/all-architectures.exp.tcl
@@ -135,6 +135,8 @@ if {[lsearch $supported_archs "arm"] >= 0} {
     gdb_assert {[llength $supported_arm_abi] != 0} "at least one arm abi"
 }
 
+set default_architecture "i386"
+
 # Exercise printing float, double and long double.
 
 proc print_floats {} {
@@ -148,15 +150,27 @@ proc print_floats {} {
     gdb_test_internal "print 1.0f" " = 1" "print, float"
 }
 
-# Run tests on the current architecture.
+# Run tests on the current architecture ARCH.
 
-proc do_arch_tests {} {
+proc do_arch_tests {arch} {
     print_floats
 
+    # When we disassemble using the default architecture then we
+    # expect that the only error we should get from the disassembler
+    # is a memory error.
+    #
+    # When we force the architecture to something other than the
+    # default then we might get the message about unknown errors, this
+    # happens if the libopcodes disassembler returns -1 without first
+    # registering a memory error.
+    set pattern "Cannot access memory at address 0x100"
+    if { $arch != $::default_architecture } {
+	set pattern "(($pattern)|(unknown disassembler error \\(error = -1\\)))"
+    }
+
     # GDB can't access memory because there is no loaded executable
     # nor live inferior.
-    gdb_test_internal "disassemble 0x0,+4" \
-	"Cannot access memory at address 0x0"
+    gdb_test_internal "disassemble 0x100,+4" "${pattern}"
 }
 
 # Given we can't change arch, osabi, endianness, etc. atomically, we
@@ -303,9 +317,9 @@ with_test_prefix "tests" {
 		# Run testing axis CUR_AXIS.  This is a recursive
 		# procedure that tries all combinations of options of
 		# all the testing axes.
-		proc run_axis {all_axes cur_axis} {
+		proc run_axis {all_axes cur_axis arch} {
 		    if {$cur_axis == [llength $all_axes]} {
-			do_arch_tests
+			do_arch_tests $arch
 			return
 		    }
 
@@ -318,12 +332,12 @@ with_test_prefix "tests" {
 		    foreach v $options {
 			with_test_prefix "$var=$v" {
 			    gdb_test_no_output_osabi "$cmd $v" "$cmd"
-			    run_axis $all_axes [expr $cur_axis + 1]
+			    run_axis $all_axes [expr $cur_axis + 1] $arch
 			}
 		    }
 		}
 
-		run_axis $all_axes 0
+		run_axis $all_axes 0 $arch
 	    }
 	}
     }
-- 
2.25.4

[PATCH 2/2] gdb: use SCOPE_EXIT to write out disassembler instructions

[Andrew Burgess]

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 makes use of
SCOPE_EXIT to print the disassembler output.  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.
---
 gdb/disasm.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/gdb/disasm.c b/gdb/disasm.c
index c045dfc94a6..29c85ea1c13 100644
--- a/gdb/disasm.c
+++ b/gdb/disasm.c
@@ -270,7 +270,31 @@ 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,
+       then arrange to write this buffer out, followed by a newline, when
+       we exit this scope.
+
+       If the disassembler returns an error then the print_insn call below
+       will throw an exception.  However, it is possible, that, if the
+       disassembler returns an error it has still written something useful
+       into its output stream, for example, additional details about what
+       caused the error.
+
+       By always printing the instruction buffer, even on exception, we
+       ensure that these additional messages are displayed to the user.  */
     m_insn_stb.clear ();
+    SCOPE_EXIT {
+      m_uiout->field_stream ("inst", m_insn_stb);
+      m_uiout->text ("\n");
+    };
+
+    /* 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);
 
     if (flags & DISASSEMBLY_RAW_INSN)
       {
@@ -282,7 +306,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 +318,7 @@ 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);
   }
-  m_uiout->text ("\n");
 
   return size;
 }
-- 
2.25.4

Re: [PATCH 0/2] Further disassembler error handling changes

[Luis Machado]

On 11/17/21 11:53 AM, Andrew Burgess wrote:
> Luis pointed out that some of my recent disassembler changes had
> caused some tests to start failing on 32-bit ARM builds of GDB.
> 
> The first commit in this series should fix these regressions.
> 
> The second commit goes further, and, I think, improves the error
> reporting from the disassembler.
> 
> I've been struggling to test on 32-bit ARM, my local board is very
> unstable.  I'm reasonably sure this series should fix the regressions,
> but it would be great if someone (Luis?) could confirm this has fixed
> the failures seen on 32-bit ARM for the
> gdb.base/all-architectures*.exp tests.

Absolutely. I'll give this a try. And thanks a lot for going out of your 
way to get these fixed. Sorry I couldn't get to these yet.

Re: [PATCH 0/2] Further disassembler error handling changes

[Luis Machado]

On 11/17/21 11:55 AM, Luis Machado wrote:
> On 11/17/21 11:53 AM, Andrew Burgess wrote:
>> Luis pointed out that some of my recent disassembler changes had
>> caused some tests to start failing on 32-bit ARM builds of GDB.
>>
>> The first commit in this series should fix these regressions.
>>
>> The second commit goes further, and, I think, improves the error
>> reporting from the disassembler.
>>
>> I've been struggling to test on 32-bit ARM, my local board is very
>> unstable.  I'm reasonably sure this series should fix the regressions,
>> but it would be great if someone (Luis?) could confirm this has fixed
>> the failures seen on 32-bit ARM for the
>> gdb.base/all-architectures*.exp tests.
> 
> Absolutely. I'll give this a try. And thanks a lot for going out of your 
> way to get these fixed. Sorry I couldn't get to these yet.

I checked on an armhf system and these two patches fix the FAIL's I was 
seeing. Thanks!

Re: [PATCH 1/2] gdb: fix disassembler regressions for 32-bit arm

[Tom Tromey]

Andrew> The reason for this is this code in opcodes/i386-dis.c (in the
Andrew> print_insn function):

Andrew>   if (address_mode == mode_64bit && sizeof (bfd_vma) < 8)
Andrew>     {
Andrew>       (*info->fprintf_func) (info->stream,
Andrew>                              _("64-bit address is disabled"));
Andrew>       return -1;
Andrew>     }

Note that I worked around this particular problem in commit 9e6978753df
("Avoid self-test failures on x86-linux").  I looked through opcodes and
this was the only scenario like this that I found.

However, I think your patch is also OK.  And perhaps you'd prefer to
revert mine, which I'd also be fine with.

thanks,
Tom

Re: [PATCH 2/2] gdb: use SCOPE_EXIT to write out disassembler instructions

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:

Andrew> +    SCOPE_EXIT {
Andrew> +      m_uiout->field_stream ("inst", m_insn_stb);
Andrew> +      m_uiout->text ("\n");
Andrew> +    };

I'm not sure that this is ok :(

The potential issue is that this could cause the pager to prompt, and
then the user could 'q', causing an exception to be thrown.  However,
SCOPE_EXIT runs its body via a destructor, so an exception here would
then cause gdb to crash.

I suspect an explicit try/catch is the way to go.

Tom

Re: [PATCH 1/2] gdb: fix disassembler regressions for 32-bit arm

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

> Andrew> The reason for this is this code in opcodes/i386-dis.c (in the
> Andrew> print_insn function):
>
> Andrew>   if (address_mode == mode_64bit && sizeof (bfd_vma) < 8)
> Andrew>     {
> Andrew>       (*info->fprintf_func) (info->stream,
> Andrew>                              _("64-bit address is disabled"));
> Andrew>       return -1;
> Andrew>     }
>
> Note that I worked around this particular problem in commit 9e6978753df
> ("Avoid self-test failures on x86-linux").  I looked through opcodes and
> this was the only scenario like this that I found.
>
> However, I think your patch is also OK.  And perhaps you'd prefer to
> revert mine, which I'd also be fine with.

Maybe I'm not understanding something here, but you patch addresses
failures in a totally different test (the self-tests)?

You patch can't fix the gdb.base/all-architectures*.exp failures, and my
patch can't fix the self-test failures...

I did consider trying to create a solution for all-architectures*.exp
thas was modelled on how your patch works, but that would require us to
figure out the bfd_vma size from within a .exp script .... in the end I
just figure it was easier to accept either error message, and add a
specific test that checks for the memory error case.

Thanks,
Andrew

Re: [PATCH 2/2] gdb: use SCOPE_EXIT to write out disassembler instructions

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Andrew> +    SCOPE_EXIT {
> Andrew> +      m_uiout->field_stream ("inst", m_insn_stb);
> Andrew> +      m_uiout->text ("\n");
> Andrew> +    };
>
> I'm not sure that this is ok :(
>
> The potential issue is that this could cause the pager to prompt, and
> then the user could 'q', causing an exception to be thrown.  However,
> SCOPE_EXIT runs its body via a destructor, so an exception here would
> then cause gdb to crash.
>
> I suspect an explicit try/catch is the way to go.

Great catch!

You're absolutely correct, I was able to reproduce this by replacing
this line:

  m_uiout->text ("\n");

with:

  m_uiout->text ("<1>\n<2>\n<3>\n<4>\n<5>\n<6>\n");

then messing with GDB's height setting.  If I set the height small,
trigger a disassembler memory error, then the pager kicks in, and when I
quit, no more GDB.

I've rewritten this work using try/catch as you suggested.

Thanks for the good catch.

Andrew

---

commit 155b57e871bc40ffb26717406f8d5072e022f3b8
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Sat Nov 6 11:25:12 2021 +0000

    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..bd78838dd70 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 ("<1>\n<2>\n<3>\n<4>\n<5>\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;
 }

Re: [PATCH 1/2] gdb: fix disassembler regressions for 32-bit arm

[Tom Tromey]

Andrew> Maybe I'm not understanding something here, but you patch addresses
Andrew> failures in a totally different test (the self-tests)?

Yeah, I was confused about what was going on here, sorry.

Andrew> I did consider trying to create a solution for all-architectures*.exp
Andrew> thas was modelled on how your patch works, but that would require us to
Andrew> figure out the bfd_vma size from within a .exp script .... in the end I
Andrew> just figure it was easier to accept either error message, and add a
Andrew> specific test that checks for the memory error case.

Makes sense.

Tom

Re: [PATCH 1/2] gdb: fix disassembler regressions for 32-bit arm

[Andrew Burgess]

* Tom Tromey <tom@tromey.com> [2021-11-18 10:47:58 -0700]:

> Andrew> Maybe I'm not understanding something here, but you patch addresses
> Andrew> failures in a totally different test (the self-tests)?
> 
> Yeah, I was confused about what was going on here, sorry.
> 
> Andrew> I did consider trying to create a solution for all-architectures*.exp
> Andrew> thas was modelled on how your patch works, but that would require us to
> Andrew> figure out the bfd_vma size from within a .exp script .... in the end I
> Andrew> just figure it was easier to accept either error message, and add a
> Andrew> specific test that checks for the memory error case.
> 
> Makes sense.

Thanks, I've pushed patch #1 of this series.  The updated patch #2
still needs a review.

Thanks,
Andrew

Ping: [PATCH 2/2] gdb: use SCOPE_EXIT to write out disassembler instructions

[Andrew Burgess]

Ping!

Thanks,
Andrew

* Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> [2021-11-18 17:36:49 +0000]:

> Tom Tromey <tom@tromey.com> writes:
> 
> >>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:
> >
> > Andrew> +    SCOPE_EXIT {
> > Andrew> +      m_uiout->field_stream ("inst", m_insn_stb);
> > Andrew> +      m_uiout->text ("\n");
> > Andrew> +    };
> >
> > I'm not sure that this is ok :(
> >
> > The potential issue is that this could cause the pager to prompt, and
> > then the user could 'q', causing an exception to be thrown.  However,
> > SCOPE_EXIT runs its body via a destructor, so an exception here would
> > then cause gdb to crash.
> >
> > I suspect an explicit try/catch is the way to go.
> 
> Great catch!
> 
> You're absolutely correct, I was able to reproduce this by replacing
> this line:
> 
>   m_uiout->text ("\n");
> 
> with:
> 
>   m_uiout->text ("<1>\n<2>\n<3>\n<4>\n<5>\n<6>\n");
> 
> then messing with GDB's height setting.  If I set the height small,
> trigger a disassembler memory error, then the pager kicks in, and when I
> quit, no more GDB.
> 
> I've rewritten this work using try/catch as you suggested.
> 
> Thanks for the good catch.
> 
> Andrew
> 
> ---
> 
> commit 155b57e871bc40ffb26717406f8d5072e022f3b8
> Author: Andrew Burgess <aburgess@redhat.com>
> Date:   Sat Nov 6 11:25:12 2021 +0000
> 
>     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..bd78838dd70 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 ("<1>\n<2>\n<3>\n<4>\n<5>\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;
>  }
> 

Re: Ping: [PATCH 2/2] gdb: use SCOPE_EXIT to write out disassembler instructions

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:

Andrew> Ping!
Andrew> Thanks,
Andrew> Andrew

Sorry about the delay.  I think this looks good.

Tom

Re: Ping: [PATCH 2/2] gdb: use SCOPE_EXIT to write out disassembler instructions

[Andrew Burgess]

* Tom Tromey <tom@tromey.com> [2021-12-07 12:16:04 -0700]:

> >>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Andrew> Ping!
> Andrew> Thanks,
> Andrew> Andrew
> 
> Sorry about the delay.  I think this looks good.

Thanks, I removed the debug printout I'd accidentally left in the
version I posted, and pushed this patch.  Final version included below
for the record.

Thanks,
Andrew

---

commit a5d8391846052cde835015c894237c799089c8cd
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Sat Nov 6 11:25:12 2021 +0000

    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;
 }