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

[uweigand@de.ibm.com]

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