[PATCH, RFC] AArch64: Implement software single step

[PATCH, RFC] AArch64: Implement software single step

[Collin May]

This moves the functionality that was previously in the
aarch64_software_single_step function to a new aarch64_deal_with_atomic_sequence
function, and if an atomic sequence is not detected, it will attempt to predict
the next location of the program counter by detecting branch instructions and
predicting their outcomes, much like how arm_get_next_pcs works.

Although AArch64 platforms typically support hardware single step, some kernels
do not. This functionality is useful when interacting with remote targets written
to run under such kernels (and to avoid sending them 's' operations in vCont when
they do not advertise support for the 's' operation).

I've noticed that the arm_software_single_step functionality is largely delegated
to an "arm_get_next_pcs" system that seems to be shared with gdbserver. Since, as
far as I can tell, gdbserver on AArch64 is only intended to run under kernels
that support hardware single step, I don't think there's any need for this code
to be shared with gdbserver.

Finally, one might notice that I haven't written tests for this functionality.
I'm not familiar with gdb's testsuite and would appreciate feedback on how to go
about writing tests for this. I have manually tested that this is working
correctly on a platform that does not support hardware single step.
---
 gdb/aarch64-tdep.c | 133 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 130 insertions(+), 3 deletions(-)

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index bc928e14e9..1b50ec5fba 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -2489,11 +2489,12 @@ value_of_aarch64_user_reg (struct frame_info *frame, const void *baton)
 }
 \f
 
-/* Implement the "software_single_step" gdbarch method, needed to
-   single step through atomic sequences on AArch64.  */
+/* Checks for an atomic sequence of instructions.  If such a sequence
+   is found, attempt to step through it.  The end of the sequence address is
+   added to the next_pcs list.  */
 
 static std::vector<CORE_ADDR>
-aarch64_software_single_step (struct regcache *regcache)
+aarch64_deal_with_atomic_sequence (struct regcache *regcache)
 {
   struct gdbarch *gdbarch = regcache->arch ();
   enum bfd_endian byte_order_for_code = gdbarch_byte_order_for_code (gdbarch);
@@ -2573,6 +2574,132 @@ aarch64_software_single_step (struct regcache *regcache)
   return next_pcs;
 }
 
+/* Returns true if the condition evaluates to true. */
+
+static int
+condition_true (unsigned long cond, unsigned long cpsr)
+{
+  int result = 0;
+
+  int pstate_n = bit (cpsr, 31);
+  int pstate_z = bit (cpsr, 30);
+  int pstate_c = bit (cpsr, 29);
+  int pstate_v = bit (cpsr, 28);
+
+  switch ((cond >> 1) & 3) {
+  case 0:
+    result = pstate_z;
+    break;
+  case 1:
+    result = pstate_c;
+    break;
+  case 2:
+    result = pstate_n;
+    break;
+  case 3:
+    result = pstate_v;
+    break;
+  case 4:
+    result = (pstate_c == 1) && (pstate_z == 0);
+    break;
+  case 5:
+    result = (pstate_n == pstate_v);
+    break;
+  case 6:
+    result = (pstate_n == pstate_v) && (pstate_z == 0);
+    break;
+  case 7:
+    result = 1;
+    break;
+  }
+
+  if ((cond & 1) == 1 && cond != 0xf) {
+    result = !result;
+  }
+
+  return result;
+}
+
+/* Implement the "software_single_step" gdbarch method.  */
+
+static std::vector<CORE_ADDR>
+aarch64_software_single_step (struct regcache *regcache)
+{
+  struct gdbarch *gdbarch = regcache->arch ();
+  enum bfd_endian byte_order_for_code = gdbarch_byte_order_for_code (gdbarch);
+  const int insn_size = 4;
+
+  CORE_ADDR pc = regcache_read_pc (regcache);
+  unsigned long status = regcache_raw_get_unsigned (regcache, AARCH64_CPSR_REGNUM);
+  unsigned long pc_val = (unsigned long) pc;
+  CORE_ADDR branch_addr = (CORE_ADDR) (pc_val + insn_size);	/* Default case */
+
+  uint32_t insn = read_memory_unsigned_integer (pc, insn_size,
+						byte_order_for_code);
+
+  std::vector<CORE_ADDR> next_pcs;
+
+  next_pcs = aarch64_deal_with_atomic_sequence (regcache);
+  if (next_pcs.empty ()) {
+    aarch64_inst inst;
+    if (aarch64_decode_insn (insn, &inst, 1, NULL) != 0)
+      return {};
+
+    /* According to ISA_v82A_A64_xml_00bet3.1, in
+       AArch64 mode, the only things that can touch the
+       PC register are:
+       - the instructions decoded below
+       - AArch64.TakeReset
+       - AArch64.TakeException
+       - AArch64.ExceptionReturn (eret)
+       - ExitDebugState (drps) */
+
+    if (inst.opcode->iclass == condbranch)
+      {
+	/* b.cond */
+	if (condition_true (inst.cond->value, status))
+	  branch_addr = pc + inst.operands[0].imm.value;
+      }
+    else if (inst.opcode->iclass == branch_imm)
+      {
+	/* b, bl */
+	branch_addr = pc + inst.operands[0].imm.value;
+      }
+    else if (inst.opcode->iclass == branch_reg)
+      {
+	/* br, blr, ret */
+	branch_addr = regcache_raw_get_unsigned (regcache,
+						 inst.operands[0].reg.regno);
+      }
+    else if (inst.opcode->iclass == compbranch)
+      {
+	/* cbz, cbnz */
+	ULONGEST reg = regcache_raw_get_unsigned (regcache,
+						  inst.operands[0].reg.regno);
+	int op = bit (insn, 24); // cbz vs cbnz
+
+	if (inst.operands[0].qualifier == AARCH64_OPND_QLF_W) // sf
+	  reg &= 0xffffffff;
+
+	if ((reg == 0) == (op == 0))
+	  branch_addr = pc + inst.operands[1].imm.value;
+      }
+    else if (inst.opcode->iclass == testbranch)
+      {
+	/* tbz, tbnz */
+	ULONGEST reg = regcache_raw_get_unsigned (regcache,
+						  inst.operands[0].reg.regno);
+	int bit_val = bit (insn, 24); // tbz vs tbnz
+	if (bit (reg, inst.operands[1].imm.value) == bit_val)
+	  branch_addr = pc + inst.operands[2].imm.value;
+      }
+
+    next_pcs.push_back (branch_addr);
+  }
+
+  return next_pcs;
+}
+
 struct aarch64_displaced_step_closure : public displaced_step_closure
 {
   /* It is true when condition instruction, such as B.CON, TBZ, etc,
-- 
2.20.1

Re: [PATCH, RFC] AArch64: Implement software single step

[Alan Hayward]

> On 27 Jan 2019, at 22:51, Collin May <collin@collinswebsite.com> wrote:
> 
> This moves the functionality that was previously in the
> aarch64_software_single_step function to a new aarch64_deal_with_atomic_sequence
> function, and if an atomic sequence is not detected, it will attempt to predict
> the next location of the program counter by detecting branch instructions and
> predicting their outcomes, much like how arm_get_next_pcs works.
> 
> Although AArch64 platforms typically support hardware single step, some kernels
> do not. This functionality is useful when interacting with remote targets written
> to run under such kernels (and to avoid sending them 's' operations in vCont when
> they do not advertise support for the 's' operation).

Do you have an example of kernels which do/don’t support it?

> 
> I've noticed that the arm_software_single_step functionality is largely delegated
> to an "arm_get_next_pcs" system that seems to be shared with gdbserver. Since, as
> far as I can tell, gdbserver on AArch64 is only intended to run under kernels
> that support hardware single step, I don't think there's any need for this code
> to be shared with gdbserver.
> 
> Finally, one might notice that I haven't written tests for this functionality.
> I'm not familiar with gdb's testsuite and would appreciate feedback on how to go
> about writing tests for this. I have manually tested that this is working
> correctly on a platform that does not support hardware single step.

To run the test suite, in the gdb/ directory within your build directory, run
“make check -j16 FORCE_PARALLEL=1” (where 16 is your number of cores).

You will get a bunch of failures, so make sure to try it with and without your
patch and compare. Once run, you’ll find all the results in gdb.sum with a full
log in the gdb.log files. The logs are useful because it’ll show you exactly what
was run.

I see a bunch of stuff now failing with this patch, specifically, this now fails:
make check RUNTESTFLAGS="gdb.base/sigstep.exp"

It does seem to fix the issue where gdb steps two instructions after a fork (in
gdb.base/step-over-syscall.exp), which is nice :)

The patch also needs a changelog and need to make sure your FSF copyright
assignment status is done. See:
https://sourceware.org/gdb/wiki/ContributionChecklist

I’ve not looked in depth at the code, but some quick comments below.


> ---
> gdb/aarch64-tdep.c | 133 ++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 130 insertions(+), 3 deletions(-)
> 
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index bc928e14e9..1b50ec5fba 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -2489,11 +2489,12 @@ value_of_aarch64_user_reg (struct frame_info *frame, const void *baton)
> }
> 
> 
> -/* Implement the "software_single_step" gdbarch method, needed to
> -   single step through atomic sequences on AArch64.  */
> +/* Checks for an atomic sequence of instructions.  If such a sequence
> +   is found, attempt to step through it.  The end of the sequence address is
> +   added to the next_pcs list.  */
> 
> static std::vector<CORE_ADDR>
> -aarch64_software_single_step (struct regcache *regcache)
> +aarch64_deal_with_atomic_sequence (struct regcache *regcache)
> {
>   struct gdbarch *gdbarch = regcache->arch ();
>   enum bfd_endian byte_order_for_code = gdbarch_byte_order_for_code (gdbarch);
> @@ -2573,6 +2574,132 @@ aarch64_software_single_step (struct regcache *regcache)
>   return next_pcs;
> }
> 
> +/* Returns true if the condition evaluates to true. */
> +
> +static int
> +condition_true (unsigned long cond, unsigned long cpsr)

Function name is a little too vague.

> +{
> +  int result = 0;
> +
> +  int pstate_n = bit (cpsr, 31);
> +  int pstate_z = bit (cpsr, 30);
> +  int pstate_c = bit (cpsr, 29);
> +  int pstate_v = bit (cpsr, 28);
> +
> +  switch ((cond >> 1) & 3) {
> +  case 0:
> +    result = pstate_z;
> +    break;
> +  case 1:
> +    result = pstate_c;
> +    break;
> +  case 2:
> +    result = pstate_n;
> +    break;
> +  case 3:
> +    result = pstate_v;
> +    break;
> +  case 4:
> +    result = (pstate_c == 1) && (pstate_z == 0);
> +    break;
> +  case 5:
> +    result = (pstate_n == pstate_v);
> +    break;
> +  case 6:
> +    result = (pstate_n == pstate_v) && (pstate_z == 0);
> +    break;
> +  case 7:
> +    result = 1;
> +    break;
> +  }
> +
> +  if ((cond & 1) == 1 && cond != 0xf) {
> +    result = !result;
> +  }
> +
> +  return result;
> +}
> +
> +/* Implement the "software_single_step" gdbarch method.  */
> +
> +static std::vector<CORE_ADDR>
> +aarch64_software_single_step (struct regcache *regcache)
> +{
> +  struct gdbarch *gdbarch = regcache->arch ();
> +  enum bfd_endian byte_order_for_code = gdbarch_byte_order_for_code (gdbarch);
> +  const int insn_size = 4;
> +
> +  CORE_ADDR pc = regcache_read_pc (regcache);
> +  unsigned long status = regcache_raw_get_unsigned (regcache, AARCH64_CPSR_REGNUM);
> +  unsigned long pc_val = (unsigned long) pc;
> +  CORE_ADDR branch_addr = (CORE_ADDR) (pc_val + insn_size);	/* Default case */

Two of the lines here are too long.


> +
> +  uint32_t insn = read_memory_unsigned_integer (pc, insn_size,
> +						byte_order_for_code);
> +
> +  std::vector<CORE_ADDR> next_pcs;
> +
> +  next_pcs = aarch64_deal_with_atomic_sequence (regcache);
> +  if (next_pcs.empty ()) {
> +    aarch64_inst inst;
> +    if (aarch64_decode_insn (insn, &inst, 1, NULL) != 0)
> +      return {};
> +
> +    /* According to ISA_v82A_A64_xml_00bet3.1, in
> +       AArch64 mode, the only things that can touch the
> +       PC register are:
> +       - the instructions decoded below
> +       - AArch64.TakeReset
> +       - AArch64.TakeException
> +       - AArch64.ExceptionReturn (eret)
> +       - ExitDebugState (drps) */
> +
> +    if (inst.opcode->iclass == condbranch)
> +      {
> +	/* b.cond */
> +	if (condition_true (inst.cond->value, status))
> +	  branch_addr = pc + inst.operands[0].imm.value;
> +      }
> +    else if (inst.opcode->iclass == branch_imm)
> +      {
> +	/* b, bl */
> +	branch_addr = pc + inst.operands[0].imm.value;
> +      }
> +    else if (inst.opcode->iclass == branch_reg)
> +      {
> +	/* br, blr, ret */
> +	branch_addr = regcache_raw_get_unsigned (regcache,
> +						 inst.operands[0].reg.regno);

Probably easier using:

regcache->raw_read (inst.operands[0].reg.regno, &branch_addr);


> +      }
> +    else if (inst.opcode->iclass == compbranch)
> +      {
> +	/* cbz, cbnz */
> +	ULONGEST reg = regcache_raw_get_unsigned (regcache,
> +						  inst.operands[0].reg.regno);
> +	int op = bit (insn, 24); // cbz vs cbnz
> +
> +	if (inst.operands[0].qualifier == AARCH64_OPND_QLF_W) // sf
> +	  reg &= 0xffffffff;
> +
> +	if ((reg == 0) == (op == 0))
> +	  branch_addr = pc + inst.operands[1].imm.value;
> +      }
> +    else if (inst.opcode->iclass == testbranch)
> +      {
> +	/* tbz, tbnz */
> +	ULONGEST reg = regcache_raw_get_unsigned (regcache,
> +						  inst.operands[0].reg.regno);
> +	int bit_val = bit (insn, 24); // tbz vs tbnz
> +	if (bit (reg, inst.operands[1].imm.value) == bit_val)
> +	  branch_addr = pc + inst.operands[2].imm.value;
> +      }
> +
> +    next_pcs.push_back (branch_addr);
> +  }
> +
> +  return next_pcs;
> +}
> +
> struct aarch64_displaced_step_closure : public displaced_step_closure
> {
>   /* It is true when condition instruction, such as B.CON, TBZ, etc,
> -- 
> 2.20.1
>