[RFC] PR 20939: Handle error in disassembly

  Hi,
GDB calls some APIs from opcodes to do disassembly and provide some
call backs.  This model makes troubles on C++ exception unwinding,
because GDB is a C++ program, and opcodes is still compiled as C.
As we can see, frame #10 and #12 are C++, while #frame 11 is C,

 #10 0x0000000000544228 in memory_error (err=TARGET_XFER_E_IO, memaddr=<optimized out>) at ../../binutils-gdb/gdb/corefile.c:237
 #11 0x00000000006b0a54 in print_insn_aarch64 (pc=0, info=0xffffffffeeb0) at ../../binutils-gdb/opcodes/aarch64-dis.c:3185
 #12 0x0000000000553590 in gdb_pretty_print_insn (gdbarch=gdbarch@entry=0xbbceb0, uiout=uiout@entry=0xbc73d0, di=di@entry=0xffffffffeeb0, 
    insn=0xffffffffed40, insn@entry=0xffffffffed90, flags=flags@entry=0, 

C++ exception unwinder can't go across frame #11 unless it has
unwind table.  However, C program on many architectures doesn't
have it in default.  As a result, GDB aborts, which is described
in PR 20939.

This is not the first time we see this kind of problem.  We've
had a commit 89525768cd086a0798a504c81fdf7ebcd4c904e1
"Propagate GDB/C++ exceptions across readline using sj/lj-based TRY/CATCH".
We can fix the disassembly bug in a similar way, this is the option one.

Alternatively, we can do more changes in opcodes, because opcodes is
built together with gdb.  Don't throw exception in dis_asm_memory_error,
and only throw exception if the return value of print_insn_$ARCH is -1
in GDB.  This is the option two, which is demonstrated by the patch
below.  This requires every print_insn_$ARCH function return -1 on
memory error, but msp430 and m68k don't follow this convention yet.

Which option do you prefer?  If we prefer option one, the change is
only within the GDB scope.  If we prefer option two, it goes out to
opcodes, and I'll bring the discussion to binutils.  I prefer this
one.

Note that, no matter which option do we take, the fix should be
backported to 7.12 branch, in which GDB can still be built as a C
program.
  

On 12/12/2016 10:48 AM, Yao Qi wrote:
> Hi,
> GDB calls some APIs from opcodes to do disassembly and provide some
> call backs.  This model makes troubles on C++ exception unwinding,
> because GDB is a C++ program, and opcodes is still compiled as C.
> As we can see, frame #10 and #12 are C++, while #frame 11 is C,
> 
>  #10 0x0000000000544228 in memory_error (err=TARGET_XFER_E_IO, memaddr=<optimized out>) at ../../binutils-gdb/gdb/corefile.c:237
>  #11 0x00000000006b0a54 in print_insn_aarch64 (pc=0, info=0xffffffffeeb0) at ../../binutils-gdb/opcodes/aarch64-dis.c:3185
>  #12 0x0000000000553590 in gdb_pretty_print_insn (gdbarch=gdbarch@entry=0xbbceb0, uiout=uiout@entry=0xbc73d0, di=di@entry=0xffffffffeeb0, 
>     insn=0xffffffffed40, insn@entry=0xffffffffed90, flags=flags@entry=0, 
> 
> C++ exception unwinder can't go across frame #11 unless it has
> unwind table.  However, C program on many architectures doesn't
> have it in default.  As a result, GDB aborts, which is described
> in PR 20939.
> 
> This is not the first time we see this kind of problem.  We've
> had a commit 89525768cd086a0798a504c81fdf7ebcd4c904e1
> "Propagate GDB/C++ exceptions across readline using sj/lj-based TRY/CATCH".
> We can fix the disassembly bug in a similar way, this is the option one.
> 
> Alternatively, we can do more changes in opcodes, because opcodes is
> built together with gdb.  Don't throw exception in dis_asm_memory_error,
> and only throw exception if the return value of print_insn_$ARCH is -1
> in GDB.  This is the option two, which is demonstrated by the patch
> below.  This requires every print_insn_$ARCH function return -1 on
> memory error, but msp430 and m68k don't follow this convention yet.
> 
> Which option do you prefer?  If we prefer option one, the change is
> only within the GDB scope.  If we prefer option two, it goes out to
> opcodes, and I'll bring the discussion to binutils.  I prefer this
> one.

Did you try to find the discussions around when the current
interface based on throwing (using longjmp at the time) was added?
Maybe the "return -1" option was considered back then, but
discarded for some reason?

E.g., looks like simply "return -1" would lose the actual
address that failed to be read, in case opcodes does several
reads in sequence and its not the first that fails.  We could
add some other means to get at that, of course.

> 
> Note that, no matter which option do we take, the fix should be
> backported to 7.12 branch, in which GDB can still be built as a C
> program.
> 

Thanks,
Pedro Alves
  

On 16-12-14 17:31:18, Pedro Alves wrote:
>
> Did you try to find the discussions around when the current
> interface based on throwing (using longjmp at the time) was added?
> Maybe the "return -1" option was considered back then, but
> discarded for some reason?

Such interface was added in 1993,
5d0734a7d74cf01b73303aeb884b719b4b220035 there wasn't any
discussions.

>
> E.g., looks like simply "return -1" would lose the actual
> address that failed to be read, in case opcodes does several
> reads in sequence and its not the first that fails.  We could
> add some other means to get at that, of course.
>

I thought about this beofre.  We can't get the actual address that
failed to be read today, because we read a piece of memory in
dis_asm_read_memory, but only report error from the starting
address.  So I think it isn't very important to report the error with
the accurate memory address failed to be read.