gdb: Handle W and X remote packets without giving a warning

In this commit:

  commit 24ed6739b699f329c2c45aedee5f8c7d2f54e493
  Date:   Thu Jan 30 14:35:40 2020 +0000

      gdb/remote: Restore support for 'S' stop reply packet

A regression was introduced such that the W and X packets would give a
warning in some cases.  The warning was:

  warning: multi-threaded target stopped without sending a thread-id, using first non-exited thread

This problem would arise when:

  1. The multi-process extensions to the remote protocol were not
  being used, and

  2. The inferior has multiple threads.

In this case when the W (or X) packet arrives the ptid of the
stop_reply is set to null_ptid, then when we arrive in
process_stop_reply GDB spots that we have multiple non-exited theads,
but the stop event didn't specify a thread-id.

The problem with this is that the W (and X) packets are actually
process wide events, they apply to all threads.  So not specifying a
thread-id is not a problem, in fact, the best these packets allow is
for the remote to specify a process-id, not a thread-id.

If we look at how the W (and X) packets deal with a specified
process-id, then what happens is GDB sets to stop_reply ptid to a
value which indicates all threads in the process, this is done by
creating a value `ptid_t (pid)`, which sets the pid field of the
ptid_t, but leaves the tid field as 0, indicating all threads.

So, this commit does the same thing for the case where there is not
process-id specified.  In process_stop_reply we not distinguish
between stop events that apply to all threads, and those that apply to
only one.  If the stop event applies to only one thread then we treat
it as before.  If, however, the stop event applies to all threads,
then we find the first non-exited thread, and use the pid from this
thread to create a `ptid_t (pid)` value.

If the target has multiple inferiors, and receives a process wide
event without specifying a process-id GDB now gives this warning:

  warning: multi-inferior target stopped without sending a process-id, using first non-exited inferior

gdb/ChangeLog:

	* remote.c (remote_target::process_stop_reply): Handle events for
	all threads differently.

gdb/testsuite/ChangeLog:

	* gdb.server/exit-multiple-threads.c: New file.
	* gdb.server/exit-multiple-threads.exp: New file.

5 files changed, 384 insertions, 10 deletions

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 1fe2e58..7f87ece 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,10 @@
 2020-03-19  Andrew Burgess  <andrew.burgess@embecosm.com>
 
+	* remote.c (remote_target::process_stop_reply): Handle events for
+	all threads differently.
+
+2020-03-19  Andrew Burgess  <andrew.burgess@embecosm.com>
+
 	* completer.c (completion_tracker::remove_completion): Define new
 	function.
 	* completer.h (completion_tracker::remove_completion): Declare new
diff --git a/gdb/remote.c b/gdb/remote.c
index 0f78b1b..ce60fe40 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -7668,28 +7668,54 @@ remote_target::process_stop_reply (struct stop_reply *stop_reply,
      non-exited thread in the current target.  */
   if (ptid == null_ptid)
     {
+      /* Some stop events apply to all threads in an inferior, while others
+	 only apply to a single thread.  */
+      bool is_stop_for_all_threads
+	= (status->kind == TARGET_WAITKIND_EXITED
+	   || status->kind == TARGET_WAITKIND_SIGNALLED);
+
       for (thread_info *thr : all_non_exited_threads (this))
 	{
-	  if (ptid != null_ptid)
+	  if (ptid != null_ptid
+	      && (!is_stop_for_all_threads
+		  || ptid.pid () != thr->ptid.pid ()))
 	    {
 	      static bool warned = false;
 
 	      if (!warned)
 		{
 		  /* If you are seeing this warning then the remote target
-		     has multiple threads and either sent an 'S' stop
-		     packet, or a 'T' stop packet without a thread-id.  In
-		     both of these cases GDB is unable to know which thread
-		     just stopped and is now having to guess.  The correct
-		     action is to fix the remote target to send the correct
-		     packet (a 'T' packet and include a thread-id).  */
-		  warning (_("multi-threaded target stopped without sending "
-			     "a thread-id, using first non-exited thread"));
+		     has stopped without specifying a thread-id, but the
+		     target does have multiple threads (or inferiors), and
+		     so GDB is having to guess which thread stopped.
+
+		     Examples of what might cause this are the target
+		     sending and 'S' stop packet, or a 'T' stop packet and
+		     not including a thread-id.
+
+		     Additionally, the target might send a 'W' or 'X
+		     packet without including a process-id, when the target
+		     has multiple running inferiors.  */
+		  if (is_stop_for_all_threads)
+		    warning (_("multi-inferior target stopped without "
+			       "sending a process-id, using first "
+			       "non-exited inferior"));
+		  else
+		    warning (_("multi-threaded target stopped without "
+			       "sending a thread-id, using first "
+			       "non-exited thread"));
 		  warned = true;
 		}
 	      break;
 	    }
-	  ptid = thr->ptid;
+
+	  /* If this is a stop for all threads then don't use a particular
+	     threads ptid, instead create a new ptid where only the pid
+	     field is set.  */
+	  if (is_stop_for_all_threads)
+	    ptid = ptid_t (thr->ptid.pid ());
+	  else
+	    ptid = thr->ptid;
 	}
       gdb_assert (ptid != null_ptid);
     }
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index a9fa54e..cb65ffa 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,5 +1,10 @@
 2020-03-19  Andrew Burgess  <andrew.burgess@embecosm.com>
 
+	* gdb.server/exit-multiple-threads.c: New file.
+	* gdb.server/exit-multiple-threads.exp: New file.
+
+2020-03-19  Andrew Burgess  <andrew.burgess@embecosm.com>
+
 	* gdb.fortran/mixed-lang-stack.c: New file.
 	* gdb.fortran/mixed-lang-stack.cpp: New file.
 	* gdb.fortran/mixed-lang-stack.exp: New file.
diff --git a/gdb/testsuite/gdb.server/exit-multiple-threads.c b/gdb/testsuite/gdb.server/exit-multiple-threads.c
new file mode 100644
index 0000000..81be841
--- /dev/null
+++ b/gdb/testsuite/gdb.server/exit-multiple-threads.c
@@ -0,0 +1,202 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2020 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 <pthread.h>
+#include <sys/types.h>
+#include <stdio.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <pthread.h>
+
+/* The number of threads to create.  */
+int thread_count = 3;
+
+/* Counter accessed from threads to ensure that all threads have been
+   started.  Is initialised to THREAD_COUNT and each thread decrements it
+   upon startup.  */
+volatile int counter;
+
+/* Lock guarding COUNTER. */
+pthread_mutex_t counter_mutex = PTHREAD_MUTEX_INITIALIZER;
+
+/* Is initialised with our pid, GDB will read this.  */
+pid_t global_pid;
+
+/* Just somewhere to put a breakpoint.  */
+static void
+breakpt ()
+{
+  /* Nothing.  */
+}
+
+/* Thread safe decrement of the COUNTER global.  */
+static void
+decrement_counter ()
+{
+  if (pthread_mutex_lock (&counter_mutex) != 0)
+    abort ();
+  --counter;
+  if (pthread_mutex_unlock (&counter_mutex) != 0)
+    abort ();
+}
+
+/* Thread safe read of the COUNTER global.  */
+static int
+read_counter ()
+{
+  int val;
+
+  if (pthread_mutex_lock (&counter_mutex) != 0)
+    abort ();
+  val = counter;
+  if (pthread_mutex_unlock (&counter_mutex) != 0)
+    abort ();
+
+  return val;
+}
+
+#if defined DO_EXIT_TEST
+
+/* Thread entry point.  ARG is a pointer to a single integer, the ID for
+   this thread numbered 1 to THREAD_COUNT (a global).  */
+static void *
+thread_worker_exiting (void *arg)
+{
+  int id;
+
+  id = *((int *) arg);
+
+  decrement_counter ();
+
+  if (id != thread_count)
+    {
+      int i;
+
+      /* All threads except the last one will wait here while the test is
+	 carried out.  Don't wait forever though, just in case the test
+	 goes wrong.  */
+      for (i = 0; i < 60; ++i)
+	sleep (1);
+    }
+  else
+    {
+      /* The last thread waits here until all other threads have been
+	 created.  */
+      while (read_counter () > 0)
+	sleep (1);
+
+      /* Hit the breakpoint so GDB can stop.  */
+      breakpt ();
+
+      /* And exit all threads.  */
+      exit (0);
+    }
+
+  return NULL;
+}
+
+#define thread_worker thread_worker_exiting
+
+#elif defined DO_SIGNAL_TEST
+
+/* Thread entry point.  ARG is a pointer to a single integer, the ID for
+   this thread numbered 1 to THREAD_COUNT (a global).  */
+static void *
+thread_worker_signalling (void *arg)
+{
+  int i, id;
+
+  id = *((int *) arg);
+
+  decrement_counter ();
+
+  if (id == thread_count)
+    {
+      /* The last thread waits here until all other threads have been
+	 created.  */
+      while (read_counter () > 0)
+	sleep (1);
+
+      /* Hit the breakpoint so GDB can stop.  */
+      breakpt ();
+    }
+
+  /* All threads wait here while the testsuite sends us a signal.  Don't
+     block forever though, just in case the test goes wrong.  */
+  for (i = 0; i < 60; ++i)
+    sleep (1);
+
+  return NULL;
+}
+
+#define thread_worker thread_worker_signalling
+
+#else
+
+#error "Compile with DO_EXIT_TEST or DO_SIGNAL_TEST defined"
+
+#endif
+
+struct thread_info
+{
+  pthread_t thread;
+  int id;
+};
+
+int
+main ()
+{
+  int i, max = thread_count;
+
+  /* Put the pid somewhere easy for GDB to read.  */
+  global_pid = getpid ();
+
+  /* Space to hold all of the thread_info objects.  */
+  struct thread_info *info = malloc (sizeof (struct thread_info) * max);
+  if (info == NULL)
+    abort ();
+
+  /* Initialise the counter.  Don't do this under lock as we only have the
+     main thread at this point.  */
+  counter = thread_count;
+
+  /* Create all of the threads.  */
+  for (i = 0; i < max; ++i)
+    {
+      struct thread_info *thr = &info[i];
+      thr->id = i + 1;
+      if (pthread_create (&thr->thread, NULL, thread_worker, &thr->id) != 0)
+	abort ();
+    }
+
+  /* Gather in all of the threads.  This never completes, as the
+     final thread created will exit the process, and all of the other
+     threads block forever.  Still, it gives the main thread something to
+     do.  */
+  for (i = 0; i < max; ++i)
+    {
+      struct thread_info *thr = &info[i];
+      if (pthread_join (thr->thread, NULL) != 0)
+	abort ();
+    }
+
+  free (info);
+
+  /* Return non-zero.  We should never get here, but if we do make sure we
+     indicate something has gone wrong.  */
+  return 1;
+}
diff --git a/gdb/testsuite/gdb.server/exit-multiple-threads.exp b/gdb/testsuite/gdb.server/exit-multiple-threads.exp
new file mode 100644
index 0000000..aede2f7
--- /dev/null
+++ b/gdb/testsuite/gdb.server/exit-multiple-threads.exp
@@ -0,0 +1,136 @@
+# This testcase is part of GDB, the GNU debugger.
+#
+# Copyright 2020 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 GDB can handle receiving the W and X packets for a target
+# with multiple threads, but only a single inferior.
+#
+# Specifically, check GDB handles this case where multi-process
+# extensions are turned off.  At one point this was causing GDB to
+# give a warning when the exit arrived that the remote needed to
+# include a thread-id, which was not correct.
+
+load_lib gdbserver-support.exp
+
+if { [skip_gdbserver_tests] } {
+    verbose "skipping gdbserver tests"
+    return -1
+}
+
+standard_testfile
+
+# Start up GDB and GDBserver debugging EXECUTABLE.  When
+# DISABLE_MULTI_PROCESS is true then disable GDB's remote
+# multi-process support, otherwise, leave it enabled.
+#
+# Places a breakpoint in function 'breakpt' and then continues to the
+# breakpoint, at which point it runs 'info threads'.
+proc prepare_for_test { executable disable_multi_process } {
+    clean_restart ${executable}
+
+    # Make sure we're disconnected, in case we're testing with an
+    # extended-remote board, therefore already connected.
+    gdb_test "disconnect" ".*"
+
+    # Disable XML-based thread listing, and possible the multi-process
+    # extensions.
+    gdb_test_no_output "set remote threads-packet off"
+    if { $disable_multi_process } {
+	gdb_test_no_output "set remote multiprocess-feature-packet off"
+    }
+
+    # Start gdbserver and connect.
+    set res [gdbserver_start "" $executable]
+    set gdbserver_protocol [lindex $res 0]
+    set gdbserver_gdbport [lindex $res 1]
+    set res [gdb_target_cmd $gdbserver_protocol $gdbserver_gdbport]
+    if ![gdb_assert {$res == 0} "connect"] {
+	return
+    }
+
+    # Run until we hit the breakpt function, then list all threads.
+    gdb_breakpoint "breakpt"
+    gdb_continue_to_breakpoint "breakpt"
+    gdb_test "info threads" ".*"
+}
+
+# Run the tests where the inferior exits normally (the W packet) while
+# we have multiple-threads.  EXECUTABLE is the binary under test, and
+# DISABLE_MULTI_PROCESS indicates if we should disable GDB's remote
+# multi-process support.
+proc run_exit_test { executable disable_multi_process } {
+    global decimal
+
+    prepare_for_test ${executable} ${disable_multi_process}
+
+    # Finally, continue until the process exits, ensure we don't see
+    # any warnings between "Continuing." and the final process has
+    # exited message.
+    if { $disable_multi_process } {
+	set process_pattern "Remote target"
+    } else {
+	set process_pattern "process $decimal"
+    }
+    gdb_test "continue" \
+	[multi_line \
+	     "Continuing\\." \
+	     "\\\[Inferior $decimal \\\(${process_pattern}\\\) exited normally\\\]" ] \
+	"continue until process exits"
+}
+
+# Run the tests where the inferior exits with a signal (the X packet)
+# while we have multiple-threads.  EXECUTABLE is the binary under
+# test, and DISABLE_MULTI_PROCESS indicates if we should disable GDB's
+# remote multi-process support.
+proc run_signal_test { executable disable_multi_process } {
+    global decimal gdb_prompt
+
+    prepare_for_test ${executable} ${disable_multi_process}
+
+    set inf_pid [get_valueof "/d" "global_pid" "unknown"]
+    gdb_assert ![string eq ${inf_pid} "unknown"] "read the pid"
+
+    # This sets the inferior running again, with all threads going
+    # into a long delay loop.
+    send_gdb "continue\n"
+
+    # Send the inferior a signal to kill it.
+    sleep 1
+    remote_exec target "kill -9 ${inf_pid}"
+
+    # Process the output from GDB.
+    gdb_test_multiple "" "inferior exited with signal" {
+	-re "Continuing\\.\r\n\r\nProgram terminated with signal SIGKILL, Killed.\r\nThe program no longer exists.\r\n$gdb_prompt $" {
+	    pass $gdb_test_name
+	}
+    }
+}
+
+# Run all of the tests.
+foreach_with_prefix test { exit signal } {
+    set def "DO_[string toupper $test]_TEST"
+    set func "run_${test}_test"
+
+    set executable "$binfile-${test}"
+    if [prepare_for_testing "failed to prepare" $executable $srcfile \
+	    [list debug pthreads additional_flags=-D${def}]] {
+	return -1
+    }
+
+    foreach_with_prefix multi_process { 0 1 } {
+	$func ${executable} ${multi_process}
+    }
+}