[RFA] Fix watchpoints when stepping over a breakpoint

[RFA] Fix watchpoints when stepping over a breakpoint

[Daniel Jacobowitz]

For some of the history of this problem, see PR gdb/81 and:
  http://sourceware.cygnus.com/ml/gdb-patches/2000-q1/msg00665.html

How Peter's or Jim's patches worked for them, I'm not entirely sure; I
suspect there's been a change to shared library debugging since then, and/or
a severe deficiency in the testsuite somewhere.  When I tried it I got stuck
on the bp_shlib_event breakpoint.  We would hit it, remove, single-step
(trap expected), check where we were... and find ourselves at the
shared library breakpoint again because of DECR_PC_AFTER_BREAK!

Instead, I now collect only non-breakpoint events.  This appears to work
fine.  It causes no changes in the testsuite; a trivial followup patch to
update the formatting in annota2.exp fixes the test in question.  Is this OK
to commit?  Anyone see a problem with my method?

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

2002-04-02  Daniel Jacobowitz  <drow@mvista.com>

	Fix PR gdb/81
	* breakpoint.c (bpstat_stop_status): Add ignore_breakpoints
	argument.  If ignore_breakpoints is nonzero, skip all
	breakpoint-like events.
	* breakpoint.h (bpstat_stop_status): Update prototype.
	* infrun.c (handle_inferior_event): Update calls to
	bpstat_stop_status.  Call bpstat_stop_status even if
	trap_expected.

Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.56
diff -u -p -r1.56 infrun.c
--- infrun.c	2002/03/18 02:26:31	1.56
+++ infrun.c	2002/04/02 23:17:48
@@ -1417,6 +1417,7 @@ handle_inferior_event (struct execution_
 {
   CORE_ADDR tmp;
   int stepped_after_stopped_by_watchpoint;
+  int ignore_breakpoints;
 
   /* Cache the last pid/waitstatus. */
   target_last_wait_ptid = ecs->ptid;
@@ -1606,7 +1607,8 @@ handle_inferior_event (struct execution_
 	stop_bpstat = bpstat_stop_status (&stop_pc, 
 					  currently_stepping (ecs) &&
 					  prev_pc != 
-					  stop_pc - DECR_PC_AFTER_BREAK);
+					  stop_pc - DECR_PC_AFTER_BREAK,
+					  0);
 	ecs->random_signal = !bpstat_explains_signal (stop_bpstat);
 	inferior_ptid = ecs->saved_inferior_ptid;
 	goto process_event_stop_test;
@@ -1665,7 +1667,8 @@ handle_inferior_event (struct execution_
 	stop_bpstat = bpstat_stop_status (&stop_pc, 
 					  currently_stepping (ecs) &&
 					  prev_pc !=
-					  stop_pc - DECR_PC_AFTER_BREAK);
+					  stop_pc - DECR_PC_AFTER_BREAK,
+					  0);
 	ecs->random_signal = !bpstat_explains_signal (stop_bpstat);
 	goto process_event_stop_test;
 
@@ -1740,7 +1743,8 @@ handle_inferior_event (struct execution_
 	stop_bpstat = bpstat_stop_status (&stop_pc, 
 					  currently_stepping (ecs) &&
 					  prev_pc !=
-					  stop_pc - DECR_PC_AFTER_BREAK);
+					  stop_pc - DECR_PC_AFTER_BREAK,
+					  0);
 	ecs->random_signal = !bpstat_explains_signal (stop_bpstat);
 	inferior_ptid = ecs->saved_inferior_ptid;
 	goto process_event_stop_test;
@@ -2097,44 +2101,45 @@ handle_inferior_event (struct execution_
 	    return;
 	  }
 
-	/* Don't even think about breakpoints
-	   if just proceeded over a breakpoint.
+	/* See if there is a breakpoint at the current PC.
+	   Older versions of GDB did not call bpstat_stop_status after
+	   proceeding over a breakpoint, causing missed watchpoints when
+	   proceeding over a breakpoint on an instruction which triggers
+	   a watchpoint.  */
 
-	   However, if we are trying to proceed over a breakpoint
-	   and end up in sigtramp, then through_sigtramp_breakpoint
-	   will be set and we should check whether we've hit the
-	   step breakpoint.  */
 	if (stop_signal == TARGET_SIGNAL_TRAP && trap_expected
 	    && through_sigtramp_breakpoint == NULL)
-	  bpstat_clear (&stop_bpstat);
+	  {
+	    ignore_breakpoints = 1;
+	  }
 	else
 	  {
-	    /* See if there is a breakpoint at the current PC.  */
-
-	    /* The second argument of bpstat_stop_status is meant to help
-	       distinguish between a breakpoint trap and a singlestep trap.
-	       This is only important on targets where DECR_PC_AFTER_BREAK
-	       is non-zero.  The prev_pc test is meant to distinguish between
-	       singlestepping a trap instruction, and singlestepping thru a
-	       jump to the instruction following a trap instruction. */
-
-	    stop_bpstat = bpstat_stop_status
-	      (&stop_pc,
-	    /* Pass TRUE if our reason for stopping is something other
-	       than hitting a breakpoint.  We do this by checking that
-	       1) stepping is going on and 2) we didn't hit a breakpoint
-	       in a signal handler without an intervening stop in
-	       sigtramp, which is detected by a new stack pointer value
-	       below any usual function calling stack adjustments.  */
-		(currently_stepping (ecs)
-		 && prev_pc != stop_pc - DECR_PC_AFTER_BREAK
-		 && !(step_range_end
-		      && INNER_THAN (read_sp (), (step_sp - 16))))
-	      );
-	    /* Following in case break condition called a
-	       function.  */
+	    ignore_breakpoints = 0;
+	    /* Following in case break condition called a function.  */
 	    stop_print_frame = 1;
 	  }
+
+	/* The second argument of bpstat_stop_status is meant to help
+	   distinguish between a breakpoint trap and a singlestep trap.
+	   This is only important on targets where DECR_PC_AFTER_BREAK
+	   is non-zero.  The prev_pc test is meant to distinguish between
+	   singlestepping a trap instruction, and singlestepping thru a
+	   jump to the instruction following a trap instruction. */
+
+	stop_bpstat = bpstat_stop_status
+	  (&stop_pc,
+	   /* Pass TRUE if our reason for stopping is something other
+	      than hitting a breakpoint.  We do this by checking that
+	      1) stepping is going on and 2) we didn't hit a breakpoint
+	      in a signal handler without an intervening stop in
+	      sigtramp, which is detected by a new stack pointer value
+	      below any usual function calling stack adjustments.  */
+	   (ignore_breakpoints
+	    || (currently_stepping (ecs)
+		&& prev_pc != stop_pc - DECR_PC_AFTER_BREAK
+		&& !(step_range_end
+		     && INNER_THAN (read_sp (), (step_sp - 16))))),
+	   ignore_breakpoints);
 
 	if (stop_signal == TARGET_SIGNAL_TRAP)
 	  ecs->random_signal
Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.68
diff -u -p -r1.68 breakpoint.c
--- breakpoint.c	2002/03/28 01:35:55	1.68
+++ breakpoint.c	2002/04/02 23:17:49
@@ -2345,11 +2345,14 @@ which its expression is valid.\n");     
    watchpoint at which we have stopped.  (We may have stopped for
    several reasons concurrently.)
 
+   If IGNORE_BREAKPOINTS, we return only non-breakpoint results
+   (e.g. any watchpoints we may have hit).
+
    Each element of the chain has valid next, breakpoint_at,
    commands, FIXME??? fields.  */
 
 bpstat
-bpstat_stop_status (CORE_ADDR *pc, int not_a_breakpoint)
+bpstat_stop_status (CORE_ADDR *pc, int not_a_breakpoint, int ignore_breakpoints)
 {
   register struct breakpoint *b, *temp;
   CORE_ADDR bp_addr;
@@ -2390,6 +2393,8 @@ bpstat_stop_status (CORE_ADDR *pc, int n
 	&& b->type != bp_catch_catch
 	&& b->type != bp_catch_throw)	/* a non-watchpoint bp */
       {
+	if (ignore_breakpoints)
+	  continue;
 	if (b->address != bp_addr) 	/* address doesn't match */
 	  continue;
 	if (overlay_debugging		/* unmapped overlay section */
Index: breakpoint.h
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.h,v
retrieving revision 1.11
diff -u -p -r1.11 breakpoint.h
--- breakpoint.h	2002/02/06 18:31:07	1.11
+++ breakpoint.h	2002/04/02 23:17:49
@@ -322,7 +322,7 @@ extern void bpstat_clear (bpstat *);
    is part of the bpstat is copied as well.  */
 extern bpstat bpstat_copy (bpstat);
 
-extern bpstat bpstat_stop_status (CORE_ADDR *, int);
+extern bpstat bpstat_stop_status (CORE_ADDR *, int, int);
 \f
 /* This bpstat_what stuff tells wait_for_inferior what to do with a
    breakpoint (a challenging task).  */

Re: [RFA] Fix watchpoints when stepping over a breakpoint

[Eli Zaretskii]

> Date: Tue, 2 Apr 2002 18:43:33 -0500
> From: Daniel Jacobowitz <drow@mvista.com>
> 
> When I tried it I got stuck
> on the bp_shlib_event breakpoint.  We would hit it, remove, single-step
> (trap expected), check where we were... and find ourselves at the
> shared library breakpoint again because of DECR_PC_AFTER_BREAK!
> 
> Instead, I now collect only non-breakpoint events.

Isn't this a bit ad hoc?  I think the issue of doing TRT when both a
breakpoint and a watchpoint fire for the same instruction needs a more
general solution.  While ignoring breakpoints might be the Right Thing
in this particular case, I wonder what will be TRT in other cases?

Did you try to arrange for a normal breakpoint and a watchpoint on the
same instruction, and see what happens in that case, with and without
this patch?

Re: [RFA] Fix watchpoints when stepping over a breakpoint

[Daniel Jacobowitz]

On Fri, Apr 05, 2002 at 10:34:30AM +0300, Eli Zaretskii wrote:
> > Date: Tue, 2 Apr 2002 18:43:33 -0500
> > From: Daniel Jacobowitz <drow@mvista.com>
> > 
> > When I tried it I got stuck
> > on the bp_shlib_event breakpoint.  We would hit it, remove, single-step
> > (trap expected), check where we were... and find ourselves at the
> > shared library breakpoint again because of DECR_PC_AFTER_BREAK!
> > 
> > Instead, I now collect only non-breakpoint events.
> 
> Isn't this a bit ad hoc?  I think the issue of doing TRT when both a
> breakpoint and a watchpoint fire for the same instruction needs a more
> general solution.  While ignoring breakpoints might be the Right Thing
> in this particular case, I wonder what will be TRT in other cases?
> 
> Did you try to arrange for a normal breakpoint and a watchpoint on the
> same instruction, and see what happens in that case, with and without
> this patch?

Yes, I did - that's 'watch a.x' in gdb.c++/annota2.exp.  Without the
patch it fails on i386-linux, with it it passes.

I don't really think it's any more ad-hoc than the trap_expected flag. 
If trap_expected is set, then we expected to stop on the next
instruction because of some property of (either single-stepping or)
that instruction.  Breakpoints are also a property of that instruction;
so we ignore them when checking for any other interesting reason to
have stopped.  Watchpoints, on the other hand, are clearly different.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA] Fix watchpoints when stepping over a breakpoint

[Eli Zaretskii]

> Date: Fri, 5 Apr 2002 10:54:16 -0500
> From: Daniel Jacobowitz <drow@mvista.com>
> > 
> > Isn't this a bit ad hoc?  I think the issue of doing TRT when both a
> > breakpoint and a watchpoint fire for the same instruction needs a more
> > general solution.  While ignoring breakpoints might be the Right Thing
> > in this particular case, I wonder what will be TRT in other cases?
> > 
> > Did you try to arrange for a normal breakpoint and a watchpoint on the
> > same instruction, and see what happens in that case, with and without
> > this patch?
> 
> Yes, I did - that's 'watch a.x' in gdb.c++/annota2.exp.  Without the
> patch it fails on i386-linux, with it it passes.

``Fails'' and ``passes'' are in the eyes of the beholder ;-)

I mean, I'm not even sure what is the ``right'' behavior in this case.
The annota2.exp test expects something very specific, but is that what
we want?  Perhaps GDB should say that both breakpoint and watchpoint
fired instead, or do something else?

I'd suggest to discuss this a bit, because otherwise I don't even know
what are the criteria for approving or rejecting the patch.  The mere
fact that the number of testsuite failures goes down is not enough,
IMHO.

> I don't really think it's any more ad-hoc than the trap_expected flag. 

Perhaps not, but that doesn't mean we should proliferate ad-hoc'ery.

More importantly, an introduction of a general-purpose mechanism to
ignore breakpoints is something that I consider to be dangerous,
because it is no longer limited to special situations such as
single-stepping.

Re: [RFA] Fix watchpoints when stepping over a breakpoint

[Daniel Jacobowitz]

On Fri, Apr 05, 2002 at 07:41:16PM +0300, Eli Zaretskii wrote:
> > Date: Fri, 5 Apr 2002 10:54:16 -0500
> > From: Daniel Jacobowitz <drow@mvista.com>
> > > 
> > > Isn't this a bit ad hoc?  I think the issue of doing TRT when both a
> > > breakpoint and a watchpoint fire for the same instruction needs a more
> > > general solution.  While ignoring breakpoints might be the Right Thing
> > > in this particular case, I wonder what will be TRT in other cases?
> > > 
> > > Did you try to arrange for a normal breakpoint and a watchpoint on the
> > > same instruction, and see what happens in that case, with and without
> > > this patch?
> > 
> > Yes, I did - that's 'watch a.x' in gdb.c++/annota2.exp.  Without the
> > patch it fails on i386-linux, with it it passes.
> 
> ``Fails'' and ``passes'' are in the eyes of the beholder ;-)
> 
> I mean, I'm not even sure what is the ``right'' behavior in this case.
> The annota2.exp test expects something very specific, but is that what
> we want?  Perhaps GDB should say that both breakpoint and watchpoint
> fired instead, or do something else?
>
> I'd suggest to discuss this a bit, because otherwise I don't even know
> what are the criteria for approving or rejecting the patch.  The mere
> fact that the number of testsuite failures goes down is not enough,
> IMHO.

I think GDB ought to show that both the breakpoint and watchpoint have
fired.  At least, that's the behavior I would expect. I also thought
that was what it would do, but I can't seem to make that happen. 
Barring that, I personally believe that watchpoints should trump
breakpoints; if you stop at a line and have a breakpoint there, you
know you hit the breakpoint.  GDB should tell you if you are stopping
for another reason.

Also bear in mind that if you have this sequence:
- write to x
- other instruction    <--- breakpoint here
You will stop based on the watchpoint, because the watchpoint happens
first.  You'll get a trap just before you would have hit the
breakpoint. GDB handles this OK, and does report the watchpoint.  It's
only if we expected a trap (single stepping for instance) that this
does not work.  The current failure is "single step over something
which triggers the watchpoint, landing on something with a breakpoint". 
Without my patch, we detect that we are at an address with a
breakpoint, and don't even try to check our watchpoints.



[In fact, I'm having a great deal of trouble with hardware watchpoints
surviving re-running.  Remember that conversation from several months
ago?

var.c:
int x;

int
main ()
{
  x = 0;
  x = 1;
  x = 2;
  return x;
}


(gdb) watch x
Hardware watchpoint 1: x
(gdb) r
Starting program: /home/drow/debugging/stabrel/foo/foo
Hardware watchpoint 1: x
Hardware watchpoint 1: x
Hardware watchpoint 1: x

Old value = 0
New value = 1
main () at var.c:8
8         x = 2;
(gdb) c
Continuing.
Hardware watchpoint 1: x

Old value = 1
New value = 2
main () at var.c:9
9         return x;
(gdb)
Continuing.

Program exited with code 02.
(gdb) r
Starting program: /home/drow/debugging/stabrel/foo/foo
Hardware watchpoint 1: x
Hardware watchpoint 1: x

Program exited with code 02.


First of all, it stops the instruction after where I would expect. 
That's probably my expectations being off, however.  More important is
the fact that it doesn't stop at all if I re-run.  I'll look into this
(at least a testcase...).]


> 
> > I don't really think it's any more ad-hoc than the trap_expected flag. 
> 
> Perhaps not, but that doesn't mean we should proliferate ad-hoc'ery.
> 
> More importantly, an introduction of a general-purpose mechanism to
> ignore breakpoints is something that I consider to be dangerous,
> because it is no longer limited to special situations such as
> single-stepping.

Well, we could just as easily call the flag "single_stepping"...  That
would probably limit abuse.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA] Fix watchpoints when stepping over a breakpoint

[Andrew Cagney]

> Also bear in mind that if you have this sequence:
> - write to x
> - other instruction    <--- breakpoint here
> You will stop based on the watchpoint, because the watchpoint happens
> first.  You'll get a trap just before you would have hit the
> breakpoint. GDB handles this OK, and does report the watchpoint.  It's
> only if we expected a trap (single stepping for instance) that this
> does not work.  The current failure is "single step over something
> which triggers the watchpoint, landing on something with a breakpoint". 
> Without my patch, we detect that we are at an address with a
> breakpoint, and don't even try to check our watchpoints.

I suspect this is a design problem.  Blame DECR_PC?  The ``stop'' 
address gets confused with the PC value and the decr's probably screw it.

Andrew

Re: [RFA] Fix watchpoints when stepping over a breakpoint

[Eli Zaretskii]

> Date: Fri, 5 Apr 2002 12:08:51 -0500
> From: Daniel Jacobowitz <drow@mvista.com>
> 
> I think GDB ought to show that both the breakpoint and watchpoint have
> fired.  At least, that's the behavior I would expect. I also thought
> that was what it would do, but I can't seem to make that happen. 

Try using a hardware-assisted breakpoint, not a normal breakpoint.
Since the latter works by replacing the instruction with a breakpoint
opcode, you cannot have a breakpoint and a watchpoint at exactly the
same PC value, because doing so replaces the instruction that's
supposed to write into some data with the breakpoint opcode.

> Also bear in mind that if you have this sequence:
> - write to x
> - other instruction    <--- breakpoint here
> You will stop based on the watchpoint, because the watchpoint happens
> first.

That's okay, since the instruction that writes to x is before the
breakpoint.  In this case, I'd expect to have a watchpoint, then,
when I continue, I'd expect to hit the breakpoint.

> It's only if we expected a trap (single stepping for instance) that
> this does not work.

If this is limited to stepping, can we check whether we are stepping
instead of (or in addition to) the test for whether to ignore
breakpoints?

> Without my patch, we detect that we are at an address with a
> breakpoint, and don't even try to check our watchpoints.

If we change GDB to report both the breakpoint and watchpoint, the
problem would go away, no?

> [In fact, I'm having a great deal of trouble with hardware watchpoints
> surviving re-running.  Remember that conversation from several months
> ago?

Yes.  This is definitely wrong behavior, IMHO.  IIRC, the problem is
that GDB doesn't initialize the ``old value'' correctly on the rerun,
and so when the watchpoint hits, it thinks it's a false positive,
because watchpoints are suppressed if the watched value doesn't
change.

> > More importantly, an introduction of a general-purpose mechanism to
> > ignore breakpoints is something that I consider to be dangerous,
> > because it is no longer limited to special situations such as
> > single-stepping.
> 
> Well, we could just as easily call the flag "single_stepping"...  That
> would probably limit abuse.

If all else fails, at least that, yes.

Re: [RFA] Fix watchpoints when stepping over a breakpoint

[Daniel Jacobowitz]

On Sat, Apr 06, 2002 at 10:41:45AM +0300, Eli Zaretskii wrote:
> > Date: Fri, 5 Apr 2002 12:08:51 -0500
> > From: Daniel Jacobowitz <drow@mvista.com>
> > 
> > I think GDB ought to show that both the breakpoint and watchpoint have
> > fired.  At least, that's the behavior I would expect. I also thought
> > that was what it would do, but I can't seem to make that happen. 
> 
> Try using a hardware-assisted breakpoint, not a normal breakpoint.
> Since the latter works by replacing the instruction with a breakpoint
> opcode, you cannot have a breakpoint and a watchpoint at exactly the
> same PC value, because doing so replaces the instruction that's
> supposed to write into some data with the breakpoint opcode.

You can't anyway.  You break before an instruction is executed and
watchpoint before the next instruction is executed, right?

> > Also bear in mind that if you have this sequence:
> > - write to x
> > - other instruction    <--- breakpoint here
> > You will stop based on the watchpoint, because the watchpoint happens
> > first.
> 
> That's okay, since the instruction that writes to x is before the
> breakpoint.  In this case, I'd expect to have a watchpoint, then,
> when I continue, I'd expect to hit the breakpoint.
> 
> > It's only if we expected a trap (single stepping for instance) that
> > this does not work.
> 
> If this is limited to stepping, can we check whether we are stepping
> instead of (or in addition to) the test for whether to ignore
> breakpoints?

Well, I set the ignore breakpoints flag in the caller only if we are
stepping.

> > Without my patch, we detect that we are at an address with a
> > breakpoint, and don't even try to check our watchpoints.
> 
> If we change GDB to report both the breakpoint and watchpoint, the
> problem would go away, no?

No.  In my original message I made a comment about shlib_event
breakpoints being a problem.  Other breakpoints would to.  This is all
because of the "watchpoint after instr, breakpoint before" thing - we
would still have to deal with this, or we'd just keep hitting the same
breakpoint over and over if there was a watchpoint on the next
instruction.

> > [In fact, I'm having a great deal of trouble with hardware watchpoints
> > surviving re-running.  Remember that conversation from several months
> > ago?
> 
> Yes.  This is definitely wrong behavior, IMHO.  IIRC, the problem is
> that GDB doesn't initialize the ``old value'' correctly on the rerun,
> and so when the watchpoint hits, it thinks it's a false positive,
> because watchpoints are suppressed if the watched value doesn't
> change.

That too.  But something more fundamental is wrong, because we never
stop -at all-.  I remember something involving initializing the
watchpoint registers...

> > > More importantly, an introduction of a general-purpose mechanism to
> > > ignore breakpoints is something that I consider to be dangerous,
> > > because it is no longer limited to special situations such as
> > > single-stepping.
> > 
> > Well, we could just as easily call the flag "single_stepping"...  That
> > would probably limit abuse.
> 
> If all else fails, at least that, yes.
> 

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA] Fix watchpoints when stepping over a breakpoint

[Eli Zaretskii]

> Date: Sat, 6 Apr 2002 10:36:21 -0500
> From: Daniel Jacobowitz <drow@mvista.com>
> > 
> > Try using a hardware-assisted breakpoint, not a normal breakpoint.
> > Since the latter works by replacing the instruction with a breakpoint
> > opcode, you cannot have a breakpoint and a watchpoint at exactly the
> > same PC value, because doing so replaces the instruction that's
> > supposed to write into some data with the breakpoint opcode.
> 
> You can't anyway.  You break before an instruction is executed and
> watchpoint before the next instruction is executed, right?

On x86, yes.  But the question what does GDB do when the watchpoint
fires remains anyway.  GDB should report the watchpoint, not the
breakpoint.

> > If this is limited to stepping, can we check whether we are stepping
> > instead of (or in addition to) the test for whether to ignore
> > breakpoints?
> 
> Well, I set the ignore breakpoints flag in the caller only if we are
> stepping.

Isn't it better to make bpstat_stop_status test for stepping directly?

> No.  In my original message I made a comment about shlib_event
> breakpoints being a problem.  Other breakpoints would to.  This is all
> because of the "watchpoint after instr, breakpoint before" thing - we
> would still have to deal with this, or we'd just keep hitting the same
> breakpoint over and over if there was a watchpoint on the next
> instruction.

Sounds like Andrew was right: the decrement-PC logic is screwed.

> But something more fundamental is wrong, because we never
> stop -at all-.  I remember something involving initializing the
> watchpoint registers...

I thought that part was solved, but perhaps I'm confused.

Re: [RFA] Fix watchpoints when stepping over a breakpoint

[Daniel Jacobowitz]

On Sat, Apr 06, 2002 at 08:17:25PM +0300, Eli Zaretskii wrote:
> > > If this is limited to stepping, can we check whether we are stepping
> > > instead of (or in addition to) the test for whether to ignore
> > > breakpoints?
> > 
> > Well, I set the ignore breakpoints flag in the caller only if we are
> > stepping.
> 
> Isn't it better to make bpstat_stop_status test for stepping directly?

The test I'm using is 'trap_expected' - well:
        if (stop_signal == TARGET_SIGNAL_TRAP && trap_expected
            && through_sigtramp_breakpoint == NULL)

It's static to infrun.c, and it describes the precise circumstances in
which we want this behavior.  We can ignore stop_signal (since we only
call bpstat_stop_status if we got a trap), but I have no idea how
through_sigtramp_breakpoint works.  I'd rather calculate this at the
call site than duplicating logic I don't understand very well in
breakpoint.c (which has no business knowing about the internals of
infrun).

> > No.  In my original message I made a comment about shlib_event
> > breakpoints being a problem.  Other breakpoints would to.  This is all
> > because of the "watchpoint after instr, breakpoint before" thing - we
> > would still have to deal with this, or we'd just keep hitting the same
> > breakpoint over and over if there was a watchpoint on the next
> > instruction.
> 
> Sounds like Andrew was right: the decrement-PC logic is screwed.

I agree.  It's a rat's nest.

> > But something more fundamental is wrong, because we never
> > stop -at all-.  I remember something involving initializing the
> > watchpoint registers...
> 
> I thought that part was solved, but perhaps I'm confused.

So did I, but it does not appear to be the case.  Even access
watchpoints die.  At a guess, in the good case:

insert_watchpoint (addr=8049574, len=4, type=data-read/write):
        CONTROL (DR7): 000f0101          STATUS (DR6): ffff4ff1
        DR0: addr=0x08049574, ref.count=1  DR1: addr=0x00000000, ref.count=0
        DR2: addr=0x00000000, ref.count=0  DR3: addr=0x00000000, ref.count=0

In the bad case:

insert_watchpoint (addr=8049574, len=4, type=data-read/write):
        CONTROL (DR7): 000f0101          STATUS (DR6): ffff4ff0
        DR0: addr=0x08049574, ref.count=2  DR1: addr=0x00000000, ref.count=0
        DR2: addr=0x00000000, ref.count=0  DR3: addr=0x00000000, ref.count=0

insert_watchpoint (addr=8049574, len=4, type=data-read/write):
        CONTROL (DR7): 000f0101          STATUS (DR6): ffff4ff0
        DR0: addr=0x08049574, ref.count=3  DR1: addr=0x00000000, ref.count=0
        DR2: addr=0x00000000, ref.count=0  DR3: addr=0x00000000, ref.count=0

The watchpoint is not removed after program normal exit, and the refcount escalates.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA] Fix watchpoints when stepping over a breakpoint

[Andrew Cagney]

> No.  In my original message I made a comment about shlib_event
>> > breakpoints being a problem.  Other breakpoints would to.  This is all
>> > because of the "watchpoint after instr, breakpoint before" thing - we
>> > would still have to deal with this, or we'd just keep hitting the same
>> > breakpoint over and over if there was a watchpoint on the next
>> > instruction.
> 
>> 
>> Sounds like Andrew was right: the decrement-PC logic is screwed.
> 
> 
> I agree.  It's a rat's nest.

Only two targets use DECR_PC_AFTER_BREAK - m68k and i386.  Core GDB 
contans all all sorts of convoluted logic for what is a target problem :-(

What would it take to replace DECR_PC_AFTER_BREAK with a read_pc() 
function that determines the stop address from the i386 hardware 
registers and other state information?

(The m68k isn't multi-arch so I can fix that one by deleting it :-^)

Andrew

Re: [RFA] Fix watchpoints when stepping over a breakpoint

[Eli Zaretskii]

> Date: Sun, 07 Apr 2002 12:16:22 -0400
> From: Andrew Cagney <ac131313@cygnus.com>
> 
> What would it take to replace DECR_PC_AFTER_BREAK with a read_pc() 
> function that determines the stop address from the i386 hardware 
> registers and other state information?

How would that help to solve this specific problem?  The original
problem happened on i386 as well, right?  So at best, you'd be pushing
the ``rat's nest'' from GDB application level to x86-specific parts of
GDB, where there still will be need to decide what kind of breakpoint
to reports to the application level.

Or am I missing something?

In general, it strikes me that breakpoint.c's design goes against what
you propose: it defines an API where GDB queries the target about the
status of all the known break/watchpoints, and then decides what
happened based on what the target reports.  You seem to suggest a
different strategy: let the target tell GDB what happened.  While
certainly a viable idea, it sounds like a major redesign of a central
GDB facility, no?

Re: [RFA] Fix watchpoints when stepping over a breakpoint

[Andrew Cagney]

> Date: Sun, 07 Apr 2002 12:16:22 -0400
>> From: Andrew Cagney <ac131313@cygnus.com>
>> 
>> What would it take to replace DECR_PC_AFTER_BREAK with a read_pc() 
>> function that determines the stop address from the i386 hardware 
>> registers and other state information?

Tried it (yes i386).  It is more complicated (suprise) than I thought. 
As far as I can tell, the i386 backend can't locally differentiate 
between a single step trap or a breakpoint trap.  Instead it needs to 
refer to GDB's internal state to figure out what it was probably trying 
to do.

> How would that help to solve this specific problem?  The original
> problem happened on i386 as well, right?  So at best, you'd be pushing
> the ``rat's nest'' from GDB application level to x86-specific parts of
> GDB, where there still will be need to decide what kind of breakpoint
> to reports to the application level.
> 
> Or am I missing something?
> 
> In general, it strikes me that breakpoint.c's design goes against what
> you propose: it defines an API where GDB queries the target about the
> status of all the known break/watchpoints, and then decides what
> happened based on what the target reports.  You seem to suggest a
> different strategy: let the target tell GDB what happened.  While
> certainly a viable idea, it sounds like a major redesign of a central
> GDB facility, no?

No that would remain.  I'm just looking at the read_pc() / 
DECR_PC_AFTER_BREAK pair.  In bpstat_stop_status(), for instance, there 
is a call to get_current_frame() that creates a frame with the wrong PC 
value :-(

Andrew

Re: [RFA] Fix watchpoints when stepping over a breakpoint

[Eli Zaretskii]

On Sun, 14 Apr 2002, Andrew Cagney wrote:

> As far as I can tell, the i386 backend can't locally differentiate 
> between a single step trap or a breakpoint trap.

Really?  I thought the single-step bit in EFLAGS should be set if we are 
stepping.

In general, I'd love to see changes in GDB that would delegate more to 
the backend.  I think GDB's application level tries to second-quess the 
target too much, which is hard without having all the target-dependent 
details.  This is particularly true in the area that started this thread.
GDB should request more information from the backend instead of trying to 
figure that out on its own, IMHO.

Re: [RFA] Fix watchpoints when stepping over a breakpoint

[Andrew Cagney]

> On Sun, 14 Apr 2002, Andrew Cagney wrote:
> 
> 
>> As far as I can tell, the i386 backend can't locally differentiate 
>> between a single step trap or a breakpoint trap.
> 
> 
> Really?  I thought the single-step bit in EFLAGS should be set if we are 
> stepping.
> 
> In general, I'd love to see changes in GDB that would delegate more to 
> the backend.  I think GDB's application level tries to second-quess the 
> target too much, which is hard without having all the target-dependent 
> details.  This is particularly true in the area that started this thread.
> GDB should request more information from the backend instead of trying to 
> figure that out on its own, IMHO.

I suspect I need two bits of information:  the stop signal and the 
hardware registers.  I didn't know the single-step bit would remain set.

I'll try to give it another go (but don't hold your breath :-)

Andrew

Re: [RFA] Fix watchpoints when stepping over a breakpoint

[Michael Elizabeth Chastain]

My test bed says: no significant changes.

  target  = native
  host    = i686-pc-linux-gnu%rh-7.2
  gdb     = HEAD%20020331 + drow infrun.c
  gcc     = 2.95.3, vendor, 3.0.4, gcc-3_1-branch%20020331, HEAD%20020331
  glibc   = vendor
  goption = -gdwarf-2, -gstabs+

I'm not qualified to give an opinion on infrun.c changes, but I can say
that this one don't break anything on native i686-pc-linux-gnu.

I also note that there is an insignificant change in this test:

  gdb.c++/annota2.exp: annotate-quit

This test is unstable and changes a lot, even on identical back-to-back runs.
Auspiciously all of the noise on this comparison was FAIL->PASS noise but
it's probably meaningless.

Michael C