RFC: Centralize DECR_PC_AFTER_BREAK handling from infrun

RFC: Centralize DECR_PC_AFTER_BREAK handling from infrun

[Daniel Jacobowitz]

The first, possibly most important step in cleaning up DECR_PC_AFTER_BREAK's
tentacles.  This changes handle_inferior_event to fix the PC immediately,
before doing anything else with it.  It removes the later decrements, but
doesn't remove all the later workarounds for possibly undecremented PC
values - that can come separately.

One case, HANDLE_NONSTEPPABLE_WATCHPOINTS and DECR_PC_AFTER_BREAK, is simply
removed.  There are no targets using this combination, and if one is added,
it's non-obvious whether a nonsteppable watchpoint really should be affected
by DECR_PC_AFTER_BREAK.

A future cleanup will change bpstat_stop_status to only take a CORE_ADDR
argument.  Also, I believe that both the tests for sigtramps and the use of
deprecated_frame_update_pc_hack can go now, but I don't want to mix that in
with this - esp. since I'm not sure how to test the former belief.

Tested i386-linux, which uses DECR_PC_AFTER_BREAK.  I'm not going to check
this in just yet; all comments and testing welcome.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

2004-01-17  Daniel Jacobowitz  <drow@mvista.com>

	* breakpoint.c (software_breakpoint_inserted_here_p): New function.
	(bpstat_stop_status): Don't decrement PC.
	* breakpoint.h (software_breakpoint_inserted_here_p): Add
	prototype.
	* infrun.c (adjust_pc_after_break): New function.
	(handle_inferior_event): Call it, early.  Remove later references
	to DECR_PC_AFTER_BREAK.
	(normal_stop): Add commentary.

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.149
diff -u -p -r1.149 breakpoint.c
--- breakpoint.c	17 Jan 2004 21:56:12 -0000	1.149
+++ breakpoint.c	17 Jan 2004 22:05:39 -0000
@@ -1721,6 +1721,37 @@ breakpoint_inserted_here_p (CORE_ADDR pc
   return 0;
 }
 
+/* This function returns non-zero iff there is a software breakpoint
+   inserted at PC.  */
+
+int
+software_breakpoint_inserted_here_p (CORE_ADDR pc)
+{
+  struct bp_location *bpt;
+  int any_breakpoint_here = 0;
+
+  ALL_BP_LOCATIONS (bpt)
+    {
+      if (bpt->loc_type != bp_loc_software_breakpoint)
+	continue;
+
+      if ((breakpoint_enabled (bpt->owner)
+	   || bpt->owner->enable_state == bp_permanent)
+	  && bpt->inserted
+	  && bpt->address == pc)	/* bp is enabled and matches pc */
+	{
+	  if (overlay_debugging 
+	      && section_is_overlay (bpt->section) 
+	      && !section_is_mapped (bpt->section))
+	    continue;		/* unmapped overlay -- can't be a match */
+	  else
+	    return 1;
+	}
+    }
+
+  return 0;
+}
+
 /* Return nonzero if FRAME is a dummy frame.  We can't use
    DEPRECATED_PC_IN_CALL_DUMMY because figuring out the saved SP would
    take too much time, at least using frame_register() on the 68k.
@@ -2578,13 +2609,7 @@ bpstat_stop_status (CORE_ADDR *pc, int n
   /* Pointer to the last thing in the chain currently.  */
   bpstat bs = root_bs;
 
-  /* Get the address where the breakpoint would have been.  The
-     "not_a_sw_breakpoint" argument is meant to distinguish between a
-     breakpoint trap event and a trace/singlestep trap event.  For a
-     trace/singlestep trap event, we would not want to subtract
-     DECR_PC_AFTER_BREAK from the PC. */
-
-  bp_addr = *pc - (not_a_sw_breakpoint ? 0 : DECR_PC_AFTER_BREAK);
+  bp_addr = *pc;
 
   ALL_BREAKPOINTS_SAFE (b, temp)
   {
@@ -2869,18 +2894,6 @@ bpstat_stop_status (CORE_ADDR *pc, int n
 
   bs->next = NULL;		/* Terminate the chain */
   bs = root_bs->next;		/* Re-grab the head of the chain */
-
-  if (real_breakpoint && bs)
-    {
-      if (bs->breakpoint_at->type != bp_hardware_breakpoint)
-	{
-	  if (DECR_PC_AFTER_BREAK != 0)
-	    {
-	      *pc = bp_addr;
-	      write_pc (bp_addr);
-	    }
-	}
-    }
 
   /* The value of a hardware watchpoint hasn't changed, but the
      intermediate memory locations we are watching may have.  */
Index: breakpoint.h
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.h,v
retrieving revision 1.26
diff -u -p -r1.26 breakpoint.h
--- breakpoint.h	6 Nov 2003 18:24:55 -0000	1.26
+++ breakpoint.h	17 Jan 2004 22:05:39 -0000
@@ -605,6 +605,8 @@ extern enum breakpoint_here breakpoint_h
 
 extern int breakpoint_inserted_here_p (CORE_ADDR);
 
+extern int software_breakpoint_inserted_here_p (CORE_ADDR);
+
 /* FIXME: cagney/2002-11-10: The current [generic] dummy-frame code
    implements a functional superset of this function.  The only reason
    it hasn't been removed is because some architectures still don't
Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.128
diff -u -p -r1.128 infrun.c
--- infrun.c	17 Jan 2004 21:56:12 -0000	1.128
+++ infrun.c	17 Jan 2004 22:05:39 -0000
@@ -1316,6 +1316,59 @@ handle_step_into_function (struct execut
   return;
 }
 
+static void
+adjust_pc_after_break (struct execution_control_state *ecs)
+{
+  CORE_ADDR stop_pc;
+
+  /* If this target does not decrement the PC after breakpoints, then
+     we have nothing to do.  */
+  if (DECR_PC_AFTER_BREAK == 0)
+    return;
+
+  /* If we've hit a breakpoint, we'll be stopped with SIGTRAP.  */
+  if (ecs->ws.kind != TARGET_WAITKIND_STOPPED)
+    return;
+
+  if (ecs->ws.value.sig != TARGET_SIGNAL_TRAP)
+    return;
+
+  /* Find the location where (if we've hit a breakpoint) the breakpoint would
+     be.  */
+  stop_pc = read_pc_pid (ecs->ptid) - DECR_PC_AFTER_BREAK;
+
+  /* If we're software-single-stepping, then assume this is a breakpoint.
+     NOTE drow/2004-01-17: This doesn't check that the PC matches, or that
+     we're even in the right thread.  The software-single-step code needs
+     some modernization.
+
+     If we're not software-single-stepping, then we first check that there
+     is an enabled software breakpoint at this address.  If there is, and
+     we weren't using hardware-single-step, then we've hit the breakpoint.
+
+     If we were using hardware-single-step, we check prev_pc; if we just
+     stepped over an inserted software breakpoint, then we should decrement
+     the PC and eventually report hitting the breakpoint.  The prev_pc check
+     prevents us from decrementing the PC if we just stepped over a jump
+     instruction and landed on the instruction after a breakpoint.
+
+     The last bit checks that 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.
+
+     NOTE drow/2004-01-17: I'm not sure that this is necessary.  The check
+     predates checking for software single step at the same time.  Also,
+     if we've moved into a signal handler we should have seen the
+     signal.  */
+
+  if ((SOFTWARE_SINGLE_STEP_P () && singlestep_breakpoints_inserted_p)
+      || (software_breakpoint_inserted_here_p (stop_pc)
+	  && !(currently_stepping (ecs)
+	       && prev_pc != stop_pc
+	       && !(step_range_end && INNER_THAN (read_sp (), (step_sp - 16))))))
+    write_pc_pid (stop_pc, ecs->ptid);
+}
+
 /* Given an execution control state that has been freshly filled in
    by an event from the inferior, figure out what it means and take
    appropriate action.  */
@@ -1335,6 +1388,8 @@ handle_inferior_event (struct execution_
   target_last_wait_ptid = ecs->ptid;
   target_last_waitstatus = *ecs->wp;
 
+  adjust_pc_after_break (ecs);
+
   switch (ecs->infwait_state)
     {
     case infwait_thread_hop_state:
@@ -1688,19 +1743,15 @@ handle_inferior_event (struct execution_
       /* Check if a regular breakpoint has been hit before checking
          for a potential single step breakpoint. Otherwise, GDB will
          not see this breakpoint hit when stepping onto breakpoints.  */
-      if (breakpoints_inserted
-          && breakpoint_here_p (stop_pc - DECR_PC_AFTER_BREAK))
+      if (breakpoints_inserted && breakpoint_here_p (stop_pc))
 	{
 	  ecs->random_signal = 0;
-	  if (!breakpoint_thread_match (stop_pc - DECR_PC_AFTER_BREAK,
-					ecs->ptid))
+	  if (!breakpoint_thread_match (stop_pc, ecs->ptid))
 	    {
 	      int remove_status;
 
 	      /* Saw a breakpoint, but it was hit by the wrong thread.
 	         Just continue. */
-	      if (DECR_PC_AFTER_BREAK)
-		write_pc_pid (stop_pc - DECR_PC_AFTER_BREAK, ecs->ptid);
 
 	      remove_status = remove_breakpoints ();
 	      /* Did we fail to remove breakpoints?  If so, try
@@ -1713,7 +1764,7 @@ handle_inferior_event (struct execution_
 	      if (remove_status != 0)
 		{
 		  /* FIXME!  This is obviously non-portable! */
-		  write_pc_pid (stop_pc - DECR_PC_AFTER_BREAK + 4, ecs->ptid);
+		  write_pc_pid (stop_pc + 4, ecs->ptid);
 		  /* We need to restart all the threads now,
 		   * unles we're running in scheduler-locked mode. 
 		   * Use currently_stepping to determine whether to 
@@ -1747,17 +1798,6 @@ handle_inferior_event (struct execution_
 	}
       else if (SOFTWARE_SINGLE_STEP_P () && singlestep_breakpoints_inserted_p)
         {
-          /* Readjust the stop_pc as it is off by DECR_PC_AFTER_BREAK
-             compared to the value it would have if the system stepping
-             capability was used. This allows the rest of the code in
-             this function to use this address without having to worry
-             whether software single step is in use or not.  */
-          if (DECR_PC_AFTER_BREAK)
-            {
-              stop_pc -= DECR_PC_AFTER_BREAK;
-              write_pc_pid (stop_pc, ecs->ptid);
-            }
-
           sw_single_step_trap_p = 1;
           ecs->random_signal = 0;
         }
@@ -1889,9 +1929,6 @@ handle_inferior_event (struct execution_
          includes evaluating watchpoints, things will come to a
          stop in the correct manner.  */
 
-      if (DECR_PC_AFTER_BREAK)
-	write_pc (stop_pc - DECR_PC_AFTER_BREAK);
-
       remove_breakpoints ();
       registers_changed ();
       target_resume (ecs->ptid, 1, TARGET_SIGNAL_0);	/* Single step */
@@ -3101,6 +3138,7 @@ normal_stop (void)
       previous_inferior_ptid = inferior_ptid;
     }
 
+  /* NOTE drow/2004-01-17: Is this still necessary?  */
   /* Make sure that the current_frame's pc is correct.  This
      is a correction for setting up the frame info before doing
      DECR_PC_AFTER_BREAK */

Re: RFC: Centralize DECR_PC_AFTER_BREAK handling from infrun

[Andrew Cagney]

> The first, possibly most important step in cleaning up DECR_PC_AFTER_BREAK's
> tentacles.  This changes handle_inferior_event to fix the PC immediately,
> before doing anything else with it.  It removes the later decrements, but
> doesn't remove all the later workarounds for possibly undecremented PC
> values - that can come separately.
> 
> One case, HANDLE_NONSTEPPABLE_WATCHPOINTS and DECR_PC_AFTER_BREAK, is simply
> removed.  There are no targets using this combination, and if one is added,
> it's non-obvious whether a nonsteppable watchpoint really should be affected
> by DECR_PC_AFTER_BREAK.
> 
> A future cleanup will change bpstat_stop_status to only take a CORE_ADDR
> argument.  Also, I believe that both the tests for sigtramps and the use of
> deprecated_frame_update_pc_hack can go now, but I don't want to mix that in
> with this - esp. since I'm not sure how to test the former belief.

Build gdb with gcov and then run the testsuite, it will quickly point 
you at the "dead" code paths and hence how well it was really tested.

Otherwize, yow!

Andrew

Re: RFC: Centralize DECR_PC_AFTER_BREAK handling from infrun

[Daniel Jacobowitz]

On Sat, Jan 17, 2004 at 05:38:41PM -0500, Andrew Cagney wrote:
> >The first, possibly most important step in cleaning up 
> >DECR_PC_AFTER_BREAK's
> >tentacles.  This changes handle_inferior_event to fix the PC immediately,
> >before doing anything else with it.  It removes the later decrements, but
> >doesn't remove all the later workarounds for possibly undecremented PC
> >values - that can come separately.
> >
> >One case, HANDLE_NONSTEPPABLE_WATCHPOINTS and DECR_PC_AFTER_BREAK, is 
> >simply
> >removed.  There are no targets using this combination, and if one is added,
> >it's non-obvious whether a nonsteppable watchpoint really should be 
> >affected
> >by DECR_PC_AFTER_BREAK.
> >
> >A future cleanup will change bpstat_stop_status to only take a CORE_ADDR
> >argument.  Also, I believe that both the tests for sigtramps and the use of
> >deprecated_frame_update_pc_hack can go now, but I don't want to mix that in
> >with this - esp. since I'm not sure how to test the former belief.
> 
> Build gdb with gcov and then run the testsuite, it will quickly point 
> you at the "dead" code paths and hence how well it was really tested.

Except, my hunch is it's related to some particular system's broken
PTRACE_SINGLESTEP :(  Well, if it doesn't show up on the systems I have
available I think I'll remove it anyway (after 6.1) - then if we need
to add it back we'll pick up a comment saying why it was necessary.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: RFC: Centralize DECR_PC_AFTER_BREAK handling from infrun

[Eli Zaretskii]

> Date: Sat, 17 Jan 2004 17:20:07 -0500
> From: Daniel Jacobowitz <drow@mvista.com>
> 
> One case, HANDLE_NONSTEPPABLE_WATCHPOINTS and DECR_PC_AFTER_BREAK, is simply
> removed.  There are no targets using this combination, and if one is added,
> it's non-obvious whether a nonsteppable watchpoint really should be affected
> by DECR_PC_AFTER_BREAK.

Right, but since we don't really know what that feature was about, I'd
suggest to leave a comment in adjust_pc_after_break that mentions
HANDLE_NONSTEPPABLE_WATCHPOINTS and that its support, if needed,
should be added.

> 	* breakpoint.c (software_breakpoint_inserted_here_p): New function.
> 	(bpstat_stop_status): Don't decrement PC.
> 	* breakpoint.h (software_breakpoint_inserted_here_p): Add
> 	prototype.
> 	* infrun.c (adjust_pc_after_break): New function.
> 	(handle_inferior_event): Call it, early.  Remove later references
> 	to DECR_PC_AFTER_BREAK.
> 	(normal_stop): Add commentary.

What happens if a location has both software and hardware
breakpoints?  Does the code still DTRT?

> +  /* If we've hit a breakpoint, we'll be stopped with SIGTRAP.  */
> +  if (ecs->ws.kind != TARGET_WAITKIND_STOPPED)
> +    return;
> +
> +  if (ecs->ws.value.sig != TARGET_SIGNAL_TRAP)
> +    return;

The original code didn't check these conditions, right?  So why add
them here?  (Also, the comment doesn't seem to describe the two
tests, only the second one.)

Re: RFC: Centralize DECR_PC_AFTER_BREAK handling from infrun

[Daniel Jacobowitz]

On Sun, Jan 18, 2004 at 09:07:23AM +0200, Eli Zaretskii wrote:
> > Date: Sat, 17 Jan 2004 17:20:07 -0500
> > From: Daniel Jacobowitz <drow@mvista.com>
> > 
> > One case, HANDLE_NONSTEPPABLE_WATCHPOINTS and DECR_PC_AFTER_BREAK, is simply
> > removed.  There are no targets using this combination, and if one is added,
> > it's non-obvious whether a nonsteppable watchpoint really should be affected
> > by DECR_PC_AFTER_BREAK.
> 
> Right, but since we don't really know what that feature was about, I'd
> suggest to leave a comment in adjust_pc_after_break that mentions
> HANDLE_NONSTEPPABLE_WATCHPOINTS and that its support, if needed,
> should be added.

Well, we know what HANDLE_NONSTEPPABLE_WATCHPOINTS was about.  I'd be
curious to see whether any target ever used these two together, or if
the decrement was just added for consistency.  I'll add a comment.

> > 	* breakpoint.c (software_breakpoint_inserted_here_p): New function.
> > 	(bpstat_stop_status): Don't decrement PC.
> > 	* breakpoint.h (software_breakpoint_inserted_here_p): Add
> > 	prototype.
> > 	* infrun.c (adjust_pc_after_break): New function.
> > 	(handle_inferior_event): Call it, early.  Remove later references
> > 	to DECR_PC_AFTER_BREAK.
> > 	(normal_stop): Add commentary.
> 
> What happens if a location has both software and hardware
> breakpoints?  Does the code still DTRT?

Hmm, I am not sure.  What _is_ the right thing?  Decrement if the
software breakpoint was inserted, and do nothing if the hardware
breakpoint was inserted, and assume that both will not be inserted?

> > +  /* If we've hit a breakpoint, we'll be stopped with SIGTRAP.  */
> > +  if (ecs->ws.kind != TARGET_WAITKIND_STOPPED)
> > +    return;
> > +
> > +  if (ecs->ws.value.sig != TARGET_SIGNAL_TRAP)
> > +    return;
> 
> The original code didn't check these conditions, right?  So why add
> them here?  (Also, the comment doesn't seem to describe the two
> tests, only the second one.)

The comment does describe both tests; if != TARGET_WAITKIND_STOPPED,
then we aren't stopped by a signal.  The other waitkinds correspond to
things like exiting and catchpoints, and with the exception of some
complications in the FORKED/EXECD cases, stop_signal will not get set
to SIGTRAP.  Also, the original code did check these conditions, though
somewhat indirectly:

  if (stop_signal == TARGET_SIGNAL_TRAP)
    {
      /* Check if a regular breakpoint has been hit before checking
         for a potential single step breakpoint. Otherwise, GDB will
         not see this breakpoint hit when stepping onto breakpoints.  */

Oh, hum, that's for the first set (thread hit thread-specific BP for a
different thread).  The second thread does this:

  if (stop_signal == TARGET_SIGNAL_TRAP
      || (breakpoints_inserted &&
          (stop_signal == TARGET_SIGNAL_ILL
           || stop_signal == TARGET_SIGNAL_EMT))
      || stop_soon == STOP_QUIETLY
      || stop_soon == STOP_QUIETLY_NO_SIGSTOP)

The stop_soon's aren't relevant here, since they're handled before
DECR_PC_AFTER_BREAK, but the ILL/EMT are relevant.  They should be
added to adjust_pc_after_break - thanks!


-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: RFC: Centralize DECR_PC_AFTER_BREAK handling from infrun

[Eli Zaretskii]

> Date: Sun, 18 Jan 2004 10:19:10 -0500
> From: Daniel Jacobowitz <drow@mvista.com>
> > 
> > What happens if a location has both software and hardware
> > breakpoints?  Does the code still DTRT?
> 
> Hmm, I am not sure.  What _is_ the right thing?

We should at least do no worse than the current code does--which is to
act as if only the first breakpoint (in the order stored in the
breakpoint data-base) were hit.  That is, if breakpoints #n and #m
both fire, the current GDB announces the one whose number is smaller
(because it walks thru the breakpoints in their numerical order).  It
doesn't seem to matter whether the breakpoints are of the software or
the hardware-assisted variety.

It would be nice if we could announce all breakpoints that break at
that point, but this might not be possible or very hard, I dunno.

Re: RFC: Centralize DECR_PC_AFTER_BREAK handling from infrun

[Andrew Cagney]

>> Date: Sun, 18 Jan 2004 10:19:10 -0500
>> From: Daniel Jacobowitz <drow@mvista.com>
> 
>> > 
>> > What happens if a location has both software and hardware
>> > breakpoints?  Does the code still DTRT?
> 
>> 
>> Hmm, I am not sure.  What _is_ the right thing?
> 
> 
> We should at least do no worse than the current code does--which is to
> act as if only the first breakpoint (in the order stored in the
> breakpoint data-base) were hit.  That is, if breakpoints #n and #m
> both fire, the current GDB announces the one whose number is smaller
> (because it walks thru the breakpoints in their numerical order).  It
> doesn't seem to matter whether the breakpoints are of the software or
> the hardware-assisted variety.
> 
> It would be nice if we could announce all breakpoints that break at
> that point, but this might not be possible or very hard, I dunno.

I'm not that sure that the current code is right though :-(  I recall 
comlaints about the current behavior - it should report all possible 
reasons for breaking and not just the first.

Andrew

Re: RFC: Centralize DECR_PC_AFTER_BREAK handling from infrun

[Eli Zaretskii]

> Date: Sun, 18 Jan 2004 13:00:52 -0500
> From: Andrew Cagney <cagney@gnu.org>
> > 
> > It would be nice if we could announce all breakpoints that break at
> > that point, but this might not be possible or very hard, I dunno.
> 
> I'm not that sure that the current code is right though :-(

Me neither.  That's why I think we should try to do better if it's
reasonable.  But we shouldn't make things worse, at the very least.

> I recall comlaints about the current behavior - it should report all
> possible reasons for breaking and not just the first.

Sure, and the commands bound to all breakpoints should certainly be
run, which we don't currently do.  That's a bug.

Re: RFC: Centralize DECR_PC_AFTER_BREAK handling from infrun

[Daniel Jacobowitz]

On Sun, Jan 18, 2004 at 07:23:38PM +0200, Eli Zaretskii wrote:
> > Date: Sun, 18 Jan 2004 10:19:10 -0500
> > From: Daniel Jacobowitz <drow@mvista.com>
> > > 
> > > What happens if a location has both software and hardware
> > > breakpoints?  Does the code still DTRT?
> > 
> > Hmm, I am not sure.  What _is_ the right thing?
> 
> We should at least do no worse than the current code does--which is to
> act as if only the first breakpoint (in the order stored in the
> breakpoint data-base) were hit.  That is, if breakpoints #n and #m
> both fire, the current GDB announces the one whose number is smaller
> (because it walks thru the breakpoints in their numerical order).  It
> doesn't seem to matter whether the breakpoints are of the software or
> the hardware-assisted variety.
> 
> It would be nice if we could announce all breakpoints that break at
> that point, but this might not be possible or very hard, I dunno.

Behavior unchanged by this patch.  Only one of the breakpoints will be
inserted - the other will be marked as a duplicate - and only inserted
breakpoints are checked when figuring out whether a software breakpoint
was hit.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: RFC: Centralize DECR_PC_AFTER_BREAK handling from infrun

[Daniel Jacobowitz]

On Sun, Jan 18, 2004 at 10:19:10AM -0500, Daniel Jacobowitz wrote:
> > > +  /* If we've hit a breakpoint, we'll be stopped with SIGTRAP.  */
> > > +  if (ecs->ws.kind != TARGET_WAITKIND_STOPPED)
> > > +    return;
> > > +
> > > +  if (ecs->ws.value.sig != TARGET_SIGNAL_TRAP)
> > > +    return;
> > 
> > The original code didn't check these conditions, right?  So why add
> > them here?  (Also, the comment doesn't seem to describe the two
> > tests, only the second one.)
> 
> The comment does describe both tests; if != TARGET_WAITKIND_STOPPED,
> then we aren't stopped by a signal.  The other waitkinds correspond to
> things like exiting and catchpoints, and with the exception of some
> complications in the FORKED/EXECD cases, stop_signal will not get set
> to SIGTRAP.  Also, the original code did check these conditions, though
> somewhat indirectly:
> 
>   if (stop_signal == TARGET_SIGNAL_TRAP)
>     {
>       /* Check if a regular breakpoint has been hit before checking
>          for a potential single step breakpoint. Otherwise, GDB will
>          not see this breakpoint hit when stepping onto breakpoints.  */
> 
> Oh, hum, that's for the first set (thread hit thread-specific BP for a
> different thread).  The second thread does this:
> 
>   if (stop_signal == TARGET_SIGNAL_TRAP
>       || (breakpoints_inserted &&
>           (stop_signal == TARGET_SIGNAL_ILL
>            || stop_signal == TARGET_SIGNAL_EMT))
>       || stop_soon == STOP_QUIETLY
>       || stop_soon == STOP_QUIETLY_NO_SIGSTOP)
> 
> The stop_soon's aren't relevant here, since they're handled before
> DECR_PC_AFTER_BREAK, but the ILL/EMT are relevant.  They should be
> added to adjust_pc_after_break - thanks!

Actually, I'm not sure they should.  The question is, on an operating
system such that hitting a breakpoint causes SIGILL, should
DECR_PC_AFTER_BREAK be applied?  I don't have a machine such that
breakpoints cause SIGILL, but I tested  x86 and alpha (both
DECR_PC_AFTER_BREAK targets).  On x86 a SIGILL leaves the PC pointing
at the beginning of the illegal instruction, and on Alpha a SIGILL
leaves the PC pointing to the second instruction after the illegal one.
In neither case does that match the behavior of DECR_PC_AFTER_BREAK. 

So this will become a comment also.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: RFC: Centralize DECR_PC_AFTER_BREAK handling from infrun

[Andrew Cagney]

Is it possible to determine the need for the decrement based solely on 
the knowledge of how the thread was resumed (step, run, ...)?  In the 
remote case there isn't convenient access to the list of currently 
inserted breakpoints.

Andrew

Re: RFC: Centralize DECR_PC_AFTER_BREAK handling from infrun

[Daniel Jacobowitz]

On Thu, Jan 22, 2004 at 09:09:09AM -0500, Andrew Cagney wrote:
> Is it possible to determine the need for the decrement based solely on 
> the knowledge of how the thread was resumed (step, run, ...)?  In the 
> remote case there isn't convenient access to the list of currently 
> inserted breakpoints.

Not sure what remote case you're referring to, can you give me an
example?

I'm not sure what the consequences of removing the
software_breakpoint_inserted_here_p test would be, but it'll be easier
to find out after I've reorganized.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: RFC: Centralize DECR_PC_AFTER_BREAK handling from infrun

[Daniel Jacobowitz]

On Sat, Jan 17, 2004 at 05:20:07PM -0500, Daniel Jacobowitz wrote:
> The first, possibly most important step in cleaning up DECR_PC_AFTER_BREAK's
> tentacles.  This changes handle_inferior_event to fix the PC immediately,
> before doing anything else with it.  It removes the later decrements, but
> doesn't remove all the later workarounds for possibly undecremented PC
> values - that can come separately.
> 
> One case, HANDLE_NONSTEPPABLE_WATCHPOINTS and DECR_PC_AFTER_BREAK, is simply
> removed.  There are no targets using this combination, and if one is added,
> it's non-obvious whether a nonsteppable watchpoint really should be affected
> by DECR_PC_AFTER_BREAK.
> 
> A future cleanup will change bpstat_stop_status to only take a CORE_ADDR
> argument.  Also, I believe that both the tests for sigtramps and the use of
> deprecated_frame_update_pc_hack can go now, but I don't want to mix that in
> with this - esp. since I'm not sure how to test the former belief.
> 
> Tested i386-linux, which uses DECR_PC_AFTER_BREAK.  I'm not going to check
> this in just yet; all comments and testing welcome.

I've checked in this version, with comment revisions (thanks Eli!) and
testing on AIX and Tru64 (thanks Joel!).  The latter convinced me that
something is seriously wrong on both of those targets, but the patch
had a non-deterministic affect on the existing failures.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

2004-01-31  Daniel Jacobowitz  <drow@mvista.com>

	* breakpoint.c (software_breakpoint_inserted_here_p): New function.
	(bpstat_stop_status): Don't decrement PC.
	* breakpoint.h (software_breakpoint_inserted_here_p): Add
	prototype.
	* infrun.c (adjust_pc_after_break): New function.
	(handle_inferior_event): Call it, early.  Remove later references
	to DECR_PC_AFTER_BREAK.
	(normal_stop): Add commentary.

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.152
diff -u -p -r1.152 breakpoint.c
--- breakpoint.c	28 Jan 2004 01:39:51 -0000	1.152
+++ breakpoint.c	31 Jan 2004 17:54:20 -0000
@@ -1717,6 +1717,37 @@ breakpoint_inserted_here_p (CORE_ADDR pc
   return 0;
 }
 
+/* This function returns non-zero iff there is a software breakpoint
+   inserted at PC.  */
+
+int
+software_breakpoint_inserted_here_p (CORE_ADDR pc)
+{
+  struct bp_location *bpt;
+  int any_breakpoint_here = 0;
+
+  ALL_BP_LOCATIONS (bpt)
+    {
+      if (bpt->loc_type != bp_loc_software_breakpoint)
+	continue;
+
+      if ((breakpoint_enabled (bpt->owner)
+	   || bpt->owner->enable_state == bp_permanent)
+	  && bpt->inserted
+	  && bpt->address == pc)	/* bp is enabled and matches pc */
+	{
+	  if (overlay_debugging 
+	      && section_is_overlay (bpt->section) 
+	      && !section_is_mapped (bpt->section))
+	    continue;		/* unmapped overlay -- can't be a match */
+	  else
+	    return 1;
+	}
+    }
+
+  return 0;
+}
+
 /* Return nonzero if FRAME is a dummy frame.  We can't use
    DEPRECATED_PC_IN_CALL_DUMMY because figuring out the saved SP would
    take too much time, at least using frame_register() on the 68k.
@@ -2571,13 +2602,7 @@ bpstat_stop_status (CORE_ADDR *pc, int n
   /* Pointer to the last thing in the chain currently.  */
   bpstat bs = root_bs;
 
-  /* Get the address where the breakpoint would have been.  The
-     "not_a_sw_breakpoint" argument is meant to distinguish between a
-     breakpoint trap event and a trace/singlestep trap event.  For a
-     trace/singlestep trap event, we would not want to subtract
-     DECR_PC_AFTER_BREAK from the PC. */
-
-  bp_addr = *pc - (not_a_sw_breakpoint ? 0 : DECR_PC_AFTER_BREAK);
+  bp_addr = *pc;
 
   ALL_BREAKPOINTS_SAFE (b, temp)
   {
@@ -2862,18 +2887,6 @@ bpstat_stop_status (CORE_ADDR *pc, int n
 
   bs->next = NULL;		/* Terminate the chain */
   bs = root_bs->next;		/* Re-grab the head of the chain */
-
-  if (real_breakpoint && bs)
-    {
-      if (bs->breakpoint_at->type != bp_hardware_breakpoint)
-	{
-	  if (DECR_PC_AFTER_BREAK != 0)
-	    {
-	      *pc = bp_addr;
-	      write_pc (bp_addr);
-	    }
-	}
-    }
 
   /* The value of a hardware watchpoint hasn't changed, but the
      intermediate memory locations we are watching may have.  */
Index: breakpoint.h
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.h,v
retrieving revision 1.26
diff -u -p -r1.26 breakpoint.h
--- breakpoint.h	6 Nov 2003 18:24:55 -0000	1.26
+++ breakpoint.h	31 Jan 2004 17:54:20 -0000
@@ -605,6 +605,8 @@ extern enum breakpoint_here breakpoint_h
 
 extern int breakpoint_inserted_here_p (CORE_ADDR);
 
+extern int software_breakpoint_inserted_here_p (CORE_ADDR);
+
 /* FIXME: cagney/2002-11-10: The current [generic] dummy-frame code
    implements a functional superset of this function.  The only reason
    it hasn't been removed is because some architectures still don't
Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.131
diff -u -p -r1.131 infrun.c
--- infrun.c	21 Jan 2004 16:32:07 -0000	1.131
+++ infrun.c	31 Jan 2004 17:54:20 -0000
@@ -1313,6 +1313,74 @@ handle_step_into_function (struct execut
   return;
 }
 
+static void
+adjust_pc_after_break (struct execution_control_state *ecs)
+{
+  CORE_ADDR stop_pc;
+
+  /* If this target does not decrement the PC after breakpoints, then
+     we have nothing to do.  */
+  if (DECR_PC_AFTER_BREAK == 0)
+    return;
+
+  /* If we've hit a breakpoint, we'll normally be stopped with SIGTRAP.  If
+     we aren't, just return.
+     
+     NOTE drow/2004-01-31: On some targets, breakpoints may generate
+     different signals (SIGILL or SIGEMT for instance), but it is less
+     clear where the PC is pointing afterwards.  It may not match
+     DECR_PC_AFTER_BREAK.  I don't know any specific target that generates
+     these signals at breakpoints (the code has been in GDB since at least
+     1992) so I can not guess how to handle them here.
+     
+     In earlier versions of GDB, a target with HAVE_NONSTEPPABLE_WATCHPOINTS
+     would have the PC after hitting a watchpoint affected by
+     DECR_PC_AFTER_BREAK.  I haven't found any target with both of these set
+     in GDB history, and it seems unlikely to be correct, so
+     HAVE_NONSTEPPABLE_WATCHPOINTS is not checked here.  */
+
+  if (ecs->ws.kind != TARGET_WAITKIND_STOPPED)
+    return;
+
+  if (ecs->ws.value.sig != TARGET_SIGNAL_TRAP)
+    return;
+
+  /* Find the location where (if we've hit a breakpoint) the breakpoint would
+     be.  */
+  stop_pc = read_pc_pid (ecs->ptid) - DECR_PC_AFTER_BREAK;
+
+  /* If we're software-single-stepping, then assume this is a breakpoint.
+     NOTE drow/2004-01-17: This doesn't check that the PC matches, or that
+     we're even in the right thread.  The software-single-step code needs
+     some modernization.
+
+     If we're not software-single-stepping, then we first check that there
+     is an enabled software breakpoint at this address.  If there is, and
+     we weren't using hardware-single-step, then we've hit the breakpoint.
+
+     If we were using hardware-single-step, we check prev_pc; if we just
+     stepped over an inserted software breakpoint, then we should decrement
+     the PC and eventually report hitting the breakpoint.  The prev_pc check
+     prevents us from decrementing the PC if we just stepped over a jump
+     instruction and landed on the instruction after a breakpoint.
+
+     The last bit checks that 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.
+
+     NOTE drow/2004-01-17: I'm not sure that this is necessary.  The check
+     predates checking for software single step at the same time.  Also,
+     if we've moved into a signal handler we should have seen the
+     signal.  */
+
+  if ((SOFTWARE_SINGLE_STEP_P () && singlestep_breakpoints_inserted_p)
+      || (software_breakpoint_inserted_here_p (stop_pc)
+	  && !(currently_stepping (ecs)
+	       && prev_pc != stop_pc
+	       && !(step_range_end && INNER_THAN (read_sp (), (step_sp - 16))))))
+    write_pc_pid (stop_pc, ecs->ptid);
+}
+
 /* Given an execution control state that has been freshly filled in
    by an event from the inferior, figure out what it means and take
    appropriate action.  */
@@ -1332,6 +1400,8 @@ handle_inferior_event (struct execution_
   target_last_wait_ptid = ecs->ptid;
   target_last_waitstatus = *ecs->wp;
 
+  adjust_pc_after_break (ecs);
+
   switch (ecs->infwait_state)
     {
     case infwait_thread_hop_state:
@@ -1685,19 +1755,15 @@ handle_inferior_event (struct execution_
       /* Check if a regular breakpoint has been hit before checking
          for a potential single step breakpoint. Otherwise, GDB will
          not see this breakpoint hit when stepping onto breakpoints.  */
-      if (breakpoints_inserted
-          && breakpoint_here_p (stop_pc - DECR_PC_AFTER_BREAK))
+      if (breakpoints_inserted && breakpoint_here_p (stop_pc))
 	{
 	  ecs->random_signal = 0;
-	  if (!breakpoint_thread_match (stop_pc - DECR_PC_AFTER_BREAK,
-					ecs->ptid))
+	  if (!breakpoint_thread_match (stop_pc, ecs->ptid))
 	    {
 	      int remove_status;
 
 	      /* Saw a breakpoint, but it was hit by the wrong thread.
 	         Just continue. */
-	      if (DECR_PC_AFTER_BREAK)
-		write_pc_pid (stop_pc - DECR_PC_AFTER_BREAK, ecs->ptid);
 
 	      remove_status = remove_breakpoints ();
 	      /* Did we fail to remove breakpoints?  If so, try
@@ -1710,7 +1776,7 @@ handle_inferior_event (struct execution_
 	      if (remove_status != 0)
 		{
 		  /* FIXME!  This is obviously non-portable! */
-		  write_pc_pid (stop_pc - DECR_PC_AFTER_BREAK + 4, ecs->ptid);
+		  write_pc_pid (stop_pc + 4, ecs->ptid);
 		  /* We need to restart all the threads now,
 		   * unles we're running in scheduler-locked mode. 
 		   * Use currently_stepping to determine whether to 
@@ -1744,17 +1810,6 @@ handle_inferior_event (struct execution_
 	}
       else if (SOFTWARE_SINGLE_STEP_P () && singlestep_breakpoints_inserted_p)
         {
-          /* Readjust the stop_pc as it is off by DECR_PC_AFTER_BREAK
-             compared to the value it would have if the system stepping
-             capability was used. This allows the rest of the code in
-             this function to use this address without having to worry
-             whether software single step is in use or not.  */
-          if (DECR_PC_AFTER_BREAK)
-            {
-              stop_pc -= DECR_PC_AFTER_BREAK;
-              write_pc_pid (stop_pc, ecs->ptid);
-            }
-
           sw_single_step_trap_p = 1;
           ecs->random_signal = 0;
         }
@@ -1886,9 +1941,6 @@ handle_inferior_event (struct execution_
          includes evaluating watchpoints, things will come to a
          stop in the correct manner.  */
 
-      if (DECR_PC_AFTER_BREAK)
-	write_pc (stop_pc - DECR_PC_AFTER_BREAK);
-
       remove_breakpoints ();
       registers_changed ();
       target_resume (ecs->ptid, 1, TARGET_SIGNAL_0);	/* Single step */
@@ -3121,6 +3173,7 @@ normal_stop (void)
       previous_inferior_ptid = inferior_ptid;
     }
 
+  /* NOTE drow/2004-01-17: Is this still necessary?  */
   /* Make sure that the current_frame's pc is correct.  This
      is a correction for setting up the frame info before doing
      DECR_PC_AFTER_BREAK */