[pushed] Consecutive step-overs trigger internal error.

[Pedro Alves <palves at redhat dot com>]

If a thread trips on a breakpoint that needs stepping over just after
finishing a step over, GDB currently fails an assertion.  This is a
regression caused by the "Handle multiple step-overs." patch
(99619beac6252113fed212fdb9e1ab97bface423) at
https://sourceware.org/ml/gdb-patches/2014-02/msg00765.html.

 (gdb) x /4i $pc
 => 0x400540 <main+4>:   movl   $0x0,0x2003da(%rip)        # 0x600924 <i>
    0x40054a <main+14>:  movl   $0x1,0x2003d0(%rip)        # 0x600924 <i>
    0x400554 <main+24>:  movl   $0x2,0x2003c6(%rip)        # 0x600924 <i>
    0x40055e <main+34>:  movl   $0x3,0x2003bc(%rip)        # 0x600924 <i>
 (gdb) PASS: gdb.base/consecutive-step-over.exp: get breakpoint addresses
 break *0x40054a
 Breakpoint 2 at 0x40054a: file ../../../src/gdb/testsuite/gdb.base/consecutive-step-over.c, line 23.
 (gdb) PASS: gdb.base/consecutive-step-over.exp: insn 1: set breakpoint
 condition $bpnum condition
 (gdb) PASS: gdb.base/consecutive-step-over.exp: insn 1: set condition
 break *0x400554
 Breakpoint 3 at 0x400554: file ../../../src/gdb/testsuite/gdb.base/consecutive-step-over.c, line 24.
 (gdb) PASS: gdb.base/consecutive-step-over.exp: insn 2: set breakpoint
 condition $bpnum condition
 (gdb) PASS: gdb.base/consecutive-step-over.exp: insn 2: set condition
 break *0x40055e
 Breakpoint 4 at 0x40055e: file ../../../src/gdb/testsuite/gdb.base/consecutive-step-over.c, line 25.
 (gdb) PASS: gdb.base/consecutive-step-over.exp: insn 3: set breakpoint
 condition $bpnum condition
 (gdb) PASS: gdb.base/consecutive-step-over.exp: insn 3: set condition
 break 27
 Breakpoint 5 at 0x400568: file ../../../src/gdb/testsuite/gdb.base/consecutive-step-over.c, line 27.
 (gdb) continue
 Continuing.
 ../../src/gdb/infrun.c:5200: internal-error: switch_back_to_stepped_thread: Assertion `!tp->control.trap_expected' failed.
 A problem internal to GDB has been detected,
 further debugging may prove unreliable.
 FAIL: gdb.base/consecutive-step-over.exp: continue to breakpoint: break here (GDB internal error)

The assertion fails, because the code is not expecting that the event
thread itself might need another step over.  IOW, not expecting that
TP in:

     tp = find_thread_needs_step_over (stepping_thread != NULL,
                                      stepping_thread);

could be the event thread.

A small fix for this would be to clear the event thread's
trap_expected earlier, before asserting.  But looking deeper, although
currently_stepping_or_nexting_callback's intention is finding the
thread that is doing a step/next, it also returns the thread that is
doing a step-over dance, with trap_expected set.  If there ever was a
reason for that (it was I who added
currently_stepping_or_nexting_callback , but I can't recall why I put
trap_expected there in the first place), the only remaining reason
nowadays is to aid in implementing switch_back_to_stepped_thread's
assertion that is now triggering, by piggybacking on the walk over all
threads, thus avoiding a separate walk.  This is quite obscure, and I
think we can do even better, by merging the walks that look for the
stepping thread, and the walk that looks for some thread that might
need a step over.

Tested on x86_64 Fedora 17, native and gdbserver, and also native on
top of my "software single-step on x86_64" series.

gdb/
2014-04-22  Pedro Alves  <palves@redhat.com>

	* infrun.c (schedlock_applies): New function, factored out from
	find_thread_needs_step_over.
	(find_thread_needs_step_over): Use it.
	(switch_back_to_stepped_thread): Always clear trap_expected if the
	step over is finished.  Return early if scheduler locking applies.
	Look for the stepping thread and a potential step-over thread with
	a single loop.
	(currently_stepping_or_nexting_callback): Delete.

2014-04-22  Pedro Alves  <palves@redhat.com>

	* gdb.base/consecutive-step-over.c: New file.
	* gdb.base/consecutive-step-over.exp: New file.
---
 gdb/ChangeLog                                    |  11 ++
 gdb/infrun.c                                     | 125 +++++++++++++++--------
 gdb/testsuite/ChangeLog                          |   5 +
 gdb/testsuite/gdb.base/consecutive-step-over.c   |  28 +++++
 gdb/testsuite/gdb.base/consecutive-step-over.exp |  70 +++++++++++++
 5 files changed, 195 insertions(+), 44 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/consecutive-step-over.c
 create mode 100644 gdb/testsuite/gdb.base/consecutive-step-over.exp

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 120b70b..6de9f69 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,14 @@
+2014-04-22  Pedro Alves  <palves@redhat.com>
+
+	* infrun.c (schedlock_applies): New function, factored out from
+	find_thread_needs_step_over.
+	(find_thread_needs_step_over): Use it.
+	(switch_back_to_stepped_thread): Always clear trap_expected if the
+	step over is finished.  Return early if scheduler locking applies.
+	Look for the stepping thread and a potential step-over thread with
+	a single loop.
+	(currently_stepping_or_nexting_callback): Delete.
+
 2014-04-22  Nick Clifton  <nickc@redhat.com>
 
 	* NEWS: Mention that ARM sim now supports tracing.
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 31bb132..ab39b6e 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -85,9 +85,6 @@ static void set_schedlock_func (char *args, int from_tty,
 
 static int currently_stepping (struct thread_info *tp);
 
-static int currently_stepping_or_nexting_callback (struct thread_info *tp,
-						   void *data);
-
 static void xdb_handle_command (char *args, int from_tty);
 
 static void print_exited_reason (int exitstatus);
@@ -2107,6 +2104,17 @@ thread_still_needs_step_over (struct thread_info *tp)
   return 0;
 }
 
+/* Returns true if scheduler locking applies.  STEP indicates whether
+   we're about to do a step/next-like command to a thread.  */
+
+static int
+schedlock_applies (int step)
+{
+  return (scheduler_mode == schedlock_on
+	  || (scheduler_mode == schedlock_step
+	      && step));
+}
+
 /* Look a thread other than EXCEPT that has previously reported a
    breakpoint event, and thus needs a step-over in order to make
    progress.  Returns NULL is none is found.  STEP indicates whether
@@ -2116,21 +2124,16 @@ thread_still_needs_step_over (struct thread_info *tp)
 static struct thread_info *
 find_thread_needs_step_over (int step, struct thread_info *except)
 {
-  int schedlock_enabled;
   struct thread_info *tp, *current;
 
   /* With non-stop mode on, threads are always handled individually.  */
   gdb_assert (! non_stop);
 
-  schedlock_enabled = (scheduler_mode == schedlock_on
-		       || (scheduler_mode == schedlock_step
-			   && step));
-
   current = inferior_thread ();
 
   /* If scheduler locking applies, we can avoid iterating over all
      threads.  */
-  if (schedlock_enabled)
+  if (schedlock_applies (step))
     {
       if (except != current
 	  && thread_still_needs_step_over (current))
@@ -5137,6 +5140,7 @@ switch_back_to_stepped_thread (struct execution_control_state *ecs)
     {
       struct thread_info *tp;
       struct thread_info *stepping_thread;
+      struct thread_info *step_over;
 
       /* If any thread is blocked on some internal breakpoint, and we
 	 simply need to step over that breakpoint to get it going
@@ -5179,17 +5183,72 @@ switch_back_to_stepped_thread (struct execution_control_state *ecs)
 	 return 1;
        }
 
-      stepping_thread
-	= iterate_over_threads (currently_stepping_or_nexting_callback,
-				ecs->event_thread);
+      /* Otherwise, we no longer expect a trap in the current thread.
+	 Clear the trap_expected flag before switching back -- this is
+	 what keep_going does as well, if we call it.  */
+      ecs->event_thread->control.trap_expected = 0;
+
+      /* If scheduler locking applies even if not stepping, there's no
+	 need to walk over threads.  Above we've checked whether the
+	 current thread is stepping.  If some other thread not the
+	 event thread is stepping, then it must be that scheduler
+	 locking is not in effect.  */
+      if (schedlock_applies (0))
+	return 0;
+
+      /* Look for the stepping/nexting thread, and check if any other
+	 thread other than the stepping thread needs to start a
+	 step-over.  Do all step-overs before actually proceeding with
+	 step/next/etc.  */
+      stepping_thread = NULL;
+      step_over = NULL;
+      ALL_THREADS (tp)
+        {
+	  /* Ignore threads of processes we're not resuming.  */
+	  if (!sched_multi
+	      && ptid_get_pid (tp->ptid) != ptid_get_pid (inferior_ptid))
+	    continue;
+
+	  /* When stepping over a breakpoint, we lock all threads
+	     except the one that needs to move past the breakpoint.
+	     If a non-event thread has this set, the "incomplete
+	     step-over" check above should have caught it earlier.  */
+	  gdb_assert (!tp->control.trap_expected);
+
+	  /* Did we find the stepping thread?  */
+	  if (tp->control.step_range_end)
+	    {
+	      /* Yep.  There should only one though.  */
+	      gdb_assert (stepping_thread == NULL);
+
+	      /* The event thread is handled at the top, before we
+		 enter this loop.  */
+	      gdb_assert (tp != ecs->event_thread);
+
+	      /* If some thread other than the event thread is
+		 stepping, then scheduler locking can't be in effect,
+		 otherwise we wouldn't have resumed the current event
+		 thread in the first place.  */
+	      gdb_assert (!schedlock_applies (1));
+
+	      stepping_thread = tp;
+	    }
+	  else if (thread_still_needs_step_over (tp))
+	    {
+	      step_over = tp;
+
+	      /* At the top we've returned early if the event thread
+		 is stepping.  If some other thread not the event
+		 thread is stepping, then scheduler locking can't be
+		 in effect, and we can resume this thread.  No need to
+		 keep looking for the stepping thread then.  */
+	      break;
+	    }
+	}
 
-      /* Check if any other thread except the stepping thread that
-	 needs to start a step-over.  Do that before actually
-	 proceeding with step/next/etc.  */
-      tp = find_thread_needs_step_over (stepping_thread != NULL,
-					stepping_thread);
-      if (tp != NULL)
+      if (step_over != NULL)
 	{
+	  tp = step_over;
 	  if (debug_infrun)
 	    {
 	      fprintf_unfiltered (gdb_stdlog,
@@ -5197,14 +5256,9 @@ switch_back_to_stepped_thread (struct execution_control_state *ecs)
 				  target_pid_to_str (tp->ptid));
 	    }
 
-	  gdb_assert (!tp->control.trap_expected);
+	  /* Only the stepping thread should have this set.  */
 	  gdb_assert (tp->control.step_range_end == 0);
 
-	  /* We no longer expect a trap in the current thread.  Clear
-	     the trap_expected flag before switching.  This is what
-	     keep_going would do as well, if we called it.  */
-	  ecs->event_thread->control.trap_expected = 0;
-
 	  ecs->ptid = tp->ptid;
 	  ecs->event_thread = tp;
 	  switch_to_thread (ecs->ptid);
@@ -5212,12 +5266,13 @@ switch_back_to_stepped_thread (struct execution_control_state *ecs)
 	  return 1;
 	}
 
-      tp = stepping_thread;
-      if (tp != NULL)
+      if (stepping_thread != NULL)
 	{
 	  struct frame_info *frame;
 	  struct gdbarch *gdbarch;
 
+	  tp = stepping_thread;
+
 	  /* If the stepping thread exited, then don't try to switch
 	     back and resume it, which could fail in several different
 	     ways depending on the target.  Instead, just keep going.
@@ -5250,11 +5305,6 @@ switch_back_to_stepped_thread (struct execution_control_state *ecs)
 	      return 1;
 	    }
 
-	  /* 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->control.trap_expected = 0;
-
 	  if (debug_infrun)
 	    fprintf_unfiltered (gdb_stdlog,
 				"infrun: switching back to stepped thread\n");
@@ -5325,19 +5375,6 @@ currently_stepping (struct thread_info *tp)
 	  || bpstat_should_step ());
 }
 
-/* Returns true if any thread *but* the one passed in "data" is in the
-   middle of stepping or of handling a "next".  */
-
-static int
-currently_stepping_or_nexting_callback (struct thread_info *tp, void *data)
-{
-  if (tp == data)
-    return 0;
-
-  return (tp->control.step_range_end
- 	  || tp->control.trap_expected);
-}
-
 /* Inferior has stepped into a subroutine call with source code that
    we should not step over.  Do step to the first line of code in
    it.  */
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 1de6dc0..2f42ad1 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,5 +1,10 @@
 2014-04-22  Pedro Alves  <palves@redhat.com>
 
+	* gdb.base/consecutive-step-over.c: New file.
+	* gdb.base/consecutive-step-over.exp: New file.
+
+2014-04-22  Pedro Alves  <palves@redhat.com>
+
 	* lib/gdb.exp (gdb_continue_to_breakpoint): Use gdb_test_multiple
 	instead of send_gdb/gdb_expect.
 
diff --git a/gdb/testsuite/gdb.base/consecutive-step-over.c b/gdb/testsuite/gdb.base/consecutive-step-over.c
new file mode 100644
index 0000000..e7c8e9d
--- /dev/null
+++ b/gdb/testsuite/gdb.base/consecutive-step-over.c
@@ -0,0 +1,28 @@
+/* Copyright 2014 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+volatile int i;
+volatile int condition;
+
+int
+main (void)
+{
+  i = 0;
+  i = 1;
+  i = 2;
+  i = 3;
+
+  return 0; /* break here */
+}
diff --git a/gdb/testsuite/gdb.base/consecutive-step-over.exp b/gdb/testsuite/gdb.base/consecutive-step-over.exp
new file mode 100644
index 0000000..3f78042
--- /dev/null
+++ b/gdb/testsuite/gdb.base/consecutive-step-over.exp
@@ -0,0 +1,70 @@
+# Copyright 2014 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+#
+# Regression test for a bug where GDB would internal error if a thread
+# runs into a breakpoint that needs stepping over, just after stepping
+# over another breakpoint, without a user visible stop in between.
+#
+standard_testfile
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
+    return -1
+}
+
+if ![runto_main] then {
+    fail "Can't run to main"
+    return 0
+}
+
+# Make sure the target doesn't hide the breakpoint hits (that don't
+# cause a user visible stop) from GDB.
+gdb_test_no_output "set breakpoint condition-evaluation host"
+
+set up_to_nl "\[^\r\n\]+\[\r\n\]+"
+
+# Number of consecutive breakpoints in a row to try.
+set n_insns 3
+
+# Extract addresses of a few consecutive instructions.
+set test "get breakpoint addresses"
+if { [gdb_test_multiple "x /[expr $n_insns + 1]i \$pc" $test {
+    -re "=> $hex${up_to_nl}   ($hex)${up_to_nl}   ($hex)${up_to_nl}   ($hex)${up_to_nl}$gdb_prompt $" {
+	for {set i 1} {$i <= $n_insns} {incr i} {
+	    set bp_addrs($i) $expect_out($i,string)
+	}
+	pass $test
+    }
+}] != 0 } {
+    # No use proceeding if bp_addrs wasn't set.
+    return
+}
+
+for {set i 1} {$i <= $n_insns} {incr i} {
+    with_test_prefix "insn $i" {
+	gdb_test "break \*$bp_addrs($i)" \
+	    "Breakpoint $decimal at $bp_addrs($i): file .*" \
+	    "set breakpoint"
+
+	# Give the breakpoint a condition that always fails, so that
+	# the thread is immediately re-resumed.
+	gdb_test_no_output "condition \$bpnum condition" \
+	    "set condition"
+    }
+}
+
+set lineno [gdb_get_line_number "break here"]
+gdb_breakpoint $lineno
+gdb_continue_to_breakpoint "break here" ".*break here.*"
-- 
1.7.11.7