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

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

Hello,

here's a patch to fix the problem reported in
http://sourceware.org/ml/gdb-patches/2007-07/msg00278.html

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.

As cleanup, the patch follows a suggestion in a comment in prepare_to_proceed
and makes switch_to_thread in thread.c global, and calls it instead of 
duplicating its contents.  That routine is then also called by the new code
in handle_inferior_event to switch back; it also appeared natural to use
the routine in context_switch.

This patch does not attempt to unify handling of this condition with
the stepping_past_singlestep_breakpoint mechanism.  The problem is that
this condition must be detected already in proceed, before the main
"struct execution_control_state" is even set up.  Thus, we cannot do
a proper context_switch back and forth.  Maybe there is a better way
to handle this ...

Tested with no new regressions on i368-linux and powerpc64-linux.

I'd appreciate comments on this approach! 

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	1 Aug 2007 18:30:09 -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	1 Aug 2007 18:30:10 -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,1634 ----
  
    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");
+ 
+ 	  /* 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	1 Aug 2007 18:30:10 -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