[2/2,aarch64] Use opcodes to decode instructions in GDB
Commit Message
I just noticed that opcodes, at least in aarch64, exposes some good
interface to decode instructions. It is good for GDB to use them
rather than reinventing the wheel.
In this patch, I expose disas_aarch64_insn in opcodes, and use it in
aarch64_software_single_step to decode instructions. If this is a
good way to go, I'll continue using disas_aarch64_insn in other
places such as prologue analysis and even fast tracepoint in GDBserver.
Regression tested GDB for target aarch64-linux-gnu. Is opcodes change OK?
opcodes:
2015-10-01 Yao Qi <yao.qi@linaro.org>
* aarch64-dis.c (disas_aarch64_insn): Make it external. Update
comments.
* aarch64-dis.h (disas_aarch64_insn): Declare it.
gdb:
2015-10-01 Yao Qi <yao.qi@linaro.org>
* aarch64-tdep.c: Include opcodes/aarch64-dis.h.
(submask): Move it above.
(bit): Likewise.
(bits): Likewise.
(aarch64_software_single_step): Call disas_aarch64_insn.
Decode instruction by aarch64_inst instead of using
aarch64_decode_bcond.
---
gdb/aarch64-tdep.c | 29 ++++++++++++++++++-----------
opcodes/aarch64-dis.c | 4 ++--
opcodes/aarch64-dis.h | 5 +++++
3 files changed, 25 insertions(+), 13 deletions(-)
Comments
Hi Yao
On 1 October 2015 at 17:35, Yao Qi <qiyaoltc@gmail.com> wrote:
> In this patch, I expose disas_aarch64_insn in opcodes, and use it in
> aarch64_software_single_step to decode instructions. If this is a
> good way to go, I'll continue using disas_aarch64_insn in other
> places such as prologue analysis and even fast tracepoint in GDBserver.
>
> Regression tested GDB for target aarch64-linux-gnu. Is opcodes change OK?
>
> opcodes:
>
> 2015-10-01 Yao Qi <yao.qi@linaro.org>
>
> * aarch64-dis.c (disas_aarch64_insn): Make it external. Update
> comments.
> * aarch64-dis.h (disas_aarch64_insn): Declare it.
I'll let others comment on the gdb aspect of this patch, w.r.t
opcodes, the aarch64-dis.h header is internal to opcodes. The public
interface to opcodes is exposed via include/opcode/aarch64.h or
include/dis-asm.h. The latter exposes just the cross architecture
disassembler interface so I think includes/opcode/aarch64.h is the
right choice in this case.
Before we expose this function, can we put in a patch to rename it to
following the name space convention used by the other exposed
functions, something like aarch64_disassemble_insn.? I think we should
take the patch to drop the PC argument first, then rename and expose
the function, then the gdb patch to use the interface.
> +/* Decode INSN and fill in *INST the instruction information. Return zero
> + on success. */
> +
> +int disas_aarch64_insn (uint32_t insn, aarch64_inst *inst);
The prototype should drop the formal argument names, irrespective of
which .h file it lands in.
Cheers
/Marcus
Hi Marcus,
On 02/10/15 08:51, Marcus Shawcroft wrote:
> I'll let others comment on the gdb aspect of this patch, w.r.t
> opcodes, the aarch64-dis.h header is internal to opcodes. The public
> interface to opcodes is exposed via include/opcode/aarch64.h or
> include/dis-asm.h. The latter exposes just the cross architecture
> disassembler interface so I think includes/opcode/aarch64.h is the
> right choice in this case.
OK, I move the declaration to includes/opcode/aarch64.h in V2.
>
> Before we expose this function, can we put in a patch to rename it to
> following the name space convention used by the other exposed
> functions, something like aarch64_disassemble_insn.? I think we should
Patch #2 in V2 does this, but renames it to aarch64_decode_insn,
because aarch64_decode_insn can be used in disassemble, but also in
something else.
> take the patch to drop the PC argument first, then rename and expose
> the function, then the gdb patch to use the interface.
OK, in V2, there are three patches. Patch #1 remove the PC argument,
patch #2 exposes and renames disas_aarch64_insn, and patch #3 use it
GDB.
>
>> >+/* Decode INSN and fill in *INST the instruction information. Return zero
>> >+ on success. */
>> >+
>> >+int disas_aarch64_insn (uint32_t insn, aarch64_inst *inst);
> The prototype should drop the formal argument names, irrespective of
> which .h file it lands in.
Fixed in V2.
*** BLURB HERE ***
Yao Qi (3):
[aarch64] Remove argument pc from disas_aarch64_insn
[aarch64] expose disas_aarch64_insn and rename it to
aarch64_decode_insn
[aarch64] use aarch64_decode_insn to decode instructions in GDB
gdb/aarch64-tdep.c | 29 ++++++++++++++++++-----------
include/opcode/aarch64.h | 3 +++
opcodes/aarch64-dis.c | 10 +++++-----
3 files changed, 26 insertions(+), 16 deletions(-)
@@ -59,6 +59,12 @@
#include "arch/aarch64-insn.h"
+#include "opcodes/aarch64-dis.h"
+
+#define submask(x) ((1L << ((x) + 1)) - 1)
+#define bit(obj,st) (((obj) >> (st)) & 1)
+#define bits(obj,st,fn) (((obj) >> (st)) & submask ((fn) - (st)))
+
/* Pseudo register base numbers. */
#define AARCH64_Q0_REGNUM 0
#define AARCH64_D0_REGNUM (AARCH64_Q0_REGNUM + 32)
@@ -2491,35 +2497,40 @@ aarch64_software_single_step (struct frame_info *frame)
int insn_count;
int bc_insn_count = 0; /* Conditional branch instruction count. */
int last_breakpoint = 0; /* Defaults to 0 (no breakpoints placed). */
+ aarch64_inst inst;
+
+ if (disas_aarch64_insn (insn, &inst) != 0)
+ return 0;
/* Look for a Load Exclusive instruction which begins the sequence. */
- if (!decode_masked_match (insn, 0x3fc00000, 0x08400000))
+ if (inst.opcode->iclass != ldstexcl || bit (insn, 22) == 0)
return 0;
for (insn_count = 0; insn_count < atomic_sequence_length; ++insn_count)
{
- int32_t offset;
- unsigned cond;
-
loc += insn_size;
insn = read_memory_unsigned_integer (loc, insn_size,
byte_order_for_code);
+ if (disas_aarch64_insn (insn, &inst) != 0)
+ return 0;
/* Check if the instruction is a conditional branch. */
- if (aarch64_decode_bcond (loc, insn, &cond, &offset))
+ if (inst.opcode->iclass == condbranch)
{
+ gdb_assert (inst.operands[0].type == AARCH64_OPND_ADDR_PCREL19);
+
if (bc_insn_count >= 1)
return 0;
/* It is, so we'll try to set a breakpoint at the destination. */
- breaks[1] = loc + offset;
+ breaks[1] = loc + inst.operands[0].imm.value;
bc_insn_count++;
last_breakpoint++;
}
/* Look for the Store Exclusive which closes the atomic sequence. */
- if (decode_masked_match (insn, 0x3fc00000, 0x08000000))
+ if (inst.opcode->iclass == ldstexcl && bit (insn, 22) == 0)
{
closing_insn = loc;
break;
@@ -2771,10 +2782,6 @@ When on, AArch64 specific debugging is enabled."),
/* AArch64 process record-replay related structures, defines etc. */
-#define submask(x) ((1L << ((x) + 1)) - 1)
-#define bit(obj,st) (((obj) >> (st)) & 1)
-#define bits(obj,st,fn) (((obj) >> (st)) & submask ((fn) - (st)))
-
#define REG_ALLOC(REGS, LENGTH, RECORD_BUF) \
do \
{ \
@@ -2029,9 +2029,9 @@ user_friendly_fixup (aarch64_inst *inst)
}
}
-/* Decode INSN and fill in *INST the instruction information. */
+/* See aarch64-dis.h. */
-static int
+int
disas_aarch64_insn (uint32_t insn, aarch64_inst *inst)
{
const aarch64_opcode *opcode = aarch64_opcode_lookup (insn);
@@ -36,6 +36,11 @@
const aarch64_opcode* aarch64_opcode_lookup (uint32_t);
const aarch64_opcode* aarch64_find_next_opcode (const aarch64_opcode *);
+/* Decode INSN and fill in *INST the instruction information. Return zero
+ on success. */
+
+int disas_aarch64_insn (uint32_t insn, aarch64_inst *inst);
+
/* Given OPCODE, return its alias, e.g. given UBFM, return LSL.
In the case of multiple alias candidates, the one of the highest priority