[rfc] Fix infrun.c fall-out from the software single-step logic change

[rfc] Fix infrun.c fall-out from the software single-step logic change

[Ulrich Weigand]

Hello,

this fixes one outstanding issue after the ppc software single-step patch.

adjust_pc_after_break was using the assumption that SOFTWARE_SINGLE_STEP_P
was true if and only if we're using software single-step.  With the new
logic this is no longer true; instead the arch can decide on a case-by-case
basis whether to use software or hardware single-stepping.

This patch re-works the adjust_pc_after_break logic to remove that 
assumption.  It also simplifies the logic a bit, due to the fact that
now the software single-step breakpoints also show up via
software_breakpoint_inserted_here_p.

I've also removed a couple of checks for SOFTWARE_SINGLE_STEP_P in
handle_inferior_event -- simply checking for singlestep_breakpoints_inserted_p
should be OK now.

Tested on powerpc64-linux and spu-elf.

Any comments?  I don't actually have a test case where the old code
shows a bug, but it is clearly wrong ...

Bye,
Ulrich


ChangeLog:

	* infrun.c (adjust_pc_after_break): Do not assume software single-step
	is always active if SOFTWARE_SINGLE_STEP_P is true.
	(handle_inferior_event): Do not check for SOFTWARE_SINGLE_STEP_P.

diff -urNp gdb-orig/gdb/infrun.c gdb-head/gdb/infrun.c
--- gdb-orig/gdb/infrun.c	2007-06-15 19:20:34.030743111 +0200
+++ gdb-head/gdb/infrun.c	2007-06-15 21:08:14.534043469 +0200
@@ -1183,53 +1183,34 @@ adjust_pc_after_break (struct execution_
   breakpoint_pc = read_pc_pid (ecs->ptid) - gdbarch_decr_pc_after_break
 					    (current_gdbarch);
 
-  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.  */
+  /* Check whether there actually is a software breakpoint inserted
+     at that location.  */
+  if (software_breakpoint_inserted_here_p (breakpoint_pc))
+    {
+      /* 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.
+
+	 The SIGTRAP can be due to a completed hardware single-step only if 
+	  - we didn't insert software single-step breakpoints
+	  - the thread to be examined is still the current thread
+	  - this thread is currently being stepped
+
+	 If any of these events did not occur, we must have stopped
+	 due to hitting a the software breakpoint, and have to back
+	 up the breakpoint address.
+
+	 As a special case, we could have hardware single-stepped a
+	 a software breakpoint.  In this case (prev_pc == breakpoint_pc),
+	 we also need to back up to the breakpoint address.  */
+
+      if (singlestep_breakpoints_inserted_p
+	  || !ptid_equal (ecs->ptid, inferior_ptid)
+	  || !currently_stepping (ecs)
+	  || prev_pc == breakpoint_pc)
 	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.
-
-         When the thread to be examined does not match the current thread
-         context we can't use currently_stepping, so assume no
-         single-stepping in this case.  */
-      if (ptid_equal (ecs->ptid, inferior_ptid) && 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
@@ -1372,7 +1353,7 @@ handle_inferior_event (struct execution_
 					   (LONGEST) ecs->ws.value.integer));
       gdb_flush (gdb_stdout);
       target_mourn_inferior ();
-      singlestep_breakpoints_inserted_p = 0;	/* SOFTWARE_SINGLE_STEP_P() */
+      singlestep_breakpoints_inserted_p = 0;
       stop_print_frame = 0;
       stop_stepping (ecs);
       return;
@@ -1392,7 +1373,7 @@ handle_inferior_event (struct execution_
       target_mourn_inferior ();
 
       print_stop_reason (SIGNAL_EXITED, stop_signal);
-      singlestep_breakpoints_inserted_p = 0;	/* SOFTWARE_SINGLE_STEP_P() */
+      singlestep_breakpoints_inserted_p = 0;
       stop_stepping (ecs);
       return;
 
@@ -1548,8 +1529,7 @@ handle_inferior_event (struct execution_
 
   if (stepping_past_singlestep_breakpoint)
     {
-      gdb_assert (SOFTWARE_SINGLE_STEP_P ()
-		  && singlestep_breakpoints_inserted_p);
+      gdb_assert (singlestep_breakpoints_inserted_p);
       gdb_assert (ptid_equal (singlestep_ptid, ecs->ptid));
       gdb_assert (!ptid_equal (singlestep_ptid, saved_singlestep_ptid));
 
@@ -1598,7 +1578,7 @@ handle_inferior_event (struct execution_
 	  if (!breakpoint_thread_match (stop_pc, ecs->ptid))
 	    thread_hop_needed = 1;
 	}
-      else if (SOFTWARE_SINGLE_STEP_P () && singlestep_breakpoints_inserted_p)
+      else if (singlestep_breakpoints_inserted_p)
 	{
 	  /* We have not context switched yet, so this should be true
 	     no matter which thread hit the singlestep breakpoint.  */
@@ -1669,7 +1649,7 @@ handle_inferior_event (struct execution_
 	  /* Saw a breakpoint, but it was hit by the wrong thread.
 	     Just continue. */
 
-	  if (SOFTWARE_SINGLE_STEP_P () && singlestep_breakpoints_inserted_p)
+	  if (singlestep_breakpoints_inserted_p)
 	    {
 	      /* Pull the single step breakpoints out of the target. */
 	      remove_single_step_breakpoints ();
@@ -1718,7 +1698,7 @@ handle_inferior_event (struct execution_
 	      return;
 	    }
 	}
-      else if (SOFTWARE_SINGLE_STEP_P () && singlestep_breakpoints_inserted_p)
+      else if (singlestep_breakpoints_inserted_p)
 	{
 	  sw_single_step_trap_p = 1;
 	  ecs->random_signal = 0;
@@ -1740,7 +1720,7 @@ handle_inferior_event (struct execution_
 	deprecated_context_hook (pid_to_thread_id (ecs->ptid));
     }
 
-  if (SOFTWARE_SINGLE_STEP_P () && singlestep_breakpoints_inserted_p)
+  if (singlestep_breakpoints_inserted_p)
     {
       /* Pull the single step breakpoints out of the target. */
       remove_single_step_breakpoints ();
-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] Fix infrun.c fall-out from the software single-step logic change

[Ulrich Weigand]

> this fixes one outstanding issue after the ppc software single-step patch.

Here's a slightly modified version of that patch; it fixes some errors in
the comment wording, and also removes the SOFTWARE_SINGLE_STEP target macro
(the last one remaining!).

Tested on powerpc64-linux.

Bye,
Ulrich

ChangeLog:

	* infrun.c (adjust_pc_after_break): Do not assume software single-step
	is always active if SOFTWARE_SINGLE_STEP_P is true.
	(resume): Use gdbarch_software_single_step[_p] instead of
	SOFTWARE_SINGLE_STEP[_P].
	(handle_inferior_event): Do not check for SOFTWARE_SINGLE_STEP_P.

	* gdbarch.sh (software_single_step): Remove target macro.
	* gdbarch.h, gdbarch.c: Regenerate.

diff -urNp gdb-orig/gdb/gdbarch.c gdb-head/gdb/gdbarch.c
--- gdb-orig/gdb/gdbarch.c	2007-06-21 00:56:50.032443000 +0200
+++ gdb-head/gdb/gdbarch.c	2007-06-21 00:57:56.924903103 +0200
@@ -976,21 +976,9 @@ gdbarch_dump (struct gdbarch *current_gd
   fprintf_unfiltered (file,
                       "gdbarch_dump: smash_text_address = <0x%lx>\n",
                       (long) current_gdbarch->smash_text_address);
-#ifdef SOFTWARE_SINGLE_STEP_P
-  fprintf_unfiltered (file,
-                      "gdbarch_dump: %s # %s\n",
-                      "SOFTWARE_SINGLE_STEP_P()",
-                      XSTRING (SOFTWARE_SINGLE_STEP_P ()));
-#endif
   fprintf_unfiltered (file,
                       "gdbarch_dump: gdbarch_software_single_step_p() = %d\n",
                       gdbarch_software_single_step_p (current_gdbarch));
-#ifdef SOFTWARE_SINGLE_STEP
-  fprintf_unfiltered (file,
-                      "gdbarch_dump: %s # %s\n",
-                      "SOFTWARE_SINGLE_STEP(frame)",
-                      XSTRING (SOFTWARE_SINGLE_STEP (frame)));
-#endif
   fprintf_unfiltered (file,
                       "gdbarch_dump: software_single_step = <0x%lx>\n",
                       (long) current_gdbarch->software_single_step);
diff -urNp gdb-orig/gdb/gdbarch.h gdb-head/gdb/gdbarch.h
--- gdb-orig/gdb/gdbarch.h	2007-06-21 00:56:50.074437000 +0200
+++ gdb-head/gdb/gdbarch.h	2007-06-21 00:57:56.967896904 +0200
@@ -526,30 +526,11 @@ extern void set_gdbarch_smash_text_addre
    A return value of 1 means that the software_single_step breakpoints
    were inserted; 0 means they were not. */
 
-#if defined (SOFTWARE_SINGLE_STEP)
-/* Legacy for systems yet to multi-arch SOFTWARE_SINGLE_STEP */
-#if !defined (SOFTWARE_SINGLE_STEP_P)
-#define SOFTWARE_SINGLE_STEP_P() (1)
-#endif
-#endif
-
 extern int gdbarch_software_single_step_p (struct gdbarch *gdbarch);
-#if !defined (GDB_TM_FILE) && defined (SOFTWARE_SINGLE_STEP_P)
-#error "Non multi-arch definition of SOFTWARE_SINGLE_STEP"
-#endif
-#if !defined (SOFTWARE_SINGLE_STEP_P)
-#define SOFTWARE_SINGLE_STEP_P() (gdbarch_software_single_step_p (current_gdbarch))
-#endif
 
 typedef int (gdbarch_software_single_step_ftype) (struct frame_info *frame);
 extern int gdbarch_software_single_step (struct gdbarch *gdbarch, struct frame_info *frame);
 extern void set_gdbarch_software_single_step (struct gdbarch *gdbarch, gdbarch_software_single_step_ftype *software_single_step);
-#if !defined (GDB_TM_FILE) && defined (SOFTWARE_SINGLE_STEP)
-#error "Non multi-arch definition of SOFTWARE_SINGLE_STEP"
-#endif
-#if !defined (SOFTWARE_SINGLE_STEP)
-#define SOFTWARE_SINGLE_STEP(frame) (gdbarch_software_single_step (current_gdbarch, frame))
-#endif
 
 /* Return non-zero if the processor is executing a delay slot and a
    further single-step is needed before the instruction finishes. */
diff -urNp gdb-orig/gdb/gdbarch.sh gdb-head/gdb/gdbarch.sh
--- gdb-orig/gdb/gdbarch.sh	2007-06-21 00:56:50.082436000 +0200
+++ gdb-head/gdb/gdbarch.sh	2007-06-21 00:57:57.012890417 +0200
@@ -582,7 +582,7 @@ f::CORE_ADDR:smash_text_address:CORE_ADD
 #
 # A return value of 1 means that the software_single_step breakpoints 
 # were inserted; 0 means they were not.
-F:=:int:software_single_step:struct frame_info *frame:frame
+F::int:software_single_step:struct frame_info *frame:frame
 
 # Return non-zero if the processor is executing a delay slot and a
 # further single-step is needed before the instruction finishes.
diff -urNp gdb-orig/gdb/infrun.c gdb-head/gdb/infrun.c
--- gdb-orig/gdb/infrun.c	2007-06-21 00:56:50.132429000 +0200
+++ gdb-head/gdb/infrun.c	2007-06-21 00:57:57.060883498 +0200
@@ -537,10 +537,10 @@ how to step past a permanent breakpoint 
 a command like `return' or `jump' to continue execution."));
     }
 
-  if (SOFTWARE_SINGLE_STEP_P () && step)
+  if (step && gdbarch_software_single_step_p (current_gdbarch))
     {
       /* Do it the hard way, w/temp breakpoints */
-      if (SOFTWARE_SINGLE_STEP (get_current_frame ()))
+      if (gdbarch_software_single_step (current_gdbarch, get_current_frame ()))
         {
           /* ...and don't ask hardware to do it.  */
           step = 0;
@@ -1184,53 +1184,34 @@ adjust_pc_after_break (struct execution_
   breakpoint_pc = read_pc_pid (ecs->ptid) - gdbarch_decr_pc_after_break
 					    (current_gdbarch);
 
-  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.  */
+  /* Check whether there actually is a software breakpoint inserted
+     at that location.  */
+  if (software_breakpoint_inserted_here_p (breakpoint_pc))
+    {
+      /* 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.
+
+	 The SIGTRAP can be due to a completed hardware single-step only if 
+	  - we didn't insert software single-step breakpoints
+	  - the thread to be examined is still the current thread
+	  - this thread is currently being stepped
+
+	 If any of these events did not occur, we must have stopped due
+	 to hitting a software breakpoint, and have to back up to the
+	 breakpoint address.
+
+	 As a special case, we could have hardware single-stepped a
+	 software breakpoint.  In this case (prev_pc == breakpoint_pc),
+	 we also need to back up to the breakpoint address.  */
+
+      if (singlestep_breakpoints_inserted_p
+	  || !ptid_equal (ecs->ptid, inferior_ptid)
+	  || !currently_stepping (ecs)
+	  || prev_pc == breakpoint_pc)
 	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.
-
-         When the thread to be examined does not match the current thread
-         context we can't use currently_stepping, so assume no
-         single-stepping in this case.  */
-      if (ptid_equal (ecs->ptid, inferior_ptid) && 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
@@ -1373,7 +1354,7 @@ handle_inferior_event (struct execution_
 					   (LONGEST) ecs->ws.value.integer));
       gdb_flush (gdb_stdout);
       target_mourn_inferior ();
-      singlestep_breakpoints_inserted_p = 0;	/* SOFTWARE_SINGLE_STEP_P() */
+      singlestep_breakpoints_inserted_p = 0;
       stop_print_frame = 0;
       stop_stepping (ecs);
       return;
@@ -1393,7 +1374,7 @@ handle_inferior_event (struct execution_
       target_mourn_inferior ();
 
       print_stop_reason (SIGNAL_EXITED, stop_signal);
-      singlestep_breakpoints_inserted_p = 0;	/* SOFTWARE_SINGLE_STEP_P() */
+      singlestep_breakpoints_inserted_p = 0;
       stop_stepping (ecs);
       return;
 
@@ -1549,8 +1530,7 @@ handle_inferior_event (struct execution_
 
   if (stepping_past_singlestep_breakpoint)
     {
-      gdb_assert (SOFTWARE_SINGLE_STEP_P ()
-		  && singlestep_breakpoints_inserted_p);
+      gdb_assert (singlestep_breakpoints_inserted_p);
       gdb_assert (ptid_equal (singlestep_ptid, ecs->ptid));
       gdb_assert (!ptid_equal (singlestep_ptid, saved_singlestep_ptid));
 
@@ -1599,7 +1579,7 @@ handle_inferior_event (struct execution_
 	  if (!breakpoint_thread_match (stop_pc, ecs->ptid))
 	    thread_hop_needed = 1;
 	}
-      else if (SOFTWARE_SINGLE_STEP_P () && singlestep_breakpoints_inserted_p)
+      else if (singlestep_breakpoints_inserted_p)
 	{
 	  /* We have not context switched yet, so this should be true
 	     no matter which thread hit the singlestep breakpoint.  */
@@ -1670,7 +1650,7 @@ handle_inferior_event (struct execution_
 	  /* Saw a breakpoint, but it was hit by the wrong thread.
 	     Just continue. */
 
-	  if (SOFTWARE_SINGLE_STEP_P () && singlestep_breakpoints_inserted_p)
+	  if (singlestep_breakpoints_inserted_p)
 	    {
 	      /* Pull the single step breakpoints out of the target. */
 	      remove_single_step_breakpoints ();
@@ -1719,7 +1699,7 @@ handle_inferior_event (struct execution_
 	      return;
 	    }
 	}
-      else if (SOFTWARE_SINGLE_STEP_P () && singlestep_breakpoints_inserted_p)
+      else if (singlestep_breakpoints_inserted_p)
 	{
 	  sw_single_step_trap_p = 1;
 	  ecs->random_signal = 0;
@@ -1741,7 +1721,7 @@ handle_inferior_event (struct execution_
 	deprecated_context_hook (pid_to_thread_id (ecs->ptid));
     }
 
-  if (SOFTWARE_SINGLE_STEP_P () && singlestep_breakpoints_inserted_p)
+  if (singlestep_breakpoints_inserted_p)
     {
       /* Pull the single step breakpoints out of the target. */
       remove_single_step_breakpoints ();


-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] Fix infrun.c fall-out from the software single-step logic change

[Ulrich Weigand]

> 	* infrun.c (adjust_pc_after_break): Do not assume software single-step
> 	is always active if SOFTWARE_SINGLE_STEP_P is true.
> 	(resume): Use gdbarch_software_single_step[_p] instead of
> 	SOFTWARE_SINGLE_STEP[_P].
> 	(handle_inferior_event): Do not check for SOFTWARE_SINGLE_STEP_P.
> 
> 	* gdbarch.sh (software_single_step): Remove target macro.
> 	* gdbarch.h, gdbarch.c: Regenerate.

I've committed this as well.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] Fix infrun.c fall-out from the software single-step logic change

[Daniel Jacobowitz]

On Fri, Jun 22, 2007 at 02:55:32PM +0200, Ulrich Weigand wrote:
> 
> > 	* infrun.c (adjust_pc_after_break): Do not assume software single-step
> > 	is always active if SOFTWARE_SINGLE_STEP_P is true.
> > 	(resume): Use gdbarch_software_single_step[_p] instead of
> > 	SOFTWARE_SINGLE_STEP[_P].
> > 	(handle_inferior_event): Do not check for SOFTWARE_SINGLE_STEP_P.
> > 
> > 	* gdbarch.sh (software_single_step): Remove target macro.
> > 	* gdbarch.h, gdbarch.c: Regenerate.
> 
> I've committed this as well.

Thanks - it looked right to me, but I forgot to send the mail.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc] Fix infrun.c fall-out from the software single-step logic change

[Ulrich Weigand]

Daniel Jacobowitz wrote:
> On Fri, Jun 22, 2007 at 02:55:32PM +0200, Ulrich Weigand wrote:
> > 
> > > 	* infrun.c (adjust_pc_after_break): Do not assume software single-step
> > > 	is always active if SOFTWARE_SINGLE_STEP_P is true.
> > > 	(resume): Use gdbarch_software_single_step[_p] instead of
> > > 	SOFTWARE_SINGLE_STEP[_P].
> > > 	(handle_inferior_event): Do not check for SOFTWARE_SINGLE_STEP_P.
> > > 
> > > 	* gdbarch.sh (software_single_step): Remove target macro.
> > > 	* gdbarch.h, gdbarch.c: Regenerate.
> > 
> > I've committed this as well.
> 
> Thanks - it looked right to me, but I forgot to send the mail.

Thanks for looking at the patch!

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com