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

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

[Jie Zhang]

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;

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

[Daniel Jacobowitz]

On Tue, Aug 02, 2005 at 11:11:45PM +0800, Jie Zhang wrote:
> 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.

Yes... I fixed this once already, but I failed to consider the case
where both threads have hit the singlestep breakpoint, rather than
another thread hitting the breakpoint before the original thread has
had a chance to step.

I've fixed the version you encountered before, but it appears I never
submitted the patch.  I'm not sure if it still applies, but could you
give this a try?

> 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.

Interesting approach.  I don't like the implementation - I'd rather not
extend context_switch - but the concept may be better than mine.  Let
me think about this for a little.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Index: infrun.c
===================================================================
RCS file: /big/fsf/rsync/src-cvs/src/gdb/infrun.c,v
retrieving revision 1.137
diff -u -p -r1.137 infrun.c
--- infrun.c	16 Feb 2004 20:49:51 -0000	1.137
+++ infrun.c	6 Mar 2004 04:23:06 -0000
@@ -479,6 +479,9 @@ static int singlestep_breakpoints_insert
 /* The thread we inserted single-step breakpoints for.  */
 static ptid_t singlestep_ptid;
 
+/* PC when we started this single-step.  */
+static CORE_ADDR singlestep_pc;
+
 /* If another thread hit the singlestep breakpoint, we save the original
    thread here so that we can resume single-stepping it later.  */
 static ptid_t saved_singlestep_ptid;
@@ -570,6 +573,7 @@ resume (int step, enum target_signal sig
          `wait_for_inferior' */
       singlestep_breakpoints_inserted_p = 1;
       singlestep_ptid = inferior_ptid;
+      singlestep_pc = read_pc ();
     }
 
   /* Handle any optimized stores to the inferior NOW...  */
@@ -1201,6 +1205,7 @@ context_switch (struct execution_control
 			 &ecs->current_line, &ecs->current_symtab, &step_sp);
     }
   inferior_ptid = ecs->ptid;
+  flush_cached_frames ();
 }
 
 /* Wrapper for PC_IN_SIGTRAMP that takes care of the need to find the
@@ -1803,7 +1808,10 @@ handle_inferior_event (struct execution_
 	}
       else if (SOFTWARE_SINGLE_STEP_P () && singlestep_breakpoints_inserted_p)
 	{
+	  gdb_assert (ptid_equal (inferior_ptid, singlestep_ptid));
+
 	  ecs->random_signal = 0;
+
 	  /* The call to in_thread_list is necessary because PTIDs sometimes
 	     change when we go from single-threaded to multi-threaded.  If
 	     the singlestep_ptid is still in the list, assume that it is
@@ -1811,9 +1819,32 @@ handle_inferior_event (struct execution_
 	  if (!ptid_equal (singlestep_ptid, ecs->ptid)
 	      && in_thread_list (singlestep_ptid))
 	    {
-	      thread_hop_needed = 1;
-	      stepping_past_singlestep_breakpoint = 1;
-	      saved_singlestep_ptid = singlestep_ptid;
+	      /* If the PC of the thread we were trying to single-step
+		 has changed, discard this event (which we were going
+		 to ignore anyway), and pretend we saw that thread
+		 trap.  This runs a risk of losing signal information
+		 for singlestep_ptid, but prevents us continuously
+		 moving the single-step breakpoint.  This situation
+		 means that the thread has trapped or been signalled,
+		 but the event has not been reported to GDB yet.
+		 Really we should arrange to report all events, or to
+		 re-poll the remote looking for this particular
+		 thread (i.e. temporarily enable schedlock).  */
+	      if (read_pc_pid (singlestep_ptid) != singlestep_pc)
+		{
+		  /* The current context still belongs to
+		     singlestep_ptid.  Don't swap here, since that's
+		     the context we want to use.  Just fudge our
+		     state and continue.  */
+		  ecs->ptid = singlestep_ptid;
+		  stop_pc = read_pc_pid (ecs->ptid);
+		}
+	      else
+		{
+		  thread_hop_needed = 1;
+		  stepping_past_singlestep_breakpoint = 1;
+		  saved_singlestep_ptid = singlestep_ptid;
+		}
 	    }
 	}
 
@@ -1942,8 +1973,6 @@ handle_inferior_event (struct execution_
 
       if (context_hook)
 	context_hook (pid_to_thread_id (ecs->ptid));
-
-      flush_cached_frames ();
     }
 
   if (SOFTWARE_SINGLE_STEP_P () && singlestep_breakpoints_inserted_p)

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

[Jie Zhang]

Daniel,

Thanks for your thoughts. I tried your patch. It also works. However,
I think the concept of reinserting the hopped stepping breakpoint is
more intuitive.

Thanks,
Jie


On 8/2/05, Daniel Jacobowitz <drow@false.org> wrote:
> On Tue, Aug 02, 2005 at 11:11:45PM +0800, Jie Zhang wrote:
> > 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.
> 
> Yes... I fixed this once already, but I failed to consider the case
> where both threads have hit the singlestep breakpoint, rather than
> another thread hitting the breakpoint before the original thread has
> had a chance to step.
> 
> I've fixed the version you encountered before, but it appears I never
> submitted the patch.  I'm not sure if it still applies, but could you
> give this a try?
> 
> > 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.
> 
> Interesting approach.  I don't like the implementation - I'd rather not
> extend context_switch - but the concept may be better than mine.  Let
> me think about this for a little.
> 
> --
> Daniel Jacobowitz
> CodeSourcery, LLC
> 
> Index: infrun.c
> ===================================================================
> RCS file: /big/fsf/rsync/src-cvs/src/gdb/infrun.c,v
> retrieving revision 1.137
> diff -u -p -r1.137 infrun.c
> --- infrun.c    16 Feb 2004 20:49:51 -0000      1.137
> +++ infrun.c    6 Mar 2004 04:23:06 -0000
> @@ -479,6 +479,9 @@ static int singlestep_breakpoints_insert
>  /* The thread we inserted single-step breakpoints for.  */
>  static ptid_t singlestep_ptid;
> 
> +/* PC when we started this single-step.  */
> +static CORE_ADDR singlestep_pc;
> +
>  /* If another thread hit the singlestep breakpoint, we save the original
>     thread here so that we can resume single-stepping it later.  */
>  static ptid_t saved_singlestep_ptid;
> @@ -570,6 +573,7 @@ resume (int step, enum target_signal sig
>           `wait_for_inferior' */
>        singlestep_breakpoints_inserted_p = 1;
>        singlestep_ptid = inferior_ptid;
> +      singlestep_pc = read_pc ();
>      }
> 
>    /* Handle any optimized stores to the inferior NOW...  */
> @@ -1201,6 +1205,7 @@ context_switch (struct execution_control
>                          &ecs->current_line, &ecs->current_symtab, &step_sp);
>      }
>    inferior_ptid = ecs->ptid;
> +  flush_cached_frames ();
>  }
> 
>  /* Wrapper for PC_IN_SIGTRAMP that takes care of the need to find the
> @@ -1803,7 +1808,10 @@ handle_inferior_event (struct execution_
>         }
>        else if (SOFTWARE_SINGLE_STEP_P () && singlestep_breakpoints_inserted_p)
>         {
> +         gdb_assert (ptid_equal (inferior_ptid, singlestep_ptid));
> +
>           ecs->random_signal = 0;
> +
>           /* The call to in_thread_list is necessary because PTIDs sometimes
>              change when we go from single-threaded to multi-threaded.  If
>              the singlestep_ptid is still in the list, assume that it is
> @@ -1811,9 +1819,32 @@ handle_inferior_event (struct execution_
>           if (!ptid_equal (singlestep_ptid, ecs->ptid)
>               && in_thread_list (singlestep_ptid))
>             {
> -             thread_hop_needed = 1;
> -             stepping_past_singlestep_breakpoint = 1;
> -             saved_singlestep_ptid = singlestep_ptid;
> +             /* If the PC of the thread we were trying to single-step
> +                has changed, discard this event (which we were going
> +                to ignore anyway), and pretend we saw that thread
> +                trap.  This runs a risk of losing signal information
> +                for singlestep_ptid, but prevents us continuously
> +                moving the single-step breakpoint.  This situation
> +                means that the thread has trapped or been signalled,
> +                but the event has not been reported to GDB yet.
> +                Really we should arrange to report all events, or to
> +                re-poll the remote looking for this particular
> +                thread (i.e. temporarily enable schedlock).  */
> +             if (read_pc_pid (singlestep_ptid) != singlestep_pc)
> +               {
> +                 /* The current context still belongs to
> +                    singlestep_ptid.  Don't swap here, since that's
> +                    the context we want to use.  Just fudge our
> +                    state and continue.  */
> +                 ecs->ptid = singlestep_ptid;
> +                 stop_pc = read_pc_pid (ecs->ptid);
> +               }
> +             else
> +               {
> +                 thread_hop_needed = 1;
> +                 stepping_past_singlestep_breakpoint = 1;
> +                 saved_singlestep_ptid = singlestep_ptid;
> +               }
>             }
>         }
> 
> @@ -1942,8 +1973,6 @@ handle_inferior_event (struct execution_
> 
>        if (context_hook)
>         context_hook (pid_to_thread_id (ecs->ptid));
> -
> -      flush_cached_frames ();
>      }
> 
>    if (SOFTWARE_SINGLE_STEP_P () && singlestep_breakpoints_inserted_p)
>