This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 2/6] Delegate opcodes to select disassembler in GDB
- From: Yao Qi <qiyaoltc at gmail dot com>
- To: "Maciej W. Rozycki" <macro at imgtec dot com>
- Cc: <binutils at sourceware dot org>, <gdb-patches at sourceware dot org>
- Date: Fri, 07 Jul 2017 17:41:17 +0100
- Subject: Re: [PATCH 2/6] Delegate opcodes to select disassembler in GDB
- Authentication-results: sourceware.org; auth=none
- References: <1494931698-15309-1-git-send-email-yao.qi@linaro.org> <1494931698-15309-3-git-send-email-yao.qi@linaro.org> <alpine.DEB.2.00.1706300107040.31404@tp.orcam.me.uk> <86efu1diwp.fsf@gmail.com> <alpine.DEB.2.00.1707060024070.3339@tp.orcam.me.uk>
"Maciej W. Rozycki" <macro@imgtec.com> writes:
> 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.
> 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. */
>
Yes, that is the point of my patch series.
> 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.
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.
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.
--
Yao (齐尧)
diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
index c1800e4..b848015 100644
--- a/gdb/mips-tdep.c
+++ b/gdb/mips-tdep.c
@@ -7007,14 +7007,31 @@ gdb_print_insn_mips (bfd_vma memaddr, struct disassemble_info *info)
disassembler needs to be able to locally determine the ISA, and
not rely on GDB. Otherwize the stand-alone 'objdump -d' will not
work. */
- if (mips_pc_is_mips16 (gdbarch, memaddr))
- info->mach = bfd_mach_mips16;
- else if (mips_pc_is_micromips (gdbarch, memaddr))
- info->mach = bfd_mach_mips_micromips;
-
- /* Round down the instruction address to the appropriate boundary. */
- memaddr &= (info->mach == bfd_mach_mips16
- || info->mach == bfd_mach_mips_micromips) ? ~1 : ~3;
+ if (mips_pc_is_mips16 (gdbarch, memaddr)
+ || mips_pc_is_micromips (gdbarch, memaddr))
+ {
+ static asymbol asym;
+ static asymbol *asymp = &asym;
+
+ /* Create a fake symbol to tell disassembler in opcodes that
+ the code is MIPS16 or miscroMIPS. */
+ asym.flags = BSF_SYNTHETIC;
+ info->symtab_pos = 0;
+ info->symtab_size = 1;
+ info->symtab = ≈
+ info->symbols = ≈
+
+ if (mips_pc_is_mips16 (gdbarch, memaddr))
+ asym.udata.i = ELF_ST_SET_MIPS16 (asym.udata.i);
+ else
+ asym.udata.i = ELF_ST_SET_MICROMIPS (asym.udata.i);
+
+ /* Round down the instruction address to the appropriate
+ boundary. */
+ memaddr &= ~1;
+ }
+ else
+ memaddr &= ~3;
/* Set the disassembler options. */
if (!info->disassembler_options)
diff --git a/opcodes/mips-dis.c b/opcodes/mips-dis.c
index 4519500..d413841 100644
--- a/opcodes/mips-dis.c
+++ b/opcodes/mips-dis.c
@@ -2452,7 +2452,7 @@ is_compressed_mode_p (struct disassemble_info *info, bfd_boolean micromips_p)
int i;
int l;
- for (i = info->symtab_pos, l = i + info->num_symbols; i < l; i++)
+ for (i = info->symtab_pos, l = info->symtab_size; i < l; i++)
if (((info->symtab[i])->flags & BSF_SYNTHETIC) != 0
&& ((!micromips_p
&& ELF_ST_IS_MIPS16 ((*info->symbols)->udata.i))