Re: [PATCH 2/6] Delegate opcodes to select disassembler in GDB

["Maciej W. Rozycki" <macro@imgtec.com>]

Hi Yao,

On Fri, 30 Jun 2017, Yao Qi wrote:

> >  This change causes an assertion failure when trying to disassemble a 
> > MIPS16 function:
> >
> > (gdb) disassemble main
> > Dump of assembler code for function main:
> >    0x00400209 <+0>:
> > .../gdb/arch-utils.c:979: internal-error: int default_print_insn(bfd_vma, disassemble_info*): Assertion `info->mach == bfd_get_mach (exec_bfd)' failed.
> > A problem internal to GDB has been detected,
> > further debugging may prove unreliable.
> > Quit this debugging session? (y or n)
> 
> Sorry about that.  I did deliberately run
> testsuite/gdb.base/all-architectures-[0-7].exp tests to cover the case
> that using disassembler for each architecture.  It covers mips, but it
> doesn't cover mips16 and micromips.

 These are smoke tests really AFAICT, and for this to trigger they would 
have to cover the usual case where the gdbarch's BFD is different from one 
chosen for the disassembly, and for that `set architecture <foo>' is (or 
at least may not be) enough.  You'd really have to run full MIPS 
regression testing with a MIPS16 or microMIPS multilib, and I realise you 
may not have the necessary infrastructure available at hand.

> > This is because `info->mach' is 16 (the `bfd_mach_mips16' aka `mips:16' 
> > BFD) whereas `bfd_get_mach (exec_bfd)' is 33 (the `bfd_mach_mipsisa32r2' 
> > aka `mips:isa32r2' BFD).  This is expected for MIPS16 code within a 
> > program that has been built for the MIPS32r2 ISA; see 
> > `gdb_print_insn_mips' for the origin.
> >
> >  So what's the purpose of this assertion?
> 
> The assertion is based on assumption that "info->mach" is got from
> bfd_get_mach (exec_bfd), but it is not right for mips16.  We can remove
> this line.  If you agree, I'll post a patch to remove this line.

 I can see the assumption from the assertion itself, but what is its 
purpose?

 Normally you place an assertion in code to guard against a case that is 
not handled (correctly) and if let through it would lead execution to go 
astray, e.g. because it is a new complicated feature we have not yet got 
to implementing or because it is a case we think that will never happen, 
and code that follows has assumptions that will not be met.

 So if you say that you can remove the assertion with no adverse effect on 
processing, then I think it should not have been placed there in the first 
place.

 Anyway, if you look at code in `gdb_print_insn_mips', then you'll find 
this comment:

  /* FIXME: cagney/2003-06-26: Is this even necessary?  The
     disassembler needs to be able to locally determine the ISA, and
     not rely on GDB.  Otherwize the stand-alone 'objdump -d' will not
     work.  */

and it is indeed the case that `objdump' handles this correctly without 
the need to switch away from the BFD selected for the binary being 
handled.  However in GDB we have this problem that we do not pass the 
symbol table down to libopcodes for the disassembler, and in the case of 
the MIPS target it is the symbol table that carries information about 
which functions contain regular MIPS code, MIPS16 code and microMIPS code 
respectively.

 Lacking symbol information we resort to this hack of overriding the BFD 
for the purpose of disassembly only, and this has the adverse effect of 
getting instruction subsetting wrong: the `bfd_mach_mips16' and 
`bfd_mach_mips_micromips' BFDs always choose the maximum instruction set 
supported possible whereas the binary handled may only support a subset 
(or worse yet an alternative set), as indicated by the original BFD 
selected.  This in turn may confuse and mislead the person running a debug 
session into thinking code disassembled is not the problem they are after.

 Do you think it would be possible as a part of your disassembler rework 
to make the symbol table available to libopcodes?  Then the hack currently 
present in `gdb_print_insn_mips' could go.  I remember looking into it a 
while ago and concluding it would be somewhat tricky because the symbol 
table format expected by libopcodes is not the same that we use in GDB.

  Maciej