[PATCH 3/3] Fix non-stop regressions caused by "breakpoints always-inserted off" changes

[PATCH 3/3] Fix non-stop regressions caused by "breakpoints always-inserted off" changes

[Pedro Alves]

Commit a25a5a45 (Fix "breakpoint always-inserted off"; remove
"breakpoint always-inserted auto") regressed non-stop remote
debugging.

This was exposed by mi-nsintrall.exp intermittently failing with a
spurious SIGTRAP.

The problem is that when debugging with "target remote", new threads
the target has spawned but have never reported a stop aren't visible
to GDB until it explicitly resyncs its thread list with the target's.

For example, in a program like this:

 int
 main (void)
 {
   pthread_t child_thread;
   pthread_create (&child_thread, NULL, child_function, NULL);
   return 0;  <<<< set breakpoint here
 }

If the user sets a breakpoint at the "return" statement, and runs the
program, when that breakpoint hit is reported, GDB is only aware of
the main thread.  So if we base the decision to remove or insert
breakpoints from the target based on whether all the threads we know
about are stopped, we'll miss that child_thread is running, and thus
we'll remove breakpoints from the target, even through they should
still remain inserted, otherwise child_thread will miss them.

The break-while-running.exp test actually should also be exposing this
thread-list-out-of-synch problem.  That test sets a breakpoint while
the main thread is stopped, but other threads are running.  Because
other threads are running, the breakpoint is supposed to be inserted
immediately.  But, unless something forces a refetch of the thread
list, like, e.g., "info threads", GDB won't be aware of the other
threads that had been spawned by the main thread, and so won't insert
new or old breakpoints in the target.  And it turns out that the test
is exactly doing an explicit "info threads", masking out the
problem...  This commit adjust the test to exercise the case of not
issuing "info threads".  The test then fails without the GDB fix.

In the ni-nsintrall.exp case, what happens is that several threads hit
the same breakpoint, and when the first thread reports the stop,
because GDB wasn't aware other threads exist, all threads known to GDB
are found stopped, so GDB removes the breakpoints from the target.
The other threads follow up with SIGTRAPs too for that same
breakpoint, which has already been removed.  For the first few
threads, the moribund breakpoints machinery suppresses the SIGTRAPs,
but after a few events (precisely '3 * thread_count () + 1' at the
time the breakpoint was removed, see update_global_location_list), the
moribund breakpoint machinery is no longer aware of the removed
breakpoint, and the SIGTRAP is reported as a spurious stop.

The fix is naturally then to stop assuming that if no thread in the
list is executing, then the target is fully stopped.  We can't know
that until we fully sync the thread list.  Because updating the thread
list on every stop would be too much RSP traffic, I chose instead to
update it whenever we're about to present a stop to the user.

Actually updating the thread list at that point happens to be an item
I had added to the local/remote parity wiki page a while ago:

  Native GNU/Linux debugging adds new threads to the thread list as
  the program creates them "The [New Thread foo] messages". Remote
  debugging can't do that, and it's arguable whether we shouldn't even
  stop native debugging from doing that, as it hinders inferior
  performance. However, a related issue is that with remote targets
  (and gdbserver), even after the program stops, the user still needs
  to do "info threads" to pull an updated thread list. This, should
  most likely be addressed, so that GDB pulls the list itself, perhaps
  just before presenting a stop to the user.

With that in place, the need to delay "Program received signal FOO"
was actually caught by the manythreads.exp test.  Without that bit, I
was getting:

  [Thread 0x7ffff7f13700 (LWP 4499) exited]
  [New Thread 0x7ffff7f0b700 (LWP 4500)]
  ^C
  Program received signal SIGINT, Interrupt.
  [New Thread 0x7ffff7f03700 (LWP 4501)]           <<< new output
  [Switching to Thread 0x7ffff7f0b700 (LWP 4500)]
  __GI___nptl_death_event () at events.c:31
  31      {
  (gdb) FAIL: gdb.threads/manythreads.exp: stop threads 1

That is, I was now getting "New Thread" lines after the "Program
received signal" line, and the test doesn't expect them.  As the
number of new threads discovered before and after the "Program
received signal" output is unbounded, it's much nicer to defer
"Program received signal" until after synching the thread list, thus
close to the "switching to thread" output and "current frame/source"
info:

  [Thread 0x7ffff7863700 (LWP 7647) exited]
  ^C[New Thread 0x7ffff786b700 (LWP 7648)]

  Program received signal SIGINT, Interrupt.
  [Switching to Thread 0x7ffff7fc4740 (LWP 6243)]
  __GI___nptl_create_event () at events.c:25
  25      {
  (gdb) PASS: gdb.threads/manythreads.exp: stop threads 1

Tested on x86_64 Fedora 20, native and gdbserver.

gdb/
2014-09-25  Pedro Alves  <palves@redhat.com>

	* breakpoint.c (breakpoints_should_be_inserted_now): Use
	threads_are_executing.
	* breakpoint.h (breakpoints_should_be_inserted_now): Add
	describing comment.
	* gdbthread.h (threads_are_executing): Declare.
	(handle_signal_stop) <random signals>: Don't print about the
	signal here if stopping.
	(end_stepping_range): Don't notify observers here.
	(normal_stop): Update the thread list.  If stopped by a random
	signal or a stepping range ended, notify observers.
	* thread.c (threads_executing): New global.
	(init_thread_list): Clear 'threads_executing'.
	(set_executing): Set or clear 'threads_executing'.
	(threads_are_executing): New function.
	(update_threads_executing): New function.
	(update_thread_list): Use it.

gdb/testsuite/
2014-09-25  Pedro Alves  <palves@redhat.com>

	* gdb.threads/break-while-running.exp (test): Add new
	'update_thread_list' argument.  Skip "info threads" if false.
	(top level): Add new 'update_thread_list' axis.
---
 gdb/breakpoint.c                                  | 11 ++---
 gdb/breakpoint.h                                  | 10 +++++
 gdb/gdbthread.h                                   |  3 ++
 gdb/infrun.c                                      | 54 +++++++++++++++--------
 gdb/testsuite/gdb.threads/break-while-running.exp | 35 ++++++++++-----
 gdb/thread.c                                      | 44 ++++++++++++++++++
 6 files changed, 119 insertions(+), 38 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index c14bb49..e805c11 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -473,6 +473,8 @@ show_always_inserted_mode (struct ui_file *file, int from_tty,
 		    value);
 }
 
+/* See breakpoint.h.  */
+
 int
 breakpoints_should_be_inserted_now (void)
 {
@@ -485,8 +487,6 @@ breakpoints_should_be_inserted_now (void)
     }
   else if (target_has_execution)
     {
-      struct thread_info *tp;
-
       if (always_inserted_mode)
 	{
 	  /* The user wants breakpoints inserted even if all threads
@@ -494,11 +494,8 @@ breakpoints_should_be_inserted_now (void)
 	  return 1;
 	}
 
-      ALL_NON_EXITED_THREADS (tp)
-        {
-	  if (tp->executing)
-	    return 1;
-	}
+      if (threads_are_executing ())
+	return 1;
     }
   return 0;
 }
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index e191c10..d65405f 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1491,6 +1491,16 @@ extern void breakpoint_xfer_memory (gdb_byte *readbuf, gdb_byte *writebuf,
 				    const gdb_byte *writebuf_org,
 				    ULONGEST memaddr, LONGEST len);
 
+/* Return true if breakpoints should be inserted now.  That'll be the
+   case if either:
+
+    - the target has global breakpoints.
+
+    - "breakpoint always-inserted" is on, and the target has
+      execution.
+
+    - threads are executing.
+*/
 extern int breakpoints_should_be_inserted_now (void);
 
 /* Called each time new event from target is processed.
diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index 6768491..26ca925 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -381,6 +381,9 @@ extern void set_executing (ptid_t ptid, int executing);
 /* Reports if thread PTID is executing.  */
 extern int is_executing (ptid_t ptid);
 
+/* True if any (known or unknown) thread is or may be executing.  */
+extern int threads_are_executing (void);
+
 /* Merge the executing property of thread PTID over to its thread
    state property (frontend running/stopped view).
 
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 3725f2d..52997f1 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -4196,13 +4196,6 @@ handle_signal_stop (struct execution_control_state *ecs)
 
       stopped_by_random_signal = 1;
 
-      if (signal_print[ecs->event_thread->suspend.stop_signal])
-	{
-	  /* The signal table tells us to print about this signal.  */
-	  printed = 1;
-	  target_terminal_ours_for_output ();
-	  observer_notify_signal_received (ecs->event_thread->suspend.stop_signal);
-	}
       /* Always stop on signals if we're either just gaining control
 	 of the program, or the user explicitly requested this thread
 	 to remain stopped.  */
@@ -4214,10 +4207,17 @@ handle_signal_stop (struct execution_control_state *ecs)
 	  stop_waiting (ecs);
 	  return;
 	}
-      /* If not going to stop, give terminal back
-         if we took it away.  */
-      else if (printed)
-	target_terminal_inferior ();
+
+      /* Notify observers the signal has "handle print" set.  Note we
+	 returned early above if stopping; normal_stop handles the
+	 printing in that case.  */
+      if (signal_print[ecs->event_thread->suspend.stop_signal])
+	{
+	  /* The signal table tells us to print about this signal.  */
+	  target_terminal_ours_for_output ();
+	  observer_notify_signal_received (ecs->event_thread->suspend.stop_signal);
+	  target_terminal_inferior ();
+	}
 
       /* Clear the signal if it should not be passed.  */
       if (signal_program[ecs->event_thread->suspend.stop_signal] == 0)
@@ -5852,15 +5852,12 @@ prepare_to_wait (struct execution_control_state *ecs)
 }
 
 /* We are done with the step range of a step/next/si/ni command.
-   Called once for each n of a "step n" operation.  Notify observers
-   if not in the middle of doing a "step N" operation for N > 1.  */
+   Called once for each n of a "step n" operation.  */
 
 static void
 end_stepping_range (struct execution_control_state *ecs)
 {
   ecs->event_thread->control.stop_step = 1;
-  if (!ecs->event_thread->step_multi)
-    observer_notify_end_stepping_range ();
   stop_waiting (ecs);
 }
 
@@ -6071,6 +6068,19 @@ normal_stop (void)
 	   && last.kind != TARGET_WAITKIND_NO_RESUMED)
     make_cleanup (finish_thread_state_cleanup, &inferior_ptid);
 
+  /* As we're presenting a stop, and potentially removing breakpoints,
+     update the thread list so we can tell whether there are threads
+     running on the target.  With target remote, for example, we can
+     only learn about new threads when we explicitly update the thread
+     list.  Do this before notifying the interpreters about signal
+     stops, end of stepping ranges, etc., so that the "new thread"
+     output is emitted before e.g., "Program received signal FOO",
+     instead of after.  */
+  update_thread_list ();
+
+  if (last.kind == TARGET_WAITKIND_STOPPED && stopped_by_random_signal)
+    observer_notify_signal_received (inferior_thread ()->suspend.stop_signal);
+
   /* As with the notification of thread events, we want to delay
      notifying the user that we've switched thread context until
      the inferior actually stops.
@@ -6109,6 +6119,7 @@ normal_stop (void)
       printf_filtered (_("No unwaited-for children left.\n"));
     }
 
+  /* Note: this depends on the update_thread_list call above.  */
   if (!breakpoints_should_be_inserted_now () && target_has_execution)
     {
       if (remove_breakpoints ())
@@ -6126,14 +6137,19 @@ normal_stop (void)
   if (stopped_by_random_signal)
     disable_current_display ();
 
-  /* Don't print a message if in the middle of doing a "step n"
-     operation for n > 1 */
+  /* Notify observers if we finished a "step"-like command, etc.  */
   if (target_has_execution
       && last.kind != TARGET_WAITKIND_SIGNALLED
       && last.kind != TARGET_WAITKIND_EXITED
-      && inferior_thread ()->step_multi
       && inferior_thread ()->control.stop_step)
-    goto done;
+    {
+      /* But not if if in the middle of doing a "step n" operation for
+	 n > 1 */
+      if (inferior_thread ()->step_multi)
+	goto done;
+
+      observer_notify_end_stepping_range ();
+    }
 
   target_terminal_ours ();
   async_enable_stdin ();
diff --git a/gdb/testsuite/gdb.threads/break-while-running.exp b/gdb/testsuite/gdb.threads/break-while-running.exp
index 690d6ab..ea28bf0 100644
--- a/gdb/testsuite/gdb.threads/break-while-running.exp
+++ b/gdb/testsuite/gdb.threads/break-while-running.exp
@@ -28,11 +28,13 @@ if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug pthreads}
     return -1
 }
 
-# The test proper.  NON_STOP indicates whether we're testing in
-# non-stop, or all-stop mode.  ALWAYS_INSERTED indicates whether
-# testing in "breakpoint always-inserted" mode.
+# The test proper.  UPDATE_THREAD_LIST indicates whether we should do
+# an "info threads" to sync the thread list after the first stop.
+# ALWAYS_INSERTED indicates whether testing in "breakpoint
+# always-inserted" mode.  NON_STOP indicates whether we're testing in
+# non-stop, or all-stop mode.
 
-proc test { always_inserted non_stop } {
+proc test { update_thread_list always_inserted non_stop } {
     global srcfile binfile
     global gdb_prompt
     global decimal
@@ -70,9 +72,15 @@ proc test { always_inserted non_stop } {
 	gdb_test "thread 1" "Switching to .*" "switch back to main thread"
     }
 
-    gdb_test "info threads" \
-	"\\\(running\\\).*\\\(running\\\).* main .*" \
-	"only main stopped"
+    # Test with and without pulling the thread list explicitly with
+    # "info threads".  GDB should be able to figure out itself whether
+    # the target is running and thus breakpoints should be inserted,
+    # without the user explicitly fetching the thread list.
+    if {$update_thread_list} {
+	gdb_test "info threads" \
+	    "\\\(running\\\).*\\\(running\\\).* main .*" \
+	    "only main stopped"
+    }
 
     # Don't use gdb_test as it's racy in this case -- gdb_test matches
     # the prompt with an end anchor.  Sometimes expect will manage to
@@ -141,11 +149,14 @@ proc test { always_inserted non_stop } {
     }
 }
 
-foreach always_inserted { "off" "on" } {
-    foreach non_stop { "off" "on" } {
-	set stop_mode [expr ($non_stop=="off")?"all-stop":"non-stop"]
-	with_test_prefix "always-inserted $always_inserted: $stop_mode" {
-	    test $always_inserted $non_stop
+foreach update_thread_list { true false } {
+    foreach always_inserted { "off" "on" } {
+	foreach non_stop { "off" "on" } {
+	    set stop_mode [expr ($non_stop=="off")?"all-stop":"non-stop"]
+	    set update_list_mode [expr ($update_thread_list)?"w/ithr":"wo/ithr"]
+	    with_test_prefix "$update_list_mode: always-inserted $always_inserted: $stop_mode" {
+		test $update_thread_list $always_inserted $non_stop
+	    }
 	}
     }
 }
diff --git a/gdb/thread.c b/gdb/thread.c
index bceaf49..ac1d8a1 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -56,6 +56,13 @@ void _initialize_thread (void);
 struct thread_info *thread_list = NULL;
 static int highest_thread_num;
 
+/* True if any thread is, or may be executing.  We need to track this
+   separately because until we fully sync the thread list, we won't
+   know whether the target is fully stopped, even if we see stop
+   events for all known threads, because any of those threads may have
+   spawned new threads we haven't heard of yet.  */
+static int threads_executing;
+
 static void thread_command (char *tidstr, int from_tty);
 static void thread_apply_all_command (char *, int);
 static int thread_alive (struct thread_info *);
@@ -167,6 +174,7 @@ init_thread_list (void)
     }
 
   thread_list = NULL;
+  threads_executing = 0;
 }
 
 /* Allocate a new thread with target id PTID and add it to the thread
@@ -702,6 +710,22 @@ set_executing (ptid_t ptid, int executing)
       gdb_assert (tp);
       tp->executing = executing;
     }
+
+  /* It only takes one running thread to spawn more threads.*/
+  if (executing)
+    threads_executing = 1;
+  /* Only clear the flag if the caller is telling us everything is
+     stopped.  */
+  else if (ptid_equal (minus_one_ptid, ptid))
+    threads_executing = 0;
+}
+
+/* See gdbthread.h.  */
+
+int
+threads_are_executing (void)
+{
+  return threads_executing;
 }
 
 void
@@ -1501,11 +1525,31 @@ gdb_thread_select (struct ui_out *uiout, char *tidstr, char **error_message)
   return GDB_RC_OK;
 }
 
+/* Update the 'threads_executing' global based on the threads we know
+   about right now.  */
+
+static void
+update_threads_executing (void)
+{
+  struct thread_info *tp;
+
+  threads_executing = 0;
+  ALL_NON_EXITED_THREADS (tp)
+    {
+      if (tp->executing)
+	{
+	  threads_executing = 1;
+	  break;
+	}
+    }
+}
+
 void
 update_thread_list (void)
 {
   prune_threads ();
   target_find_new_threads ();
+  update_threads_executing ();
 }
 
 /* Return a new value for the selected thread's id.  Return a value of 0 if
-- 
1.9.3

[PATCH 2/3] PR17431: following execs with "breakpoint always-inserted on"

[Pedro Alves]

Following an exec with "breakpoint always-inserted on" tries to insert
breakpoints in the new image at the addresses the symbols had in the
old image.

With "always-inserted off", we see:

 gdb gdb.multi/multi-arch-exec -ex "set breakpoint always-inserted off"
 GNU gdb (GDB) 7.8.50.20140924-cvs
 ...
 (gdb) b main
 Breakpoint 1 at 0x400664: file gdb.multi/multi-arch-exec.c, line 24.
		 ^^^^^^^^
 (gdb) c
 The program is not being run.
 (gdb) r
 Starting program: testsuite/gdb.multi/multi-arch-exec

 Breakpoint 1, main () at gdb/testsuite/gdb.multi/multi-arch-exec.c:24
 24        execl (BASEDIR "/multi-arch-exec-hello",
 (gdb) c
 Continuing.
 process 9212 is executing new program: gdb/testsuite/gdb.multi/multi-arch-exec-hello

 Breakpoint 1, main () at gdb/testsuite/gdb.multi/hello.c:40
 40        bar();
 (gdb) info breakpoints
 Num     Type           Disp Enb Address    What
 1       breakpoint     keep y   0x080484e4 in main at gdb/testsuite/gdb.multi/hello.c:40
				 ^^^^^^^^^^
	 breakpoint already hit 2 times
 (gdb)

Note how main was 0x400664 in multi-arch-exec, and 0x080484e4 in
gdb.multi/hello.

With "always-inserted on", we get:

 Breakpoint 1, main () at gdb/testsuite/gdb.multi/multi-arch-exec.c:24
 24        execl (BASEDIR "/multi-arch-exec-hello",
 (gdb) c
 Continuing.
 infrun: target_wait (-1, status) =
 infrun:   9444 [process 9444],
 infrun:   status->kind = execd
 infrun: infwait_normal_state
 infrun: TARGET_WAITKIND_EXECD
 Warning:
 Cannot insert breakpoint 1.
 Cannot access memory at address 0x400664

(gdb)

That is, GDB is trying to insert a breakpoint at 0x400664, after the
exec, and then that address happens to not be mapped at all in the new
image.

The problem is that update_breakpoints_after_exec is creating
breakpoints, which ends up in update_global_location_list immediately
inserting breakpoints if "breakpoints always-inserted" is "on".
update_breakpoints_after_exec is called very early when we see an exec
event.  At that point, we haven't loaded the symbols of the new
post-exec image yet, and thus haven't reset breakpoint's addresses to
whatever they may be in the new image.  All we should be doing in
update_breakpoints_after_exec is deleting breakpoints that no longer
make sense after an exec.  So the fix removes those breakpoint
creations.

The question is then, if not here, where are those breakpoints
re-created?  Turns out we don't need to do anything else, because at
the end of follow_exec, we call breakpoint_re_set, whose tail is also
creating exactly the same breakpoints update_breakpoints_after_exec is
currently creating:

  breakpoint_re_set (void)
  {
  ...
    create_overlay_event_breakpoint ();
    create_longjmp_master_breakpoint ();
    create_std_terminate_master_breakpoint ();
    create_exception_master_breakpoint ();
  }

A new test is added to exercise this.

Tested on x86_64 Fedora 20.

gdb/
2014-09-25  Pedro Alves  <palves@redhat.com>

	PR breakpoints/17431
	* breakpoint.c (update_breakpoints_after_exec): Don't create
	overlay, longjmp, std terminate nor exception breakpoints here.

gdb/testsuite/
2014-09-25  Pedro Alves  <palves@redhat.com>

	PR breakpoints/17431
	* gdb.base/execl-update-breakpoints.c: New file.
	* gdb.base/execl-update-breakpoints.exp: New file.
---
 gdb/breakpoint.c                                   |   5 -
 gdb/testsuite/gdb.base/execl-update-breakpoints.c  |  38 +++++++
 .../gdb.base/execl-update-breakpoints.exp          | 114 +++++++++++++++++++++
 3 files changed, 152 insertions(+), 5 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/execl-update-breakpoints.c
 create mode 100644 gdb/testsuite/gdb.base/execl-update-breakpoints.exp

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 3675b4f..c14bb49 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -3824,11 +3824,6 @@ update_breakpoints_after_exec (void)
 	continue;
       }
   }
-  /* FIXME what about longjmp breakpoints?  Re-create them here?  */
-  create_overlay_event_breakpoint ();
-  create_longjmp_master_breakpoint ();
-  create_std_terminate_master_breakpoint ();
-  create_exception_master_breakpoint ();
 }
 
 int
diff --git a/gdb/testsuite/gdb.base/execl-update-breakpoints.c b/gdb/testsuite/gdb.base/execl-update-breakpoints.c
new file mode 100644
index 0000000..4eb76e8
--- /dev/null
+++ b/gdb/testsuite/gdb.base/execl-update-breakpoints.c
@@ -0,0 +1,38 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   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/>.  */
+
+#include <unistd.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+int
+main (int argc, char **argv)
+{
+  size_t len;
+  char *bin;
+
+  len = strlen (argv[0]);
+  bin = malloc (len + 1);
+  memcpy (bin, argv[0], len + 1);
+  if (bin[len - 1] == '1')
+    bin[len - 1] = '2';
+
+  execl (bin, bin, (char *) NULL);
+  perror ("execl failed");
+  exit (1);
+}
diff --git a/gdb/testsuite/gdb.base/execl-update-breakpoints.exp b/gdb/testsuite/gdb.base/execl-update-breakpoints.exp
new file mode 100644
index 0000000..eb1024b
--- /dev/null
+++ b/gdb/testsuite/gdb.base/execl-update-breakpoints.exp
@@ -0,0 +1,114 @@
+# 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/>.
+
+# Test that when following an exec, we don't try to insert breakpoints
+# in the new image at the addresses the symbols had before the exec.
+
+# Remote protocol does not support follow-exec notifications.
+
+if [is_remote target] {
+    continue
+}
+
+standard_testfile
+
+# Build two copies of the program, each linked at a different address.
+# The address of "main" in the first binary should end up being an
+# unmapped address in the second binary.
+
+set exec1 ${testfile}1
+set exec2 ${testfile}2
+set binfile1 ${binfile}1
+set binfile2 ${binfile}2
+
+if { [prepare_for_testing "failed to prepare" ${exec1} "${srcfile}" \
+	  [list debug ldflags=-Wl,-Ttext=0x1000000]] } {
+    return -1
+}
+if { [prepare_for_testing "failed to prepare" ${exec2} "${srcfile}" \
+	  [list debug ldflags=-Wl,-Ttext=0x2000000]] } {
+    return -1
+}
+
+# First check whether the address of "main" in exec1 is readable in
+# exec2.  If it is, then skip the test as unsupported.
+
+clean_restart ${exec1}
+if ![runto_main] then {
+    fail "Couldn't run to main"
+    return -1
+}
+
+set addr ""
+set test "main address first"
+gdb_test_multiple "p/x &main" $test {
+    -re " = (0x\[0-9a-f\]+)\r\n$gdb_prompt $" {
+	set addr $expect_out(1,string)
+	pass $test
+    }
+}
+
+clean_restart ${exec2}
+if ![runto_main] then {
+    fail "Couldn't run to main"
+    return -1
+}
+
+set cannot_access 0
+set test "probe memory access"
+gdb_test_multiple "x $addr" $test {
+    -re "Cannot access memory at address .*$gdb_prompt $" {
+	set cannot_access 1
+	pass $test
+    }
+    -re ".*$gdb_prompt $" {
+	pass $test
+    }
+}
+
+if {!$cannot_access} {
+    unsupported "main address is readable in second binary"
+    return
+}
+
+# The test proper.  ALWAYS_INSERTED indicates whether testing in
+# "breakpoint always-inserted" mode.
+
+proc test { always_inserted } {
+    global exec1
+
+    clean_restart ${exec1}
+
+    gdb_test_no_output "set breakpoint always-inserted $always_inserted"
+
+    if ![runto_main] then {
+	fail "Couldn't run to main"
+	return -1
+    }
+
+    # On a buggy GDB, with always-inserted on, we'd see:
+    #  (gdb) continue
+    #  Continuing.
+    #  Warning:
+    #  Cannot insert breakpoint 1.
+    #  Cannot access memory at address 0x10000ff
+    gdb_test "continue" "Breakpoint 1, main.*" "continue across exec"
+}
+
+foreach always_inserted { "off" "on" } {
+    with_test_prefix "always-inserted $always_inserted" {
+	test $always_inserted
+    }
+}
-- 
1.9.3

[PATCH 1/3] Reduce Hg packet (select remote general thread) bouncing

[Pedro Alves]

A patch I wrote made GDB pull the thread list sooner when debugging
with target remote, and I noticed an intended consequence.  GDB
started bouncing around the currently selected remote/general thread
more frequently.  E.g.:

  Sending packet: $qTMinFTPILen#3b...Packet received: 5
 +Sending packet: $Hgp726d.726d#53...Packet received: OK
  Sending packet: $m400680,40#2f...Packet received: 85c0741455bff00d60004889e5ffd05de97bffffff0f1f00e973ffffff0f1f00554889e5c745fc00000000c745fc01000000e900000000c745fc02000000b800
 +Sending packet: $Hgp726d.7278#28...Packet received: OK
  Sending packet: $m4006b2,1#28...Packet received: e9
  Fast tracepoint 2 at 0x4006b2: file gdb/testsuite/gdb.trace/range-stepping.c, line 53.
  Sending packet: $qTStatus#49...Packet received: T0;tnotrun:0;tframes:0;tcreated:0;tfree:500000;tsize:500000;circular:0;disconn:0;starttime:0;stoptime:0;username:;notes::

This ended up breaking "tstart" when one has fast tracepoints set,
because gdbserver isn't expecting an Hg packet in response to
qRelocInsn:

 (gdb) ftrace *set_point
 Fast tracepoint 3 at 0x4006b2: file gdb/testsuite/gdb.trace/range-stepping.c, line 53.
 (gdb) PASS: gdb.trace/range-stepping.exp: ftrace: ftrace *set_point
 tstart
 gdbserver: Malformed response to qRelocInsn, ignoring: Hgp2783.2783

 Target does not support this command.
 (gdb) FAIL: gdb.trace/range-stepping.exp: ftrace: tstart

remote_trace_start should probably start by making sure the remote
current thread matches inferior_ptid (calling set_general_thread), but
still, reducing unnecessary bouncing is a good idea.  It happens
because the memory/symbol/breakpoint routines use
switch_to_program_space_and_thread to do something in the right
context and then revert back to the previously current thread.

The fix is to simply make any_thread_of_process,
find_inferior_for_program_space, etc. give preference to the current
thread/inferior it if matches.

gdb/
2014-09-25  Pedro Alves  <palves@redhat.com>

	* gdbthread.h (any_thread_of_process, any_live_thread_of_process):
	Adjust comments.
	* inferior.c (find_inferior_for_program_space): Give preference to
	the current inferior.
	* inferior.h (find_inferior_for_program_space): Update comment.
	* progspace.c (switch_to_program_space_and_thread): Prefer the
	current inferior if it's bound to the program space requested.  If
	the inferior found doesn't have a PID yet, don't bother looking up
	a thread.
	* progspace.h (switch_to_program_space_and_thread): Adjust
	comment.
	* thread.c (any_thread_of_process, any_live_thread_of_process):
	Give preference to the current thread.
---
 gdb/gdbthread.h |  7 ++++---
 gdb/inferior.c  |  7 +++++--
 gdb/inferior.h  |  3 ++-
 gdb/progspace.c |  5 ++---
 gdb/progspace.h |  3 ++-
 gdb/thread.c    | 40 ++++++++++++++++++++++++++++++++++------
 6 files changed, 49 insertions(+), 16 deletions(-)

diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index 522b674..6768491 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -306,11 +306,12 @@ struct thread_info *find_thread_id (int num);
    returns the first thread in the list.  */
 struct thread_info *first_thread_of_process (int pid);
 
-/* Returns any thread of process PID.  */
+/* Returns any thread of process PID, giving preference to the current
+   thread.  */
 extern struct thread_info *any_thread_of_process (int pid);
 
-/* Returns any non-exited thread of process PID, giving preference for
-   not executing threads.  */
+/* Returns any non-exited thread of process PID, giving preference to
+   the current thread, and to not executing threads.  */
 extern struct thread_info *any_live_thread_of_process (int pid);
 
 /* Change the ptid of thread OLD_PTID to NEW_PTID.  */
diff --git a/gdb/inferior.c b/gdb/inferior.c
index 23da0c7..fb02439 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -367,12 +367,15 @@ find_inferior_pid (int pid)
   return NULL;
 }
 
-/* Find an inferior bound to PSPACE.  */
+/* See inferior.h.  */
 
 struct inferior *
 find_inferior_for_program_space (struct program_space *pspace)
 {
-  struct inferior *inf;
+  struct inferior *inf = current_inferior ();
+
+  if (inf->pspace == pspace)
+    return inf;
 
   for (inf = inferior_list; inf != NULL; inf = inf->next)
     {
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 58557a4..e716005 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -470,7 +470,8 @@ extern struct inferior *find_inferior_pid (int pid);
 /* Search function to lookup an inferior by GDB 'num'.  */
 extern struct inferior *find_inferior_id (int num);
 
-/* Find an inferior bound to PSPACE.  */
+/* Find an inferior bound to PSPACE, giving preference to the current
+   inferior.  */
 extern struct inferior *
   find_inferior_for_program_space (struct program_space *pspace);
 
diff --git a/gdb/progspace.c b/gdb/progspace.c
index a74b6ab..1894559 100644
--- a/gdb/progspace.c
+++ b/gdb/progspace.c
@@ -463,8 +463,7 @@ save_current_space_and_thread (void)
   return old_chain;
 }
 
-/* Switches full context to program space PSPACE.  Switches to the
-   first thread found bound to PSPACE.  */
+/* See progspace.h  */
 
 void
 switch_to_program_space_and_thread (struct program_space *pspace)
@@ -472,7 +471,7 @@ switch_to_program_space_and_thread (struct program_space *pspace)
   struct inferior *inf;
 
   inf = find_inferior_for_program_space (pspace);
-  if (inf != NULL)
+  if (inf != NULL && inf->pid != 0)
     {
       struct thread_info *tp;
 
diff --git a/gdb/progspace.h b/gdb/progspace.h
index 08e04eb..cf2295c 100644
--- a/gdb/progspace.h
+++ b/gdb/progspace.h
@@ -264,7 +264,8 @@ extern void set_current_program_space (struct program_space *pspace);
 extern struct cleanup *save_current_space_and_thread (void);
 
 /* Switches full context to program space PSPACE.  Switches to the
-   first thread found bound to PSPACE.  */
+   first thread found bound to PSPACE, giving preference to the
+   current thread, if there's one and it isn't executing.  */
 extern void switch_to_program_space_and_thread (struct program_space *pspace);
 
 /* Create a new address space object, and add it to the list.  */
diff --git a/gdb/thread.c b/gdb/thread.c
index 65890e1..bceaf49 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -466,7 +466,13 @@ any_thread_of_process (int pid)
 {
   struct thread_info *tp;
 
-  for (tp = thread_list; tp; tp = tp->next)
+  gdb_assert (pid != 0);
+
+  /* Prefer the current thread.  */
+  if (ptid_get_pid (inferior_ptid) == pid)
+    return inferior_thread ();
+
+  ALL_NON_EXITED_THREADS (tp)
     if (ptid_get_pid (tp->ptid) == pid)
       return tp;
 
@@ -476,18 +482,40 @@ any_thread_of_process (int pid)
 struct thread_info *
 any_live_thread_of_process (int pid)
 {
+  struct thread_info *curr_tp = NULL;
   struct thread_info *tp;
   struct thread_info *tp_executing = NULL;
 
-  for (tp = thread_list; tp; tp = tp->next)
-    if (tp->state != THREAD_EXITED && ptid_get_pid (tp->ptid) == pid)
+  gdb_assert (pid != 0);
+
+  /* Prefer the current thread if it's not executing.  */
+  if (ptid_get_pid (inferior_ptid) == pid)
+    {
+      /* If the current thread is dead, forget it.  If it's not
+	 executing, use it.  Otherwise, still choose it (below), but
+	 only if no other non-executing thread is found.  */
+      curr_tp = inferior_thread ();
+      if (curr_tp->state == THREAD_EXITED)
+	curr_tp = NULL;
+      else if (!curr_tp->executing)
+	return curr_tp;
+    }
+
+  ALL_NON_EXITED_THREADS (tp)
+    if (ptid_get_pid (tp->ptid) == pid)
       {
-	if (tp->executing)
-	  tp_executing = tp;
-	else
+	if (!tp->executing)
 	  return tp;
+
+	tp_executing = tp;
       }
 
+  /* If both the current thread and all live threads are executing,
+     prefer the current thread.  */
+  if (curr_tp != NULL)
+    return curr_tp;
+
+  /* Otherwise, just return an executing thread, if any.  */
   return tp_executing;
 }
 
-- 
1.9.3

[PATCH 0/3] Fix non-stop + "breakpoints always-inserted off" regressions

[Pedro Alves]

This series fixes the non-stop regressions caused by the recent
"breakpoints always-inserted off" changes.

  https://sourceware.org/ml/gdb-patches/2014-09/msg00671.html

Patch 3/3 is the real fix.  The others may seem a bit unrelated, but
are actually latent problems that patch 3/3 exposed in the testsuite,
and need to be applied before 3/3 is.

Tested on x86_64 Fedora 20, native and gdbserver.

Pedro Alves (3):
  Reduce Hg packet (select remote general thread) bouncing
  PR17431: following execs with "breakpoint always-inserted on"
  Fix non-stop regressions caused by "breakpoints always-inserted off"
    changes

 gdb/breakpoint.c                                   |  16 +--
 gdb/breakpoint.h                                   |  10 ++
 gdb/gdbthread.h                                    |  10 +-
 gdb/inferior.c                                     |   7 +-
 gdb/inferior.h                                     |   3 +-
 gdb/infrun.c                                       |  54 ++++++----
 gdb/progspace.c                                    |   5 +-
 gdb/progspace.h                                    |   3 +-
 gdb/testsuite/gdb.base/execl-update-breakpoints.c  |  38 +++++++
 .../gdb.base/execl-update-breakpoints.exp          | 114 +++++++++++++++++++++
 gdb/testsuite/gdb.threads/break-while-running.exp  |  35 ++++---
 gdb/thread.c                                       |  84 +++++++++++++--
 12 files changed, 320 insertions(+), 59 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/execl-update-breakpoints.c
 create mode 100644 gdb/testsuite/gdb.base/execl-update-breakpoints.exp

-- 
1.9.3

Re: [PATCH 0/3] Fix non-stop + "breakpoints always-inserted off" regressions

[Pedro Alves]

On 09/25/2014 03:58 PM, Pedro Alves wrote:
> This series fixes the non-stop regressions caused by the recent
> "breakpoints always-inserted off" changes.
> 
>   https://sourceware.org/ml/gdb-patches/2014-09/msg00671.html
> 
> Patch 3/3 is the real fix.  The others may seem a bit unrelated, but
> are actually latent problems that patch 3/3 exposed in the testsuite,
> and need to be applied before 3/3 is.

I've now pushed this.

Thanks,
Pedro Alves

Re: [PATCH 3/3] Fix non-stop regressions caused by "breakpoints always-inserted off" changes

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

[ThunderBird puts this series into a wrong thread, so I don't see it
until today]

The patch looks reasonable to me.

> -  /* Don't print a message if in the middle of doing a "step n"
> -     operation for n > 1 */
> +  /* Notify observers if we finished a "step"-like command, etc.  */
>    if (target_has_execution
>        && last.kind != TARGET_WAITKIND_SIGNALLED
>        && last.kind != TARGET_WAITKIND_EXITED
> -      && inferior_thread ()->step_multi
>        && inferior_thread ()->control.stop_step)
> -    goto done;
> +    {
> +      /* But not if if in the middle of doing a "step n" operation for
                    ^^^^^
Duplicated "if"?

-- 
Yao (齐尧)

Re: [PATCH 3/3] Fix non-stop regressions caused by "breakpoints always-inserted off" changes

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> -      if (signal_print[ecs->event_thread->suspend.stop_signal])
> -	{
> -	  /* The signal table tells us to print about this signal.  */
> -	  printed = 1;
> -	  target_terminal_ours_for_output ();
> -	  observer_notify_signal_received (ecs->event_thread->suspend.stop_signal);
> -	}
>        /* Always stop on signals if we're either just gaining control
>  	 of the program, or the user explicitly requested this thread
>  	 to remain stopped.  */
> @@ -4214,10 +4207,17 @@ handle_signal_stop (struct execution_control_state *ecs)
>  	  stop_waiting (ecs);
>  	  return;
>  	}
> -      /* If not going to stop, give terminal back
> -         if we took it away.  */
> -      else if (printed)

The use of local variable 'printed' is removed by this patch.  We can
remove 'printed' too, as the patch below does.  It is obvious, and I'll
push it in.

-- 
Yao (齐尧)

Subject: [PATCH] Remove unused local variable

As a result of commit b57bacec, local variable 'printed' is no longer
used.  This patch is to remove it.

gdb:

2014-10-08  Yao Qi  <yao@codesourcery.com>

	* infrun.c (handle_signal_stop): Remove local variable 'printed'.

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 7b43785..4681175 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -4425,7 +4425,6 @@ handle_signal_stop (struct execution_control_state *ecs)
   if (random_signal)
     {
       /* Signal not for debugging purposes.  */
-      int printed = 0;
       struct inferior *inf = find_inferior_pid (ptid_get_pid (ecs->ptid));
       enum gdb_signal stop_signal = ecs->event_thread->suspend.stop_signal;
 

Re: [PATCH 3/3] Fix non-stop regressions caused by "breakpoints always-inserted off" changes

[Pedro Alves]

On 10/08/2014 12:29 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> -      if (signal_print[ecs->event_thread->suspend.stop_signal])
>> -	{
>> -	  /* The signal table tells us to print about this signal.  */
>> -	  printed = 1;
>> -	  target_terminal_ours_for_output ();
>> -	  observer_notify_signal_received (ecs->event_thread->suspend.stop_signal);
>> -	}
>>        /* Always stop on signals if we're either just gaining control
>>  	 of the program, or the user explicitly requested this thread
>>  	 to remain stopped.  */
>> @@ -4214,10 +4207,17 @@ handle_signal_stop (struct execution_control_state *ecs)
>>  	  stop_waiting (ecs);
>>  	  return;
>>  	}
>> -      /* If not going to stop, give terminal back
>> -         if we took it away.  */
>> -      else if (printed)
> 
> The use of local variable 'printed' is removed by this patch.  We can
> remove 'printed' too, as the patch below does.  It is obvious, and I'll
> push it in.

Thanks Yao.


Thanks,
Pedro Alves

Re: [PATCH 3/3] Fix non-stop regressions caused by "breakpoints always-inserted off" changes

[Pedro Alves]

On 10/08/2014 11:55 AM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
> [ThunderBird puts this series into a wrong thread, so I don't see it
> until today]
> 
> The patch looks reasonable to me.

Thanks.

> 
>> -  /* Don't print a message if in the middle of doing a "step n"
>> -     operation for n > 1 */
>> +  /* Notify observers if we finished a "step"-like command, etc.  */
>>    if (target_has_execution
>>        && last.kind != TARGET_WAITKIND_SIGNALLED
>>        && last.kind != TARGET_WAITKIND_EXITED
>> -      && inferior_thread ()->step_multi
>>        && inferior_thread ()->control.stop_step)
>> -    goto done;
>> +    {
>> +      /* But not if if in the middle of doing a "step n" operation for
>                     ^^^^^
> Duplicated "if"?

Yep.  Fixed now.

---------
From 31cc0b807b2fde7d0110175418a6eea01a982489 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Fri, 10 Oct 2014 13:50:05 +0100
Subject: [PATCH] infrun.c:normal_stop: Fix typo in comment

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

	* infrun.c (normal_stop): Fix typo in comment.
---
 gdb/ChangeLog | 4 ++++
 gdb/infrun.c  | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index aeba430..8d5b1ee 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@
+2014-10-10  Pedro Alves  <palves@redhat.com>
+
+	* infrun.c (normal_stop): Fix typo in comment.
+
 2014-10-09  Sergio Durigan Junior  <sergiodj@redhat.com>
 
 	PR tdep/9390
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 4681175..5536350 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -6381,7 +6381,7 @@ normal_stop (void)
       && last.kind != TARGET_WAITKIND_EXITED
       && inferior_thread ()->control.stop_step)
     {
-      /* But not if if in the middle of doing a "step n" operation for
+      /* But not if in the middle of doing a "step n" operation for
 	 n > 1 */
       if (inferior_thread ()->step_multi)
 	goto done;
-- 
1.9.3