This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[rfc] Fix PR 2250 - multithreaded single-step problems in all-stop mode


Hello,

a while ago I posted a patch to fix problem PR 2250:
http://sourceware.org/ml/gdb-patches/2008-05/msg00089.html

This problem is about the behaviour of GDB when single-stepping
a thread of a multi-threaded application.  If during a single-step
operation, some other event happens in another thread, GDB may get
into an inconsistent state, where some internal data structures
still think a single-step operation is going on, while this is
in fact no longer true.

The patch I originally proposed, however, conflicted with the
new non-stop mode (where single-step operations can be outstanding
in multiple threads at the same time), so it was never applied.
Now that all the non-stop code is in mainline, the original bug
is still present in the all-stop mode.  I've reworked the patch
to fix the problem in all-stop mode, while keeping the new behaviour
in non-stop mode unchanges.

As discussed in the message refered to above, I'm implementing the
the following two changes:

- If the step operation is interrupted by an *internal* breakpoint
  that is handled transparently, the operation continues in a 
  transparent and consistent manner after the breakpoint was handled.
  (This is the handle_inferior_event change in the patch below.)

- If the step operation is interrupted by an *explicit* breakpoint
  that breaks to a user prompt, it is completely cancelled.  It is
  then up to the user how to continue from the prompt.
  (This is the clear_proceed_status change in the patch below.)

Regression tested on powerpc64-linux.  This patch fixes the problem
originally reported as PR 2250 (thanks to Emi Suzuki for verifying!).

I didn't test anything with all-stop mode, but by construction the
patch should not modify the behaviour at all in this case.

Does this look right?   If there are no objections, I'm planning
on committing this in a couple of days.

Bye,
Ulrich


ChangeLog:

	PR gdb/2250
	* infrun.c (clear_proceed_status_thread): New function.
	(clear_proceed_status_callback): New function.
	(clear_proceed_status): In all-stop mode, clear per-thread
	proceed status of *all* threads, not only the current.
	(handle_inferior_event): In all-stop mode, if we're stepping
	one thread, but got some inferior event in another thread
	that does not cause GDB to break to the user interface,
	ensure the interrupted stepping operation continues in the
	original thread.
	(currently_stepping): Move thread-related tests to ...
	(currently_stepping_thread): ... this new function.
	(currently_stepping_callback): New function.

Index: gdb/infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.336
diff -u -p -r1.336 infrun.c
--- gdb/infrun.c	5 Nov 2008 20:23:07 -0000	1.336
+++ gdb/infrun.c	13 Nov 2008 19:54:06 -0000
@@ -75,6 +75,8 @@ static void set_schedlock_func (char *ar
 
 static int currently_stepping (struct thread_info *tp);
 
+static int currently_stepping_callback (struct thread_info *tp, void *data);
+
 static void xdb_handle_command (char *args, int from_tty);
 
 static int prepare_to_proceed (int);
@@ -1161,31 +1163,59 @@ a command like `return' or `jump' to con
 /* Clear out all variables saying what to do when inferior is continued.
    First do this, then set the ones you want, then call `proceed'.  */
 
+static void
+clear_proceed_status_thread (struct thread_info *tp)
+{
+  if (debug_infrun)
+    fprintf_unfiltered (gdb_stdlog,
+			"infrun: clear_proceed_status_thread (%s)\n",
+			target_pid_to_str (tp->ptid));
+
+  tp->trap_expected = 0;
+  tp->step_range_start = 0;
+  tp->step_range_end = 0;
+  tp->step_frame_id = null_frame_id;
+  tp->step_over_calls = STEP_OVER_UNDEBUGGABLE;
+  tp->stop_requested = 0;
+
+  tp->stop_step = 0;
+
+  tp->proceed_to_finish = 0;
+
+  /* Discard any remaining commands or status from previous stop.  */
+  bpstat_clear (&tp->stop_bpstat);
+}
+
+static int
+clear_proceed_status_callback (struct thread_info *tp, void *data)
+{
+  if (is_exited (tp->ptid))
+    return 0;
+
+  clear_proceed_status_thread (tp);
+  return 0;
+}
+
 void
 clear_proceed_status (void)
 {
   if (!ptid_equal (inferior_ptid, null_ptid))
     {
-      struct thread_info *tp;
       struct inferior *inferior;
 
-      tp = inferior_thread ();
-
-      tp->trap_expected = 0;
-      tp->step_range_start = 0;
-      tp->step_range_end = 0;
-      tp->step_frame_id = null_frame_id;
-      tp->step_over_calls = STEP_OVER_UNDEBUGGABLE;
-      tp->stop_requested = 0;
-
-      tp->stop_step = 0;
-
-      tp->proceed_to_finish = 0;
-
-      /* Discard any remaining commands or status from previous
-	 stop.  */
-      bpstat_clear (&tp->stop_bpstat);
-
+      if (non_stop)
+	{
+	  /* If in non-stop mode, only delete the per-thread status
+	     of the current thread.  */
+	  clear_proceed_status_thread (inferior_thread ());
+	}
+      else
+	{
+	  /* In all-stop mode, delete the per-thread status of
+	     *all* threads.  */
+	  iterate_over_threads (clear_proceed_status_callback, NULL);
+	}
+  
       inferior = current_inferior ();
       inferior->stop_soon = NO_STOP_QUIETLY;
     }
@@ -3222,6 +3252,43 @@ infrun: BPSTAT_WHAT_SET_LONGJMP_RESUME (
      test for stepping.  But, if not stepping,
      do not stop.  */
 
+  /* In all-stop mode, if we're currently stepping but have stopped in
+     some other thread, we need to switch back to the stepped thread.  */
+  if (!non_stop)
+    {
+      struct thread_info *tp;
+      tp = iterate_over_threads (currently_stepping_callback,
+				 ecs->event_thread);
+      if (tp)
+	{
+	  /* However, if the current thread is blocked on some internal
+	     breakpoint, and we simply need to step over that breakpoint
+	     to get it going again, do that first.  */
+	  if ((ecs->event_thread->trap_expected
+	       && ecs->event_thread->stop_signal != TARGET_SIGNAL_TRAP)
+	      || ecs->event_thread->stepping_over_breakpoint)
+	    {
+	      keep_going (ecs);
+	      return;
+	    }
+
+	  /* Otherwise, we no longer expect a trap in the current thread.
+	     Clear the trap_expected flag before switching back -- this is
+	     what keep_going would do as well, if we called it.  */
+	  ecs->event_thread->trap_expected = 0;
+
+	  if (debug_infrun)
+	    fprintf_unfiltered (gdb_stdlog,
+				"infrun: switching back to stepped thread\n");
+
+	  ecs->event_thread = tp;
+	  ecs->ptid = tp->ptid;
+	  context_switch (ecs->ptid);
+	  keep_going (ecs);
+	  return;
+	}
+    }
+
   /* Are we stepping to get the inferior out of the dynamic linker's
      hook (and possibly the dld itself) after catching a shlib
      event?  */
@@ -3641,12 +3708,25 @@ infrun: BPSTAT_WHAT_SET_LONGJMP_RESUME (
 /* Are we in the middle of stepping?  */
 
 static int
+currently_stepping_thread (struct thread_info *tp)
+{
+  return (tp->step_range_end && tp->step_resume_breakpoint == NULL)
+	 || tp->trap_expected
+	 || tp->stepping_through_solib_after_catch;
+}
+
+static int
+currently_stepping_callback (struct thread_info *tp, void *data)
+{
+  /* Return true if any thread *but* the one passed in "data" is
+     in the middle of stepping.  */
+  return tp != data && currently_stepping_thread (tp);
+}
+
+static int
 currently_stepping (struct thread_info *tp)
 {
-  return (((tp->step_range_end && tp->step_resume_breakpoint == NULL)
-	   || tp->trap_expected)
-	  || tp->stepping_through_solib_after_catch
-	  || bpstat_should_step ());
+  return currently_stepping_thread (tp) || bpstat_should_step ();
 }
 
 /* Inferior has stepped into a subroutine call with source code that
-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]