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

Ulrich Weigand
2007-05-06T21:20:47Z

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