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

[Alan Hayward <Alan dot Hayward at arm dot com>]

> 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
>