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

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

On Fri, 7 Jul 2017, Yao Qi wrote:

> >  I can see the assumption from the assertion itself, but what is its 
> > purpose?
> >
> 
> The purpose of this assert is to check that the disassemble_info passed
> to opcodes is correctly got from the executable.

 What is there in opcodes that needs guarding with the assertion though, 
i.e. will anything break if the `disassemble_info' passed is not exactly 
one obtained from the executable?

 I'd at least imagine a need for the user to be able to explicitly 
override the disassembler's machine via `set architecture ...', e.g. to 
get a meaningful disassembly of code that has been built with a temporary 
instruction set override for encodings that are not included in the ISA 
recorded in ELF file structures.

> >  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.
> 
> We can pass the symbol table to opcodes, so it can choose the right
> disassembler.  However, current GDB uses both symbol and address to
> determine some address is in a MIPS16 or microMIPS function (done by
> mips_pc_is_mips16, mips_pc_is_micromips).  If we want to use symbol
> table to tell the address is in microMIPS or MIPS16, and GDB can't find
> a symbol for a given address, I suppose we need to create a fake
> symbol, and pass it to disassembler.  Why don't we teach GDB
> unconditionally create a fake symbol with the right bits set in order to
> meet the check in mips-dis.c:is_compressed_mode_p?
> arm-tdep.c:gdb_print_insn_arm has already do so to disassemble Thumb
> instructions.

 We also need symbols for other purposes, specifically telling code and 
data apart in PLT entries, jump tables and constant pools, so a fake 
symbol won't address all cases.  Ideally we'd present output corresponding 
to that produced with `objdump -d', with a way for the user to request 
`objdump -D' equivalent if disassembly of what is supposed to be data is 
required.

> What do you think of the patch below?  IMO, change in
> is_compressed_mode_p fixes a bug on usage of info->num_symbols vs.
> info->symtab_size.  I can't test this patch.

 There is no bug, as it's `info->num_symbols' that tells the disassembler 
the number of symbols starting from `info->symtab_pos' in the symbol table 
affecting the location requested.  Any further symbols are irrelevant (the 
symbol table is sorted by the increasing address, so those will be beyond 
the location disassembled).  You also need to set `info->num_symbols' to 1 
if you're only passing a single symbol in the first place; it's a part of 
this internal API.

 The setting of BSF_SYNTHETIC might be dangerous as code elsewhere assumes 
it's only set for PLT symbols.  Right now it affects `is_mips16_plt_tail', 
causing it to crash, because with your change `asym.section' remains null 
and that function effectively accesses `asym.section->vma'.  I feel uneasy 
about papering it over by adding a check for the section being non-null 
there, I think we need to look for a better solution as this is getting 
too fragile IMO.

 So given that this is a grave regression I think short-term we just need 
to remove the offending assertion checks.

 NB for the purpose of validation I have now bisected the actual 
regression to commit 6394c606997f ("Don't use print_insn_XXX in GDB"), 
rather than commit 39503f82427e ("Delegate opcodes to select disassembler 
in GDB") where the assertion check has been introduced.  There is also 
commit 003ca0fd2286 ("Refactor disassembler selection") on the opcodes' 
side, causing assertion failures triggering once the checks in GDB have 
been removed.  I'll be following up with a fix right away.

 Also in the course of this investigation I have discovered a grave 
regression in compressed breakpoint handling.  I'll be posting a fix 
separately.

  Maciej