[PATCH] gdb: improve error reporting from the disassembler

[Andrew Burgess]

* Simon Marchi <simark@simark.ca> [2021-10-12 11:23:51 -0400]:

> On 2021-10-11 9:10 a.m., Andrew Burgess wrote:
> > If the libopcodes disassembler returns a negative value then this
> > indicates that the disassembly failed for some reason.  In disas.c, in
> > the function gdb_disassembler::print_insn we can see how this is
> > handled; when we get a negative value back, we call the memory_error
> > function, which throws an exception.
> >
> > The problem here is that the address used in the memory_error call is
> > gdb_disassembler::m_err_memaddr, which is set in
> > gdb_disassembler::dis_asm_memory_error, which is called from within
> > the libopcodes disassembler through the
> > disassembler_info::memory_error_func callback.
> >
> > However, for this to work correctly, every time the libopcodes
> > disassembler returns a negative value, the libopcodes disassembler
> > must have first called the memory_error_func callback.
> >
> > My first plan was to make m_err_memaddr a gdb::optional, and assert
> > that it always had a value prior to calling memory_error, however, a
> > quick look in opcodes/*-dis.c shows that there _are_ cases where a
> > negative value is returned without first calling the memory_error_func
> > callback, for example in arc-dis.c and cris-dis.c.
> >
> > Now, I think that a good argument can be made that these disassemblers
> > must therefore be broken, except for the case where we can't read
> > memory, we should always be able to disassemble the memory contents to
> > _something_, even if it's just '.word 0x....'.  However, I certainly
> > don't plan to go and fix all of the disassemblers.
> >
> > What I do propose to do then, is make m_err_memaddr a gdb::optional,
> > but now, instead of always calling memory_error, I add a new path
> > which just calls error complaining about an unknown error.  This new
> > path is only used if m_err_memaddr doesn't have a value (indicating
> > that the memory_error_func callback was not called).
> >
> > To test this I just augmented one of the disassemblers to always
> > return -1, before this patch I see this:
> >
> >   Dump of assembler code for function main:
> >      0x000101aa <+0>:	Cannot access memory at address 0x0
> >
> > And after this commit I now see:
> >
> >   Dump of assembler code for function main:
> >      0x000101aa <+0>:	unknown disassembler error (error = -1)
> >
> > This doesn't really help much, but that's because there's no way to
> > report non memory errors out of the disasembler, because, it was not
> > expected that the disassembler would ever report non memory errors.
> 
> The patch LGTM since it makes GDB more resilient against the bugs in
> opcodes.  Given infinite time, we would fix opcodes, but this looks like
> a good compromise.

Thanks.  I agree fixing libopcodes would be best in an ideal world,
but it's nice to harden GDB against these things.

> I'm wondering how you stumbled on that, did you actually get bitten by
> opcodes returning -1 but not calling memory_error_func?

No.  I have an incoming patch to add a Python API for the
disassembler.  As part of this work I was reviewing when the
disassembler can return -1.  I initially reasoned that it should only
return -1 following a memory_error, but a quick grep disproved that
idea.

Thanks,
Andrew