[PATCH] gdb/remote: fix assertion failure when connecting while native inferior is running

[PATCH] gdb/remote: fix assertion failure when connecting while native inferior is running

[Simon Marchi]

I noticed that connecting to a remote, while a native inferior is
running, leads to an internal error:

    (gdb) r &
    Starting program: /usr/bin/sleep 1234
    (gdb) [Thread debugging using libthread_db enabled]
    Using host libthread_db library "/usr/lib/../lib/libthread_db.so.1".
    add-inferior
    [New inferior 2]
    Added inferior 2 on connection 1 (native)
    (gdb) inferior 2
    [Switching to inferior 2 [<null>] (<noexec>)]
    (gdb) tar ext :1234
    Remote debugging using :1234
    Reading /usr/bin/sleep from remote target...
    warning: File transfers from remote targets can be slow. Use "set sysroot" to access files locally instead.
    Reading /usr/bin/sleep from remote target...
    Reading symbols from target:/usr/bin/sleep...
    Reading /usr/local/lib/debug/.build-id/48/e312e04335ae8a4d66e40c6c59e089f33846f5.debug from remote target...
    (No debugging symbols found in target:/usr/bin/sleep)
    /home/simark/src/binutils-gdb/gdb/target.c:3779: internal-error: target_stop: Assertion `!proc_target->commit_resumed_state' failed.

When connecting, the remote target stops all threads using
stop_all_threads.  This tries to stop the threads of inferior 1, calling
target_stop while its commit_resumed_state is set.

This patch fixes that by adding scoped_disable_commit_resumed in two
paths that call stop_all_threads.

The question (which I won't solve with this patch, I prefer not to get
in this rabbit hole) is, when in all-stop mode, when connecting to a
remote target and another inferior is running, what do we what to do
with that other inferior?  Do we leave it alone, or do we stop it?  The
behavior is not consistent right now.

The first call to stop_all_threads is in wait_for_inferior, in infrun.c.
Despite the name and location, this function is only used when
connecting to a remote connection, to wait for the initial stop event
sent after connecting.  It is only called when the remote target is
itself in all-stop: it is only called by start_remote, which is only
called by remote_target::start_remote_1 in a `!target_is_non_stop_p ()`
path.  So I wondered, why have that stop_all_threads call there at all.
stop_all_threads is only useful for non-stop targets.  The only reason
to have it there is if we want to stop threads of other non-stop targets
when connecting to an all-stop remote target.  This call was actually
added recently, in e0c01ce66d02 ("Don't stop all threads prematurely
after first step of "step N"").  Before that, it was done in
stop_waiting, so we would have hit it anyway, just earlier.

I decided to keep the call there, since my goal is not to change the
behavior, just fix the crash.  Supposedly, this would have worked before
the introduction of the commit_resumed concept.  So, add a
scoped_disable_commit_resumed there.  We also need to unconditionally
call finish_thread_state when leaving, such that the stopped state of
the threads of the native non-stop target gets reflected in the UI
(otherwise, we end up with threads shown as "running" but they are not
really running).  I'm not sure if this has unintended consequences
elsewhere...

The second call to stop_all_threads is in
remote_target::process_initial_stop_replies.  This is used when the
remote target is non-stop, and we need to stop all of its threads one by
one.  Add a scoped_disable_commit_resumed, and a call to
finish_thread_state, for the same reason as explained above.

In these two situations (non-stop native + all-stop remote, and non-stop
native + non-stop remote), the threads of the native target get stopped,
thanks to those stop_all_threads calls.  However, if the native target
is all-stop, its threads don't get stopped.  This makes the behavior a
bit inconsistent.  But I don't really want to get into fixing this,
because I feel like it would be opening a big can of worms, trying to
improve the infrun interaction of all-stop and non-stop targets.

Add a test that tries the case described above, with various
configurations.

Change-Id: Ie6831c14d9b6189be0e6b109636e4433e7fd0a4d
---
 gdb/infrun.c                                  | 10 +--
 gdb/remote.c                                  |  4 +
 .../connect-remote-while-native-running.c     | 66 +++++++++++++++
 .../connect-remote-while-native-running.exp   | 82 +++++++++++++++++++
 4 files changed, 154 insertions(+), 8 deletions(-)
 create mode 100644 gdb/testsuite/gdb.multi/connect-remote-while-native-running.c
 create mode 100644 gdb/testsuite/gdb.multi/connect-remote-while-native-running.exp

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 181d961d80d7..b28d869c4f32 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3966,11 +3966,7 @@ wait_for_inferior (inferior *inf)
 
   SCOPE_EXIT { delete_just_stopped_threads_infrun_breakpoints (); };
 
-  /* If an error happens while handling the event, propagate GDB's
-     knowledge of the executing state to the frontend/user running
-     state.  */
-  scoped_finish_thread_state finish_state
-    (inf->process_target (), minus_one_ptid);
+  scoped_finish_thread_state finish_state (nullptr, minus_one_ptid);
 
   while (1)
     {
@@ -3997,10 +3993,8 @@ wait_for_inferior (inferior *inf)
 	break;
     }
 
+  scoped_disable_commit_resumed disable_commit_resumed ("remote connect");
   stop_all_threads_if_all_stop_mode ();
-
-  /* No error, don't finish the state yet.  */
-  finish_state.release ();
 }
 
 /* Cleanup that reinstalls the readline callback handler, if the
diff --git a/gdb/remote.c b/gdb/remote.c
index 218bca30d047..57cb7e79ab4b 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -4647,7 +4647,11 @@ remote_target::process_initial_stop_replies (int from_tty)
 	gdb_assert (!this->is_async_p ());
 	SCOPE_EXIT { target_async (false); };
 	target_async (true);
+
+	scoped_disable_commit_resumed disable_commit_resumed
+	  ("remote connect in all-stop-on-top-of-non-stop");
 	stop_all_threads ("remote connect in all-stop");
+	finish_thread_state (nullptr, minus_one_ptid);
       }
 
       /* If all threads of an inferior were already stopped, we
diff --git a/gdb/testsuite/gdb.multi/connect-remote-while-native-running.c b/gdb/testsuite/gdb.multi/connect-remote-while-native-running.c
new file mode 100644
index 000000000000..a0a6ca3f5de7
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/connect-remote-while-native-running.c
@@ -0,0 +1,66 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2008-2023 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 <unistd.h>
+#include <assert.h>
+
+static pthread_barrier_t barrier;
+
+static void
+barrier_wait ()
+{
+  int ret = pthread_barrier_wait (&barrier);
+  assert (ret == 0 || ret == PTHREAD_BARRIER_SERIAL_THREAD);
+}
+
+static void *
+thread_func (void *p)
+{
+  barrier_wait ();
+  sleep (30);
+  return NULL;
+}
+
+static void
+all_started (void)
+{
+}
+
+int
+main (void)
+{
+  pthread_t thread;
+  int ret;
+
+  ret = pthread_barrier_init (&barrier, NULL, 2);
+  assert (ret == 0);
+
+  ret = pthread_create (&thread, NULL, thread_func, NULL);
+  assert (ret == 0);
+
+  barrier_wait ();
+  all_started ();
+
+  ret = pthread_join (thread, NULL);
+  assert (ret == 0);
+
+  ret = pthread_barrier_destroy (&barrier);
+  assert (ret == 0);
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.multi/connect-remote-while-native-running.exp b/gdb/testsuite/gdb.multi/connect-remote-while-native-running.exp
new file mode 100644
index 000000000000..94d08297cfeb
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/connect-remote-while-native-running.exp
@@ -0,0 +1,82 @@
+# Copyright 2023 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 opening a remote connection while a native inferior.
+
+standard_testfile
+
+if { [skip_gdbserver_tests] } {
+    return
+}
+
+if { [build_executable "failed to prepare" $testfile $srcfile] != 0 } {
+    return
+}
+
+set server_spawn_ids [list]
+
+# Run one variant of the test.
+
+proc do_test { maint_set_target_non_stop remote_protocol } {
+    save_vars { ::GDBFLAGS } {
+	append ::GDBFLAGS " -ex \"maintenance set target-non-stop ${maint_set_target_non_stop}\""
+	clean_restart $::binfile
+    }
+
+    # Make sure we're disconnected, in case we're testing with an
+    # extended-remote board, therefore already connected.
+    gdb_test "disconnect" ".*"
+
+    # Run the native inferior (1) to the point where we know both threads are
+    # started, then resume asynchronously.
+    if { ![runto all_started] } {
+	return
+    }
+
+    gdb_test -no-prompt-anchor "continue &" "Continuing\\."
+
+    # Add second inferior, start gdbserver, connect.
+    gdb_test "add-inferior" "Added inferior 2 on connection 1 \\(native\\)"
+    gdb_test "inferior 2" "Switching to inferior 2.*"
+    gdb_file_cmd $::binfile
+
+    set res [gdbserver_start "" $::binfile]
+    set port [lindex $res 1]
+    gdb_target_cmd $remote_protocol $port
+
+    # Depending on the configuration, the first inferior may or may not be
+    # stopped.  This is not ideal, as we'd like the behavior to be consistent
+    # across all variant.  But this is the way it is right now, we at least
+    # test that GDB does not crash.
+    if { $maint_set_target_non_stop == "off" } {
+	gdb_test "info threads" \
+	    [multi_line "  1\\.1 .* \\(running\\)" \
+			"  1\\.2 .* \\(running\\)" \
+			"\\* 2\\.1 .* $::hex in .*"]
+    } else {
+	gdb_test "info threads" \
+	    [multi_line "  1\\.1 .* $::hex in .*" \
+			"  1\\.2 .* $::hex in .*" \
+			"\\* 2\\.1 .* $::hex in .*"]
+    }
+
+    gdbserver_exit 0
+}
+
+foreach_with_prefix maint_set_target_non_stop {"auto" "off" "on"} {
+    foreach_with_prefix remote_protocol {"remote" "extended-remote"} {
+	do_test $maint_set_target_non_stop $remote_protocol
+    }
+}

base-commit: edf64cd235f5ecb3725e7cf1ff83bbdb6dd53340
-- 
2.39.0