breakpoint.c patch

[Kevin Buettner <kevinb@cygnus.com>]

I've been investigating a bug in which temporary breakpoints were not
getting deleted on Linux/x86.  They would get deleted properly if they
were hit as a result of a continue, but would not if you single
stepped onto them.

It turns out that there's a larger bug here.  In general, breakpoints
were not getting hit when you single stepped onto them on Linux/x86.

I originally speculated that the reason gdb is behaving in this fashion
on Linux is due to the fact that linux/86 has a hardware assisted single
step (i.e, single step is not implemented by setting breakpoints).  It
turns out that this speculation was largely incorrect.

The real reason for this bug in linux is centered around the fact
that DECR_PC_AFTER_BREAK is non-zero.  If done some checking and it
turns out that very few targets have a non-zero DECR_PC_AFTER_BREAK.
(tm-arc.h, tm-alpha.h, tm-i386.h, and tm-m68k.h are the only places
where DECR_PC_AFTER_BREAK is defined to be non-zero.)

The bug is in bpstat_stop_status() in breakpoint.c. 
bpstat_stop_status is given the address of the PC (pc) and a flag
(not_a_breakpoint) which indicates whether execution was stopped due
to hitting a breakpoint.  After examining the code, it seems to me
that a better name for this flag would be am_single_stepping, or even
am_single_stepping_on_a_target_with_a_nonzero_DECR_PC_AFTER_BREAK, or
somesuch.  (I'm about to explain why...)

It should be noted that bpstat_stop_status() is called from four
locations all within handle_inferior_event() in infrun.c.  Targets
which define DECR_PC_AFTER_BREAK to be 0 (i.e, nearly all of them)
will always end up with not_a_breakpoint being set to zero upon
entry to bpstat_stop_status().  Targets which have a non-zero
DECR_PC_AFTER_BREAK will (essentially) end up with not_a_breakpoint
being set to 1 if single stepping, and 0 otherwise.  The following
snippet of code from infrun.c offers the best explanation regarding
how this parameter gets set:

	    /* 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)
	      );

For an x86 target, there were two problems with the code in
bp_stop_status():

    1) bp_addr (the address to compare breakpoint addresses against)
       was getting set incorrectly when bp_stop_status() was entered
       due to a single step.  

    2) The following clause was getting triggered due to not_a_breakpoint
       being set:

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

       This was causing the breakpoint to be disregarded.  I don't
       really understand the purpose of this code and would prefer
       to simply remove it, but have decided to retain it in a modified
       form just in case some of the other DECR_PC_AFTER_BREAK != 0
       targets require it.  (Recall that not_a_breakpoint will always
       be zero for targets where DECR_PC_AFTER_BREAK is zero.  Therefore,
       the entire condition evaluates to zero for these targets and the
       continue statement will never be reached.)

I believe I have fixed these problems in the patch below.  I invite
comments and criticism...

Index: breakpoint.c
===================================================================
RCS file: /cvs/cvsfiles/devo/gdb/breakpoint.c,v
retrieving revision 1.233
diff -u -r1.233 breakpoint.c
--- breakpoint.c	1999/08/16 23:12:27	1.233
+++ breakpoint.c	1999/08/19 17:45:28
@@ -2118,7 +2118,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)
   {
@@ -2151,7 +2152,8 @@
 	&& b->type != bp_hardware_watchpoint
 	&& b->type != bp_read_watchpoint
 	&& b->type != bp_access_watchpoint
-	&& not_a_breakpoint)
+	&& not_a_breakpoint
+	&& SOFTWARE_SINGLE_STEP_P)
       continue;
 
     /* Is this a catchpoint of a load or unload?  If so, did we