This is the mail archive of the gdb-patches@sourceware.cygnus.com mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]

RFA: breakpoint.c and infrun.c changes


I am looking at a bug on i386 Linux in which temporary breakpoints
are not getting removed upon being stepped onto.  It turns out that
the bug affects all break points, not just temporary ones.  On Linux,
you wouldn't see any indication that you hit a breakpoint if you were
to single step onto it.

My patch for fixing this bug is below.  The changes to breakpoint.c
alone fix the bug (on the i386 anyway); the changes to infrun.c are
cleanup.

I will now describe my changes and (attempt to) justify them.  I'll
begin by examining the calls to bpstat_stop_status().

....

The second parameter to bpstat_stop_status() is called
`not_a_breakpoint'.  Understanding how this gets set is critical to
understanding the reasons for my changes to breakpoint.c.  There are
exactly four call sites for bpstat_stop_status() and they all occur in
handle_inferior_event() in infrun.c.  Three of these calls are
identical; the other one contains some additional baggage.

If you look at the most complicated call to bpstat_stop_status() in
infrun.c, you'll see that it looks like this:

  (1)       /* See if there is a breakpoint at the current PC.  */
            stop_bpstat = bpstat_stop_status
              (&stop_pc,
               (DECR_PC_AFTER_BREAK ?
            /* Notice the case of stepping through a jump
               that lands just after a breakpoint.
               Don't confuse that with hitting the breakpoint.
               What we check for is that 1) stepping is going on
               and 2) the pc before the last insn does not match
               the address of the breakpoint before the current pc
               and 3) 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)))) :
                0)
              );

Even one of the simpler ones is still pretty complicated:

  (2)       stop_bpstat = bpstat_stop_status
              (&stop_pc,
               (DECR_PC_AFTER_BREAK ?
                (prev_pc != stop_pc - DECR_PC_AFTER_BREAK
                 && currently_stepping (ecs))
                : 0)
              );

In either of these cases, the first thing to notice is that
not_a_breakpoint will be set to zero if DECR_PC_AFTER_BREAK
is also zero.

When DECR_PC_AFTER_BREAK is zero, not_a_breakpoint will be TRUE
if we're currently stepping and some other conditions hold.  (See
the above code snippets for a precise list.)

The main thing I want you to notice at this point is that
not_a_breakpoint will be zero if DECR_PC_AFTER_BREAK is zero.
(I will argue that this is wrong later on.)

With that out of the way, let's begin a detailed examination of
the changes to breakpoint.c.

....

The first change regards the setting of bp_addr which is actually the
point at which execution has been stopped after accounting for any
adjustments needed due to the processor possibly leaving the PC after
a breakpoint instead of on it.

The old code looks like this

  (3)       bp_addr = *pc - DECR_PC_AFTER_BREAK;

It is wrong because it fails to take into account why we arrived at
the breakpoint.  (I.e, was it due to actually hitting a breakpoint, or
was it due to a hardware singlestep, or possibly some other reason
entirely.) On any of the architectures where DECR_PC_AFTER_BREAK is
non-zero, this constant tells us how much to back up to get to the
real instruction when a breakpoint is hit.  However, if we've stopped
for some other reason than hitting a breakpoint, we don't want to back
up since that would put us at the wrong instruction.

My suggested replacement is as follows:

  (4)       bp_addr = *pc - (not_a_breakpoint && !SOFTWARE_SINGLE_STEP_P ? 
		             0 : DECR_PC_AFTER_BREAK);

An application of De Morgan's Law reveals that we'll back up by
DECR_PC_AFTER_BREAK either when we're not at a breakpoint or when the
architecture implements single stepping via breakpoints.  This is
precisely what we want.

....

My second change to breakpoint.c concerns the following lines:

  (5)       if (b->type != bp_watchpoint
                && b->type != bp_hardware_watchpoint
                && b->type != bp_read_watchpoint
                && b->type != bp_access_watchpoint
                && not_a_breakpoint)
              continue;

I removed these lines.
 
What was the intended purpose of this code?  I submit to you that
it is to prevent the problem described in this comment taken from
infrun.c [which is a part of code snippet (1) above]:

  (6)       /* Notice the case of stepping through a jump
               that lands just after a breakpoint.
               Don't confuse that with hitting the breakpoint.
               What we check for is that 1) stepping is going on
               and 2) the pc before the last insn does not match
               the address of the breakpoint before the current pc
               and 3) 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.  */

Now at this point, you should recall that earlier in this message I
analyzed how not_a_breakpoint gets set.  Basically, it turns out to
always be 0 on targets for which DECR_PC_AFTER_BREAK is 0.  The
continue statement will never be reached on these targets so this code
is effectively removed.

On targets for which DECR_PC_AFTER_BREAK is non-zero, not_a_breakpoint
will be 1 if we're single stepping and some other conditions are met
(see the call sites in infrun.c for more details).

So the code in question allowed these targets which have a non-zero
DECR_PC_AFTER_BREAK to continue analyzing the breakpoint if at some
sort of watchpoint.  Otherwise, the analysis was curtailed for fear
that the PC would inadvertently be pointing at the previous instruction
and we would falsely assume that a breakpoint was hit when stepping
through a jump.

I contend that this code is just plain wrong.  The correct thing to
do is to make sure that we put the correct address at which we were
stopped (the PC) in bp_addr.  The problem of course is that on some
targets, the PC doesn't point at the place where we stopped but somewhat
beyond.  This is indicated by DECR_PC_AFTER_BREAK.

But since the first part of my patch correctly sets bp_addr to the
address at which execution has stopped, it is no longer necessary to
curtail the breakpoint analysis in fear that we will falsely identify
a breakpoint that's been set on the previous instruction.

....

Now let's consider the changes to the call sites in infrun.c.  In
three out of the four cases, the call has been simplified from the
way it looks in code snippet (2) to look like this:

  (7)	stop_bpstat = bpstat_stop_status (&stop_pc, currently_stepping (ecs));

What has really changed here is the second parameter.  Let us examine
the logical expression for the second parameter in snippet (2):
	
  (8)      (DECR_PC_AFTER_BREAK ?
	    (prev_pc != stop_pc - DECR_PC_AFTER_BREAK
	     && currently_stepping (ecs))
	    : 0)

I already noted before that when DECR_PC_AFTER_BREAK is zero, we would
pass zero.  Why did we do this?  We did this because if we didn't,
we'd curtail the breakpoint analysis in bpstat_stop_status().  (See
the above section where I describe this in detail.) With the changes
that I have made to breakpoint.c, it doesn't matter what we pass in as
the second parameter when DECR_PC_AFTER_BREAK is zero.  (Go back and
reread the previous sentence a few times.)

To see why, you need to reexamine my first change to
bpstat_stop_status:

  (4)       bp_addr = *pc - (not_a_breakpoint && !SOFTWARE_SINGLE_STEP_P ? 
		             0 : DECR_PC_AFTER_BREAK);

The variable `not_a_breakpoint' takes on the value of logical
expression (8).  Moreover, after the removal of the code that
constitutes my second change to bpstat_stop_status() [code snippet
(5)], this is the *only* occurence of `not_a_breakpoint'.

So when DECR_PC_AFTER_BREAK is zero, the above statement may be logically
transformed to

  (9)       bp_addr = *pc;

which (effectively) removes not_a_breakpoint entirely.  (But only when
DECR_PC_AFTER_BREAK is zero.)

Since it doesn't matter what we pass in for the `not_a_breakpoint'
parameter when DECR_PC_AFTER_BREAK is zero, we might as well try to
pass a reliable indicator of whether we're at a breakpoint or not.
Therefore, I removed the test regarding DECR_PC_AFTER_BREAK.  Our
logical expression under consideration then becomes:

  (10)      (prev_pc != stop_pc - DECR_PC_AFTER_BREAK
             && currently_stepping (ecs))

I'm not removing the conditional simply because logic tells me that I
can, but because I think it's important to attempt to get the
`not_a_breakpoint' parameter set correctly regardless of the value of
DECR_PC_AFTER_BREAK.  There may be some future uses of
not_a_breakpoint in bpstat_stop_status() which will need it to be set
accurately.

But I'm not done simplifing yet.  The conjuct

  (11)      prev_pc != stop_pc - DECR_PC_AFTER_BREAK

really bothered me.  Let's think about when it might be that prev_pc
is equal to stop_pc - DECR_PC_AFTER_BREAK.  It seems to me that this
will (most commonly) happen on a RISC architecture when we are single
stepping by machine instructions.  It can also happen on a CISC, but
(11) will hardly be a reliable indicator due to the fact that
instructions are generally not of a fixed length.

So how does this contribute to an expression that we're attempting to
construct which says that our reason for stopping is due to the fact
that we didn't hit a breakpoint?  It doesn't!  Expression (11) can be
true regardless of whether we've hit a breakpoint or not.

So I removed this conjunct so that our "not-a-breakpoint" test merely
becomes a call to currently_stepping().  I believe the comment in code
snippet (6) explains why this conjunct appears.

We still have to consider the last (and I think most used) call
to bpstat_stop_status().  This was code snippet (1) and is almost
logically identical to (2) except for a bit of curious logic which
is supposed to accomodate sigtramp cases.

If our transformation of snippet (2) into (7) is correct, then we
can transform snippet (1) into:

  (12)      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)
                 && !(step_range_end
                      && INNER_THAN (read_sp (), (step_sp - 16))))
              );


The expression

  (13)              !(step_range_end
                      && INNER_THAN (read_sp (), (step_sp - 16))))

really bothers me, but it *does* seem to be needed.  (I see a couple of
additional test suite failures on Linux if it's not there.)  The reason
this test bothers me is that it was written for the i386 architecture
and I have doubts about whether that magic number 16 is correct for
all architectures.  Also, on architectures where INNER_THAN will evaluate
to >, I think the (step_sp - 16) should be changed to (step_sp + 16).

The need for expression (13) in statement (12) leads me to think that
perhaps currently_stepping() isn't quite right, but I have no proof of
this.  Also, currently_stepping() is quite complicated and I do not
understand how it works.

It would be nice if we had some simple expression which would tell us
if the reason for stopping was due to hitting a breakpoint.  I was
hoping stop_signal would give us a good indication and even tried
using the expression

  (14)       stop_signal != TARGET_SIGNAL_TRAP

as the second parameter to bpstat_stop_status(), but this didn't pan
out.  (There were test suite failures.)  I think the reason that it
didn't work is that stop_signal gets assigned TARGET_SIGNAL_TRAP
in a number of places in handle_inferior_event().  By the time we
get around to calling bpstat_stop_status(), it no longer is able
to tell us the real reason for stopping.

Enough analysis for now.  Tell me what you think.

Here are my ChangeLog entries and diffs:

	* breakpoint.c (bpstat_stop_status): Use not_a_breakpoint to
	help properly set bp_addr.
	* infrun.c (handle_inferior_event): Simplify calls to
	bp_stop_status.


Index: breakpoint.c
===================================================================
RCS file: /cvs/cvsfiles/devo/gdb/breakpoint.c,v
retrieving revision 1.243
diff -u -r1.243 breakpoint.c
--- breakpoint.c	1999/09/15 16:30:53	1.243
+++ breakpoint.c	1999/09/17 17:44:34
@@ -2195,7 +2195,8 @@
   char message[sizeof (message1) + 30 /* slop */ ];
 
   /* Get the address where the breakpoint would have been.  */
-  bp_addr = *pc - DECR_PC_AFTER_BREAK;
+  bp_addr = *pc - (not_a_breakpoint && !SOFTWARE_SINGLE_STEP_P ? 
+                   0 : DECR_PC_AFTER_BREAK);
 
   ALL_BREAKPOINTS_SAFE (b, temp)
   {
@@ -2222,13 +2223,6 @@
 
     if (b->type == bp_hardware_breakpoint
 	&& b->address != (*pc - DECR_PC_AFTER_HW_BREAK))
-      continue;
-
-    if (b->type != bp_watchpoint
-	&& b->type != bp_hardware_watchpoint
-	&& b->type != bp_read_watchpoint
-	&& b->type != bp_access_watchpoint
-	&& not_a_breakpoint)
       continue;
 
     /* Is this a catchpoint of a load or unload?  If so, did we
Index: infrun.c
===================================================================
RCS file: /cvs/cvsfiles/devo/gdb/infrun.c,v
retrieving revision 1.245
diff -u -r1.245 infrun.c
--- infrun.c	1999/09/14 23:17:58	1.245
+++ infrun.c	1999/09/17 17:44:38
@@ -1587,13 +1587,7 @@
 	stop_pc = read_pc_pid (ecs->pid);
 	ecs->saved_inferior_pid = inferior_pid;
 	inferior_pid = ecs->pid;
-	stop_bpstat = bpstat_stop_status
-	  (&stop_pc,
-	   (DECR_PC_AFTER_BREAK ?
-	    (prev_pc != stop_pc - DECR_PC_AFTER_BREAK
-	     && currently_stepping (ecs))
-	    : 0)
-	  );
+	stop_bpstat = bpstat_stop_status (&stop_pc, currently_stepping (ecs));
 	ecs->random_signal = !bpstat_explains_signal (stop_bpstat);
 	inferior_pid = ecs->saved_inferior_pid;
 	goto process_event_stop_test;
@@ -1641,13 +1635,7 @@
 	  }
 
 	stop_pc = read_pc ();
-	stop_bpstat = bpstat_stop_status
-	  (&stop_pc,
-	   (DECR_PC_AFTER_BREAK ?
-	    (prev_pc != stop_pc - DECR_PC_AFTER_BREAK
-	     && currently_stepping (ecs))
-	    : 0)
-	  );
+	stop_bpstat = bpstat_stop_status (&stop_pc, currently_stepping (ecs));
 	ecs->random_signal = !bpstat_explains_signal (stop_bpstat);
 	goto process_event_stop_test;
 
@@ -1712,13 +1700,7 @@
 	stop_pc = read_pc_pid (ecs->pid);
 	ecs->saved_inferior_pid = inferior_pid;
 	inferior_pid = ecs->pid;
-	stop_bpstat = bpstat_stop_status
-	  (&stop_pc,
-	   (DECR_PC_AFTER_BREAK ?
-	    (prev_pc != stop_pc - DECR_PC_AFTER_BREAK
-	     && currently_stepping (ecs))
-	    : 0)
-	  );
+	stop_bpstat = bpstat_stop_status (&stop_pc, currently_stepping (ecs));
 	ecs->random_signal = !bpstat_explains_signal (stop_bpstat);
 	inferior_pid = ecs->saved_inferior_pid;
 	goto process_event_stop_test;
@@ -2104,22 +2086,15 @@
 	    /* See if there is a breakpoint at the current PC.  */
 	    stop_bpstat = bpstat_stop_status
 	      (&stop_pc,
-	       (DECR_PC_AFTER_BREAK ?
-	    /* Notice the case of stepping through a jump
-	       that lands just after a breakpoint.
-	       Don't confuse that with hitting the breakpoint.
-	       What we check for is that 1) stepping is going on
-	       and 2) the pc before the last insn does not match
-	       the address of the breakpoint before the current pc
-	       and 3) 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.  */
+	    /* 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)))) :
-		0)
+		      && INNER_THAN (read_sp (), (step_sp - 16))))
 	      );
 	    /* Following in case break condition called a
 	       function.  */


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]