aarch64: detect atomic sequences like other ll/sc architectures

Add similar single-stepping over atomic sequences support like other
load-locked/store-conditional architectures (alpha, powerpc, arm, etc.)
do. Verified the decode_masked_match, and decode_bcond works against the
atomic sequences used in the Linux kernel atomic.h, and also gcc
libatomic. Thanks to Richard Henderson for feedback on my initial
attempt at this patch!

2014-03-23  Kyle McMartin  <kyle@redhat.com>

	* aarch64-tdep.c (aarch64_deal_with_atomic_sequence): New function.
	(aarch64_gdbarch_init): Handle single stepping of atomic sequences
	with aarch64_deal_with_atomic_sequence.

Hi!

On 03/24/2014 04:10 PM, Kyle McMartin wrote:
> Add similar single-stepping over atomic sequences support like other
> load-locked/store-conditional architectures (alpha, powerpc, arm, etc.)
> do. Verified the decode_masked_match, and decode_bcond works against the
> atomic sequences used in the Linux kernel atomic.h, and also gcc
> libatomic. Thanks to Richard Henderson for feedback on my initial
> attempt at this patch!

Thanks!  It'd be nice to have a test in the test suite.  Could you
add one?

PPC64's equivalent seems to be gdb.arch/ppc64-atomic-inst.c|exp.

On Mon, Mar 24, 2014 at 04:24:58PM +0000, Pedro Alves wrote:
> Hi!
> 
> On 03/24/2014 04:10 PM, Kyle McMartin wrote:
> > Add similar single-stepping over atomic sequences support like other
> > load-locked/store-conditional architectures (alpha, powerpc, arm, etc.)
> > do. Verified the decode_masked_match, and decode_bcond works against the
> > atomic sequences used in the Linux kernel atomic.h, and also gcc
> > libatomic. Thanks to Richard Henderson for feedback on my initial
> > attempt at this patch!
> 
> Thanks!  It'd be nice to have a test in the test suite.  Could you
> add one?
> 
> PPC64's equivalent seems to be gdb.arch/ppc64-atomic-inst.c|exp.
> 

Absolutely, I'll work on a set of tests for this today.

--Kyle

Hello Kyle,

> 2014-03-23  Kyle McMartin  <kyle@redhat.com>
> 
> 	* aarch64-tdep.c (aarch64_deal_with_atomic_sequence): New function.
> 	(aarch64_gdbarch_init): Handle single stepping of atomic sequences
> 	with aarch64_deal_with_atomic_sequence.

Some small commments...

The convention for functions implementing gdbarch hooks has been
to name the callback (nearly) the same as the gdbarch hook. In your
case, the function should be called aarch64_software_single_step.

The rest of my comments are mostly Coding Style. GDB follows the GNU
Coding Style with some additional requirements.
https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards

> +static int
> +aarch64_deal_with_atomic_sequence (struct frame_info *frame)

An introductory comment is required for all functions, now.
In this case, since it implements a gdbarch hook which is expected
to be already documented (in gdbarch.sh, gdbarch.h), you only need
to say:

/* Implements the "software_single_step" gdbarch method.  */

I would probably add a comment explaining that you only deal with
atomic sequences in this implementation.  That way, we know why
you had to implement that hook for AArch64, and what its scope is;
and you then won't need to add the comment next to the
set_gdbarch_software_single_step call.

> +{
> +  struct gdbarch *gdbarch = get_frame_arch (frame);
> +  struct address_space *aspace = get_frame_address_space (frame);
> +  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> +  const int insn_size = 4;
> +  const int atomic_sequence_length = 16; /* Instruction sequence length.  */
> +  CORE_ADDR pc = get_frame_pc (frame);
> +  CORE_ADDR breaks[2] = { -1, -1 };
> +  CORE_ADDR loc = pc;
> +  CORE_ADDR closing_insn = 0;
> +  uint32_t insn = read_memory_unsigned_integer (loc, insn_size, byte_order);
> +  int index;
> +  int insn_count;
> +  int bc_insn_count = 0; /* Conditional branch instruction count.  */
> +  int last_breakpoint = 0; /* Defaults to 0 (no breakpoints placed).  */
> +
> +  /* look for a load-exclusive to begin the sequence... */
> +  if (!decode_masked_match(insn, 0x3fc00000, 0x08400000))

Missing space before the '('.

> +    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);
> +
> +      /* look for a conditional branch to set a breakpoint on the destination. */

This line looks too long?

> +      if (decode_bcond(loc, insn, &cond, &offset))

Missing space before '('.

> +	{
> +
> +	  if (bc_insn_count >= 1)
> +	    return 0;
> +
> +	  breaks[1] = loc + offset;
> +
> +	  bc_insn_count++;
> +	  last_breakpoint++;
> +	}
> +
> +      /* and the matching store-exclusive to close it. */
> +      if (decode_masked_match(insn, 0x3fc00000, 0x08000000))

Same here...

> +	{
> +          closing_insn = loc;
> +	  break;
> +	}
> +    }
> +
> +  /* didn't find a stxr to end the sequence... */
> +  if (!closing_insn)
> +    return 0;
> +
> +  loc += insn_size;
> +  insn = read_memory_unsigned_integer (loc, insn_size, byte_order);
> +
> +  /* insert breakpoint at the end of the atomic sequence */
> +  breaks[0] = loc;
> +
> +  /* check for duplicated breakpoints. also check for a breakpoint placed on
> +   * the conditional branch destination isn't within the sequence. */

No '*' on the second line...

> +  if (last_breakpoint &&
> +      (breaks[1] == breaks[0] ||
> +       (breaks[1] >= pc && breaks[1] <= closing_insn)))

Binary operators should be at the start of the next line.

> +    last_breakpoint = 0;
> +
> +  /* insert the breakpoint at the end of the sequence, also possibly at the
> +     conditional branch destination */
> +  for (index = 0; index <= last_breakpoint; index++)
> +    insert_single_step_breakpoint (gdbarch, aspace, breaks[index]);
> +
> +  return 1;
> +}
> +
>  /* Initialize the current architecture based on INFO.  If possible,
>     re-use an architecture from ARCHES, which is a list of
>     architectures already created during this debugging session.
> @@ -2624,6 +2700,8 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>    set_gdbarch_breakpoint_from_pc (gdbarch, aarch64_breakpoint_from_pc);
>    set_gdbarch_cannot_step_breakpoint (gdbarch, 1);
>    set_gdbarch_have_nonsteppable_watchpoint (gdbarch, 1);
> +  /* Handles single stepping of atomic sequences.  */
> +  set_gdbarch_software_single_step (gdbarch, aarch64_deal_with_atomic_sequence);
>  
>    /* Information about registers, etc.  */
>    set_gdbarch_sp_regnum (gdbarch, AARCH64_SP_REGNUM);

Thanks!

On 24/03/14 16:57, Joel Brobecker wrote:
> Hello Kyle,
>> +      /* look for a conditional branch to set a breakpoint on the destination. */
> 
> This line looks too long?

Also, comments are full sentences, so begin with a capital letter and
end with a full stop and two spaces.

>> +      /* and the matching store-exclusive to close it. */
>> +      if (decode_masked_match(insn, 0x3fc00000, 0x08000000))
> 
> Same here...

If you really need a continuation comment like this, then end the
previous one with "..." and start the current one with "... and".
Otherwise, this should be re-written as a stand-alone sentence.

On Mon, Mar 24, 2014 at 06:00:47PM +0000, Richard Earnshaw wrote:
> On 24/03/14 16:57, Joel Brobecker wrote:
> > Hello Kyle,
> >> +      /* look for a conditional branch to set a breakpoint on the destination. */
> > 
> > This line looks too long?
> 
> Also, comments are full sentences, so begin with a capital letter and
> end with a full stop and two spaces.
> 
> >> +      /* and the matching store-exclusive to close it. */
> >> +      if (decode_masked_match(insn, 0x3fc00000, 0x08000000))
> > 
> > Same here...
> 
> If you really need a continuation comment like this, then end the
> previous one with "..." and start the current one with "... and".
> Otherwise, this should be re-written as a stand-alone sentence.
> 

Thanks very much, I've just posted a v2 which I hope addresses your
issues with my patch (and adds a testcase.)

regards, Kyle