From patchwork Thu Jan 12 14:37:53 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Simon Marchi X-Patchwork-Id: 63103 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 9FB08385439E for ; Thu, 12 Jan 2023 14:38:24 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 9FB08385439E DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1673534304; bh=h3yaJrGMAOwbHyf4XNg1ZHcLpqvi7bFI1Ib5mhlooBg=; h=To:Cc:Subject:Date:List-Id:List-Unsubscribe:List-Archive: List-Post:List-Help:List-Subscribe:From:Reply-To:From; b=GVrVSYAO6ilO+z79uKk0IN67/FASsMjkE9sbMRk2gWpQXbaZUcBled5IB2Xffgr6i nboJrZCXrl367cxhlNnOxnD2jhRXbkYK9+EK0/2gdkGsaTWx8NwbabUSDv3l3eKhr2 G0+mZq2HrqY7yV68EnItdXyc0OHcGy0F4mFHzwzQ= X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id A73D83858C66 for ; Thu, 12 Jan 2023 14:37:59 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org A73D83858C66 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 30CEbrJS007970 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 12 Jan 2023 09:37:58 -0500 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 30CEbrJS007970 Received: from simark.localdomain (unknown [217.28.27.60]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id D9E371E112; Thu, 12 Jan 2023 09:37:53 -0500 (EST) To: gdb-patches@sourceware.org Cc: Simon Marchi Subject: [PATCH] gdb/remote: fix assertion failure when connecting while native inferior is running Date: Thu, 12 Jan 2023 09:37:53 -0500 Message-Id: <20230112143753.2190300-1-simon.marchi@polymtl.ca> X-Mailer: git-send-email 2.39.0 MIME-Version: 1.0 X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Thu, 12 Jan 2023 14:37:54 +0000 X-Spam-Status: No, score=-3189.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Simon Marchi via Gdb-patches From: Simon Marchi Reply-To: Simon Marchi Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org Sender: "Gdb-patches" 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 [] ()] (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 base-commit: edf64cd235f5ecb3725e7cf1ff83bbdb6dd53340 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 . */ + +#include +#include +#include + +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 . + +# 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 + } +}