Re: [RFC] "single step" atomic instruction sequences as a whole on PPC

["Ulrich Weigand" <uweigand at de dot ibm dot com>]

Luis Machado wrote:

> I've modified the patch according to your suggestions. Any additional
> comments on it?

Thanks for the modifications.  There's still a number of coding style
issues that I'll point out below.

> Any suggestions for the duplicate breakpoint detection? Maybe it could
> be handled in a better way.

Unfortunately your attempt to handle more than a single conditional
branch within the sequence doesn't work, as insert_single_step_breakpoint
cannot be called more than twice (it'll raise an assertion).

I think you should make the assumption that there will be at most
one conditional branch, but fail gracefully it that assumption is
in fact violated (e.g. by issuing a warning and returning 0 to fall
back on the standard single-step code).

> +static int 
> +deal_with_atomic_sequence(struct regcache *regcache)
Space before '('.

> +{
> +  CORE_ADDR pc = read_pc ();
> +  CORE_ADDR breaks[16] = {-1, -1, -1, -1, -1, -1, -1, -1,
> +                          -1, -1, -1, -1, -1, -1, -1, -1};
> +  CORE_ADDR branch_bp; /* Breakpoint at destination of a banch instruction */
Typo: "banch"
Comment should end with '.' followed by two spaces.

> +  int index; /* Index used for the "breaks" arrays */
> +  int last_breakpoint = 0; /* Defaults to 0 (no breakpoints placed) */  
> +  const int opcode = BC_INSTRUCTION; /* Branch instruction's OPcode */
Likewise.

> +  if ((insn & LWARX_MASK) != LWARX_INSTRUCTION
> +   && (insn & LWARX_MASK) != LDARX_INSTRUCTION)
Indentation wrong.

> +      /* Check for conditiconal branches in the middle of the sequence
Typo "conditiconal"

> +          branch_bp = branch_dest(opcode, insn, pc, breaks[0]);
Space before '('.

> +      if ((insn & STWCX_MASK) == STWCX_INSTRUCTION
> +       || (insn & STWCX_MASK) == STDCX_INSTRUCTION)
Indentation.

> +  /* Assume that the atomic sequence ends with a stwcx/stdcx instruction */
Comment should end with full stop.

> +  if ((insn & STWCX_MASK) != STWCX_INSTRUCTION
> +   && (insn & STWCX_MASK) != STDCX_INSTRUCTION)
Indentation.

> +      warning (_("\nTried to step over an atomic sequence of instructions at %s\n \
> +                  but could not find the end of the sequence."),
This leaves a lot of spaces in the warning text.

> +                  core_addr_to_string(pc));
Space before '('.

> +  /* Insert a breakpoint right after the end of the atomic sequence */
Comment should end with full stop.

> +  /* Effectively inserts all the breakpoints */
Likewise

> +  for (index = 0; index < last_breakpoint; index++)
> +    insert_single_step_breakpoint (breaks[index]);
> +
> +  gdb_flush (gdb_stdout);
I don't think this is necessary.

> +  if (deal_with_atomic_sequence(regcache))
Space before '('.

> +  /* Handles single stepping of atomic sequences */
Comment should end with full stop.
> +  set_gdbarch_software_single_step(gdbarch, deal_with_atomic_sequence);
Space before '('.


Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com