Re: [patch, rfc, rft] Multi-threaded single-step vs. breakpoint problems (prepare_to_proceed)

["Ulrich Weigand" <uweigand at de dot ibm dot com>]

Daniel Jacobowitz wrote:
> On Wed, Aug 01, 2007 at 09:52:21PM +0200, Ulrich Weigand wrote:
> > The main changes to prepare_to_proceed are do only ever switch threads
> > if we stopped due to hitting a breakpoint which is still set, and the
> > user switched threads in the meantime.  If we're also about to single-
> > step, we remember which thread we were supposed to step, and switch
> > back to it in handle_inferior_event after we're stepped past that
> > breakpoint.
> 
> I've no objection and the patch seems fine to me.
Thanks for the review!

> I wonder what
> happens if the other thread hit a watchpoint, or some other
> non-breakpoint event, though; on some platforms watchpoints must be
> stepped past similarly.

Those are handled differently: stepping past a watchpoint happens
in the thread where it was hit, and is done immediately when the
inferior first stopped, before even breaking to the user interface.

When hitting a breakpoint, on the other hand, stepping past it
is done only as part of the next "proceed" invocation after
the user has restarted the inferior again.  Only in this case
can the user have switched manually to another thread in the
meantime.


As an aside, I've found a bug in my original patch: it didn't work
for targets using software single-stepping, because I forget to
remove the software single-step breakpoints in the new special
case I introduced.  The version below fixes that problem.

Bye,
Ulrich


ChangeLog:

	* infrun.c (stepping_past_breakpoint): New global variable.
	(stepping_past_breakpoint_ptid): Likewise.
	(prepare_to_proceed): Add STEP parameter.  Do not check for Ctrl-C.
	Only switch threads if we need to single-step over a breakpoint hit
	in the previously selected thread.  If stepping, remember previous
	thread to switch back to in STEPPING_PAST_BREAKPOINT[_PTID].  Call
	switch_to_thread instead of copying its contents.
	(proceed): Pass STEP to prepare_to_proceed.  Always set ONEPROC if
	prepare_to_proceed returns true.
	(init_wait_for_inferior): Reset STEPPING_PAST_BREAKPOINT.
	(context_switch): Call switch_to_thread.
	(handle_inferior_event): Switch back to previous thread if requested
	in STEPPING_PAST_BREAKPOINT[_PTID] by prepare_to_proceed.
	* gdbthread.h (switch_to_thread): Add prototype.
	* thread.c (switch_to_thread): Make global.

Index: gdb/gdbthread.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbthread.h,v
retrieving revision 1.14
diff -c -p -r1.14 gdbthread.h
*** gdb/gdbthread.h	9 Jan 2007 17:58:51 -0000	1.14
--- gdb/gdbthread.h	3 Aug 2007 13:15:36 -0000
*************** extern void load_infrun_state (ptid_t pt
*** 137,142 ****
--- 137,145 ----
  			       int       *current_line,
  			       struct symtab **current_symtab);
  
+ /* Switch from one thread to another.  */
+ extern void switch_to_thread (ptid_t ptid);
+ 
  /* Commands with a prefix of `thread'.  */
  extern struct cmd_list_element *thread_cmd_list;
  
Index: gdb/infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.244
diff -c -p -r1.244 infrun.c
*** gdb/infrun.c	2 Jul 2007 21:29:27 -0000	1.244
--- gdb/infrun.c	3 Aug 2007 13:15:37 -0000
*************** static int currently_stepping (struct ex
*** 80,86 ****
  
  static void xdb_handle_command (char *args, int from_tty);
  
! static int prepare_to_proceed (void);
  
  void _initialize_infrun (void);
  
--- 80,86 ----
  
  static void xdb_handle_command (char *args, int from_tty);
  
! static int prepare_to_proceed (int);
  
  void _initialize_infrun (void);
  
*************** static CORE_ADDR singlestep_pc;
*** 447,452 ****
--- 447,457 ----
     thread here so that we can resume single-stepping it later.  */
  static ptid_t saved_singlestep_ptid;
  static int stepping_past_singlestep_breakpoint;
+ 
+ /* Similarly, if we are stepping another thread past a breakpoint,
+    save the original thread here so that we can resume stepping it later.  */
+ static ptid_t stepping_past_breakpoint_ptid;
+ static int stepping_past_breakpoint;
  
  
  /* Things to clean up if we QUIT out of resume ().  */
*************** clear_proceed_status (void)
*** 644,650 ****
  /* This should be suitable for any targets that support threads. */
  
  static int
! prepare_to_proceed (void)
  {
    ptid_t wait_ptid;
    struct target_waitstatus wait_status;
--- 649,655 ----
  /* This should be suitable for any targets that support threads. */
  
  static int
! prepare_to_proceed (int step)
  {
    ptid_t wait_ptid;
    struct target_waitstatus wait_status;
*************** prepare_to_proceed (void)
*** 652,693 ****
    /* Get the last target status returned by target_wait().  */
    get_last_target_status (&wait_ptid, &wait_status);
  
!   /* Make sure we were stopped either at a breakpoint, or because
!      of a Ctrl-C.  */
    if (wait_status.kind != TARGET_WAITKIND_STOPPED
!       || (wait_status.value.sig != TARGET_SIGNAL_TRAP
! 	  && wait_status.value.sig != TARGET_SIGNAL_INT))
      {
        return 0;
      }
  
    if (!ptid_equal (wait_ptid, minus_one_ptid)
!       && !ptid_equal (inferior_ptid, wait_ptid))
      {
!       /* Switched over from WAIT_PID.  */
!       CORE_ADDR wait_pc = read_pc_pid (wait_ptid);
! 
!       if (wait_pc != read_pc ())
  	{
! 	  /* Switch back to WAIT_PID thread.  */
! 	  inferior_ptid = wait_ptid;
! 
! 	  /* FIXME: This stuff came from switch_to_thread() in
! 	     thread.c (which should probably be a public function).  */
! 	  reinit_frame_cache ();
! 	  registers_changed ();
! 	  stop_pc = wait_pc;
  	}
  
        /* We return 1 to indicate that there is a breakpoint here,
!          so we need to step over it before continuing to avoid
!          hitting it straight away. */
!       if (breakpoint_here_p (wait_pc))
! 	return 1;
      }
  
    return 0;
- 
  }
  
  /* Record the pc of the program the last time it stopped.  This is
--- 657,691 ----
    /* Get the last target status returned by target_wait().  */
    get_last_target_status (&wait_ptid, &wait_status);
  
!   /* Make sure we were stopped at a breakpoint.  */
    if (wait_status.kind != TARGET_WAITKIND_STOPPED
!       || wait_status.value.sig != TARGET_SIGNAL_TRAP)
      {
        return 0;
      }
  
+   /* Switched over from WAIT_PID.  */
    if (!ptid_equal (wait_ptid, minus_one_ptid)
!       && !ptid_equal (inferior_ptid, wait_ptid)
!       && breakpoint_here_p (read_pc_pid (wait_ptid)))
      {
!       /* If stepping, remember current thread to switch back to.  */
!       if (step)
  	{
! 	  stepping_past_breakpoint = 1;
! 	  stepping_past_breakpoint_ptid = inferior_ptid;
  	}
  
+       /* Switch back to WAIT_PID thread.  */
+       switch_to_thread (wait_ptid);
+ 
        /* We return 1 to indicate that there is a breakpoint here,
! 	 so we need to step over it before continuing to avoid
! 	 hitting it straight away. */
!       return 1;
      }
  
    return 0;
  }
  
  /* Record the pc of the program the last time it stopped.  This is
*************** proceed (CORE_ADDR addr, enum target_sig
*** 753,759 ****
       prepare_to_proceed checks the current thread against the thread
       that reported the most recent event.  If a step-over is required
       it returns TRUE and sets the current thread to the old thread. */
!   if (prepare_to_proceed () && breakpoint_here_p (read_pc ()))
      oneproc = 1;
  
    if (oneproc)
--- 751,757 ----
       prepare_to_proceed checks the current thread against the thread
       that reported the most recent event.  If a step-over is required
       it returns TRUE and sets the current thread to the old thread. */
!   if (prepare_to_proceed (step))
      oneproc = 1;
  
    if (oneproc)
*************** init_wait_for_inferior (void)
*** 874,879 ****
--- 872,878 ----
    clear_proceed_status ();
  
    stepping_past_singlestep_breakpoint = 0;
+   stepping_past_breakpoint = 0;
  }
  
  /* This enum encodes possible reasons for doing a target_wait, so that
*************** context_switch (struct execution_control
*** 1143,1150 ****
  			 &ecs->stepping_through_solib_catchpoints,
  			 &ecs->current_line, &ecs->current_symtab);
      }
!   inferior_ptid = ecs->ptid;
!   reinit_frame_cache ();
  }
  
  static void
--- 1142,1149 ----
  			 &ecs->stepping_through_solib_catchpoints,
  			 &ecs->current_line, &ecs->current_symtab);
      }
! 
!   switch_to_thread (ecs->ptid);
  }
  
  static void
*************** handle_inferior_event (struct execution_
*** 1606,1611 ****
--- 1605,1641 ----
  
    stepping_past_singlestep_breakpoint = 0;
  
+   if (stepping_past_breakpoint)
+     {
+       stepping_past_breakpoint = 0;
+ 
+       /* If we stopped for some other reason than single-stepping, ignore
+ 	 the fact that we were supposed to switch back.  */
+       if (stop_signal == TARGET_SIGNAL_TRAP)
+ 	{
+ 	  if (debug_infrun)
+ 	    fprintf_unfiltered (gdb_stdlog,
+ 				"infrun: stepping_past_breakpoint\n");
+ 
+ 	  /* Pull the single step breakpoints out of the target.  */
+ 	  if (singlestep_breakpoints_inserted_p)
+ 	    {
+ 	      remove_single_step_breakpoints ();
+ 	      singlestep_breakpoints_inserted_p = 0;
+ 	    }
+ 
+ 	  /* Note: We do not call context_switch at this point, as the
+ 	     context is already set up for stepping the original thread.  */
+ 	  switch_to_thread (stepping_past_breakpoint_ptid);
+ 	  /* Suppress spurious "Switching to ..." message.  */
+ 	  previous_inferior_ptid = inferior_ptid;
+ 
+ 	  resume (1, TARGET_SIGNAL_0);
+ 	  prepare_to_wait (ecs);
+ 	  return;
+ 	}
+     }
+ 
    /* See if a thread hit a thread-specific breakpoint that was meant for
       another thread.  If so, then step that thread past the breakpoint,
       and continue it.  */
Index: gdb/thread.c
===================================================================
RCS file: /cvs/src/src/gdb/thread.c,v
retrieving revision 1.53
diff -c -p -r1.53 thread.c
*** gdb/thread.c	10 Apr 2007 14:53:46 -0000	1.53
--- gdb/thread.c	3 Aug 2007 13:15:37 -0000
*************** static int thread_alive (struct thread_i
*** 61,67 ****
  static void info_threads_command (char *, int);
  static void thread_apply_command (char *, int);
  static void restore_current_thread (ptid_t);
- static void switch_to_thread (ptid_t ptid);
  static void prune_threads (void);
  static struct cleanup *make_cleanup_restore_current_thread (ptid_t,
                                                              struct frame_id);
--- 61,66 ----
*************** info_threads_command (char *arg, int fro
*** 454,460 ****
  
  /* Switch from one thread to another. */
  
! static void
  switch_to_thread (ptid_t ptid)
  {
    if (ptid_equal (ptid, inferior_ptid))
--- 453,459 ----
  
  /* Switch from one thread to another. */
  
! void
  switch_to_thread (ptid_t ptid)
  {
    if (ptid_equal (ptid, inferior_ptid))


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