* [PATCH] gdb: enable target_async around stop_all_threads call in process_initial_stop_replies
@ 2021-06-07 17:15 Simon Marchi
2021-08-05 16:28 ` Simon Marchi
0 siblings, 1 reply; 4+ messages in thread
From: Simon Marchi @ 2021-06-07 17:15 UTC (permalink / raw)
To: gdb-patches
The following scenario hangs:
- maint set target-non-stop on
- `gdbserver --attach`
- a multi-threaded program
For example:
Terminal 1:
$ gnome-calculator&
[1] 495731
$ ../gdbserver/gdbserver --once --attach :1234 495731
Attached; pid = 495731
Listening on port 1234
Terminal 2:
$ ./gdb -nx -q --data-directory=data-directory /usr/bin/gnome-calculator -ex "maint set target-non-stop on" -ex "tar rem :1234"
Reading symbols from /usr/bin/gnome-calculator...
(No debugging symbols found in /usr/bin/gnome-calculator)
Remote debugging using :1234
* hangs *
What happens is:
- The protocol between gdb and gdbserver is in non-stop mode, but the
user-visible behavior is all-stop
- On connect, gdbserver sends one stop reply for one thread that is
stops, the others stay running
- In process_initial_stop_replies, gdb calls stop_all_threads to stop
these other threads, because we are using the all-stop user-visible
mode
- stop_all_threads sends a stop request for all the running threads and
then waits for resulting events
- At this point, the remote target is in target_async(0) mode, which
makes stop_all_threads not consider it for events
- stop_all_threads loops indefinitely (it does not event block
indefinitely, it is in an infinite loop) because there are no event
sources. wait_one_event returns a TARGET_WAITKIND_NO_RESUMED wait
status.
Fix that by making the remote target async around the stop_all_threads
call.
I haven't implemented it because I'm not sure how to do it, but I think
it would be a good idea to have, in stop_all_threads / wait_one /
handle_one, an assert to check that if we are expecting one or more
event, then there are some targets that are in a state where they can
supply some events. Otherwise, we'll necessarily be stuck in this
infinite loop, and it's probably due to a bug in GDB. I'm not too sure
where to put this or how to express it though. Perhaps in
stop_all_threads, here:
for (int i = 0; i < waits_needed; i++)
{
wait_one_event event = wait_one ();
*here*
if (handle_one (event))
break;
}
If at that point, the returned event is TARGET_WAITKIND_NO_RESUMED,
there's a problem. We expect some event, because we've asked some
threads to stop, but all targets are answering that they won't have any
events for us. That's a contradiction, and a sign that something has
gone wrong. It could perhaps event be:
gdb_assert (event.ws.kind != TARGET_WAITKIND_NO_RESUMED);
in handle_one, as the idea is the same in prepare_for_detach.
A bit more sophisticated would be: we know which targets we are
expecting waits from, since we know which threads we have asked to
stop. So if any of these targets returns TARGET_WAITKIND_NO_RESUMED,
something is fishy.
Add a test that tests attaching with gdbserver's --attach flag to a
multi-threaded program, and then connecting to it. Without the fix, the
test reproduces the hang.
gdb/ChangeLog:
* remote.c (remote_target::process_initial_stop_replies): Enable
target_async around stop_all_threads call.
gdb/testsuite/ChangeLog:
* gdb.server/attach-flag.c: New test.
* gdb.server/attach-flag.exp: New test.
Change-Id: If6f6690a4887ca66693ef1af64791dda4c65f24f
---
gdb/remote.c | 10 ++-
gdb/testsuite/gdb.server/attach-flag.c | 29 +++++++++
gdb/testsuite/gdb.server/attach-flag.exp | 79 ++++++++++++++++++++++++
3 files changed, 117 insertions(+), 1 deletion(-)
create mode 100644 gdb/testsuite/gdb.server/attach-flag.c
create mode 100644 gdb/testsuite/gdb.server/attach-flag.exp
diff --git a/gdb/remote.c b/gdb/remote.c
index de04aab43dc9..cd4b0e1c5a5b 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -4591,7 +4591,15 @@ remote_target::process_initial_stop_replies (int from_tty)
the inferiors. */
if (!non_stop)
{
- stop_all_threads ();
+ {
+ /* At this point, the remote target is not async. It needs to be for
+ the poll in stop_all_threads to consider events from it, so enable
+ it temporarily. */
+ gdb_assert (!this->is_async_p ());
+ SCOPE_EXIT { target_async (0); };
+ target_async (1);
+ stop_all_threads ();
+ }
/* If all threads of an inferior were already stopped, we
haven't setup the inferior yet. */
diff --git a/gdb/testsuite/gdb.server/attach-flag.c b/gdb/testsuite/gdb.server/attach-flag.c
new file mode 100644
index 000000000000..f6ed6180eef9
--- /dev/null
+++ b/gdb/testsuite/gdb.server/attach-flag.c
@@ -0,0 +1,29 @@
+#include <pthread.h>
+#include <unistd.h>
+
+static const int NTHREADS = 10;
+static pthread_barrier_t barrier;
+
+static void *
+thread_func (void *p)
+{
+ pthread_barrier_wait (&barrier);
+ return NULL;
+}
+
+int
+main (void)
+{
+ alarm (60);
+
+ pthread_t threads[NTHREADS];
+ pthread_barrier_init (&barrier, NULL, NTHREADS + 2);
+
+ for (int i = 0; i < NTHREADS; i++)
+ pthread_create (&threads[i], NULL, thread_func, NULL);
+
+ pthread_barrier_wait (&barrier);
+
+ for (int i = 0; i < NTHREADS; i++)
+ pthread_join (threads[i], NULL);
+}
diff --git a/gdb/testsuite/gdb.server/attach-flag.exp b/gdb/testsuite/gdb.server/attach-flag.exp
new file mode 100644
index 000000000000..dc949386e0e2
--- /dev/null
+++ b/gdb/testsuite/gdb.server/attach-flag.exp
@@ -0,0 +1,79 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2021 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 attaching to a multi-threaded process using gdbserver's --attach flag.
+
+load_lib gdbserver-support.exp
+
+standard_testfile
+
+if { [skip_gdbserver_tests] } {
+ return 0
+}
+
+if {![can_spawn_for_attach]} {
+ return 0
+}
+
+# Start the test program, attach to it using gdbserver's --attach flag, connect
+# to it with GDB, check that what we see makes sense.
+
+proc run_one_test { non-stop target-non-stop } {
+ save_vars { ::GDBFLAGS } {
+ # If GDB and GDBserver are both running locally, set the sysroot to avoid
+ # reading files via the remote protocol.
+ if { ![is_remote host] && ![is_remote target] } {
+ set ::GDBFLAGS "$::GDBFLAGS -ex \"set sysroot\""
+ }
+
+ if { [prepare_for_testing "failed to prepare" $::testfile $::srcfile \
+ {debug pthreads}] } {
+ return -1
+ }
+ }
+
+ # Make sure we're disconnected, in case we're testing with an
+ # extended-remote board, therefore already connected.
+ gdb_test "disconnect" ".*"
+
+ set target_exec [gdbserver_download_current_prog]
+ set test_spawn_id [spawn_wait_for_attach $::binfile]
+ set testpid [spawn_id_get_pid $test_spawn_id]
+
+ lassign [gdbserver_start "" "--attach $testpid"] unused gdbserver_address
+
+ gdb_test_no_output "set non-stop ${non-stop}"
+ gdb_test_no_output "maint set target-non-stop ${target-non-stop}"
+ gdb_target_cmd "remote" $gdbserver_address
+
+ # There should be 11 threads.
+ gdb_test "thread 11" "Switching to thread 11.*"
+
+ kill_wait_spawned_process $test_spawn_id
+ gdbserver_exit 0
+}
+
+foreach_with_prefix non-stop {0 1} {
+ foreach_with_prefix target-non-stop {0 1} {
+ # This combination does not make sense.
+ if { ${non-stop} == 1 && ${target-non-stop} == 0} {
+ continue
+ }
+
+ run_one_test ${non-stop} ${target-non-stop}
+ }
+}
--
2.31.1
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] gdb: enable target_async around stop_all_threads call in process_initial_stop_replies
2021-06-07 17:15 [PATCH] gdb: enable target_async around stop_all_threads call in process_initial_stop_replies Simon Marchi
@ 2021-08-05 16:28 ` Simon Marchi
2021-08-26 1:39 ` Simon Marchi
0 siblings, 1 reply; 4+ messages in thread
From: Simon Marchi @ 2021-08-05 16:28 UTC (permalink / raw)
To: gdb-patches
Ping.
On 2021-06-07 1:15 p.m., Simon Marchi wrote:
> The following scenario hangs:
>
> - maint set target-non-stop on
> - `gdbserver --attach`
> - a multi-threaded program
>
> For example:
>
> Terminal 1:
>
> $ gnome-calculator&
> [1] 495731
> $ ../gdbserver/gdbserver --once --attach :1234 495731
> Attached; pid = 495731
> Listening on port 1234
>
> Terminal 2:
>
> $ ./gdb -nx -q --data-directory=data-directory /usr/bin/gnome-calculator -ex "maint set target-non-stop on" -ex "tar rem :1234"
> Reading symbols from /usr/bin/gnome-calculator...
> (No debugging symbols found in /usr/bin/gnome-calculator)
> Remote debugging using :1234
> * hangs *
>
> What happens is:
>
> - The protocol between gdb and gdbserver is in non-stop mode, but the
> user-visible behavior is all-stop
> - On connect, gdbserver sends one stop reply for one thread that is
> stops, the others stay running
> - In process_initial_stop_replies, gdb calls stop_all_threads to stop
> these other threads, because we are using the all-stop user-visible
> mode
> - stop_all_threads sends a stop request for all the running threads and
> then waits for resulting events
> - At this point, the remote target is in target_async(0) mode, which
> makes stop_all_threads not consider it for events
> - stop_all_threads loops indefinitely (it does not event block
> indefinitely, it is in an infinite loop) because there are no event
> sources. wait_one_event returns a TARGET_WAITKIND_NO_RESUMED wait
> status.
>
> Fix that by making the remote target async around the stop_all_threads
> call.
>
> I haven't implemented it because I'm not sure how to do it, but I think
> it would be a good idea to have, in stop_all_threads / wait_one /
> handle_one, an assert to check that if we are expecting one or more
> event, then there are some targets that are in a state where they can
> supply some events. Otherwise, we'll necessarily be stuck in this
> infinite loop, and it's probably due to a bug in GDB. I'm not too sure
> where to put this or how to express it though. Perhaps in
> stop_all_threads, here:
>
> for (int i = 0; i < waits_needed; i++)
> {
> wait_one_event event = wait_one ();
> *here*
> if (handle_one (event))
> break;
> }
>
> If at that point, the returned event is TARGET_WAITKIND_NO_RESUMED,
> there's a problem. We expect some event, because we've asked some
> threads to stop, but all targets are answering that they won't have any
> events for us. That's a contradiction, and a sign that something has
> gone wrong. It could perhaps event be:
>
> gdb_assert (event.ws.kind != TARGET_WAITKIND_NO_RESUMED);
>
> in handle_one, as the idea is the same in prepare_for_detach.
>
> A bit more sophisticated would be: we know which targets we are
> expecting waits from, since we know which threads we have asked to
> stop. So if any of these targets returns TARGET_WAITKIND_NO_RESUMED,
> something is fishy.
>
> Add a test that tests attaching with gdbserver's --attach flag to a
> multi-threaded program, and then connecting to it. Without the fix, the
> test reproduces the hang.
>
> gdb/ChangeLog:
>
> * remote.c (remote_target::process_initial_stop_replies): Enable
> target_async around stop_all_threads call.
>
> gdb/testsuite/ChangeLog:
>
> * gdb.server/attach-flag.c: New test.
> * gdb.server/attach-flag.exp: New test.
>
> Change-Id: If6f6690a4887ca66693ef1af64791dda4c65f24f
> ---
> gdb/remote.c | 10 ++-
> gdb/testsuite/gdb.server/attach-flag.c | 29 +++++++++
> gdb/testsuite/gdb.server/attach-flag.exp | 79 ++++++++++++++++++++++++
> 3 files changed, 117 insertions(+), 1 deletion(-)
> create mode 100644 gdb/testsuite/gdb.server/attach-flag.c
> create mode 100644 gdb/testsuite/gdb.server/attach-flag.exp
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index de04aab43dc9..cd4b0e1c5a5b 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -4591,7 +4591,15 @@ remote_target::process_initial_stop_replies (int from_tty)
> the inferiors. */
> if (!non_stop)
> {
> - stop_all_threads ();
> + {
> + /* At this point, the remote target is not async. It needs to be for
> + the poll in stop_all_threads to consider events from it, so enable
> + it temporarily. */
> + gdb_assert (!this->is_async_p ());
> + SCOPE_EXIT { target_async (0); };
> + target_async (1);
> + stop_all_threads ();
> + }
>
> /* If all threads of an inferior were already stopped, we
> haven't setup the inferior yet. */
> diff --git a/gdb/testsuite/gdb.server/attach-flag.c b/gdb/testsuite/gdb.server/attach-flag.c
> new file mode 100644
> index 000000000000..f6ed6180eef9
> --- /dev/null
> +++ b/gdb/testsuite/gdb.server/attach-flag.c
> @@ -0,0 +1,29 @@
> +#include <pthread.h>
> +#include <unistd.h>
> +
> +static const int NTHREADS = 10;
> +static pthread_barrier_t barrier;
> +
> +static void *
> +thread_func (void *p)
> +{
> + pthread_barrier_wait (&barrier);
> + return NULL;
> +}
> +
> +int
> +main (void)
> +{
> + alarm (60);
> +
> + pthread_t threads[NTHREADS];
> + pthread_barrier_init (&barrier, NULL, NTHREADS + 2);
> +
> + for (int i = 0; i < NTHREADS; i++)
> + pthread_create (&threads[i], NULL, thread_func, NULL);
> +
> + pthread_barrier_wait (&barrier);
> +
> + for (int i = 0; i < NTHREADS; i++)
> + pthread_join (threads[i], NULL);
> +}
> diff --git a/gdb/testsuite/gdb.server/attach-flag.exp b/gdb/testsuite/gdb.server/attach-flag.exp
> new file mode 100644
> index 000000000000..dc949386e0e2
> --- /dev/null
> +++ b/gdb/testsuite/gdb.server/attach-flag.exp
> @@ -0,0 +1,79 @@
> +# This testcase is part of GDB, the GNU debugger.
> +
> +# Copyright 2021 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 attaching to a multi-threaded process using gdbserver's --attach flag.
> +
> +load_lib gdbserver-support.exp
> +
> +standard_testfile
> +
> +if { [skip_gdbserver_tests] } {
> + return 0
> +}
> +
> +if {![can_spawn_for_attach]} {
> + return 0
> +}
> +
> +# Start the test program, attach to it using gdbserver's --attach flag, connect
> +# to it with GDB, check that what we see makes sense.
> +
> +proc run_one_test { non-stop target-non-stop } {
> + save_vars { ::GDBFLAGS } {
> + # If GDB and GDBserver are both running locally, set the sysroot to avoid
> + # reading files via the remote protocol.
> + if { ![is_remote host] && ![is_remote target] } {
> + set ::GDBFLAGS "$::GDBFLAGS -ex \"set sysroot\""
> + }
> +
> + if { [prepare_for_testing "failed to prepare" $::testfile $::srcfile \
> + {debug pthreads}] } {
> + return -1
> + }
> + }
> +
> + # Make sure we're disconnected, in case we're testing with an
> + # extended-remote board, therefore already connected.
> + gdb_test "disconnect" ".*"
> +
> + set target_exec [gdbserver_download_current_prog]
> + set test_spawn_id [spawn_wait_for_attach $::binfile]
> + set testpid [spawn_id_get_pid $test_spawn_id]
> +
> + lassign [gdbserver_start "" "--attach $testpid"] unused gdbserver_address
> +
> + gdb_test_no_output "set non-stop ${non-stop}"
> + gdb_test_no_output "maint set target-non-stop ${target-non-stop}"
> + gdb_target_cmd "remote" $gdbserver_address
> +
> + # There should be 11 threads.
> + gdb_test "thread 11" "Switching to thread 11.*"
> +
> + kill_wait_spawned_process $test_spawn_id
> + gdbserver_exit 0
> +}
> +
> +foreach_with_prefix non-stop {0 1} {
> + foreach_with_prefix target-non-stop {0 1} {
> + # This combination does not make sense.
> + if { ${non-stop} == 1 && ${target-non-stop} == 0} {
> + continue
> + }
> +
> + run_one_test ${non-stop} ${target-non-stop}
> + }
> +}
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] gdb: enable target_async around stop_all_threads call in process_initial_stop_replies
2021-08-05 16:28 ` Simon Marchi
@ 2021-08-26 1:39 ` Simon Marchi
2021-09-29 0:19 ` Simon Marchi
0 siblings, 1 reply; 4+ messages in thread
From: Simon Marchi @ 2021-08-26 1:39 UTC (permalink / raw)
To: gdb-patches
Ping.
On 2021-08-05 12:28 p.m., Simon Marchi via Gdb-patches wrote:
> Ping.
>
> On 2021-06-07 1:15 p.m., Simon Marchi wrote:
>> The following scenario hangs:
>>
>> - maint set target-non-stop on
>> - `gdbserver --attach`
>> - a multi-threaded program
>>
>> For example:
>>
>> Terminal 1:
>>
>> $ gnome-calculator&
>> [1] 495731
>> $ ../gdbserver/gdbserver --once --attach :1234 495731
>> Attached; pid = 495731
>> Listening on port 1234
>>
>> Terminal 2:
>>
>> $ ./gdb -nx -q --data-directory=data-directory /usr/bin/gnome-calculator -ex "maint set target-non-stop on" -ex "tar rem :1234"
>> Reading symbols from /usr/bin/gnome-calculator...
>> (No debugging symbols found in /usr/bin/gnome-calculator)
>> Remote debugging using :1234
>> * hangs *
>>
>> What happens is:
>>
>> - The protocol between gdb and gdbserver is in non-stop mode, but the
>> user-visible behavior is all-stop
>> - On connect, gdbserver sends one stop reply for one thread that is
>> stops, the others stay running
>> - In process_initial_stop_replies, gdb calls stop_all_threads to stop
>> these other threads, because we are using the all-stop user-visible
>> mode
>> - stop_all_threads sends a stop request for all the running threads and
>> then waits for resulting events
>> - At this point, the remote target is in target_async(0) mode, which
>> makes stop_all_threads not consider it for events
>> - stop_all_threads loops indefinitely (it does not event block
>> indefinitely, it is in an infinite loop) because there are no event
>> sources. wait_one_event returns a TARGET_WAITKIND_NO_RESUMED wait
>> status.
>>
>> Fix that by making the remote target async around the stop_all_threads
>> call.
>>
>> I haven't implemented it because I'm not sure how to do it, but I think
>> it would be a good idea to have, in stop_all_threads / wait_one /
>> handle_one, an assert to check that if we are expecting one or more
>> event, then there are some targets that are in a state where they can
>> supply some events. Otherwise, we'll necessarily be stuck in this
>> infinite loop, and it's probably due to a bug in GDB. I'm not too sure
>> where to put this or how to express it though. Perhaps in
>> stop_all_threads, here:
>>
>> for (int i = 0; i < waits_needed; i++)
>> {
>> wait_one_event event = wait_one ();
>> *here*
>> if (handle_one (event))
>> break;
>> }
>>
>> If at that point, the returned event is TARGET_WAITKIND_NO_RESUMED,
>> there's a problem. We expect some event, because we've asked some
>> threads to stop, but all targets are answering that they won't have any
>> events for us. That's a contradiction, and a sign that something has
>> gone wrong. It could perhaps event be:
>>
>> gdb_assert (event.ws.kind != TARGET_WAITKIND_NO_RESUMED);
>>
>> in handle_one, as the idea is the same in prepare_for_detach.
>>
>> A bit more sophisticated would be: we know which targets we are
>> expecting waits from, since we know which threads we have asked to
>> stop. So if any of these targets returns TARGET_WAITKIND_NO_RESUMED,
>> something is fishy.
>>
>> Add a test that tests attaching with gdbserver's --attach flag to a
>> multi-threaded program, and then connecting to it. Without the fix, the
>> test reproduces the hang.
>>
>> gdb/ChangeLog:
>>
>> * remote.c (remote_target::process_initial_stop_replies): Enable
>> target_async around stop_all_threads call.
>>
>> gdb/testsuite/ChangeLog:
>>
>> * gdb.server/attach-flag.c: New test.
>> * gdb.server/attach-flag.exp: New test.
>>
>> Change-Id: If6f6690a4887ca66693ef1af64791dda4c65f24f
>> ---
>> gdb/remote.c | 10 ++-
>> gdb/testsuite/gdb.server/attach-flag.c | 29 +++++++++
>> gdb/testsuite/gdb.server/attach-flag.exp | 79 ++++++++++++++++++++++++
>> 3 files changed, 117 insertions(+), 1 deletion(-)
>> create mode 100644 gdb/testsuite/gdb.server/attach-flag.c
>> create mode 100644 gdb/testsuite/gdb.server/attach-flag.exp
>>
>> diff --git a/gdb/remote.c b/gdb/remote.c
>> index de04aab43dc9..cd4b0e1c5a5b 100644
>> --- a/gdb/remote.c
>> +++ b/gdb/remote.c
>> @@ -4591,7 +4591,15 @@ remote_target::process_initial_stop_replies (int from_tty)
>> the inferiors. */
>> if (!non_stop)
>> {
>> - stop_all_threads ();
>> + {
>> + /* At this point, the remote target is not async. It needs to be for
>> + the poll in stop_all_threads to consider events from it, so enable
>> + it temporarily. */
>> + gdb_assert (!this->is_async_p ());
>> + SCOPE_EXIT { target_async (0); };
>> + target_async (1);
>> + stop_all_threads ();
>> + }
>>
>> /* If all threads of an inferior were already stopped, we
>> haven't setup the inferior yet. */
>> diff --git a/gdb/testsuite/gdb.server/attach-flag.c b/gdb/testsuite/gdb.server/attach-flag.c
>> new file mode 100644
>> index 000000000000..f6ed6180eef9
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.server/attach-flag.c
>> @@ -0,0 +1,29 @@
>> +#include <pthread.h>
>> +#include <unistd.h>
>> +
>> +static const int NTHREADS = 10;
>> +static pthread_barrier_t barrier;
>> +
>> +static void *
>> +thread_func (void *p)
>> +{
>> + pthread_barrier_wait (&barrier);
>> + return NULL;
>> +}
>> +
>> +int
>> +main (void)
>> +{
>> + alarm (60);
>> +
>> + pthread_t threads[NTHREADS];
>> + pthread_barrier_init (&barrier, NULL, NTHREADS + 2);
>> +
>> + for (int i = 0; i < NTHREADS; i++)
>> + pthread_create (&threads[i], NULL, thread_func, NULL);
>> +
>> + pthread_barrier_wait (&barrier);
>> +
>> + for (int i = 0; i < NTHREADS; i++)
>> + pthread_join (threads[i], NULL);
>> +}
>> diff --git a/gdb/testsuite/gdb.server/attach-flag.exp b/gdb/testsuite/gdb.server/attach-flag.exp
>> new file mode 100644
>> index 000000000000..dc949386e0e2
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.server/attach-flag.exp
>> @@ -0,0 +1,79 @@
>> +# This testcase is part of GDB, the GNU debugger.
>> +
>> +# Copyright 2021 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 attaching to a multi-threaded process using gdbserver's --attach flag.
>> +
>> +load_lib gdbserver-support.exp
>> +
>> +standard_testfile
>> +
>> +if { [skip_gdbserver_tests] } {
>> + return 0
>> +}
>> +
>> +if {![can_spawn_for_attach]} {
>> + return 0
>> +}
>> +
>> +# Start the test program, attach to it using gdbserver's --attach flag, connect
>> +# to it with GDB, check that what we see makes sense.
>> +
>> +proc run_one_test { non-stop target-non-stop } {
>> + save_vars { ::GDBFLAGS } {
>> + # If GDB and GDBserver are both running locally, set the sysroot to avoid
>> + # reading files via the remote protocol.
>> + if { ![is_remote host] && ![is_remote target] } {
>> + set ::GDBFLAGS "$::GDBFLAGS -ex \"set sysroot\""
>> + }
>> +
>> + if { [prepare_for_testing "failed to prepare" $::testfile $::srcfile \
>> + {debug pthreads}] } {
>> + return -1
>> + }
>> + }
>> +
>> + # Make sure we're disconnected, in case we're testing with an
>> + # extended-remote board, therefore already connected.
>> + gdb_test "disconnect" ".*"
>> +
>> + set target_exec [gdbserver_download_current_prog]
>> + set test_spawn_id [spawn_wait_for_attach $::binfile]
>> + set testpid [spawn_id_get_pid $test_spawn_id]
>> +
>> + lassign [gdbserver_start "" "--attach $testpid"] unused gdbserver_address
>> +
>> + gdb_test_no_output "set non-stop ${non-stop}"
>> + gdb_test_no_output "maint set target-non-stop ${target-non-stop}"
>> + gdb_target_cmd "remote" $gdbserver_address
>> +
>> + # There should be 11 threads.
>> + gdb_test "thread 11" "Switching to thread 11.*"
>> +
>> + kill_wait_spawned_process $test_spawn_id
>> + gdbserver_exit 0
>> +}
>> +
>> +foreach_with_prefix non-stop {0 1} {
>> + foreach_with_prefix target-non-stop {0 1} {
>> + # This combination does not make sense.
>> + if { ${non-stop} == 1 && ${target-non-stop} == 0} {
>> + continue
>> + }
>> +
>> + run_one_test ${non-stop} ${target-non-stop}
>> + }
>> +}
>>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] gdb: enable target_async around stop_all_threads call in process_initial_stop_replies
2021-08-26 1:39 ` Simon Marchi
@ 2021-09-29 0:19 ` Simon Marchi
0 siblings, 0 replies; 4+ messages in thread
From: Simon Marchi @ 2021-09-29 0:19 UTC (permalink / raw)
To: gdb-patches
I pushed this.
Simon
On 2021-08-25 21:39, Simon Marchi via Gdb-patches wrote:
> Ping.
>
> On 2021-08-05 12:28 p.m., Simon Marchi via Gdb-patches wrote:
>> Ping.
>>
>> On 2021-06-07 1:15 p.m., Simon Marchi wrote:
>>> The following scenario hangs:
>>>
>>> - maint set target-non-stop on
>>> - `gdbserver --attach`
>>> - a multi-threaded program
>>>
>>> For example:
>>>
>>> Terminal 1:
>>>
>>> $ gnome-calculator&
>>> [1] 495731
>>> $ ../gdbserver/gdbserver --once --attach :1234 495731
>>> Attached; pid = 495731
>>> Listening on port 1234
>>>
>>> Terminal 2:
>>>
>>> $ ./gdb -nx -q --data-directory=data-directory /usr/bin/gnome-calculator -ex "maint set target-non-stop on" -ex "tar rem :1234"
>>> Reading symbols from /usr/bin/gnome-calculator...
>>> (No debugging symbols found in /usr/bin/gnome-calculator)
>>> Remote debugging using :1234
>>> * hangs *
>>>
>>> What happens is:
>>>
>>> - The protocol between gdb and gdbserver is in non-stop mode, but the
>>> user-visible behavior is all-stop
>>> - On connect, gdbserver sends one stop reply for one thread that is
>>> stops, the others stay running
>>> - In process_initial_stop_replies, gdb calls stop_all_threads to stop
>>> these other threads, because we are using the all-stop user-visible
>>> mode
>>> - stop_all_threads sends a stop request for all the running threads and
>>> then waits for resulting events
>>> - At this point, the remote target is in target_async(0) mode, which
>>> makes stop_all_threads not consider it for events
>>> - stop_all_threads loops indefinitely (it does not event block
>>> indefinitely, it is in an infinite loop) because there are no event
>>> sources. wait_one_event returns a TARGET_WAITKIND_NO_RESUMED wait
>>> status.
>>>
>>> Fix that by making the remote target async around the stop_all_threads
>>> call.
>>>
>>> I haven't implemented it because I'm not sure how to do it, but I think
>>> it would be a good idea to have, in stop_all_threads / wait_one /
>>> handle_one, an assert to check that if we are expecting one or more
>>> event, then there are some targets that are in a state where they can
>>> supply some events. Otherwise, we'll necessarily be stuck in this
>>> infinite loop, and it's probably due to a bug in GDB. I'm not too sure
>>> where to put this or how to express it though. Perhaps in
>>> stop_all_threads, here:
>>>
>>> for (int i = 0; i < waits_needed; i++)
>>> {
>>> wait_one_event event = wait_one ();
>>> *here*
>>> if (handle_one (event))
>>> break;
>>> }
>>>
>>> If at that point, the returned event is TARGET_WAITKIND_NO_RESUMED,
>>> there's a problem. We expect some event, because we've asked some
>>> threads to stop, but all targets are answering that they won't have any
>>> events for us. That's a contradiction, and a sign that something has
>>> gone wrong. It could perhaps event be:
>>>
>>> gdb_assert (event.ws.kind != TARGET_WAITKIND_NO_RESUMED);
>>>
>>> in handle_one, as the idea is the same in prepare_for_detach.
>>>
>>> A bit more sophisticated would be: we know which targets we are
>>> expecting waits from, since we know which threads we have asked to
>>> stop. So if any of these targets returns TARGET_WAITKIND_NO_RESUMED,
>>> something is fishy.
>>>
>>> Add a test that tests attaching with gdbserver's --attach flag to a
>>> multi-threaded program, and then connecting to it. Without the fix, the
>>> test reproduces the hang.
>>>
>>> gdb/ChangeLog:
>>>
>>> * remote.c (remote_target::process_initial_stop_replies): Enable
>>> target_async around stop_all_threads call.
>>>
>>> gdb/testsuite/ChangeLog:
>>>
>>> * gdb.server/attach-flag.c: New test.
>>> * gdb.server/attach-flag.exp: New test.
>>>
>>> Change-Id: If6f6690a4887ca66693ef1af64791dda4c65f24f
>>> ---
>>> gdb/remote.c | 10 ++-
>>> gdb/testsuite/gdb.server/attach-flag.c | 29 +++++++++
>>> gdb/testsuite/gdb.server/attach-flag.exp | 79 ++++++++++++++++++++++++
>>> 3 files changed, 117 insertions(+), 1 deletion(-)
>>> create mode 100644 gdb/testsuite/gdb.server/attach-flag.c
>>> create mode 100644 gdb/testsuite/gdb.server/attach-flag.exp
>>>
>>> diff --git a/gdb/remote.c b/gdb/remote.c
>>> index de04aab43dc9..cd4b0e1c5a5b 100644
>>> --- a/gdb/remote.c
>>> +++ b/gdb/remote.c
>>> @@ -4591,7 +4591,15 @@ remote_target::process_initial_stop_replies (int from_tty)
>>> the inferiors. */
>>> if (!non_stop)
>>> {
>>> - stop_all_threads ();
>>> + {
>>> + /* At this point, the remote target is not async. It needs to be for
>>> + the poll in stop_all_threads to consider events from it, so enable
>>> + it temporarily. */
>>> + gdb_assert (!this->is_async_p ());
>>> + SCOPE_EXIT { target_async (0); };
>>> + target_async (1);
>>> + stop_all_threads ();
>>> + }
>>>
>>> /* If all threads of an inferior were already stopped, we
>>> haven't setup the inferior yet. */
>>> diff --git a/gdb/testsuite/gdb.server/attach-flag.c b/gdb/testsuite/gdb.server/attach-flag.c
>>> new file mode 100644
>>> index 000000000000..f6ed6180eef9
>>> --- /dev/null
>>> +++ b/gdb/testsuite/gdb.server/attach-flag.c
>>> @@ -0,0 +1,29 @@
>>> +#include <pthread.h>
>>> +#include <unistd.h>
>>> +
>>> +static const int NTHREADS = 10;
>>> +static pthread_barrier_t barrier;
>>> +
>>> +static void *
>>> +thread_func (void *p)
>>> +{
>>> + pthread_barrier_wait (&barrier);
>>> + return NULL;
>>> +}
>>> +
>>> +int
>>> +main (void)
>>> +{
>>> + alarm (60);
>>> +
>>> + pthread_t threads[NTHREADS];
>>> + pthread_barrier_init (&barrier, NULL, NTHREADS + 2);
>>> +
>>> + for (int i = 0; i < NTHREADS; i++)
>>> + pthread_create (&threads[i], NULL, thread_func, NULL);
>>> +
>>> + pthread_barrier_wait (&barrier);
>>> +
>>> + for (int i = 0; i < NTHREADS; i++)
>>> + pthread_join (threads[i], NULL);
>>> +}
>>> diff --git a/gdb/testsuite/gdb.server/attach-flag.exp b/gdb/testsuite/gdb.server/attach-flag.exp
>>> new file mode 100644
>>> index 000000000000..dc949386e0e2
>>> --- /dev/null
>>> +++ b/gdb/testsuite/gdb.server/attach-flag.exp
>>> @@ -0,0 +1,79 @@
>>> +# This testcase is part of GDB, the GNU debugger.
>>> +
>>> +# Copyright 2021 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 attaching to a multi-threaded process using gdbserver's --attach flag.
>>> +
>>> +load_lib gdbserver-support.exp
>>> +
>>> +standard_testfile
>>> +
>>> +if { [skip_gdbserver_tests] } {
>>> + return 0
>>> +}
>>> +
>>> +if {![can_spawn_for_attach]} {
>>> + return 0
>>> +}
>>> +
>>> +# Start the test program, attach to it using gdbserver's --attach flag, connect
>>> +# to it with GDB, check that what we see makes sense.
>>> +
>>> +proc run_one_test { non-stop target-non-stop } {
>>> + save_vars { ::GDBFLAGS } {
>>> + # If GDB and GDBserver are both running locally, set the sysroot to avoid
>>> + # reading files via the remote protocol.
>>> + if { ![is_remote host] && ![is_remote target] } {
>>> + set ::GDBFLAGS "$::GDBFLAGS -ex \"set sysroot\""
>>> + }
>>> +
>>> + if { [prepare_for_testing "failed to prepare" $::testfile $::srcfile \
>>> + {debug pthreads}] } {
>>> + return -1
>>> + }
>>> + }
>>> +
>>> + # Make sure we're disconnected, in case we're testing with an
>>> + # extended-remote board, therefore already connected.
>>> + gdb_test "disconnect" ".*"
>>> +
>>> + set target_exec [gdbserver_download_current_prog]
>>> + set test_spawn_id [spawn_wait_for_attach $::binfile]
>>> + set testpid [spawn_id_get_pid $test_spawn_id]
>>> +
>>> + lassign [gdbserver_start "" "--attach $testpid"] unused gdbserver_address
>>> +
>>> + gdb_test_no_output "set non-stop ${non-stop}"
>>> + gdb_test_no_output "maint set target-non-stop ${target-non-stop}"
>>> + gdb_target_cmd "remote" $gdbserver_address
>>> +
>>> + # There should be 11 threads.
>>> + gdb_test "thread 11" "Switching to thread 11.*"
>>> +
>>> + kill_wait_spawned_process $test_spawn_id
>>> + gdbserver_exit 0
>>> +}
>>> +
>>> +foreach_with_prefix non-stop {0 1} {
>>> + foreach_with_prefix target-non-stop {0 1} {
>>> + # This combination does not make sense.
>>> + if { ${non-stop} == 1 && ${target-non-stop} == 0} {
>>> + continue
>>> + }
>>> +
>>> + run_one_test ${non-stop} ${target-non-stop}
>>> + }
>>> +}
>>>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-09-29 0:19 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-07 17:15 [PATCH] gdb: enable target_async around stop_all_threads call in process_initial_stop_replies Simon Marchi
2021-08-05 16:28 ` Simon Marchi
2021-08-26 1:39 ` Simon Marchi
2021-09-29 0:19 ` Simon Marchi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).