Re: [PATCH 0/6] Unify the disassembler selection in gdb and objdump

[Pedro Alves <palves at redhat dot com>]

On 05/16/2017 11:48 AM, Yao Qi wrote:
> Nowadays, we have opcodes library which provides disassembler for each
> architecture (print_insn_XX functions), and gdb, objdump and sim use
> them.  However, they (at least gdb and objdump) select disassembler
> in different places, so this causes some duplicated code and
> inconsistencies.
> 
> This patch series change gdb selecting disassembler through
> opcodes/disassemble.c:disassembler in default, but there are still
> some targets select their disassemblers in their own way (AFAICS,
> they can be changed to disassemble.c:disassembler too in the
> follow up patches).
> 
> Patch #1 refactor the code.  Patch #2 does the major change in this
> series, but only apply to the obvious places.  Patch #3-4 do the
> similar changes too, but not as obvious as patch #2.  Patch #5
> fixes the inconsistency of rs6000 disassembler selection.  As a
> result of these changes, a lot of print_insn_XXX functions are no
> longer needed out side of opcodes library, so patch #6, as an RFC,
> moves these function declarations from include/dis-asm.h to an
> internal header in opcodes/.
> 
> If this series is on the right track, I'll change the rest of gdb
> ports (like arm, mips, aarch64, etc) to default disassembler
> selection.

I like this.

Eliminating objdump / gdb inconsistencies by design is great.

Passing arch/mach to opcodes:disassembler makes it possible to
have gdb tell opcodes to select the right disassembler based on
what arch the remote target description says the target has, even
without a bfd.

The GDB parts look good to me.

> 
> The series is tested for gdb on x86_64-linux and ppc64-linux (gcc110).

Good testing.  I was wondering whether the POWER/gdb change had any
visible effect on the testsuite.
[OTOH, if it didn't, then it sounds like we're missing some kind of
test that exercises default disassembler selection].

> It is also tested for binutils/ld/gas on x86_64-linux with all targets
> enabled.

Thanks,
Pedro Alves