This is the mail archive of the gdb-patches@sources.redhat.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]
Other format: [Raw text]

Re: [patch/rfc] Rewrite decr-pc logic, eliminate step_sp



+  if (currently_stepping (ecs))
+    {
+      if (SOFTWARE_SINGLE_STEP_P ())
+	{
+	  if (singlestep_breakpoints_inserted_p
+	      && prev_pc == breakpoint_pc)
+	    /* If we're software-single-stepping, assume we hit one of
+	       the inserted software breakpoints.  */
+	    write_pc_pid (breakpoint_pc, ecs->ptid);
+	}


I'm pretty sure that won't work.  prev_pc is where we were stopped
before we decided to single step.  breakpoint_pc is where, if we have
hit a breakpoint, the breakpoint would be.  They won't be equal;
breakpoint_pc will be the following instruction, or the target of a
branch if *prev_pc was a taken branch.  The old code assumes we hit a
breakpoint if we stopped with SIGTRAP with singlestep_breakpoints_inserted_p
- any reason not to keep that behavior?

It's plain wrong. I'm pretty sure that, when doing the thread-hop, singlestep_breakpoints_inserted_p holds, but current_stepping() doesn't.


I think Alpha OSF/1 and Alpha NetBSD are the only current
software-single-step and decr-pc targets, which makes this case a
little hard to test - at least OSF/1 had dreadful test results already,
I'm not sure about NetBSD.  Might want to verify that it isn't
catastrophic, at least.

The rest of it looks right to me, though I had to stare at it for
the last twenty minutes or so.

I gave up staring at the old code, it made no sense.


Attached is a revision.

Andrew

2004-05-09  Andrew Cagney  <cagney@redhat.com>

	* infrun.c (adjust_pc_after_break): Rewrite decr logic,
	eliminate reference to step_sp.
	(struct execution_control_state, init_execution_control_state)
	(handle_inferior_event, keep_going): Delete update_step_sp and
	step_sp.
	* infcmd.c (step_sp): Note that variable is unused.

Index: infcmd.c
===================================================================
RCS file: /cvs/src/src/gdb/infcmd.c,v
retrieving revision 1.109
diff -p -u -r1.109 infcmd.c
--- infcmd.c	8 May 2004 23:02:10 -0000	1.109
+++ infcmd.c	10 May 2004 19:10:10 -0000
@@ -187,7 +187,8 @@ CORE_ADDR step_range_end;	/* Exclusive *
 struct frame_id step_frame_id;
 
 /* Our notion of the current stack pointer.  */
-
+/* NOTE: cagney/2004-05-09: This variable is not used and should be
+   garbage collected.  */
 CORE_ADDR step_sp;
 
 enum step_over_calls_kind step_over_calls;
Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.153
diff -p -u -r1.153 infrun.c
--- infrun.c	10 May 2004 18:36:06 -0000	1.153
+++ infrun.c	10 May 2004 19:10:10 -0000
@@ -953,7 +953,6 @@ struct execution_control_state
   int handling_longjmp;		/* FIXME */
   ptid_t ptid;
   ptid_t saved_inferior_ptid;
-  int update_step_sp;
   int stepping_through_solib_after_catch;
   bpstat stepping_through_solib_catchpoints;
   int enable_hw_watchpoints_after_wait;
@@ -1106,7 +1105,6 @@ init_execution_control_state (struct exe
   ecs->random_signal = 0;
   ecs->remove_breakpoints_on_following_step = 0;
   ecs->handling_longjmp = 0;	/* FIXME */
-  ecs->update_step_sp = 0;
   ecs->stepping_through_solib_after_catch = 0;
   ecs->stepping_through_solib_catchpoints = NULL;
   ecs->enable_hw_watchpoints_after_wait = 0;
@@ -1260,7 +1258,7 @@ handle_step_into_function (struct execut
 static void
 adjust_pc_after_break (struct execution_control_state *ecs)
 {
-  CORE_ADDR stop_pc;
+  CORE_ADDR breakpoint_pc;
 
   /* If this target does not decrement the PC after breakpoints, then
      we have nothing to do.  */
@@ -1294,40 +1292,53 @@ adjust_pc_after_break (struct execution_
   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);
+  /* Find the location where (if we've hit a breakpoint) the
+     breakpoint would be.  */
+  breakpoint_pc = read_pc_pid (ecs->ptid) - DECR_PC_AFTER_BREAK;
+
+  if (SOFTWARE_SINGLE_STEP_P ())
+    {
+      /* When using software single-step, a SIGTRAP can only indicate
+	 an inserted breakpoint.  This actually makes things
+	 easier.  */
+      if (singlestep_breakpoints_inserted_p)
+	/* When software single stepping, the instruction at [prev_pc]
+	   is never a breakpoint, but the instruction following
+	   [prev_pc] (in program execution order) always is.  Assume
+	   that following instruction was reached and hence a software
+	   breakpoint was hit.  */
+	write_pc_pid (breakpoint_pc, ecs->ptid);
+      else if (software_breakpoint_inserted_here_p (breakpoint_pc))
+	/* The inferior was free running (i.e., no single-step
+	   breakpoints inserted) and it hit a software breakpoint.  */
+	write_pc_pid (breakpoint_pc, ecs->ptid);
+    }
+  else
+    {
+      /* When using hardware single-step, a SIGTRAP is reported for
+	 both a completed single-step and a software breakpoint.  Need
+	 to differentiate between the two as the latter needs
+	 adjusting but the former does not.  */
+      if (currently_stepping (ecs))
+	{
+	  if (prev_pc == breakpoint_pc
+	      && software_breakpoint_inserted_here_p (breakpoint_pc))
+	    /* Hardware single-stepped a software breakpoint (as
+	       occures when the inferior is resumed with PC pointing
+	       at not-yet-hit software breakpoint).  Since the
+	       breakpoint really is executed, the inferior needs to be
+	       backed up to the breakpoint address.  */
+	    write_pc_pid (breakpoint_pc, ecs->ptid);
+	}
+      else
+	{
+	  if (software_breakpoint_inserted_here_p (breakpoint_pc))
+	    /* The inferior was free running (i.e., no hardware
+	       single-step and no possibility of a false SIGTRAP) and
+	       hit a software breakpoint.  */
+	    write_pc_pid (breakpoint_pc, ecs->ptid);
+	}
+    }
 }
 
 /* Given an execution control state that has been freshly filled in
@@ -2410,11 +2421,6 @@ process_event_stop_test:
       return;
     }
 
-  /* We can't update step_sp every time through the loop, because
-     reading the stack pointer would slow down stepping too much.
-     But we can update it every time we leave the step range.  */
-  ecs->update_step_sp = 1;
-
   /* Did we just step into a singal trampoline (either by stepping out
      of a handler, or by taking a signal)?  */
   if (get_frame_type (get_current_frame ()) == SIGTRAMP_FRAME
@@ -2835,10 +2841,6 @@ keep_going (struct execution_control_sta
 {
   /* Save the pc before execution, to compare with pc after stop.  */
   prev_pc = read_pc ();		/* Might have been DECR_AFTER_BREAK */
-
-  if (ecs->update_step_sp)
-    step_sp = read_sp ();
-  ecs->update_step_sp = 0;
 
   /* If we did not do break;, it means we should keep running the
      inferior and not return to debugger.  */

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