STEP_SKIPS_DELAY question, sort of

STEP_SKIPS_DELAY question, sort of

[Orjan Friberg]

I'm facing a problem with the CRIS target which is related to what 
STEP_SKIPS_DELAY is aiming to solve,.  The problem occurs when 
single-stepping past a breakpoint set on an instruction with a delay 
slot, because execution resumes at the instruction *preceding* the delay 
slot.  Here's an illustration of what happens:

0x0  ba foo  (branch instruction)
0x2  nop     (delay slot)
If there's a breakpoint (implemented in CRIS with a "break" instruction) 
at address 0x0, the following happens when single-stepping (hardware 
assisted):

(1) We end up in the delay slot at address 0x2.
(2) GDB re-inserts the break instruction at address 0x0.
(3) Execution resumes at address 0x0, and we hit the breakpoint again.
Besides STEP_SKIPS_DELAY having a fixed offset of 4 bytes between 
instruction and its associated delay slot, it's concerned with a 
breakpoint being set in the delay slot, which isn't the case here.

Basically, what I'm looking for is a way to say "don't re-insert a 
breakpoint on an instruction that's going to be restarted when we resume 
execution".  I'm thinking other architectures have faced similar issues, 
but I couldn't find anything.

(The alternatives I could think of are (1) fudging the address we report 
as being stopped at, to trick GDB into stepping one more time before 
re-insterting the breakpoint, or (2) automatically single-step past the 
delay slot (without reporting to GDB), both of which may be confusing 
for a user.)

--
Orjan Friberg
Axis Communications

Re: STEP_SKIPS_DELAY question, sort of

[Andrew Cagney]

I'm facing a problem with the CRIS target which is related to what STEP_SKIPS_DELAY is aiming to solve,.  The problem occurs when single-stepping past a breakpoint set on an instruction with a delay slot, because execution resumes at the instruction *preceding* the delay slot.  Here's an illustration of what happens:

0x0  ba foo  (branch instruction)
0x2  nop     (delay slot)
If there's a breakpoint (implemented in CRIS with a "break" instruction) at address 0x0, the following happens when single-stepping (hardware assisted):

(1) We end up in the delay slot at address 0x2.
(2) GDB re-inserts the break instruction at address 0x0.
(3) Execution resumes at address 0x0, and we hit the breakpoint again.
Besides STEP_SKIPS_DELAY having a fixed offset of 4 bytes between instruction and its associated delay slot, it's concerned with a breakpoint being set in the delay slot, which isn't the case here.

Basically, what I'm looking for is a way to say "don't re-insert a breakpoint on an instruction that's going to be restarted when we resume execution".  I'm thinking other architectures have faced similar issues, but I couldn't find anything.

(The alternatives I could think of are (1) fudging the address we report as being stopped at, to trick GDB into stepping one more time before re-insterting the breakpoint, or (2) automatically single-step past the delay slot (without reporting to GDB), both of which may be confusing for a user.)
Earlier MIPS variants (at least) needed STEP_SKIPS_DELAY.  When at 
``0x0'', a hardware single-step would end up at ``foo''.  That isn't 
your problem.

Since your hardware can single-step into a delay slot, can it also 
resume from the delay slot?  An example of this is SPARC with its 
constantly shuffling PC/N[ext]PC -- the inferior is resumed with PC==0x2 
N[ext]PC==foo.

Andrew

Re: STEP_SKIPS_DELAY question, sort of

[Orjan Friberg]

Andrew Cagney wrote:
Earlier MIPS variants (at least) needed STEP_SKIPS_DELAY.  When at 
``0x0'', a hardware single-step would end up at ``foo''.  That isn't 
your problem.

Since your hardware can single-step into a delay slot, can it also 
resume from the delay slot?  An example of this is SPARC with its 
constantly shuffling PC/N[ext]PC -- the inferior is resumed with PC==0x2 
N[ext]PC==foo.
I'm not sure I understand what you mean by "can resume" - it can resume 
from a delay slot, but when doing so the branch instruction (and the 
delay slot) is re-executed.  I don't have an NPC, and AFAIK there's no 
information in the CPU registers that tells me whether the branch is 
taken or not (which seems consistent with the fact that the 
branch-instruction is re-executed).

Maybe this can be stated in even simpler terms than "don't re-insert a 
breakpoint on an instruction that's going to be restarted when we resume 
execution".  Rather, "single-step twice before re-inserting a breakpoint 
we're currently stopped at".

--
Orjan Friberg
Axis Communications

Re: STEP_SKIPS_DELAY question, sort of

[Andrew Cagney]

Andrew Cagney wrote:

Earlier MIPS variants (at least) needed STEP_SKIPS_DELAY.  When at ``0x0'', a hardware single-step would end up at ``foo''.  That isn't your problem.

Since your hardware can single-step into a delay slot, can it also resume from the delay slot?  An example of this is SPARC with its constantly shuffling PC/N[ext]PC -- the inferior is resumed with PC==0x2 N[ext]PC==foo.


I'm not sure I understand what you mean by "can resume" - it can resume from a delay slot, but when doing so the branch instruction (and the delay slot) is re-executed.  I don't have an NPC, and AFAIK there's no information in the CPU registers that tells me whether the branch is taken or not (which seems consistent with the fact that the branch-instruction is re-executed).
Ok, I'm starting to understand the wierdness

The SPARC (using PC/NextPC) resumes from the delay slot so re-inserting 
the breakpoint is safe - that branch instruction is never re-executed. 
Your architecture isn't like this.



I think there is still some missing information.  Given:
  N+0: branch foo
  N+2: nop
and PC==N+0, exactly what state information is available after doing a 
single hardware single-step (PC and status registers)?

- If I understand things correctly, one h/w single-step gets PC==N+2 and 
two h/w single-steps gets PC==foo.  For that to work there must be some 
additional state information lurking somewhere - a bit indicating 
stopped in delay slot perhaphs?

- If there were a breakpoint at N+2, what state information would be 
available then.  If the breakpoint were then yanked, where would a 
single-step end up - again additional state information is needed to 
make this work?

Maybe this can be stated in even simpler terms than "don't re-insert a breakpoint on an instruction that's going to be restarted when we resume execution".  Rather, "single-step twice before re-inserting a breakpoint we're currently stopped at". 
GDB needs to know that its in such a state.

Andrew

Re: STEP_SKIPS_DELAY question, sort of

[Orjan Friberg]

Andrew Cagney wrote:
I think there is still some missing information.  Given:
  N+0: branch foo
  N+2: nop
and PC==N+0, exactly what state information is available after doing a 
single hardware single-step (PC and status registers)?
State information is only PC (see below) and SPC (single-step PC). 
There's no information in the status registers regarding branch 
taken/not taken.

(The h/w single-step procedure works like this: after the instruction 
pointed out by the SPC has been executed, a single-step exception is 
given, and the SPC is updated to the next instruction.)

- If I understand things correctly, one h/w single-step gets PC==N+2 and 
two h/w single-steps gets PC==foo.  For that to work there must be some 
additional state information lurking somewhere - a bit indicating 
stopped in delay slot perhaphs?
One h/w single-step actually gets PC==N+1, which I then adjust to N+2 
before reporting it to GDB.  So, (yes) the bit indicating stopped in a 
delay slot is the lowest bit of the PC.

- If there were a breakpoint at N+2, what state information would be 
available then.  If the breakpoint were then yanked, where would a 
single-step end up - again additional state information is needed to 
make this work?
With a breakpoint at N+2, PC==N+1 (again, interrupted in delay slot, 
will resume execution at N) and PC is reported as N+2.  For a 
single-step at this point, SPC is set up with N+2, so a single-step 
exception fires after the delay slot has been executed.

(The decrementation of the PC when executing a break instruction is 
handled by the stub itself, which becomes a bit of a kludge when we're 
stopped in a delay slot.)

What could be implemented relatively easily is to automatically 
single-step again if we end up in a delay-slot due to a single-step (not 
due to a breakpoint though).  This would, in some sense, break the 
"stepi" command since we would step two instructions instead of one in 
those cases.

Maybe this can be stated in even simpler terms than "don't re-insert a 
breakpoint on an instruction that's going to be restarted when we 
resume execution".  Rather, "single-step twice before re-inserting a 
breakpoint we're currently stopped at". 


GDB needs to know that its in such a state.
Yeah, I had a look at resume/handle_inferior_event, and it certainly 
wasn't obvious to me how that kind of mechanism should be implemented.

--
Orjan Friberg
Axis Communications

Re: STEP_SKIPS_DELAY question, sort of

[Andrew Cagney]

Ok,

When given:

  N+0: branch foo
  N+2: nop
PC=N (or N+1)
SPC=N+2
would this result in a double single-step leaving PC==foo?  If it does 
then you've at least got STEP_SKIPS_DELAY like behavior.

It still fixes single-step but that's a, er, later step.

Andrew

Re: STEP_SKIPS_DELAY question, sort of

[Orjan Friberg]

Andrew Cagney wrote:
Ok,

When given:

  N+0: branch foo
  N+2: nop
PC=N (or N+1)
SPC=N+2
would this result in a double single-step leaving PC==foo?  If it does 
then you've at least got STEP_SKIPS_DELAY like behavior.
Actually, both when PC=N (at the branch instruction) and when PC=N+1 (in 
the delay slot) a *single* single-step will leave us at PC==foo - the 
single-step exception fires after the instruction pointed out by SPC has 
been executed.

I'll go and have a closer look at STEP_SKIPS_DELAY.

--
Orjan Friberg
Axis Communications

Re: STEP_SKIPS_DELAY question, sort of

[Andrew Cagney]

Andrew Cagney wrote:

Ok,

When given:

  N+0: branch foo
  N+2: nop
PC=N (or N+1)
SPC=N+2
would this result in a double single-step leaving PC==foo?  If it does then you've at least got STEP_SKIPS_DELAY like behavior.


Actually, both when PC=N (at the branch instruction) and when PC=N+1 (in the delay slot) a *single* single-step will leave us at PC==foo - the single-step exception fires after the instruction pointed out by SPC has been executed.

I'll go and have a closer look at STEP_SKIPS_DELAY. 
We'll likely need to change that a bit but thats ok.

Andrew

Re: STEP_SKIPS_DELAY question, sort of

[Orjan Friberg]

Orjan Friberg wrote:
> 
> I'll go and have a closer look at STEP_SKIPS_DELAY.

Short recap: the MIPS checks if there's a breakpoint in the delay slot of the 
instruction where GDB is about to resume execution.  If so, GDB single-steps 
that instruction before re-inserting the breakpoint.

For CRISv32, if there's a breakpoint on an instruction which has a delay slot, 
we cannot re-insert the breakpoint when we're stopped in the delay slot (after 
making one single-step), because the previous instruction will be re-executed 
when returning from the delay slot.

I tried implementing a fix in handle_inferior_event (the MIPS fix is in 
proceed).  It seemed easier to step one more time when we find out that we need 
to, rather than to determine beforehand that we're going to have to step twice 
(and I couldn't determine how to pass that information).

The concept patch below illustrates what I'm trying to do; by setting 
another_trap in the execution control state, by the time we get to keep_going we 
won't insert the breakpoint and we'll instead continue single-stepping. 
Comments?  Is this the right approach at all?

Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.156
diff -u -r1.156 infrun.c
--- infrun.c    11 May 2004 23:30:31 -0000      1.156
+++ infrun.c    7 Jun 2004 11:44:19 -0000
@@ -1975,7 +1975,17 @@
        /* Don't even think about breakpoints if just proceeded over a
           breakpoint.  */
        if (stop_signal == TARGET_SIGNAL_TRAP && trap_expected)
-       bpstat_clear (&stop_bpstat);
+       {
+         bpstat_clear (&stop_bpstat);
+
+         /* If we stepped into a delay slot, and the preceding instruction
+            will be re-executed when resuming, step again before re-inserting
+            the breakpoint.  */
+         if (STEP_SKIPS_IN_DELAY_P
+             && breakpoint_here_p (read_pc () - 2)
+             && STEP_SKIPS_IN_DELAY (read_pc ()))
+           ecs->another_trap = 1;
+       }
        else
         {
           /* See if there is a breakpoint at the current PC.  */


-- 
Orjan Friberg
Axis Communications

Re: STEP_SKIPS_DELAY question, sort of

[Orjan Friberg]

Orjan Friberg wrote:
> 
> I tried implementing a fix in handle_inferior_event (the MIPS fix is in 
> proceed).  It seemed easier to step one more time when we find out that 
> we need to, rather than to determine beforehand that we're going to have 
> to step twice (and I couldn't determine how to pass that information).
> 
> The concept patch below illustrates what I'm trying to do; by setting 
> another_trap in the execution control state, by the time we get to 
> keep_going we won't insert the breakpoint and we'll instead continue 
> single-stepping. Comments?  Is this the right approach at all?

Gah.  Please ignore the previous patch (and sorry); what I posted only 
works when doing a continue when stopped at the branch instruction. 
Doing a step (which leaves us in the delay slot) followed by another 
step (or continue for that matter) prematurely inserts the breakpoint.

-- 
Orjan Friberg
Axis Communications

Re: STEP_SKIPS_DELAY question, sort of

[Orjan Friberg]

Orjan Friberg wrote:
> 
> Gah.  Please ignore the previous patch (and sorry); what I posted only 
> works when doing a continue when stopped at the branch instruction. 
> Doing a step (which leaves us in the delay slot) followed by another 
> step (or continue for that matter) prematurely inserts the breakpoint.

Ok, second try (still concept patch though): the change in proceed is needed for 
when we resume from the delay slot - in that case we need to single-step again 
before re-inserting the breakpoint (similar to the MIPS case).  The change in 
handle_inferior_event (and I'll happily agree it's far more questionable) is 
needed for when we resume at the branch instruction itself.  Comments?

Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.156
diff -u -p -r1.156 infrun.c
--- infrun.c    11 May 2004 23:30:31 -0000      1.156
+++ infrun.c    7 Jun 2004 12:59:10 -0000
@@ -748,6 +748,14 @@ proceed (CORE_ADDR addr, enum target_sig
           && breakpoint_here_p (read_pc () + 4)
           && STEP_SKIPS_DELAY (read_pc ()))
         oneproc = 1;
+
+      /* If we stepped into a delay slot, and the preceding instruction
+        will be re-executed when resuming, step again before re-inserting
+        the breakpoint.  */
+      if (STEP_SKIPS_IN_DELAY_P
+         && breakpoint_here_p (read_pc () - 2)
+         && STEP_SKIPS_IN_DELAY (read_pc ()))
+       oneproc = 1;
      }
    else
      {
@@ -1975,7 +1983,17 @@ handle_inferior_event (struct execution_
        /* Don't even think about breakpoints if just proceeded over a
           breakpoint.  */
        if (stop_signal == TARGET_SIGNAL_TRAP && trap_expected)
-       bpstat_clear (&stop_bpstat);
+       {
+         bpstat_clear (&stop_bpstat);
+
+         /* If we stepped into a delay slot, and the preceding instruction
+            will be re-executed when resuming, step again before re-inserting
+            the breakpoint.  */
+         if (STEP_SKIPS_IN_DELAY_P
+             && breakpoint_here_p (read_pc () - 2)
+             && STEP_SKIPS_IN_DELAY (read_pc ()))
+           ecs->another_trap = 1;
+       }
        else
         {
           /* See if there is a breakpoint at the current PC.  */

-- 
Orjan Friberg
Axis Communications

Re: STEP_SKIPS_DELAY question, sort of

[Andrew Cagney]

> Orjan Friberg wrote:
> 
>>
>> Gah.  Please ignore the previous patch (and sorry); what I posted only works when doing a continue when stopped at the branch instruction. Doing a step (which leaves us in the delay slot) followed by another step (or continue for that matter) prematurely inserts the breakpoint.
> 
> 
> Ok, second try (still concept patch though): the change in proceed is needed for when we resume from the delay slot - in that case we need to single-step again before re-inserting the breakpoint (similar to the MIPS case).  The change in handle_inferior_event (and I'll happily agree it's far more questionable) is needed for when we resume at the branch instruction itself.  Comments?

Can this new mechanism somehow superseed STEP_SKIPS_DELAY - it seems to 
be the exact oposite but there could be common ground here.

> Index: infrun.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/infrun.c,v
> retrieving revision 1.156
> diff -u -p -r1.156 infrun.c
> --- infrun.c    11 May 2004 23:30:31 -0000      1.156
> +++ infrun.c    7 Jun 2004 12:59:10 -0000
> @@ -748,6 +748,14 @@ proceed (CORE_ADDR addr, enum target_sig
>           && breakpoint_here_p (read_pc () + 4)
>           && STEP_SKIPS_DELAY (read_pc ()))
>         oneproc = 1;
> +
> +      /* If we stepped into a delay slot, and the preceding instruction
> +        will be re-executed when resuming, step again before re-inserting
> +        the breakpoint.  */
> +      if (STEP_SKIPS_IN_DELAY_P
> +         && "
> +         && STEP_SKIPS_IN_DELAY (read_pc ()))
> +       oneproc = 1;
>      }
>    else
>      {

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.

> @@ -1975,7 +1983,17 @@ handle_inferior_event (struct execution_
>        /* Don't even think about breakpoints if just proceeded over a
>           breakpoint.  */
>        if (stop_signal == TARGET_SIGNAL_TRAP && trap_expected)
> -       bpstat_clear (&stop_bpstat);
> +       {
> +         bpstat_clear (&stop_bpstat);
> +
> +         /* If we stepped into a delay slot, and the preceding instruction
> +            will be re-executed when resuming, step again before re-inserting
> +            the breakpoint.  */
> +         if (STEP_SKIPS_IN_DELAY_P
> +             && breakpoint_here_p (read_pc () - 2)
> +             && STEP_SKIPS_IN_DELAY (read_pc ()))
> +           ecs->another_trap = 1;
> +       }
>        else
>         {
>           /* See if there is a breakpoint at the current PC.  */
> 

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

Andrew

Re: STEP_SKIPS_DELAY question, sort of

[Orjan Friberg]

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?

[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)

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

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

-- 
Orjan Friberg
Axis Communications

Re: 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

Re: STEP_SKIPS_DELAY question, sort of

[Orjan Friberg]

Andrew Cagney wrote:
> 
> 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)?)

SINGLE_STEP_THROUGH_DELAY sounds fine to me.

> 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.

Agreed; it wasn't clear from the context where I put the patch why it was there. 
  I'll come back with a proper patch once I've gotten around to do some more 
serious testing.

-- 
Orjan Friberg
Axis Communications

Re: STEP_SKIPS_DELAY question, sort of

[Orjan Friberg]

Orjan Friberg wrote:
> Andrew Cagney wrote:
>>
>> 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)?)

I'm facing a slightly different situation than the MIPS case when implementing 
SINGLE_STEP_THROUGH_DELAY, because the size of the instruction that the delay 
slot belongs to may be 2, 4, or 6 bytes.  (For MIPS, it seems the size of that 
instruction is always 4 bytes.)

So, I have to find out the size of the preceding instruction (since I want to do 
the single-step thingy when we're stopped in the delay slot), but I can't find 
anything in the opcodes directory that would let me do that.

The thing is, this information (whether I'm in a delay slot, and the address of 
the instruction the delay slot belongs to) is available in a register.  Besides 
the fact that this triggers sending a 'g' packet to the remote target, is it 
wrong to deduce this information from the registers content rather than reading 
from the program's text segment?

Basically, my implementation would look like this:

int
cris_single_step_through_delay (CORE_ADDR pc)
{
   ULONGEST erp;
   int ret = 0;
   regcache_cooked_read_unsigned (current_regcache, ERP_REGNUM, &erp);
   if (erp & 0x1)
     /* In delay slot - check if there's a breakpoint at the preceding
        instruction.  */
     if (breakpoint_here_p (erp & ~0x1))
       ret = 1;
   return ret;
}

-- 
Orjan Friberg
Axis Communications

Re: STEP_SKIPS_DELAY question, sort of

[Andrew Cagney]

> Orjan Friberg wrote:
> 
>> Andrew Cagney wrote:
>>
>>>
>>> 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)?)
> 
> 
> I'm facing a slightly different situation than the MIPS case when implementing SINGLE_STEP_THROUGH_DELAY, because the size of the instruction that the delay slot belongs to may be 2, 4, or 6 bytes.  (For MIPS, it seems the size of that instruction is always 4 bytes.)
> 
> So, I have to find out the size of the preceding instruction (since I want to do the single-step thingy when we're stopped in the delay slot), but I can't find anything in the opcodes directory that would let me do that.
> 
> The thing is, this information (whether I'm in a delay slot, and the address of the instruction the delay slot belongs to) is available in a register.  Besides the fact that this triggers sending a 'g' packet to the remote target, is it wrong to deduce this information from the registers content rather than reading from the program's text segment?
> 
> Basically, my implementation would look like this:
> 
> int
> cris_single_step_through_delay (CORE_ADDR pc)
> {
>   ULONGEST erp;
>   int ret = 0;
>   regcache_cooked_read_unsigned (current_regcache, ERP_REGNUM, &erp);
>   if (erp & 0x1)
>     /* In delay slot - check if there's a breakpoint at the preceding
>        instruction.  */
>     if (breakpoint_here_p (erp & ~0x1))
>       ret = 1;
>   return ret;
> } 

Ah! (sorry for the delay).  Sounds like a "struct frame_info 
*this_frame" is the parameter you need then.  This will let it work for 
any frame (especially a frame interrupted by a signal handler).

Andrew

Re: STEP_SKIPS_DELAY question, sort of

[Orjan Friberg]

Andrew Cagney wrote:
> 
> 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.

Now that I've done a lot more testing, I'm picking this up again.  (Below is 
just the infrun.c part; gdbarch.sh obviously needs a patch, and mips-tdep.c 
needs to have its current STEP_SKIPS_DELAY implementation converted - I'll post 
a complete patch if this part is approved of.)

A few things:
* I'm not sure how to reliably detect the situation "stepping off a breakpoint" 
in handle_inferior_event.  I used stop_signal == TARGET_SIGNAL_TRAP && 
trap_expected && currently_stepping (ecs)); could that be too inclusive?.
* Distinguishing between "step" and "continue" (using step_range_end) is not 
necessary for it to work, but explicitly returning in the "continue" case is 
making things a bit clearer.
* As the comment suggests, in the "step" case we don't want to preclude the 
stop_after_trap check - I assume that whatever is in the delay slot could 
potentially correspond to a single line of code (if nothing else, then at least 
an asm("...") construct.)

Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.177
diff -u -r1.177 infrun.c
--- infrun.c    13 Sep 2004 18:26:28 -0000      1.177
+++ infrun.c    1 Oct 2004 11:22:42 -0000
@@ -721,17 +721,12 @@

        if (read_pc () == stop_pc && breakpoint_here_p (read_pc ()))
         oneproc = 1;
-
-#ifndef STEP_SKIPS_DELAY
-#define STEP_SKIPS_DELAY(pc) (0)
-#define STEP_SKIPS_DELAY_P (0)
-#endif
-      /* Check breakpoint_here_p first, because breakpoint_here_p is fast
-         (it just checks internal GDB data structures) and STEP_SKIPS_DELAY
-         is slow (it needs to read memory from the target).  */
-      if (STEP_SKIPS_DELAY_P
-         && breakpoint_here_p (read_pc () + 4)
-         && STEP_SKIPS_DELAY (read_pc ()))
+
+      /* If we stepped into something that needs to be stepped again before
+        before re-inserting the breakpoint, then do so.  */
+      else if (gdbarch_single_step_through_delay_p (current_gdbarch)
+              && gdbarch_single_step_through_delay (current_gdbarch,
+                                                    get_current_frame ()))
         oneproc = 1;
      }
    else
@@ -1793,6 +1788,35 @@
    stopped_by_random_signal = 0;
    breakpoints_failed = 0;

+  if (gdbarch_single_step_through_delay_p (current_gdbarch)
+      && stop_signal == TARGET_SIGNAL_TRAP && trap_expected
+      && currently_stepping (ecs))
+    {
+      /* We are in the process of stepping off a breakpoint.  If we stepped
+        into something that needs to be stepped again before re-inserting
+        the breakpoint, then do so.  */
+      int step_through_delay
+       = gdbarch_single_step_through_delay (current_gdbarch,
+                                            get_current_frame ());
+      if (step_range_end == 0 && step_through_delay)
+       {
+         /* The user issued a continue when stopped at a breakpoint.
+            Set up for another trap and get out of here.  */
+         ecs->another_trap = 1;
+         keep_going (ecs);
+         return;
+       }
+      else if (step_through_delay)
+       {
+         /* The user issued a step when stopped at a breakpoint.
+            Maybe we should stop, maybe we should not - the delay slot
+            *might* correspond to a line of source.  In any case, don't decide
+            that here, just set ecs->another_trap, making sure we
+            single-step again before breakpoints are re-inserted.  */
+         ecs->another_trap = 1;
+       }
+    }
+
    /* Look at the cause of the stop, and decide what to do.
       The alternatives are:
       1) break; to really stop and return to the debugger,

-- 
Orjan Friberg
Axis Communications

Re: STEP_SKIPS_DELAY question, sort of

[Andrew Cagney]

Sorry, picking up old threads.

Orjan Friberg wrote:
> Andrew Cagney wrote:
> 
>>
>> 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.
> 
> 
> Now that I've done a lot more testing, I'm picking this up again.  
> (Below is just the infrun.c part; gdbarch.sh obviously needs a patch, 
> and mips-tdep.c needs to have its current STEP_SKIPS_DELAY 
> implementation converted - I'll post a complete patch if this part is 
> approved of.)
> 
> A few things:
> * I'm not sure how to reliably detect the situation "stepping off a 
> breakpoint" in handle_inferior_event.  I used stop_signal == 
> TARGET_SIGNAL_TRAP && trap_expected && currently_stepping (ecs)); could 
> that be too inclusive?.

I think this is sufficient.  Can you add something mentioning "stepping 
off a breakpoint" to the comment - that makes the intent clear.

> * Distinguishing between "step" and "continue" (using step_range_end) is 
> not necessary for it to work, but explicitly returning in the "continue" 
> case is making things a bit clearer.

Always a good idea.

Andrew


> * As the comment suggests, in the "step" case we don't want to preclude 
> the stop_after_trap check - I assume that whatever is in the delay slot 
> could potentially correspond to a single line of code (if nothing else, 
> then at least an asm("...") construct.)
> 
> Index: infrun.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/infrun.c,v
> retrieving revision 1.177
> diff -u -r1.177 infrun.c
> --- infrun.c    13 Sep 2004 18:26:28 -0000      1.177
> +++ infrun.c    1 Oct 2004 11:22:42 -0000
> @@ -721,17 +721,12 @@
> 
>        if (read_pc () == stop_pc && breakpoint_here_p (read_pc ()))
>         oneproc = 1;
> -
> -#ifndef STEP_SKIPS_DELAY
> -#define STEP_SKIPS_DELAY(pc) (0)
> -#define STEP_SKIPS_DELAY_P (0)
> -#endif
> -      /* Check breakpoint_here_p first, because breakpoint_here_p is fast
> -         (it just checks internal GDB data structures) and 
> STEP_SKIPS_DELAY
> -         is slow (it needs to read memory from the target).  */
> -      if (STEP_SKIPS_DELAY_P
> -         && breakpoint_here_p (read_pc () + 4)
> -         && STEP_SKIPS_DELAY (read_pc ()))
> +
> +      /* If we stepped into something that needs to be stepped again 
> before
> +        before re-inserting the breakpoint, then do so.  */
> +      else if (gdbarch_single_step_through_delay_p (current_gdbarch)
> +              && gdbarch_single_step_through_delay (current_gdbarch,
> +                                                    get_current_frame ()))
>         oneproc = 1;
>      }
>    else
> @@ -1793,6 +1788,35 @@
>    stopped_by_random_signal = 0;
>    breakpoints_failed = 0;
> 
> +  if (gdbarch_single_step_through_delay_p (current_gdbarch)
> +      && stop_signal == TARGET_SIGNAL_TRAP && trap_expected
> +      && currently_stepping (ecs))
> +    {
> +      /* We are in the process of stepping off a breakpoint.  If we 
> stepped
> +        into something that needs to be stepped again before re-inserting
> +        the breakpoint, then do so.  */
> +      int step_through_delay
> +       = gdbarch_single_step_through_delay (current_gdbarch,
> +                                            get_current_frame ());
> +      if (step_range_end == 0 && step_through_delay)
> +       {
> +         /* The user issued a continue when stopped at a breakpoint.
> +            Set up for another trap and get out of here.  */
> +         ecs->another_trap = 1;
> +         keep_going (ecs);
> +         return;
> +       }
> +      else if (step_through_delay)
> +       {
> +         /* The user issued a step when stopped at a breakpoint.
> +            Maybe we should stop, maybe we should not - the delay slot
> +            *might* correspond to a line of source.  In any case, don't 
> decide
> +            that here, just set ecs->another_trap, making sure we
> +            single-step again before breakpoints are re-inserted.  */
> +         ecs->another_trap = 1;
> +       }
> +    }
> +
>    /* Look at the cause of the stop, and decide what to do.
>       The alternatives are:
>       1) break; to really stop and return to the debugger,
>