This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] aarch64: detect atomic sequences like other ll/sc architectures


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]