STEP_SKIPS_DELAY question, sort of

[Andrew Cagney]

> Andrew Cagney wrote:
> 
>>
>> Can this new mechanism somehow superseed STEP_SKIPS_DELAY - it seems to be the exact oposite but there could be common ground here.
> 
> 
> [proceed patch snipped]
> 
>> They both seem to be asking the question: "given PC and a list of breakpoints, should the inferior be h/w single-stepped?".  That would mean pushing the alternative:
>>   breakpoint_here_p (read_pc () - 2)
>>   breakpoint_here_p (read_pc () + 4)
>> calls into that architecture method.
> 
> 
> Agreed.  (STEP_SKIPS_IN_DELAY was just to have something to put in the patch.)
> 
> What about using the name STEP_SKIPS_DELAY for both, and introducing a DELAY_SIZE which  would return a positive value (meaning the diff from the current pc to the delay slot) or a negative (meaning the diff from the delay slot to the instruction preceding it)?  Or does the word "size" imply an absolute value?

If the:
 >>   breakpoint_here_p (read_pc () - 2)
and
 >>   breakpoint_here_p (read_pc () + 4)
logic is moved to the per-architecture STEP_SKIPS_DELAY I don't think 
DELAY_SIZE is needed.

I also think this needs a new macro name that better reflects what the 
test is doing.  But I've no good ideas :-/ (SINGLE_STEP_THROUGH_DELAY (pc)?)

> [handle_inferior_event patch snipped]
> 
>> I'm just not sure how this bit of logic should fit in.  I'm guessing its the second half of the state m/c sequence:
>>
>> 1. step off breakpoint at `PC'
>> 2. step through delay
> 
> 
> Unless I missed something on the way, the procedure when doing a continue from a
> breakpoint that sits on the branch instruction is this:
> 
> 1. proceed decides it needs to step once before continuing (since read_pc () == stop_pc && breakpoint_here_p (read_pc ()))
> 2. resume is called, with step = 1
> 3. target is single-stepped
> 4. handle_inferior_event is called (at which point we're stopped in the delay slot)

yes (step off breakpoint at `PC')

> It is at this point we need to single-step again (before inserting breakpoints again), so I set ecs->another_trap.  Then:
> 
> 5. keep_going is called, and since ecs->anther_trap is set, it doesn't call insert_breakpoints.
> 6. resume is called again, with step = 1
> 7. target is single-stepped
> 8. handle_inferior_event is called again (but doesn't set ecs->another_trap this time)
> 9. keep_going is called, and inserts the breakpoints again

ok (step through delay)

> I can't say where would be a better place to put the decision of whether to single-step again.  Any suggestions?

Can a simple, separate, more explicit logic like:
	if (we just did a step and STEP_SKIPS_DELAY (pc))
	  set up for another step
	  return;
work?  The [handle_inferior_event patch snipped] was nested within other 
logic and that's not good from a readability / maintainability point of 
view.

Andrew