[RFC] Reinsert the single stepping breakpoint after being hopped over

[Jie Zhang <jzhang918 at gmail dot com>]

Hi,

I'm working on a new port of GDB for Blackfin. It's usually used for
remote debugging using gdbserver and software single stepping. It has
many FAILs for schedlock.exp. I traced these FAILs and, I think, found
a bug in GDB.

Assume there are two threads in the program and we are stepping on
one. (I use GDB for the whole of GDB and gdbserver.)

1. GDB inserts a breakpoint for stepping and resumes both threads.
2. Both threads hit this breakpoint.
3. The first stopped thread GDB looks for (for some reason) is not the
thread we are stepping. GDB make it hop over the stepping breakpoint.
4. GDB resume both threads by

    resume (1, TARGET_SIGNAL_0);

So the thread we are stepping will do a new step and the previous stop
of step will never be noticed by GDB. (It's caught by gdbserver and
saved as a pending status. However, when it stops again for the new
step, the previous stepping breakpoint has been removed, so
check_removed_breakpoint () will clear status_pending_p and the
previous stop is lost.) GDB will never get a chance to check if the
stepping is out of the range. The step command will not return.

To fix it, GDB should put the previous stepping breakpoint back, not
do a new stepping when resuming both threads. The following patch is
trying to fix this. It adds a new field "singlestep_breakpoint_addr"
in "struct thread_info" to remember the last single stepping
breakpoint address. Passing 2 as the second argument to
SOFTWARE_SINGLE_STEP () to tell the target software single stepping
function that we just reinsert the previous single stepping
breakpoint.

This is a preliminary patch. It only changes the arm port as an
example. I tested this patch, it fixes all FAILs of schedlock.exp for
our new port. However, I have no boards of other ports using software
single stepping. So I cannot test it.

And I'm not sure if this bug is only for remote debugging using
gdbserver? Or if this patch will affect native ports using software
single stepping?

Any thoughts?

Jie



2005-08-02  Jie Zhang  <jie.zhang@analog.com>

	* arm-tdep.c (arm_software_single_step): Use singlestep_breakpoint_addr.
	* gdbthread.h (struct thread_info): New field
	singlestep_breakpoint_addr.
	(save_infrun_state): New argument singlestep_breakpoint_addr.
	(load_infrun_state): Likewise.
	* infcmd.c (singlestep_breakpoint_addr): Define.
	* inferior.h (singlestep_breakpoint_addr): Declare.
	* infrun.c (context_switch): Save and load singlestep_breakpoint_addr.
	(handle_inferior_event): Reinsert the single stepping breakpoint after
	another thread has hopped over it.
	* thread.c (save_infrun_state): Save singlestep_breakpoint_addr.
	(load_infrun_state): Load singlestep_breakpoint_addr.

Index: gdb/arm-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/arm-tdep.c,v
retrieving revision 1.200
diff -u -p -r1.200 arm-tdep.c
--- gdb/arm-tdep.c	12 Jun 2005 12:57:21 -0000	1.200
+++ gdb/arm-tdep.c	2 Aug 2005 14:39:02 -0000
@@ -1872,16 +1872,17 @@ arm_get_next_pc (CORE_ADDR pc)
 static void
 arm_software_single_step (enum target_signal sig, int insert_bpt)
 {
-  static int next_pc;		 /* State between setting and unsetting.  */
   static char break_mem[BREAKPOINT_MAX]; /* Temporary storage for mem@bpt */
 
   if (insert_bpt)
     {
-      next_pc = arm_get_next_pc (read_register (ARM_PC_REGNUM));
-      target_insert_breakpoint (next_pc, break_mem);
+      if (insert_bpt == 1)
+	singlestep_breakpoint_addr
+	    = arm_get_next_pc (read_register (ARM_PC_REGNUM));
+      target_insert_breakpoint (singlestep_breakpoint_addr, break_mem);
     }
   else
-    target_remove_breakpoint (next_pc, break_mem);
+    target_remove_breakpoint (singlestep_breakpoint_addr, break_mem);
 }
 
 #include "bfd-in2.h"
Index: gdb/gdbthread.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbthread.h,v
retrieving revision 1.12
diff -u -p -r1.12 gdbthread.h
--- gdb/gdbthread.h	25 Aug 2004 15:18:05 -0000	1.12
+++ gdb/gdbthread.h	2 Aug 2005 14:39:02 -0000
@@ -47,6 +47,7 @@ struct thread_info
   struct breakpoint *step_resume_breakpoint;
   CORE_ADDR step_range_start;
   CORE_ADDR step_range_end;
+  CORE_ADDR singlestep_breakpoint_addr;
   struct frame_id step_frame_id;
   int current_line;
   struct symtab *current_symtab;
@@ -114,6 +115,7 @@ extern void save_infrun_state (ptid_t pt
 			       struct breakpoint *step_resume_breakpoint,
 			       CORE_ADDR step_range_start,
 			       CORE_ADDR step_range_end,
+			       CORE_ADDR singlestep_breakpoint_addr,
 			       const struct frame_id *step_frame_id,
 			       int       handling_longjmp,
 			       int       another_trap,
@@ -130,6 +132,7 @@ extern void load_infrun_state (ptid_t pt
 			       struct breakpoint **step_resume_breakpoint,
 			       CORE_ADDR *step_range_start,
 			       CORE_ADDR *step_range_end,
+			       CORE_ADDR *singlestep_breakpoint_addr,
 			       struct frame_id *step_frame_id,
 			       int       *handling_longjmp,
 			       int       *another_trap,
Index: gdb/infcmd.c
===================================================================
RCS file: /cvs/src/src/gdb/infcmd.c,v
retrieving revision 1.139
diff -u -p -r1.139 infcmd.c
--- gdb/infcmd.c	6 Jul 2005 14:54:33 -0000	1.139
+++ gdb/infcmd.c	2 Aug 2005 14:39:03 -0000
@@ -181,6 +181,7 @@ int stopped_by_random_signal;
 
 CORE_ADDR step_range_start;	/* Inclusive */
 CORE_ADDR step_range_end;	/* Exclusive */
+CORE_ADDR singlestep_breakpoint_addr;
 
 /* Stack frame address as of when stepping command was issued.
    This is how we know when we step into a subroutine call,
Index: gdb/inferior.h
===================================================================
RCS file: /cvs/src/src/gdb/inferior.h,v
retrieving revision 1.72
diff -u -p -r1.72 inferior.h
--- gdb/inferior.h	6 Jul 2005 14:54:33 -0000	1.72
+++ gdb/inferior.h	2 Aug 2005 14:39:03 -0000
@@ -355,6 +355,7 @@ extern int stopped_by_random_signal;
 
 extern CORE_ADDR step_range_start;	/* Inclusive */
 extern CORE_ADDR step_range_end;	/* Exclusive */
+extern CORE_ADDR singlestep_breakpoint_addr;
 
 /* Stack frame address as of when stepping command was issued.
    This is how we know when we step into a subroutine call,
Index: gdb/infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.203
diff -u -p -r1.203 infrun.c
--- gdb/infrun.c	1 Aug 2005 03:32:32 -0000	1.203
+++ gdb/infrun.c	2 Aug 2005 14:39:06 -0000
@@ -1117,8 +1117,8 @@ context_switch (struct execution_control
       /* Save infrun state for the old thread.  */
       save_infrun_state (inferior_ptid, prev_pc,
 			 trap_expected, step_resume_breakpoint,
-			 step_range_start,
-			 step_range_end, &step_frame_id,
+			 step_range_start, step_range_end,
+			 singlestep_breakpoint_addr, &step_frame_id,
 			 ecs->handling_longjmp, ecs->another_trap,
 			 ecs->stepping_through_solib_after_catch,
 			 ecs->stepping_through_solib_catchpoints,
@@ -1127,8 +1127,8 @@ context_switch (struct execution_control
       /* Load infrun state for the new thread.  */
       load_infrun_state (ecs->ptid, &prev_pc,
 			 &trap_expected, &step_resume_breakpoint,
-			 &step_range_start,
-			 &step_range_end, &step_frame_id,
+			 &step_range_start, &step_range_end,
+			 &singlestep_breakpoint_addr, &step_frame_id,
 			 &ecs->handling_longjmp, &ecs->another_trap,
 			 &ecs->stepping_through_solib_after_catch,
 			 &ecs->stepping_through_solib_catchpoints,
@@ -1555,7 +1555,10 @@ handle_inferior_event (struct execution_
 	  if (deprecated_context_hook)
 	    deprecated_context_hook (pid_to_thread_id (ecs->ptid));
 
-	  resume (1, TARGET_SIGNAL_0);
+	  SOFTWARE_SINGLE_STEP (TARGET_SIGNAL_0, 2);
+	  singlestep_breakpoints_inserted_p = 1;
+	  singlestep_ptid = inferior_ptid;
+	  resume (0, TARGET_SIGNAL_0);
 	  prepare_to_wait (ecs);
 	  return;
 	}
Index: gdb/thread.c
===================================================================
RCS file: /cvs/src/src/gdb/thread.c,v
retrieving revision 1.46
diff -u -p -r1.46 thread.c
--- gdb/thread.c	15 Feb 2005 15:49:22 -0000	1.46
+++ gdb/thread.c	2 Aug 2005 14:39:06 -0000
@@ -297,6 +297,7 @@ load_infrun_state (ptid_t ptid,
 		   struct breakpoint **step_resume_breakpoint,
 		   CORE_ADDR *step_range_start,
 		   CORE_ADDR *step_range_end,
+		   CORE_ADDR *singlestep_breakpoint_addr,
 		   struct frame_id *step_frame_id,
 		   int *handling_longjmp,
 		   int *another_trap,
@@ -318,6 +319,7 @@ load_infrun_state (ptid_t ptid,
   *step_resume_breakpoint = tp->step_resume_breakpoint;
   *step_range_start = tp->step_range_start;
   *step_range_end = tp->step_range_end;
+  *singlestep_breakpoint_addr = tp->singlestep_breakpoint_addr;
   *step_frame_id = tp->step_frame_id;
   *handling_longjmp = tp->handling_longjmp;
   *another_trap = tp->another_trap;
@@ -338,6 +340,7 @@ save_infrun_state (ptid_t ptid,
 		   struct breakpoint *step_resume_breakpoint,
 		   CORE_ADDR step_range_start,
 		   CORE_ADDR step_range_end,
+		   CORE_ADDR singlestep_breakpoint_addr,
 		   const struct frame_id *step_frame_id,
 		   int handling_longjmp,
 		   int another_trap,
@@ -359,6 +362,7 @@ save_infrun_state (ptid_t ptid,
   tp->step_resume_breakpoint = step_resume_breakpoint;
   tp->step_range_start = step_range_start;
   tp->step_range_end = step_range_end;
+  tp->singlestep_breakpoint_addr = singlestep_breakpoint_addr;
   tp->step_frame_id = (*step_frame_id);
   tp->handling_longjmp = handling_longjmp;
   tp->another_trap = another_trap;