This is the mail archive of the
mailing list for the binutils project.
Re: [PATCH 2/6] Delegate opcodes to select disassembler in GDB
- From: "Maciej W. Rozycki" <macro at imgtec dot com>
- To: Yao Qi <qiyaoltc at gmail dot com>
- Cc: <binutils at sourceware dot org>, <gdb-patches at sourceware dot org>
- Date: Tue, 1 Aug 2017 17:31:58 +0100
- Subject: Re: [PATCH 2/6] Delegate opcodes to select disassembler in GDB
- Authentication-results: sourceware.org; auth=none
- References: <firstname.lastname@example.org> <email@example.com> <alpine.DEB.firstname.lastname@example.org> <email@example.com> <alpine.DEB.firstname.lastname@example.org> <email@example.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
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
> 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