[RFA] Don't ignore consecutive breakpoints.

[RFA] Don't ignore consecutive breakpoints.

[Vladimir Prus]

Suppose we have two breakpoints at two consecutive
addresses, and we do "step" while stopped on the
first breakpoint. GDB testsuite has a test (consecutive.exp)
that the second breakpoint will be hit a reported, and the
test passes, but the code directly contradicts, saying:

      /* Don't even think about breakpoints if just proceeded over a
         breakpoint.  */
      if (stop_signal == TARGET_SIGNAL_TRAP && trap_expected)
	{
          if (debug_infrun)
	    fprintf_unfiltered (gdb_stdlog, "infrun: trap expected\n");
	  bpstat_clear (&stop_bpstat);
	}

what's happening is that we indeed ignore the breakpoint, and try
to step further. However ecs->another_trap is not set, so we step
with breakpoints inserted, and immediately hit the now-inserted
breakpoint. Therefore, I propose to remove that code.

On x86, the below patch causes a single test outcome change:

-KFAIL: gdb.base/watchpoint.exp: next after watch x (PRMS: gdb/38)
+PASS: gdb.base/watchpoint.exp: next after watch x

The test is for a breakpoint set on a line that also triggers a
watchpoint. The outcome changed because we now do check breakpoints
after stepping over breakpoint. The target does not have any
watchpoint stop address, so we read watched value, and find that
in indeed has changed.

OK?

- Volodya

	* infrun.c (handle_inferior_event): Don't
	ignore beakpoints if trap_expected is set.
---
 gdb/infrun.c |   23 ++++++-----------------
 1 files changed, 6 insertions(+), 17 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 85d889a..feff6aa 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1983,23 +1983,12 @@ handle_inferior_event (struct execution_control_state *ecs)
 	  return;
 	}
 
-      /* Don't even think about breakpoints if just proceeded over a
-         breakpoint.  */
-      if (stop_signal == TARGET_SIGNAL_TRAP && trap_expected)
-	{
-          if (debug_infrun)
-	    fprintf_unfiltered (gdb_stdlog, "infrun: trap expected\n");
-	  bpstat_clear (&stop_bpstat);
-	}
-      else
-	{
-	  /* See if there is a breakpoint at the current PC.  */
-	  stop_bpstat = bpstat_stop_status (stop_pc, ecs->ptid);
-
-	  /* Following in case break condition called a
-	     function.  */
-	  stop_print_frame = 1;
-	}
+      /* See if there is a breakpoint at the current PC.  */
+      stop_bpstat = bpstat_stop_status (stop_pc, ecs->ptid);
+      
+      /* Following in case break condition called a
+	 function.  */
+      stop_print_frame = 1;
 
       /* NOTE: cagney/2003-03-29: These two checks for a random signal
          at one stage in the past included checks for an inferior
-- 
1.5.3.5

Re: [RFA] Don't ignore consecutive breakpoints.

[Michael Snyder]

On Fri, 2007-11-23 at 23:10 +0300, Vladimir Prus wrote:
> Suppose we have two breakpoints at two consecutive
> addresses, and we do "step" while stopped on the
> first breakpoint. GDB testsuite has a test (consecutive.exp)
> that the second breakpoint will be hit a reported, and the

Yeah, I was the author of that test, back in 2001.
Several years and several employers ago, but I think 
I am able to remember a little about the context.

> test passes, but the code directly contradicts, saying:
> 
>       /* Don't even think about breakpoints if just proceeded over a
>          breakpoint.  */
>       if (stop_signal == TARGET_SIGNAL_TRAP && trap_expected)
> 	{
>           if (debug_infrun)
> 	    fprintf_unfiltered (gdb_stdlog, "infrun: trap expected\n");
> 	  bpstat_clear (&stop_bpstat);
> 	}
> 
> what's happening is that we indeed ignore the breakpoint, and try
> to step further. However ecs->another_trap is not set, so we step
> with breakpoints inserted, and immediately hit the now-inserted
> breakpoint. Therefore, I propose to remove that code.
> 
> On x86, the below patch causes a single test outcome change:
> 
> -KFAIL: gdb.base/watchpoint.exp: next after watch x (PRMS: gdb/38)
> +PASS: gdb.base/watchpoint.exp: next after watch x

Yeah, the problem is that you have only tested x86 architecture, 
and what I think I recall is that this test was for software
single-step.

You have to be aware that you have just single-stepped, so that
you interpret the trap instruction under the PC as related to 
stepping.  If you have two consecutive BP-related traps, and you
try to single step over one of them, you may miss the second one
because you believe it to be only a single-stepping trap.

Can you test your patch on an architecture that uses software SS?

Thanks,
Michael

Re: [RFA] Don't ignore consecutive breakpoints.

[Vladimir Prus]

Michael Snyder wrote:

> On Fri, 2007-11-23 at 23:10 +0300, Vladimir Prus wrote:
>> Suppose we have two breakpoints at two consecutive
>> addresses, and we do "step" while stopped on the
>> first breakpoint. GDB testsuite has a test (consecutive.exp)
>> that the second breakpoint will be hit a reported, and the
> 
> Yeah, I was the author of that test, back in 2001.
> Several years and several employers ago, but I think
> I am able to remember a little about the context.
> 
>> test passes, but the code directly contradicts, saying:
>> 
>>       /* Don't even think about breakpoints if just proceeded over a
>>          breakpoint.  */
>>       if (stop_signal == TARGET_SIGNAL_TRAP && trap_expected)
>> {
>>           if (debug_infrun)
>> fprintf_unfiltered (gdb_stdlog, "infrun: trap expected\n");
>> bpstat_clear (&stop_bpstat);
>> }
>> 
>> what's happening is that we indeed ignore the breakpoint, and try
>> to step further. However ecs->another_trap is not set, so we step
>> with breakpoints inserted, and immediately hit the now-inserted
>> breakpoint. Therefore, I propose to remove that code.
>> 
>> On x86, the below patch causes a single test outcome change:
>> 
>> -KFAIL: gdb.base/watchpoint.exp: next after watch x (PRMS: gdb/38)
>> +PASS: gdb.base/watchpoint.exp: next after watch x
> 
> Yeah, the problem is that you have only tested x86 architecture,
> and what I think I recall is that this test was for software
> single-step.
> 
> You have to be aware that you have just single-stepped, so that
> you interpret the trap instruction under the PC as related to
> stepping.  If you have two consecutive BP-related traps, and you
> try to single step over one of them, you may miss the second one
> because you believe it to be only a single-stepping trap.
> 
> Can you test your patch on an architecture that uses software SS?

Sure, I'll test it on arm, which uses software single step. Given
that Ulrich reports software single step breakage from my other
patch, it seems like software single step is an important variable
that I did not test :-(

Thanks,
Volodya

Re: [RFA] Don't ignore consecutive breakpoints.

[Vladimir Prus]

On Monday 26 November 2007 21:39:21 Michael Snyder wrote:
> On Fri, 2007-11-23 at 23:10 +0300, Vladimir Prus wrote:
> > Suppose we have two breakpoints at two consecutive
> > addresses, and we do "step" while stopped on the
> > first breakpoint. GDB testsuite has a test (consecutive.exp)
> > that the second breakpoint will be hit a reported, and the
> 
> Yeah, I was the author of that test, back in 2001.
> Several years and several employers ago, but I think 
> I am able to remember a little about the context.
> 
> > test passes, but the code directly contradicts, saying:
> > 
> >       /* Don't even think about breakpoints if just proceeded over a
> >          breakpoint.  */
> >       if (stop_signal == TARGET_SIGNAL_TRAP && trap_expected)
> > 	{
> >           if (debug_infrun)
> > 	    fprintf_unfiltered (gdb_stdlog, "infrun: trap expected\n");
> > 	  bpstat_clear (&stop_bpstat);
> > 	}
> > 
> > what's happening is that we indeed ignore the breakpoint, and try
> > to step further. However ecs->another_trap is not set, so we step
> > with breakpoints inserted, and immediately hit the now-inserted
> > breakpoint. Therefore, I propose to remove that code.
> > 
> > On x86, the below patch causes a single test outcome change:
> > 
> > -KFAIL: gdb.base/watchpoint.exp: next after watch x (PRMS: gdb/38)
> > +PASS: gdb.base/watchpoint.exp: next after watch x
> 
> Yeah, the problem is that you have only tested x86 architecture, 
> and what I think I recall is that this test was for software
> single-step.
> 
> You have to be aware that you have just single-stepped, so that
> you interpret the trap instruction under the PC as related to 
> stepping.  If you have two consecutive BP-related traps, and you
> try to single step over one of them, you may miss the second one
> because you believe it to be only a single-stepping trap.
> 
> Can you test your patch on an architecture that uses software SS?

I've tested on arm-linux/qemu, which uses software single-step,
and got no regressions.

Looking again at the patch, the code fragment I'm changing has
two side-effects:

- Setting ecs->random_signal
- Setting stop_bpstat

My patch has no effect on the way ecs->random_signal is set.
However, in the case when we've just single-stepped over
breakpoint, the original code will clear stop_bpstat, and in
my patch, it would be set. We will immediately report report
the hit of the consecutive breakpoint. Since we don't set
ecs->another_trap, the trap_expected variable will be reset
to 0 when we resume.

So, is the patch OK?

- Volodya




- Volodya

Re: [RFA] Don't ignore consecutive breakpoints.

[Michael Snyder]

On Thu, 2007-11-29 at 14:27 +0300, Vladimir Prus wrote:
> On Monday 26 November 2007 21:39:21 Michael Snyder wrote:
> > On Fri, 2007-11-23 at 23:10 +0300, Vladimir Prus wrote:
> > > Suppose we have two breakpoints at two consecutive
> > > addresses, and we do "step" while stopped on the
> > > first breakpoint. GDB testsuite has a test (consecutive.exp)
> > > that the second breakpoint will be hit a reported, and the
> > 
> > Yeah, I was the author of that test, back in 2001.
> > Several years and several employers ago, but I think 
> > I am able to remember a little about the context.
> > 
> > > test passes, but the code directly contradicts, saying:
> > > 
> > >       /* Don't even think about breakpoints if just proceeded over a
> > >          breakpoint.  */
> > >       if (stop_signal == TARGET_SIGNAL_TRAP && trap_expected)
> > > 	{
> > >           if (debug_infrun)
> > > 	    fprintf_unfiltered (gdb_stdlog, "infrun: trap expected\n");
> > > 	  bpstat_clear (&stop_bpstat);
> > > 	}
> > > 
> > > what's happening is that we indeed ignore the breakpoint, and try
> > > to step further. However ecs->another_trap is not set, so we step
> > > with breakpoints inserted, and immediately hit the now-inserted
> > > breakpoint. Therefore, I propose to remove that code.
> > > 
> > > On x86, the below patch causes a single test outcome change:
> > > 
> > > -KFAIL: gdb.base/watchpoint.exp: next after watch x (PRMS: gdb/38)
> > > +PASS: gdb.base/watchpoint.exp: next after watch x
> > 
> > Yeah, the problem is that you have only tested x86 architecture, 
> > and what I think I recall is that this test was for software
> > single-step.
> > 
> > You have to be aware that you have just single-stepped, so that
> > you interpret the trap instruction under the PC as related to 
> > stepping.  If you have two consecutive BP-related traps, and you
> > try to single step over one of them, you may miss the second one
> > because you believe it to be only a single-stepping trap.
> > 
> > Can you test your patch on an architecture that uses software SS?
> 
> I've tested on arm-linux/qemu, which uses software single-step,
> and got no regressions.
> 
> Looking again at the patch, the code fragment I'm changing has
> two side-effects:
> 
> - Setting ecs->random_signal
> - Setting stop_bpstat
> 
> My patch has no effect on the way ecs->random_signal is set.
> However, in the case when we've just single-stepped over
> breakpoint, the original code will clear stop_bpstat, and in
> my patch, it would be set. We will immediately report report
> the hit of the consecutive breakpoint. Since we don't set
> ecs->another_trap, the trap_expected variable will be reset
> to 0 when we resume.
> 
> So, is the patch OK?

Thanks for the testing and analysis.
I have no further objection.

Michael

Re: [RFA] Don't ignore consecutive breakpoints.

[Vladimir Prus]

On Friday 30 November 2007 04:14:35 Michael Snyder wrote:

> > > Can you test your patch on an architecture that uses software SS?
> >
> > I've tested on arm-linux/qemu, which uses software single-step,
> > and got no regressions.
> >
> > Looking again at the patch, the code fragment I'm changing has
> > two side-effects:
> >
> > - Setting ecs->random_signal
> > - Setting stop_bpstat
> >
> > My patch has no effect on the way ecs->random_signal is set.
> > However, in the case when we've just single-stepped over
> > breakpoint, the original code will clear stop_bpstat, and in
> > my patch, it would be set. We will immediately report report
> > the hit of the consecutive breakpoint. Since we don't set
> > ecs->another_trap, the trap_expected variable will be reset
> > to 0 when we resume.
> >
> > So, is the patch OK?
>
> Thanks for the testing and analysis.
> I have no further objection.

Thanks, checked in.

- Volodya