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

uweigand
2007-05-02T14:08:34Z

Luis Machado wrote:

> Patch updated to reflect the inclusion of the regcache parameter.

Thanks! A number of additional comments:

> +static int
> +deal_with_atomic_sequence ()

Parameter is missing here.

> +{
> + CORE_ADDR pc = read_pc ();
> + CORE_ADDR breaks[2] = {-1, -1};
> + CORE_ADDR loc = pc;
> + int insn = read_memory_integer (loc, PPC_INSN_SIZE);
> + int last_break = 0;
> + int i;
> +
> + /* Assume all atomic sequences start with an lwarx or ldarx instruction. */
Should have two spaces after '.' (multiple instances of this throughout the patch)

> + if ((insn & LWARX_MASK) != LWARX_INSTRUCTION
> + && (insn & LWARX_MASK) != LDARX_INSTRUCTION)
> + return 0;
> +
> + /* Assume that no atomic sequence is longer than 6 instructions. */
> + for (i = 1; i < 5; ++i)
> + {
> + loc += PPC_INSN_SIZE;
> + insn = read_memory_integer (loc, PPC_INSN_SIZE);
> +
> + /* Assume at most one conditional branch instruction between
> + the lwarx and stwcx instructions.*/

Well, what happens if there *is* more than one? You should at least
detect that case ...

> + if ((insn & BC_MASK) == BC_INSTRUCTION)
> + {
> + last_break = 1;
> + breaks[1] = IMMEDIATE_PART (insn);
> + if ( ! ABSOLUTE_P(insn))
Space before '('; no spaces around '!'.

> + breaks[1] += loc;
> + continue;
> + }

Why don't you use the existing branch_dest routine instead of
re-implementing (parts of) it here?

> + if ((insn & STWCX_MASK) == STWCX_INSTRUCTION
> + || (insn & STWCX_MASK) == STDCX_INSTRUCTION)
> + break;
> + }
> +
> + /* Assume that the atomic sequence ends with a stwcx instruction
> + followed by a conditional branch instruction. */
> + if ((insn & STWCX_MASK) != STWCX_INSTRUCTION
> + && (insn & STWCX_MASK) != STDCX_INSTRUCTION)
> + warning (_("Tried to step over an atomic sequence of instructions at %s\n \
> + but could not find the end of the sequence."),
> + core_addr_to_string(pc));

Shouldn't you return to the default single-step handling if you do not
understand the sequence you find?

> +
> + loc += PPC_INSN_SIZE;
> + insn = read_memory_integer (loc, PPC_INSN_SIZE);
> +
> + if ((insn & BC_MASK) != BC_INSTRUCTION)
> + warning (_("Tried to step over an atomic sequence of instructions at %s\n \
> + but the instruction sequence ended in an unexpected way."),
> + core_addr_to_string(pc));

I'm wondering why this test is necessary. So what if there is no conditional
branch immediately after the stwcx? That shouldn't really matter ...

> +
> + breaks[0] = loc;
> +
> + /* This should never happen, but make sure we don't put
> + two breakpoints on the same address. */
> + if (last_break && breaks[1] == breaks[0])
> + last_break = 0;
> +
> + for (i = 0; i <= last_break; ++i)
> + insert_single_step_breakpoint (breaks[i]);
> +
> + printf_unfiltered (_("Stepping over an atomic sequence of instructions beginning at %s\n"),
> + core_addr_to_string (pc));

Should we really output this message unconditionally? If you're automatically
single-stepping (say, due to software watch points), this message could potentially
show up very frequently ...

Bye,
Ulrich

--
Dr. Ulrich Weigand
Linux on zSeries Development
Ulrich.Weigand@de.ibm.com