This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFC] "single step" atomic instruction sequences as a whole on PPC
- From: "Ulrich Weigand" <uweigand at de dot ibm dot com>
- To: luisgpm at linux dot vnet dot ibm dot com
- Cc: gdb-patches at sourceware dot org
- Date: Sun, 6 May 2007 23:20:41 +0200 (CEST)
- Subject: Re: [RFC] "single step" atomic instruction sequences as a whole on PPC
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