* [PATCH] gdb/remote: handle attach when stop packet lacks thread-id
@ 2021-10-05 9:04 Andrew Burgess
2021-11-09 10:06 ` Andrew Burgess
2021-11-15 0:50 ` Simon Marchi
0 siblings, 2 replies; 8+ messages in thread
From: Andrew Burgess @ 2021-10-05 9:04 UTC (permalink / raw)
To: gdb-patches
Bug PR gdb/28405 reports a regression when using attach with an
extended-remote target. In this case the target is not including a
thread-id in the stop packet it sends back after the attach.
The regression was introduced with this commit:
commit 8f66807b98f7634c43149ea62e454ea8f877691d
Date: Wed Jan 13 20:26:58 2021 -0500
gdb: better handling of 'S' packets
The problem is that when GDB processes the stop packet, it sees that
there is no thread-id and so has to "guess" which thread the stop
should apply to.
In this case the target only has one thread, so really, there's no
guessing needed, but GDB still runs through the same process, this
shouldn't cause us any problems.
However, after the above commit, GDB now expects itself to be more
internally consistent, specifically, only a thread that GDB thinks is
resumed, can be a candidate for having stopped.
It turns out that, when GDB attaches to a process through an
extended-remote target, the threads of the process being attached too,
are not, initially, marked as resumed.
And so, when GDB tries to figure out which thread the stop might apply
too, it finds no threads in the processes marked resumed, and so an
assert triggers.
In extended_remote_target::attach we create a new thread with a call
to add_thread_silent, rather than remote_target::remote_add_thread,
the reason is that calling the latter will result in a call to
'add_thread' rather than 'add_thread_silent'. However,
remote_target::remote_add_thread includes additional
actions (i.e. calling remote_thread_info::set_resumed and set_running)
which are missing from extended_remote_target::attach. These missing
calls are what would serve to mark the new thread as resumed.
In this commit, I propose that we add the extra calls into
extended_remote_target::attach, this solves the problem at hand.
Additionally, in PR gdb/28405, a segfault is reported. This segfault
triggers when 'set debug remote 1' is used before trying to reproduce
the original assertion failure. The cause of this is in
remote_target::select_thread_for_ambiguous_stop_reply, where we do
this:
remote_debug_printf ("first resumed thread is %s",
pid_to_str (first_resumed_thread->ptid).c_str ());
remote_debug_printf ("is this guess ambiguous? = %d", ambiguous);
gdb_assert (first_resumed_thread != nullptr);
Notice that when debug printing is on we dereference
first_resumed_thread before we assert that the pointer is not
nullptr. This is the cause of the segfault, and is resolved by moving
the assert before the debug printing code.
I've extended an existing test, ext-attach.exp, so that the original
test is run multiple times; we run in the original mode, as normal,
but also, we now run with different packets disabled in gdbserver. In
particular, disabling Tthread would trigger the assertion as it was
reported in the original bug. I also run the test in all-stop and
non-stop modes now for extra coverage.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28405
---
gdb/remote.c | 7 +-
gdb/testsuite/gdb.server/ext-attach.exp | 94 +++++++++++++++----------
2 files changed, 62 insertions(+), 39 deletions(-)
diff --git a/gdb/remote.c b/gdb/remote.c
index d5eb40ce578..4db17ed7fb5 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -6075,6 +6075,9 @@ extended_remote_target::attach (const char *args, int from_tty)
/* Don't consider the thread stopped until we've processed the
saved stop reply. */
+ get_remote_thread_info (thr)->set_resumed ();
+ set_running (this, thr->ptid, true);
+ set_resumed (this, thr->ptid, true);
set_executing (this, thr->ptid, true);
}
@@ -7992,12 +7995,12 @@ remote_target::select_thread_for_ambiguous_stop_reply
ambiguous = true;
}
+ gdb_assert (first_resumed_thread != nullptr);
+
remote_debug_printf ("first resumed thread is %s",
pid_to_str (first_resumed_thread->ptid).c_str ());
remote_debug_printf ("is this guess ambiguous? = %d", ambiguous);
- gdb_assert (first_resumed_thread != nullptr);
-
/* Warn if the remote target is sending ambiguous stop replies. */
if (ambiguous)
{
diff --git a/gdb/testsuite/gdb.server/ext-attach.exp b/gdb/testsuite/gdb.server/ext-attach.exp
index c9766e35317..abd8c9b5b6d 100644
--- a/gdb/testsuite/gdb.server/ext-attach.exp
+++ b/gdb/testsuite/gdb.server/ext-attach.exp
@@ -30,53 +30,73 @@ if {![can_spawn_for_attach]} {
return 0
}
-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]} {
- return -1
- }
+if {[build_executable "failed to prepare" $testfile $srcfile debug]} {
+ return -1
}
-# Make sure we're disconnected, in case we're testing with an
-# extended-remote board, therefore already connected.
-gdb_test "disconnect" ".*"
+# Run the test. TARGET_NON_STOP should be 'on' or 'off'. TO_DISABLE
+# should be either the empty string, or something that can be passed
+# to gdbserver's --disable-packet command line option.
+proc run_test { target_non_stop to_disable } {
+ save_vars { ::GDBFLAGS } {
+ append ::GDBFLAGS " -ex \"maint set target-non-stop $target_non_stop\""
-set target_exec [gdbserver_download_current_prog]
-gdbserver_start_extended
+ # 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\""
+ }
-gdb_test_no_output "set remote exec-file $target_exec" "set remote exec-file"
+ clean_restart $::binfile
+ }
-set test_spawn_id [spawn_wait_for_attach $binfile]
-set testpid [spawn_id_get_pid $test_spawn_id]
+ # Make sure we're disconnected, in case we're testing with an
+ # extended-remote board, therefore already connected.
+ gdb_test "disconnect" ".*"
-gdb_test "attach $testpid" \
- "Attaching to program: .*, process $testpid.*(in|at).*" \
- "attach to remote program 1"
+ if { [gdb_target_supports_trace] } then {
+ # Test predefined TSVs are uploaded.
+ gdb_test_sequence "info tvariables" "check uploaded tsv" {
+ "\[\r\n\]+Name\[\t \]+Initial\[\t \]+Current"
+ "\[\r\n\]+\\\$trace_timestamp 0"
+ }
+ }
-if { [gdb_target_supports_trace] } then {
- # Test predefined TSVs are uploaded.
- gdb_test_sequence "info tvariables" "check uploaded tsv" {
- "\[\r\n\]+Name\[\t \]+Initial\[\t \]+Current"
- "\[\r\n\]+\\\$trace_timestamp 0"
+ set target_exec [gdbserver_download_current_prog]
+ if { $to_disable != "" } {
+ set gdbserver_opts "--disable-packet=${to_disable}"
+ } else {
+ set gdbserver_opts ""
}
-}
+ gdbserver_start_extended $gdbserver_opts
+
+ gdb_test_no_output "set remote exec-file $target_exec" "set remote exec-file"
-gdb_test "backtrace" ".*main.*" "backtrace 1"
+ set test_spawn_id [spawn_wait_for_attach $::binfile]
+ set testpid [spawn_id_get_pid $test_spawn_id]
-gdb_test "detach" "Detaching from program.*process.*"
-gdb_test "backtrace" "No stack\\." "backtrace with no program"
+ gdb_test "attach $testpid" \
+ "Attaching to program: .*, process $testpid.*(in|at).*" \
+ "attach to remote program 1"
-gdb_test "attach $testpid" \
- "Attaching to program: .*, process $testpid.*(in|at).*" \
- "attach to remote program 2"
-gdb_test "backtrace" ".*main.*" "backtrace 2"
+ gdb_test "backtrace" ".*main.*" "backtrace 1"
-gdb_test "kill" "" "kill" "Kill the program being debugged. .y or n. " "y"
-gdb_test_no_output "monitor exit"
+ gdb_test "detach" "Detaching from program.*process.*"
+ gdb_test "backtrace" "No stack\\." "backtrace with no program"
-kill_wait_spawned_process $test_spawn_id
+ gdb_test "attach $testpid" \
+ "Attaching to program: .*, process $testpid.*(in|at).*" \
+ "attach to remote program 2"
+ gdb_test "backtrace" ".*main.*" "backtrace 2"
+
+ gdb_test "kill" "" "kill" "Kill the program being debugged. .y or n. " "y"
+ gdb_test_no_output "monitor exit"
+
+ kill_wait_spawned_process $test_spawn_id
+}
+
+foreach_with_prefix target_non_stop {"off" "on"} {
+ foreach_with_prefix to_disable { "" Tthread T } {
+ run_test ${target_non_stop} $to_disable
+ }
+}
--
2.25.4
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] gdb/remote: handle attach when stop packet lacks thread-id
2021-10-05 9:04 [PATCH] gdb/remote: handle attach when stop packet lacks thread-id Andrew Burgess
@ 2021-11-09 10:06 ` Andrew Burgess
2021-11-15 0:50 ` Simon Marchi
1 sibling, 0 replies; 8+ messages in thread
From: Andrew Burgess @ 2021-11-09 10:06 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches; +Cc: Andrew Burgess
Ping!
Thanks,
Andrew
Andrew Burgess <andrew.burgess@embecosm.com> writes:
> Bug PR gdb/28405 reports a regression when using attach with an
> extended-remote target. In this case the target is not including a
> thread-id in the stop packet it sends back after the attach.
>
> The regression was introduced with this commit:
>
> commit 8f66807b98f7634c43149ea62e454ea8f877691d
> Date: Wed Jan 13 20:26:58 2021 -0500
>
> gdb: better handling of 'S' packets
>
> The problem is that when GDB processes the stop packet, it sees that
> there is no thread-id and so has to "guess" which thread the stop
> should apply to.
>
> In this case the target only has one thread, so really, there's no
> guessing needed, but GDB still runs through the same process, this
> shouldn't cause us any problems.
>
> However, after the above commit, GDB now expects itself to be more
> internally consistent, specifically, only a thread that GDB thinks is
> resumed, can be a candidate for having stopped.
>
> It turns out that, when GDB attaches to a process through an
> extended-remote target, the threads of the process being attached too,
> are not, initially, marked as resumed.
>
> And so, when GDB tries to figure out which thread the stop might apply
> too, it finds no threads in the processes marked resumed, and so an
> assert triggers.
>
> In extended_remote_target::attach we create a new thread with a call
> to add_thread_silent, rather than remote_target::remote_add_thread,
> the reason is that calling the latter will result in a call to
> 'add_thread' rather than 'add_thread_silent'. However,
> remote_target::remote_add_thread includes additional
> actions (i.e. calling remote_thread_info::set_resumed and set_running)
> which are missing from extended_remote_target::attach. These missing
> calls are what would serve to mark the new thread as resumed.
>
> In this commit, I propose that we add the extra calls into
> extended_remote_target::attach, this solves the problem at hand.
>
> Additionally, in PR gdb/28405, a segfault is reported. This segfault
> triggers when 'set debug remote 1' is used before trying to reproduce
> the original assertion failure. The cause of this is in
> remote_target::select_thread_for_ambiguous_stop_reply, where we do
> this:
>
> remote_debug_printf ("first resumed thread is %s",
> pid_to_str (first_resumed_thread->ptid).c_str ());
> remote_debug_printf ("is this guess ambiguous? = %d", ambiguous);
>
> gdb_assert (first_resumed_thread != nullptr);
>
> Notice that when debug printing is on we dereference
> first_resumed_thread before we assert that the pointer is not
> nullptr. This is the cause of the segfault, and is resolved by moving
> the assert before the debug printing code.
>
> I've extended an existing test, ext-attach.exp, so that the original
> test is run multiple times; we run in the original mode, as normal,
> but also, we now run with different packets disabled in gdbserver. In
> particular, disabling Tthread would trigger the assertion as it was
> reported in the original bug. I also run the test in all-stop and
> non-stop modes now for extra coverage.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28405
> ---
> gdb/remote.c | 7 +-
> gdb/testsuite/gdb.server/ext-attach.exp | 94 +++++++++++++++----------
> 2 files changed, 62 insertions(+), 39 deletions(-)
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index d5eb40ce578..4db17ed7fb5 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -6075,6 +6075,9 @@ extended_remote_target::attach (const char *args, int from_tty)
>
> /* Don't consider the thread stopped until we've processed the
> saved stop reply. */
> + get_remote_thread_info (thr)->set_resumed ();
> + set_running (this, thr->ptid, true);
> + set_resumed (this, thr->ptid, true);
> set_executing (this, thr->ptid, true);
> }
>
> @@ -7992,12 +7995,12 @@ remote_target::select_thread_for_ambiguous_stop_reply
> ambiguous = true;
> }
>
> + gdb_assert (first_resumed_thread != nullptr);
> +
> remote_debug_printf ("first resumed thread is %s",
> pid_to_str (first_resumed_thread->ptid).c_str ());
> remote_debug_printf ("is this guess ambiguous? = %d", ambiguous);
>
> - gdb_assert (first_resumed_thread != nullptr);
> -
> /* Warn if the remote target is sending ambiguous stop replies. */
> if (ambiguous)
> {
> diff --git a/gdb/testsuite/gdb.server/ext-attach.exp b/gdb/testsuite/gdb.server/ext-attach.exp
> index c9766e35317..abd8c9b5b6d 100644
> --- a/gdb/testsuite/gdb.server/ext-attach.exp
> +++ b/gdb/testsuite/gdb.server/ext-attach.exp
> @@ -30,53 +30,73 @@ if {![can_spawn_for_attach]} {
> return 0
> }
>
> -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]} {
> - return -1
> - }
> +if {[build_executable "failed to prepare" $testfile $srcfile debug]} {
> + return -1
> }
>
> -# Make sure we're disconnected, in case we're testing with an
> -# extended-remote board, therefore already connected.
> -gdb_test "disconnect" ".*"
> +# Run the test. TARGET_NON_STOP should be 'on' or 'off'. TO_DISABLE
> +# should be either the empty string, or something that can be passed
> +# to gdbserver's --disable-packet command line option.
> +proc run_test { target_non_stop to_disable } {
> + save_vars { ::GDBFLAGS } {
> + append ::GDBFLAGS " -ex \"maint set target-non-stop $target_non_stop\""
>
> -set target_exec [gdbserver_download_current_prog]
> -gdbserver_start_extended
> + # 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\""
> + }
>
> -gdb_test_no_output "set remote exec-file $target_exec" "set remote exec-file"
> + clean_restart $::binfile
> + }
>
> -set test_spawn_id [spawn_wait_for_attach $binfile]
> -set testpid [spawn_id_get_pid $test_spawn_id]
> + # Make sure we're disconnected, in case we're testing with an
> + # extended-remote board, therefore already connected.
> + gdb_test "disconnect" ".*"
>
> -gdb_test "attach $testpid" \
> - "Attaching to program: .*, process $testpid.*(in|at).*" \
> - "attach to remote program 1"
> + if { [gdb_target_supports_trace] } then {
> + # Test predefined TSVs are uploaded.
> + gdb_test_sequence "info tvariables" "check uploaded tsv" {
> + "\[\r\n\]+Name\[\t \]+Initial\[\t \]+Current"
> + "\[\r\n\]+\\\$trace_timestamp 0"
> + }
> + }
>
> -if { [gdb_target_supports_trace] } then {
> - # Test predefined TSVs are uploaded.
> - gdb_test_sequence "info tvariables" "check uploaded tsv" {
> - "\[\r\n\]+Name\[\t \]+Initial\[\t \]+Current"
> - "\[\r\n\]+\\\$trace_timestamp 0"
> + set target_exec [gdbserver_download_current_prog]
> + if { $to_disable != "" } {
> + set gdbserver_opts "--disable-packet=${to_disable}"
> + } else {
> + set gdbserver_opts ""
> }
> -}
> + gdbserver_start_extended $gdbserver_opts
> +
> + gdb_test_no_output "set remote exec-file $target_exec" "set remote exec-file"
>
> -gdb_test "backtrace" ".*main.*" "backtrace 1"
> + set test_spawn_id [spawn_wait_for_attach $::binfile]
> + set testpid [spawn_id_get_pid $test_spawn_id]
>
> -gdb_test "detach" "Detaching from program.*process.*"
> -gdb_test "backtrace" "No stack\\." "backtrace with no program"
> + gdb_test "attach $testpid" \
> + "Attaching to program: .*, process $testpid.*(in|at).*" \
> + "attach to remote program 1"
>
> -gdb_test "attach $testpid" \
> - "Attaching to program: .*, process $testpid.*(in|at).*" \
> - "attach to remote program 2"
> -gdb_test "backtrace" ".*main.*" "backtrace 2"
> + gdb_test "backtrace" ".*main.*" "backtrace 1"
>
> -gdb_test "kill" "" "kill" "Kill the program being debugged. .y or n. " "y"
> -gdb_test_no_output "monitor exit"
> + gdb_test "detach" "Detaching from program.*process.*"
> + gdb_test "backtrace" "No stack\\." "backtrace with no program"
>
> -kill_wait_spawned_process $test_spawn_id
> + gdb_test "attach $testpid" \
> + "Attaching to program: .*, process $testpid.*(in|at).*" \
> + "attach to remote program 2"
> + gdb_test "backtrace" ".*main.*" "backtrace 2"
> +
> + gdb_test "kill" "" "kill" "Kill the program being debugged. .y or n. " "y"
> + gdb_test_no_output "monitor exit"
> +
> + kill_wait_spawned_process $test_spawn_id
> +}
> +
> +foreach_with_prefix target_non_stop {"off" "on"} {
> + foreach_with_prefix to_disable { "" Tthread T } {
> + run_test ${target_non_stop} $to_disable
> + }
> +}
> --
> 2.25.4
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] gdb/remote: handle attach when stop packet lacks thread-id
2021-10-05 9:04 [PATCH] gdb/remote: handle attach when stop packet lacks thread-id Andrew Burgess
2021-11-09 10:06 ` Andrew Burgess
@ 2021-11-15 0:50 ` Simon Marchi
2021-11-23 14:24 ` [PATCHv2] " Andrew Burgess
1 sibling, 1 reply; 8+ messages in thread
From: Simon Marchi @ 2021-11-15 0:50 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches
On 2021-10-05 05:04, Andrew Burgess wrote:
> Bug PR gdb/28405 reports a regression when using attach with an
> extended-remote target. In this case the target is not including a
> thread-id in the stop packet it sends back after the attach.
>
> The regression was introduced with this commit:
>
> commit 8f66807b98f7634c43149ea62e454ea8f877691d
> Date: Wed Jan 13 20:26:58 2021 -0500
>
> gdb: better handling of 'S' packets
>
> The problem is that when GDB processes the stop packet, it sees that
> there is no thread-id and so has to "guess" which thread the stop
> should apply to.
>
> In this case the target only has one thread, so really, there's no
> guessing needed, but GDB still runs through the same process, this
> shouldn't cause us any problems.
>
> However, after the above commit, GDB now expects itself to be more
> internally consistent, specifically, only a thread that GDB thinks is
> resumed, can be a candidate for having stopped.
>
> It turns out that, when GDB attaches to a process through an
> extended-remote target, the threads of the process being attached too,
> are not, initially, marked as resumed.
>
> And so, when GDB tries to figure out which thread the stop might apply
> too, it finds no threads in the processes marked resumed, and so an
> assert triggers.
>
> In extended_remote_target::attach we create a new thread with a call
> to add_thread_silent, rather than remote_target::remote_add_thread,
> the reason is that calling the latter will result in a call to
> 'add_thread' rather than 'add_thread_silent'. However,
> remote_target::remote_add_thread includes additional
> actions (i.e. calling remote_thread_info::set_resumed and set_running)
> which are missing from extended_remote_target::attach. These missing
> calls are what would serve to mark the new thread as resumed.
>
> In this commit, I propose that we add the extra calls into
> extended_remote_target::attach, this solves the problem at hand.
To avoid duplication, did you consider calling remote_add_thread, but
add some more parameters to have the caller have control over the
thread's initial state?
But the way to resolve the problem (marking the initial thread as
resumed) sounds fine.
While reading the code, I saw there is an additional code path for
"target_can_async_p false". I suppose this can be exercised with "maint
set target-async off", it might be useful to add an axis for that in
your test?
Simon
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCHv2] gdb/remote: handle attach when stop packet lacks thread-id
2021-11-15 0:50 ` Simon Marchi
@ 2021-11-23 14:24 ` Andrew Burgess
2021-12-04 11:40 ` Joel Brobecker
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Andrew Burgess @ 2021-11-23 14:24 UTC (permalink / raw)
To: gdb-patches
* Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2021-11-14 19:50:01 -0500]:
>
>
> On 2021-10-05 05:04, Andrew Burgess wrote:
> > Bug PR gdb/28405 reports a regression when using attach with an
> > extended-remote target. In this case the target is not including a
> > thread-id in the stop packet it sends back after the attach.
> >
> > The regression was introduced with this commit:
> >
> > commit 8f66807b98f7634c43149ea62e454ea8f877691d
> > Date: Wed Jan 13 20:26:58 2021 -0500
> >
> > gdb: better handling of 'S' packets
> >
> > The problem is that when GDB processes the stop packet, it sees that
> > there is no thread-id and so has to "guess" which thread the stop
> > should apply to.
> >
> > In this case the target only has one thread, so really, there's no
> > guessing needed, but GDB still runs through the same process, this
> > shouldn't cause us any problems.
> >
> > However, after the above commit, GDB now expects itself to be more
> > internally consistent, specifically, only a thread that GDB thinks is
> > resumed, can be a candidate for having stopped.
> >
> > It turns out that, when GDB attaches to a process through an
> > extended-remote target, the threads of the process being attached too,
> > are not, initially, marked as resumed.
> >
> > And so, when GDB tries to figure out which thread the stop might apply
> > too, it finds no threads in the processes marked resumed, and so an
> > assert triggers.
> >
> > In extended_remote_target::attach we create a new thread with a call
> > to add_thread_silent, rather than remote_target::remote_add_thread,
> > the reason is that calling the latter will result in a call to
> > 'add_thread' rather than 'add_thread_silent'. However,
> > remote_target::remote_add_thread includes additional
> > actions (i.e. calling remote_thread_info::set_resumed and set_running)
> > which are missing from extended_remote_target::attach. These missing
> > calls are what would serve to mark the new thread as resumed.
> >
> > In this commit, I propose that we add the extra calls into
> > extended_remote_target::attach, this solves the problem at hand.
>
> To avoid duplication, did you consider calling remote_add_thread, but
> add some more parameters to have the caller have control over the
> thread's initial state?
I've made this change in the patch below.
>
> But the way to resolve the problem (marking the initial thread as
> resumed) sounds fine.
>
> While reading the code, I saw there is an additional code path for
> "target_can_async_p false". I suppose this can be exercised with "maint
> set target-async off", it might be useful to add an axis for that in
> your test?
Sigh! So I tried running with 'maint set target-async off'. And went
down the rabbit hole of debugging non-async mode. See:
https://sourceware.org/pipermail/gdb-patches/2021-November/183712.html
I've added this to the test for now:
# Currently, this test is only run with 'target-async on' as setting
# this off highlights some test failures.
foreach_with_prefix target_async {"on"} {
...
}
If/when the series I linked above is merged then that foreach loop can
be expanded to include both "off" and "on".
My intention would be that this patch not be merged to master until
the series I linked above is merged, but I'm posting this revised
patch here as this was raised as a possible candidate for 11.2. And
as the non-async testing really has nothing to do with the core of
this patch we could if we wanted, merge this version of the patch to
the gdb-11 branch, then later, once the async fixes I linked above are
merged, push a version of this patch with the full testing to master.
Thoughts welcome,
Thanks,
Andrew
---
commit 1aaa4dfbb8ce50cd64375394077ee5dc854ebba9
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date: Mon Oct 4 15:48:11 2021 +0100
gdb/remote: handle attach when stop packet lacks thread-id
Bug PR gdb/28405 reports a regression when using attach with an
extended-remote target. In this case the target is not including a
thread-id in the stop packet it sends back after the attach.
The regression was introduced with this commit:
commit 8f66807b98f7634c43149ea62e454ea8f877691d
Date: Wed Jan 13 20:26:58 2021 -0500
gdb: better handling of 'S' packets
The problem is that when GDB processes the stop packet, it sees that
there is no thread-id and so has to "guess" which thread the stop
should apply to.
In this case the target only has one thread, so really, there's no
guessing needed, but GDB still runs through the same process, this
shouldn't cause us any problems.
However, after the above commit, GDB now expects itself to be more
internally consistent, specifically, only a thread that GDB thinks is
resumed, can be a candidate for having stopped.
It turns out that, when GDB attaches to a process through an
extended-remote target, the threads of the process being attached too,
are not, initially, marked as resumed.
And so, when GDB tries to figure out which thread the stop might apply
too, it finds no threads in the processes marked resumed, and so an
assert triggers.
In extended_remote_target::attach we create a new thread with a call
to add_thread_silent, rather than remote_target::remote_add_thread,
the reason is that calling the latter will result in a call to
'add_thread' rather than 'add_thread_silent'. However,
remote_target::remote_add_thread includes additional
actions (i.e. calling remote_thread_info::set_resumed and set_running)
which are missing from extended_remote_target::attach. These missing
calls are what would serve to mark the new thread as resumed.
In this commit I propose that we add an extra parameter to
remote_target::remote_add_thread. This new parameter will force the
new thread to be added with a call to add_thread_silent. We can now
call remote_add_thread from the ::attach method, the extra
actions (listed above) will now be performed, and the thread will be
left in the correct state.
Additionally, in PR gdb/28405, a segfault is reported. This segfault
triggers when 'set debug remote 1' is used before trying to reproduce
the original assertion failure. The cause of this is in
remote_target::select_thread_for_ambiguous_stop_reply, where we do
this:
remote_debug_printf ("first resumed thread is %s",
pid_to_str (first_resumed_thread->ptid).c_str ());
remote_debug_printf ("is this guess ambiguous? = %d", ambiguous);
gdb_assert (first_resumed_thread != nullptr);
Notice that when debug printing is on we dereference
first_resumed_thread before we assert that the pointer is not
nullptr. This is the cause of the segfault, and is resolved by moving
the assert before the debug printing code.
I've extended an existing test, ext-attach.exp, so that the original
test is run multiple times; we run in the original mode, as normal,
but also, we now run with different packets disabled in gdbserver. In
particular, disabling Tthread would trigger the assertion as it was
reported in the original bug. I also run the test in all-stop and
non-stop modes now for extra coverage.
It was pointed out during review that I should also run the test with
'maint set target-async off', however, this currently doesn't
work (there are some test failures), a fix for these issues has been
posted here:
https://sourceware.org/pipermail/gdb-patches/2021-November/183712.html
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28405
diff --git a/gdb/remote.c b/gdb/remote.c
index abf63de622a..652b182138e 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -769,7 +769,8 @@ class remote_target : public process_stratum_target
void print_one_stopped_thread (thread_info *thread);
void process_initial_stop_replies (int from_tty);
- thread_info *remote_add_thread (ptid_t ptid, bool running, bool executing);
+ thread_info *remote_add_thread (ptid_t ptid, bool running, bool executing,
+ bool silent_p);
void btrace_sync_conf (const btrace_config *conf);
@@ -2528,10 +2529,13 @@ static remote_thread_info *get_remote_thread_info (remote_target *target,
ptid_t ptid);
/* Add thread PTID to GDB's thread list. Tag it as executing/running
- according to RUNNING. */
+ according to EXECUTING and RUNNING respectively. If SILENT_P (or the
+ remote_state::starting_up flag) is true then the new thread is added
+ silently, otherwise the new thread will be announced to the user. */
thread_info *
-remote_target::remote_add_thread (ptid_t ptid, bool running, bool executing)
+remote_target::remote_add_thread (ptid_t ptid, bool running, bool executing,
+ bool silent_p)
{
struct remote_state *rs = get_remote_state ();
struct thread_info *thread;
@@ -2542,7 +2546,7 @@ remote_target::remote_add_thread (ptid_t ptid, bool running, bool executing)
consider that a single-threaded target, mentioning a new thread
might be confusing to the user. Be silent then, preserving the
age old behavior. */
- if (rs->starting_up)
+ if (rs->starting_up || silent_p)
thread = add_thread_silent (this, ptid);
else
thread = add_thread (this, ptid);
@@ -2580,7 +2584,7 @@ remote_target::remote_notice_new_inferior (ptid_t currthread, bool executing)
{
/* We're seeing an event on a thread id we knew had exited.
This has to be a new thread reusing the old id. Add it. */
- remote_add_thread (currthread, running, executing);
+ remote_add_thread (currthread, running, executing, false);
return;
}
@@ -2602,7 +2606,7 @@ remote_target::remote_notice_new_inferior (ptid_t currthread, bool executing)
else
{
thread_info *thr
- = remote_add_thread (currthread, running, executing);
+ = remote_add_thread (currthread, running, executing, false);
switch_to_thread (thr);
}
return;
@@ -2634,7 +2638,7 @@ remote_target::remote_notice_new_inferior (ptid_t currthread, bool executing)
/* This is really a new thread. Add it. */
thread_info *new_thr
- = remote_add_thread (currthread, running, executing);
+ = remote_add_thread (currthread, running, executing, false);
/* If we found a new inferior, let the common code do whatever
it needs to with it (e.g., read shared libraries, insert
@@ -6067,14 +6071,11 @@ extended_remote_target::attach (const char *args, int from_tty)
ptid. */
ptid_t curr_ptid = remote_current_thread (ptid_t (pid));
- /* Add the main thread to the thread list. */
- thread_info *thr = add_thread_silent (this, curr_ptid);
+ /* Add the main thread to the thread list. We add the thread
+ silently in this case (the final true parameter). */
+ thread_info *thr = remote_add_thread (curr_ptid, true, true, true);
switch_to_thread (thr);
-
- /* Don't consider the thread stopped until we've processed the
- saved stop reply. */
- set_executing (this, thr->ptid, true);
}
/* Next, if the target can specify a description, read it. We do
@@ -7976,12 +7977,12 @@ remote_target::select_thread_for_ambiguous_stop_reply
ambiguous = true;
}
+ gdb_assert (first_resumed_thread != nullptr);
+
remote_debug_printf ("first resumed thread is %s",
pid_to_str (first_resumed_thread->ptid).c_str ());
remote_debug_printf ("is this guess ambiguous? = %d", ambiguous);
- gdb_assert (first_resumed_thread != nullptr);
-
/* Warn if the remote target is sending ambiguous stop replies. */
if (ambiguous)
{
diff --git a/gdb/testsuite/gdb.server/ext-attach.exp b/gdb/testsuite/gdb.server/ext-attach.exp
index c9766e35317..75b33718545 100644
--- a/gdb/testsuite/gdb.server/ext-attach.exp
+++ b/gdb/testsuite/gdb.server/ext-attach.exp
@@ -30,53 +30,79 @@ if {![can_spawn_for_attach]} {
return 0
}
-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 {[build_executable "failed to prepare" $testfile $srcfile debug]} {
+ return -1
+}
- if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
- return -1
+# Run the test. TARGET_NON_STOP and TARGET_ASYNC should be 'on'
+# or 'off'. TO_DISABLE should be either the empty string, or
+# something that can be passed to gdbserver's --disable-packet command
+# line option.
+proc run_test { target_async target_non_stop to_disable } {
+ save_vars { ::GDBFLAGS } {
+ append ::GDBFLAGS " -ex \"maint set target-non-stop $target_non_stop\""
+ append ::GDBFLAGS " -ex \"maintenance set target-async ${target_async}\""
+
+ # 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\""
+ }
+
+ clean_restart $::binfile
}
-}
-# Make sure we're disconnected, in case we're testing with an
-# extended-remote board, therefore already connected.
-gdb_test "disconnect" ".*"
+ # 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]
-gdbserver_start_extended
+ if { [gdb_target_supports_trace] } then {
+ # Test predefined TSVs are uploaded.
+ gdb_test_sequence "info tvariables" "check uploaded tsv" {
+ "\[\r\n\]+Name\[\t \]+Initial\[\t \]+Current"
+ "\[\r\n\]+\\\$trace_timestamp 0"
+ }
+ }
-gdb_test_no_output "set remote exec-file $target_exec" "set remote exec-file"
+ set target_exec [gdbserver_download_current_prog]
+ if { $to_disable != "" } {
+ set gdbserver_opts "--disable-packet=${to_disable}"
+ } else {
+ set gdbserver_opts ""
+ }
+ gdbserver_start_extended $gdbserver_opts
-set test_spawn_id [spawn_wait_for_attach $binfile]
-set testpid [spawn_id_get_pid $test_spawn_id]
+ gdb_test_no_output "set remote exec-file $target_exec" "set remote exec-file"
-gdb_test "attach $testpid" \
- "Attaching to program: .*, process $testpid.*(in|at).*" \
- "attach to remote program 1"
+ set test_spawn_id [spawn_wait_for_attach $::binfile]
+ set testpid [spawn_id_get_pid $test_spawn_id]
-if { [gdb_target_supports_trace] } then {
- # Test predefined TSVs are uploaded.
- gdb_test_sequence "info tvariables" "check uploaded tsv" {
- "\[\r\n\]+Name\[\t \]+Initial\[\t \]+Current"
- "\[\r\n\]+\\\$trace_timestamp 0"
- }
-}
+ gdb_test "attach $testpid" \
+ "Attaching to program: .*, process $testpid.*(in|at).*" \
+ "attach to remote program 1"
+
+ gdb_test "backtrace" ".*main.*" "backtrace 1"
-gdb_test "backtrace" ".*main.*" "backtrace 1"
+ gdb_test "detach" "Detaching from program.*process.*"
+ gdb_test "backtrace" "No stack\\." "backtrace with no program"
-gdb_test "detach" "Detaching from program.*process.*"
-gdb_test "backtrace" "No stack\\." "backtrace with no program"
+ gdb_test "attach $testpid" \
+ "Attaching to program: .*, process $testpid.*(in|at).*" \
+ "attach to remote program 2"
+ gdb_test "backtrace" ".*main.*" "backtrace 2"
-gdb_test "attach $testpid" \
- "Attaching to program: .*, process $testpid.*(in|at).*" \
- "attach to remote program 2"
-gdb_test "backtrace" ".*main.*" "backtrace 2"
+ gdb_test "kill" "" "kill" "Kill the program being debugged. .y or n. " "y"
+ gdb_test_no_output "monitor exit"
-gdb_test "kill" "" "kill" "Kill the program being debugged. .y or n. " "y"
-gdb_test_no_output "monitor exit"
+ kill_wait_spawned_process $test_spawn_id
+}
-kill_wait_spawned_process $test_spawn_id
+# Currently, this test is only run with 'target-async on' as setting
+# this off highlights some test failures.
+foreach_with_prefix target_async {"on"} {
+ foreach_with_prefix target_non_stop {"off" "on"} {
+ foreach_with_prefix to_disable { "" Tthread T } {
+ run_test ${target_async} ${target_non_stop} $to_disable
+ }
+ }
+}
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCHv2] gdb/remote: handle attach when stop packet lacks thread-id
2021-11-23 14:24 ` [PATCHv2] " Andrew Burgess
@ 2021-12-04 11:40 ` Joel Brobecker
2021-12-18 10:48 ` Joel Brobecker
2021-12-18 11:15 ` [PATCHv3] " Andrew Burgess
2 siblings, 0 replies; 8+ messages in thread
From: Joel Brobecker @ 2021-12-04 11:40 UTC (permalink / raw)
To: Andrew Burgess via Gdb-patches, simon.marchi; +Cc: Joel Brobecker
Hi Simon,
This fix is listed as one of the desirable ones for 11.2, so
I was wondering if you had a bit of time to take a look at it
again?
Irrespective of whether the patch is good in its current form or not,
do you think there is a reasonable change that the final version
is going to be safe enough for a .2 (corrective) release?
Thank you!
On Tue, Nov 23, 2021 at 02:24:43PM +0000, Andrew Burgess via Gdb-patches wrote:
> * Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2021-11-14 19:50:01 -0500]:
>
> >
> >
> > On 2021-10-05 05:04, Andrew Burgess wrote:
> > > Bug PR gdb/28405 reports a regression when using attach with an
> > > extended-remote target. In this case the target is not including a
> > > thread-id in the stop packet it sends back after the attach.
> > >
> > > The regression was introduced with this commit:
> > >
> > > commit 8f66807b98f7634c43149ea62e454ea8f877691d
> > > Date: Wed Jan 13 20:26:58 2021 -0500
> > >
> > > gdb: better handling of 'S' packets
> > >
> > > The problem is that when GDB processes the stop packet, it sees that
> > > there is no thread-id and so has to "guess" which thread the stop
> > > should apply to.
> > >
> > > In this case the target only has one thread, so really, there's no
> > > guessing needed, but GDB still runs through the same process, this
> > > shouldn't cause us any problems.
> > >
> > > However, after the above commit, GDB now expects itself to be more
> > > internally consistent, specifically, only a thread that GDB thinks is
> > > resumed, can be a candidate for having stopped.
> > >
> > > It turns out that, when GDB attaches to a process through an
> > > extended-remote target, the threads of the process being attached too,
> > > are not, initially, marked as resumed.
> > >
> > > And so, when GDB tries to figure out which thread the stop might apply
> > > too, it finds no threads in the processes marked resumed, and so an
> > > assert triggers.
> > >
> > > In extended_remote_target::attach we create a new thread with a call
> > > to add_thread_silent, rather than remote_target::remote_add_thread,
> > > the reason is that calling the latter will result in a call to
> > > 'add_thread' rather than 'add_thread_silent'. However,
> > > remote_target::remote_add_thread includes additional
> > > actions (i.e. calling remote_thread_info::set_resumed and set_running)
> > > which are missing from extended_remote_target::attach. These missing
> > > calls are what would serve to mark the new thread as resumed.
> > >
> > > In this commit, I propose that we add the extra calls into
> > > extended_remote_target::attach, this solves the problem at hand.
> >
> > To avoid duplication, did you consider calling remote_add_thread, but
> > add some more parameters to have the caller have control over the
> > thread's initial state?
>
> I've made this change in the patch below.
>
> >
> > But the way to resolve the problem (marking the initial thread as
> > resumed) sounds fine.
> >
> > While reading the code, I saw there is an additional code path for
> > "target_can_async_p false". I suppose this can be exercised with "maint
> > set target-async off", it might be useful to add an axis for that in
> > your test?
>
> Sigh! So I tried running with 'maint set target-async off'. And went
> down the rabbit hole of debugging non-async mode. See:
>
> https://sourceware.org/pipermail/gdb-patches/2021-November/183712.html
>
> I've added this to the test for now:
>
> # Currently, this test is only run with 'target-async on' as setting
> # this off highlights some test failures.
> foreach_with_prefix target_async {"on"} {
> ...
> }
>
> If/when the series I linked above is merged then that foreach loop can
> be expanded to include both "off" and "on".
>
> My intention would be that this patch not be merged to master until
> the series I linked above is merged, but I'm posting this revised
> patch here as this was raised as a possible candidate for 11.2. And
> as the non-async testing really has nothing to do with the core of
> this patch we could if we wanted, merge this version of the patch to
> the gdb-11 branch, then later, once the async fixes I linked above are
> merged, push a version of this patch with the full testing to master.
>
> Thoughts welcome,
>
> Thanks,
> Andrew
>
> ---
>
> commit 1aaa4dfbb8ce50cd64375394077ee5dc854ebba9
> Author: Andrew Burgess <andrew.burgess@embecosm.com>
> Date: Mon Oct 4 15:48:11 2021 +0100
>
> gdb/remote: handle attach when stop packet lacks thread-id
>
> Bug PR gdb/28405 reports a regression when using attach with an
> extended-remote target. In this case the target is not including a
> thread-id in the stop packet it sends back after the attach.
>
> The regression was introduced with this commit:
>
> commit 8f66807b98f7634c43149ea62e454ea8f877691d
> Date: Wed Jan 13 20:26:58 2021 -0500
>
> gdb: better handling of 'S' packets
>
> The problem is that when GDB processes the stop packet, it sees that
> there is no thread-id and so has to "guess" which thread the stop
> should apply to.
>
> In this case the target only has one thread, so really, there's no
> guessing needed, but GDB still runs through the same process, this
> shouldn't cause us any problems.
>
> However, after the above commit, GDB now expects itself to be more
> internally consistent, specifically, only a thread that GDB thinks is
> resumed, can be a candidate for having stopped.
>
> It turns out that, when GDB attaches to a process through an
> extended-remote target, the threads of the process being attached too,
> are not, initially, marked as resumed.
>
> And so, when GDB tries to figure out which thread the stop might apply
> too, it finds no threads in the processes marked resumed, and so an
> assert triggers.
>
> In extended_remote_target::attach we create a new thread with a call
> to add_thread_silent, rather than remote_target::remote_add_thread,
> the reason is that calling the latter will result in a call to
> 'add_thread' rather than 'add_thread_silent'. However,
> remote_target::remote_add_thread includes additional
> actions (i.e. calling remote_thread_info::set_resumed and set_running)
> which are missing from extended_remote_target::attach. These missing
> calls are what would serve to mark the new thread as resumed.
>
> In this commit I propose that we add an extra parameter to
> remote_target::remote_add_thread. This new parameter will force the
> new thread to be added with a call to add_thread_silent. We can now
> call remote_add_thread from the ::attach method, the extra
> actions (listed above) will now be performed, and the thread will be
> left in the correct state.
>
> Additionally, in PR gdb/28405, a segfault is reported. This segfault
> triggers when 'set debug remote 1' is used before trying to reproduce
> the original assertion failure. The cause of this is in
> remote_target::select_thread_for_ambiguous_stop_reply, where we do
> this:
>
> remote_debug_printf ("first resumed thread is %s",
> pid_to_str (first_resumed_thread->ptid).c_str ());
> remote_debug_printf ("is this guess ambiguous? = %d", ambiguous);
>
> gdb_assert (first_resumed_thread != nullptr);
>
> Notice that when debug printing is on we dereference
> first_resumed_thread before we assert that the pointer is not
> nullptr. This is the cause of the segfault, and is resolved by moving
> the assert before the debug printing code.
>
> I've extended an existing test, ext-attach.exp, so that the original
> test is run multiple times; we run in the original mode, as normal,
> but also, we now run with different packets disabled in gdbserver. In
> particular, disabling Tthread would trigger the assertion as it was
> reported in the original bug. I also run the test in all-stop and
> non-stop modes now for extra coverage.
>
> It was pointed out during review that I should also run the test with
> 'maint set target-async off', however, this currently doesn't
> work (there are some test failures), a fix for these issues has been
> posted here:
>
> https://sourceware.org/pipermail/gdb-patches/2021-November/183712.html
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28405
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index abf63de622a..652b182138e 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -769,7 +769,8 @@ class remote_target : public process_stratum_target
> void print_one_stopped_thread (thread_info *thread);
> void process_initial_stop_replies (int from_tty);
>
> - thread_info *remote_add_thread (ptid_t ptid, bool running, bool executing);
> + thread_info *remote_add_thread (ptid_t ptid, bool running, bool executing,
> + bool silent_p);
>
> void btrace_sync_conf (const btrace_config *conf);
>
> @@ -2528,10 +2529,13 @@ static remote_thread_info *get_remote_thread_info (remote_target *target,
> ptid_t ptid);
>
> /* Add thread PTID to GDB's thread list. Tag it as executing/running
> - according to RUNNING. */
> + according to EXECUTING and RUNNING respectively. If SILENT_P (or the
> + remote_state::starting_up flag) is true then the new thread is added
> + silently, otherwise the new thread will be announced to the user. */
>
> thread_info *
> -remote_target::remote_add_thread (ptid_t ptid, bool running, bool executing)
> +remote_target::remote_add_thread (ptid_t ptid, bool running, bool executing,
> + bool silent_p)
> {
> struct remote_state *rs = get_remote_state ();
> struct thread_info *thread;
> @@ -2542,7 +2546,7 @@ remote_target::remote_add_thread (ptid_t ptid, bool running, bool executing)
> consider that a single-threaded target, mentioning a new thread
> might be confusing to the user. Be silent then, preserving the
> age old behavior. */
> - if (rs->starting_up)
> + if (rs->starting_up || silent_p)
> thread = add_thread_silent (this, ptid);
> else
> thread = add_thread (this, ptid);
> @@ -2580,7 +2584,7 @@ remote_target::remote_notice_new_inferior (ptid_t currthread, bool executing)
> {
> /* We're seeing an event on a thread id we knew had exited.
> This has to be a new thread reusing the old id. Add it. */
> - remote_add_thread (currthread, running, executing);
> + remote_add_thread (currthread, running, executing, false);
> return;
> }
>
> @@ -2602,7 +2606,7 @@ remote_target::remote_notice_new_inferior (ptid_t currthread, bool executing)
> else
> {
> thread_info *thr
> - = remote_add_thread (currthread, running, executing);
> + = remote_add_thread (currthread, running, executing, false);
> switch_to_thread (thr);
> }
> return;
> @@ -2634,7 +2638,7 @@ remote_target::remote_notice_new_inferior (ptid_t currthread, bool executing)
>
> /* This is really a new thread. Add it. */
> thread_info *new_thr
> - = remote_add_thread (currthread, running, executing);
> + = remote_add_thread (currthread, running, executing, false);
>
> /* If we found a new inferior, let the common code do whatever
> it needs to with it (e.g., read shared libraries, insert
> @@ -6067,14 +6071,11 @@ extended_remote_target::attach (const char *args, int from_tty)
> ptid. */
> ptid_t curr_ptid = remote_current_thread (ptid_t (pid));
>
> - /* Add the main thread to the thread list. */
> - thread_info *thr = add_thread_silent (this, curr_ptid);
> + /* Add the main thread to the thread list. We add the thread
> + silently in this case (the final true parameter). */
> + thread_info *thr = remote_add_thread (curr_ptid, true, true, true);
>
> switch_to_thread (thr);
> -
> - /* Don't consider the thread stopped until we've processed the
> - saved stop reply. */
> - set_executing (this, thr->ptid, true);
> }
>
> /* Next, if the target can specify a description, read it. We do
> @@ -7976,12 +7977,12 @@ remote_target::select_thread_for_ambiguous_stop_reply
> ambiguous = true;
> }
>
> + gdb_assert (first_resumed_thread != nullptr);
> +
> remote_debug_printf ("first resumed thread is %s",
> pid_to_str (first_resumed_thread->ptid).c_str ());
> remote_debug_printf ("is this guess ambiguous? = %d", ambiguous);
>
> - gdb_assert (first_resumed_thread != nullptr);
> -
> /* Warn if the remote target is sending ambiguous stop replies. */
> if (ambiguous)
> {
> diff --git a/gdb/testsuite/gdb.server/ext-attach.exp b/gdb/testsuite/gdb.server/ext-attach.exp
> index c9766e35317..75b33718545 100644
> --- a/gdb/testsuite/gdb.server/ext-attach.exp
> +++ b/gdb/testsuite/gdb.server/ext-attach.exp
> @@ -30,53 +30,79 @@ if {![can_spawn_for_attach]} {
> return 0
> }
>
> -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 {[build_executable "failed to prepare" $testfile $srcfile debug]} {
> + return -1
> +}
>
> - if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
> - return -1
> +# Run the test. TARGET_NON_STOP and TARGET_ASYNC should be 'on'
> +# or 'off'. TO_DISABLE should be either the empty string, or
> +# something that can be passed to gdbserver's --disable-packet command
> +# line option.
> +proc run_test { target_async target_non_stop to_disable } {
> + save_vars { ::GDBFLAGS } {
> + append ::GDBFLAGS " -ex \"maint set target-non-stop $target_non_stop\""
> + append ::GDBFLAGS " -ex \"maintenance set target-async ${target_async}\""
> +
> + # 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\""
> + }
> +
> + clean_restart $::binfile
> }
> -}
>
> -# Make sure we're disconnected, in case we're testing with an
> -# extended-remote board, therefore already connected.
> -gdb_test "disconnect" ".*"
> + # 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]
> -gdbserver_start_extended
> + if { [gdb_target_supports_trace] } then {
> + # Test predefined TSVs are uploaded.
> + gdb_test_sequence "info tvariables" "check uploaded tsv" {
> + "\[\r\n\]+Name\[\t \]+Initial\[\t \]+Current"
> + "\[\r\n\]+\\\$trace_timestamp 0"
> + }
> + }
>
> -gdb_test_no_output "set remote exec-file $target_exec" "set remote exec-file"
> + set target_exec [gdbserver_download_current_prog]
> + if { $to_disable != "" } {
> + set gdbserver_opts "--disable-packet=${to_disable}"
> + } else {
> + set gdbserver_opts ""
> + }
> + gdbserver_start_extended $gdbserver_opts
>
> -set test_spawn_id [spawn_wait_for_attach $binfile]
> -set testpid [spawn_id_get_pid $test_spawn_id]
> + gdb_test_no_output "set remote exec-file $target_exec" "set remote exec-file"
>
> -gdb_test "attach $testpid" \
> - "Attaching to program: .*, process $testpid.*(in|at).*" \
> - "attach to remote program 1"
> + set test_spawn_id [spawn_wait_for_attach $::binfile]
> + set testpid [spawn_id_get_pid $test_spawn_id]
>
> -if { [gdb_target_supports_trace] } then {
> - # Test predefined TSVs are uploaded.
> - gdb_test_sequence "info tvariables" "check uploaded tsv" {
> - "\[\r\n\]+Name\[\t \]+Initial\[\t \]+Current"
> - "\[\r\n\]+\\\$trace_timestamp 0"
> - }
> -}
> + gdb_test "attach $testpid" \
> + "Attaching to program: .*, process $testpid.*(in|at).*" \
> + "attach to remote program 1"
> +
> + gdb_test "backtrace" ".*main.*" "backtrace 1"
>
> -gdb_test "backtrace" ".*main.*" "backtrace 1"
> + gdb_test "detach" "Detaching from program.*process.*"
> + gdb_test "backtrace" "No stack\\." "backtrace with no program"
>
> -gdb_test "detach" "Detaching from program.*process.*"
> -gdb_test "backtrace" "No stack\\." "backtrace with no program"
> + gdb_test "attach $testpid" \
> + "Attaching to program: .*, process $testpid.*(in|at).*" \
> + "attach to remote program 2"
> + gdb_test "backtrace" ".*main.*" "backtrace 2"
>
> -gdb_test "attach $testpid" \
> - "Attaching to program: .*, process $testpid.*(in|at).*" \
> - "attach to remote program 2"
> -gdb_test "backtrace" ".*main.*" "backtrace 2"
> + gdb_test "kill" "" "kill" "Kill the program being debugged. .y or n. " "y"
> + gdb_test_no_output "monitor exit"
>
> -gdb_test "kill" "" "kill" "Kill the program being debugged. .y or n. " "y"
> -gdb_test_no_output "monitor exit"
> + kill_wait_spawned_process $test_spawn_id
> +}
>
> -kill_wait_spawned_process $test_spawn_id
> +# Currently, this test is only run with 'target-async on' as setting
> +# this off highlights some test failures.
> +foreach_with_prefix target_async {"on"} {
> + foreach_with_prefix target_non_stop {"off" "on"} {
> + foreach_with_prefix to_disable { "" Tthread T } {
> + run_test ${target_async} ${target_non_stop} $to_disable
> + }
> + }
> +}
>
--
Joel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCHv2] gdb/remote: handle attach when stop packet lacks thread-id
2021-11-23 14:24 ` [PATCHv2] " Andrew Burgess
2021-12-04 11:40 ` Joel Brobecker
@ 2021-12-18 10:48 ` Joel Brobecker
2021-12-18 11:15 ` [PATCHv3] " Andrew Burgess
2 siblings, 0 replies; 8+ messages in thread
From: Joel Brobecker @ 2021-12-18 10:48 UTC (permalink / raw)
To: Andrew Burgess via Gdb-patches; +Cc: Joel Brobecker
Hello everyone,
> My intention would be that this patch not be merged to master until
> the series I linked above is merged, but I'm posting this revised
> patch here as this was raised as a possible candidate for 11.2. And
> as the non-async testing really has nothing to do with the core of
> this patch we could if we wanted, merge this version of the patch to
> the gdb-11 branch, then later, once the async fixes I linked above are
> merged, push a version of this patch with the full testing to master.
I think we need to decide what we want to do with this patch
with respect to the 11.2 release.
I looked it over, and the patch looks fairly simple and localized,
and the patch makes sense to me, so I would be OK myself with
having it being pushed to the gdb-11-branch, while we wait for it
to be pushed to master. On the other hand, I'm no remote.c expert,
so it would be nice if this patch had the go ahead from someone
more versed on it.
This is the last issue we're waiting a resolution for before we can
release GDB 11.2. I'm happy to wait further, but I think we should
try to release soon, and one option would be to go without this fix
if deemed too risky or too late.
> commit 1aaa4dfbb8ce50cd64375394077ee5dc854ebba9
> Author: Andrew Burgess <andrew.burgess@embecosm.com>
> Date: Mon Oct 4 15:48:11 2021 +0100
>
> gdb/remote: handle attach when stop packet lacks thread-id
>
> Bug PR gdb/28405 reports a regression when using attach with an
> extended-remote target. In this case the target is not including a
> thread-id in the stop packet it sends back after the attach.
>
> The regression was introduced with this commit:
>
> commit 8f66807b98f7634c43149ea62e454ea8f877691d
> Date: Wed Jan 13 20:26:58 2021 -0500
>
> gdb: better handling of 'S' packets
>
> The problem is that when GDB processes the stop packet, it sees that
> there is no thread-id and so has to "guess" which thread the stop
> should apply to.
>
> In this case the target only has one thread, so really, there's no
> guessing needed, but GDB still runs through the same process, this
> shouldn't cause us any problems.
>
> However, after the above commit, GDB now expects itself to be more
> internally consistent, specifically, only a thread that GDB thinks is
> resumed, can be a candidate for having stopped.
>
> It turns out that, when GDB attaches to a process through an
> extended-remote target, the threads of the process being attached too,
> are not, initially, marked as resumed.
>
> And so, when GDB tries to figure out which thread the stop might apply
> too, it finds no threads in the processes marked resumed, and so an
> assert triggers.
>
> In extended_remote_target::attach we create a new thread with a call
> to add_thread_silent, rather than remote_target::remote_add_thread,
> the reason is that calling the latter will result in a call to
> 'add_thread' rather than 'add_thread_silent'. However,
> remote_target::remote_add_thread includes additional
> actions (i.e. calling remote_thread_info::set_resumed and set_running)
> which are missing from extended_remote_target::attach. These missing
> calls are what would serve to mark the new thread as resumed.
>
> In this commit I propose that we add an extra parameter to
> remote_target::remote_add_thread. This new parameter will force the
> new thread to be added with a call to add_thread_silent. We can now
> call remote_add_thread from the ::attach method, the extra
> actions (listed above) will now be performed, and the thread will be
> left in the correct state.
>
> Additionally, in PR gdb/28405, a segfault is reported. This segfault
> triggers when 'set debug remote 1' is used before trying to reproduce
> the original assertion failure. The cause of this is in
> remote_target::select_thread_for_ambiguous_stop_reply, where we do
> this:
>
> remote_debug_printf ("first resumed thread is %s",
> pid_to_str (first_resumed_thread->ptid).c_str ());
> remote_debug_printf ("is this guess ambiguous? = %d", ambiguous);
>
> gdb_assert (first_resumed_thread != nullptr);
>
> Notice that when debug printing is on we dereference
> first_resumed_thread before we assert that the pointer is not
> nullptr. This is the cause of the segfault, and is resolved by moving
> the assert before the debug printing code.
>
> I've extended an existing test, ext-attach.exp, so that the original
> test is run multiple times; we run in the original mode, as normal,
> but also, we now run with different packets disabled in gdbserver. In
> particular, disabling Tthread would trigger the assertion as it was
> reported in the original bug. I also run the test in all-stop and
> non-stop modes now for extra coverage.
>
> It was pointed out during review that I should also run the test with
> 'maint set target-async off', however, this currently doesn't
> work (there are some test failures), a fix for these issues has been
> posted here:
>
> https://sourceware.org/pipermail/gdb-patches/2021-November/183712.html
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28405
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index abf63de622a..652b182138e 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -769,7 +769,8 @@ class remote_target : public process_stratum_target
> void print_one_stopped_thread (thread_info *thread);
> void process_initial_stop_replies (int from_tty);
>
> - thread_info *remote_add_thread (ptid_t ptid, bool running, bool executing);
> + thread_info *remote_add_thread (ptid_t ptid, bool running, bool executing,
> + bool silent_p);
>
> void btrace_sync_conf (const btrace_config *conf);
>
> @@ -2528,10 +2529,13 @@ static remote_thread_info *get_remote_thread_info (remote_target *target,
> ptid_t ptid);
>
> /* Add thread PTID to GDB's thread list. Tag it as executing/running
> - according to RUNNING. */
> + according to EXECUTING and RUNNING respectively. If SILENT_P (or the
> + remote_state::starting_up flag) is true then the new thread is added
> + silently, otherwise the new thread will be announced to the user. */
>
> thread_info *
> -remote_target::remote_add_thread (ptid_t ptid, bool running, bool executing)
> +remote_target::remote_add_thread (ptid_t ptid, bool running, bool executing,
> + bool silent_p)
> {
> struct remote_state *rs = get_remote_state ();
> struct thread_info *thread;
> @@ -2542,7 +2546,7 @@ remote_target::remote_add_thread (ptid_t ptid, bool running, bool executing)
> consider that a single-threaded target, mentioning a new thread
> might be confusing to the user. Be silent then, preserving the
> age old behavior. */
> - if (rs->starting_up)
> + if (rs->starting_up || silent_p)
> thread = add_thread_silent (this, ptid);
> else
> thread = add_thread (this, ptid);
> @@ -2580,7 +2584,7 @@ remote_target::remote_notice_new_inferior (ptid_t currthread, bool executing)
> {
> /* We're seeing an event on a thread id we knew had exited.
> This has to be a new thread reusing the old id. Add it. */
> - remote_add_thread (currthread, running, executing);
> + remote_add_thread (currthread, running, executing, false);
> return;
> }
>
> @@ -2602,7 +2606,7 @@ remote_target::remote_notice_new_inferior (ptid_t currthread, bool executing)
> else
> {
> thread_info *thr
> - = remote_add_thread (currthread, running, executing);
> + = remote_add_thread (currthread, running, executing, false);
> switch_to_thread (thr);
> }
> return;
> @@ -2634,7 +2638,7 @@ remote_target::remote_notice_new_inferior (ptid_t currthread, bool executing)
>
> /* This is really a new thread. Add it. */
> thread_info *new_thr
> - = remote_add_thread (currthread, running, executing);
> + = remote_add_thread (currthread, running, executing, false);
>
> /* If we found a new inferior, let the common code do whatever
> it needs to with it (e.g., read shared libraries, insert
> @@ -6067,14 +6071,11 @@ extended_remote_target::attach (const char *args, int from_tty)
> ptid. */
> ptid_t curr_ptid = remote_current_thread (ptid_t (pid));
>
> - /* Add the main thread to the thread list. */
> - thread_info *thr = add_thread_silent (this, curr_ptid);
> + /* Add the main thread to the thread list. We add the thread
> + silently in this case (the final true parameter). */
> + thread_info *thr = remote_add_thread (curr_ptid, true, true, true);
>
> switch_to_thread (thr);
> -
> - /* Don't consider the thread stopped until we've processed the
> - saved stop reply. */
> - set_executing (this, thr->ptid, true);
> }
>
> /* Next, if the target can specify a description, read it. We do
> @@ -7976,12 +7977,12 @@ remote_target::select_thread_for_ambiguous_stop_reply
> ambiguous = true;
> }
>
> + gdb_assert (first_resumed_thread != nullptr);
> +
> remote_debug_printf ("first resumed thread is %s",
> pid_to_str (first_resumed_thread->ptid).c_str ());
> remote_debug_printf ("is this guess ambiguous? = %d", ambiguous);
>
> - gdb_assert (first_resumed_thread != nullptr);
> -
> /* Warn if the remote target is sending ambiguous stop replies. */
> if (ambiguous)
> {
> diff --git a/gdb/testsuite/gdb.server/ext-attach.exp b/gdb/testsuite/gdb.server/ext-attach.exp
> index c9766e35317..75b33718545 100644
> --- a/gdb/testsuite/gdb.server/ext-attach.exp
> +++ b/gdb/testsuite/gdb.server/ext-attach.exp
> @@ -30,53 +30,79 @@ if {![can_spawn_for_attach]} {
> return 0
> }
>
> -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 {[build_executable "failed to prepare" $testfile $srcfile debug]} {
> + return -1
> +}
>
> - if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
> - return -1
> +# Run the test. TARGET_NON_STOP and TARGET_ASYNC should be 'on'
> +# or 'off'. TO_DISABLE should be either the empty string, or
> +# something that can be passed to gdbserver's --disable-packet command
> +# line option.
> +proc run_test { target_async target_non_stop to_disable } {
> + save_vars { ::GDBFLAGS } {
> + append ::GDBFLAGS " -ex \"maint set target-non-stop $target_non_stop\""
> + append ::GDBFLAGS " -ex \"maintenance set target-async ${target_async}\""
> +
> + # 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\""
> + }
> +
> + clean_restart $::binfile
> }
> -}
>
> -# Make sure we're disconnected, in case we're testing with an
> -# extended-remote board, therefore already connected.
> -gdb_test "disconnect" ".*"
> + # 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]
> -gdbserver_start_extended
> + if { [gdb_target_supports_trace] } then {
> + # Test predefined TSVs are uploaded.
> + gdb_test_sequence "info tvariables" "check uploaded tsv" {
> + "\[\r\n\]+Name\[\t \]+Initial\[\t \]+Current"
> + "\[\r\n\]+\\\$trace_timestamp 0"
> + }
> + }
>
> -gdb_test_no_output "set remote exec-file $target_exec" "set remote exec-file"
> + set target_exec [gdbserver_download_current_prog]
> + if { $to_disable != "" } {
> + set gdbserver_opts "--disable-packet=${to_disable}"
> + } else {
> + set gdbserver_opts ""
> + }
> + gdbserver_start_extended $gdbserver_opts
>
> -set test_spawn_id [spawn_wait_for_attach $binfile]
> -set testpid [spawn_id_get_pid $test_spawn_id]
> + gdb_test_no_output "set remote exec-file $target_exec" "set remote exec-file"
>
> -gdb_test "attach $testpid" \
> - "Attaching to program: .*, process $testpid.*(in|at).*" \
> - "attach to remote program 1"
> + set test_spawn_id [spawn_wait_for_attach $::binfile]
> + set testpid [spawn_id_get_pid $test_spawn_id]
>
> -if { [gdb_target_supports_trace] } then {
> - # Test predefined TSVs are uploaded.
> - gdb_test_sequence "info tvariables" "check uploaded tsv" {
> - "\[\r\n\]+Name\[\t \]+Initial\[\t \]+Current"
> - "\[\r\n\]+\\\$trace_timestamp 0"
> - }
> -}
> + gdb_test "attach $testpid" \
> + "Attaching to program: .*, process $testpid.*(in|at).*" \
> + "attach to remote program 1"
> +
> + gdb_test "backtrace" ".*main.*" "backtrace 1"
>
> -gdb_test "backtrace" ".*main.*" "backtrace 1"
> + gdb_test "detach" "Detaching from program.*process.*"
> + gdb_test "backtrace" "No stack\\." "backtrace with no program"
>
> -gdb_test "detach" "Detaching from program.*process.*"
> -gdb_test "backtrace" "No stack\\." "backtrace with no program"
> + gdb_test "attach $testpid" \
> + "Attaching to program: .*, process $testpid.*(in|at).*" \
> + "attach to remote program 2"
> + gdb_test "backtrace" ".*main.*" "backtrace 2"
>
> -gdb_test "attach $testpid" \
> - "Attaching to program: .*, process $testpid.*(in|at).*" \
> - "attach to remote program 2"
> -gdb_test "backtrace" ".*main.*" "backtrace 2"
> + gdb_test "kill" "" "kill" "Kill the program being debugged. .y or n. " "y"
> + gdb_test_no_output "monitor exit"
>
> -gdb_test "kill" "" "kill" "Kill the program being debugged. .y or n. " "y"
> -gdb_test_no_output "monitor exit"
> + kill_wait_spawned_process $test_spawn_id
> +}
>
> -kill_wait_spawned_process $test_spawn_id
> +# Currently, this test is only run with 'target-async on' as setting
> +# this off highlights some test failures.
> +foreach_with_prefix target_async {"on"} {
> + foreach_with_prefix target_non_stop {"off" "on"} {
> + foreach_with_prefix to_disable { "" Tthread T } {
> + run_test ${target_async} ${target_non_stop} $to_disable
> + }
> + }
> +}
>
--
Joel
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCHv3] gdb/remote: handle attach when stop packet lacks thread-id
2021-11-23 14:24 ` [PATCHv2] " Andrew Burgess
2021-12-04 11:40 ` Joel Brobecker
2021-12-18 10:48 ` Joel Brobecker
@ 2021-12-18 11:15 ` Andrew Burgess
2021-12-23 12:22 ` Andrew Burgess
2 siblings, 1 reply; 8+ messages in thread
From: Andrew Burgess @ 2021-12-18 11:15 UTC (permalink / raw)
To: gdb-patches
* Andrew Burgess <aburgess@redhat.com> [2021-11-23 14:24:43 +0000]:
> * Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2021-11-14 19:50:01 -0500]:
>
> >
> >
> > On 2021-10-05 05:04, Andrew Burgess wrote:
> > > Bug PR gdb/28405 reports a regression when using attach with an
> > > extended-remote target. In this case the target is not including a
> > > thread-id in the stop packet it sends back after the attach.
> > >
> > > The regression was introduced with this commit:
> > >
> > > commit 8f66807b98f7634c43149ea62e454ea8f877691d
> > > Date: Wed Jan 13 20:26:58 2021 -0500
> > >
> > > gdb: better handling of 'S' packets
> > >
> > > The problem is that when GDB processes the stop packet, it sees that
> > > there is no thread-id and so has to "guess" which thread the stop
> > > should apply to.
> > >
> > > In this case the target only has one thread, so really, there's no
> > > guessing needed, but GDB still runs through the same process, this
> > > shouldn't cause us any problems.
> > >
> > > However, after the above commit, GDB now expects itself to be more
> > > internally consistent, specifically, only a thread that GDB thinks is
> > > resumed, can be a candidate for having stopped.
> > >
> > > It turns out that, when GDB attaches to a process through an
> > > extended-remote target, the threads of the process being attached too,
> > > are not, initially, marked as resumed.
> > >
> > > And so, when GDB tries to figure out which thread the stop might apply
> > > too, it finds no threads in the processes marked resumed, and so an
> > > assert triggers.
> > >
> > > In extended_remote_target::attach we create a new thread with a call
> > > to add_thread_silent, rather than remote_target::remote_add_thread,
> > > the reason is that calling the latter will result in a call to
> > > 'add_thread' rather than 'add_thread_silent'. However,
> > > remote_target::remote_add_thread includes additional
> > > actions (i.e. calling remote_thread_info::set_resumed and set_running)
> > > which are missing from extended_remote_target::attach. These missing
> > > calls are what would serve to mark the new thread as resumed.
> > >
> > > In this commit, I propose that we add the extra calls into
> > > extended_remote_target::attach, this solves the problem at hand.
> >
> > To avoid duplication, did you consider calling remote_add_thread, but
> > add some more parameters to have the caller have control over the
> > thread's initial state?
>
> I've made this change in the patch below.
>
> >
> > But the way to resolve the problem (marking the initial thread as
> > resumed) sounds fine.
> >
> > While reading the code, I saw there is an additional code path for
> > "target_can_async_p false". I suppose this can be exercised with "maint
> > set target-async off", it might be useful to add an axis for that in
> > your test?
>
> Sigh! So I tried running with 'maint set target-async off'. And went
> down the rabbit hole of debugging non-async mode. See:
>
> https://sourceware.org/pipermail/gdb-patches/2021-November/183712.html
>
> I've added this to the test for now:
>
> # Currently, this test is only run with 'target-async on' as setting
> # this off highlights some test failures.
> foreach_with_prefix target_async {"on"} {
> ...
> }
>
> If/when the series I linked above is merged then that foreach loop can
> be expanded to include both "off" and "on".
The above patch to fix the "target-async off" issues has now been
merged. Below is an updated version of the patch that includes the
full test coverage.
The changes within GDB itself are unchanged since v2.
Thanks,
Andrew
---
commit 24654176f35e189575bb4488cb44676ca48f62a1
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date: Mon Oct 4 15:48:11 2021 +0100
gdb/remote: handle attach when stop packet lacks thread-id
Bug PR gdb/28405 reports a regression when using attach with an
extended-remote target. In this case the target is not including a
thread-id in the stop packet it sends back after the attach.
The regression was introduced with this commit:
commit 8f66807b98f7634c43149ea62e454ea8f877691d
Date: Wed Jan 13 20:26:58 2021 -0500
gdb: better handling of 'S' packets
The problem is that when GDB processes the stop packet, it sees that
there is no thread-id and so has to "guess" which thread the stop
should apply to.
In this case the target only has one thread, so really, there's no
guessing needed, but GDB still runs through the same process, this
shouldn't cause us any problems.
However, after the above commit, GDB now expects itself to be more
internally consistent, specifically, only a thread that GDB thinks is
resumed, can be a candidate for having stopped.
It turns out that, when GDB attaches to a process through an
extended-remote target, the threads of the process being attached too,
are not, initially, marked as resumed.
And so, when GDB tries to figure out which thread the stop might apply
too, it finds no threads in the processes marked resumed, and so an
assert triggers.
In extended_remote_target::attach we create a new thread with a call
to add_thread_silent, rather than remote_target::remote_add_thread,
the reason is that calling the latter will result in a call to
'add_thread' rather than 'add_thread_silent'. However,
remote_target::remote_add_thread includes additional
actions (i.e. calling remote_thread_info::set_resumed and set_running)
which are missing from extended_remote_target::attach. These missing
calls are what would serve to mark the new thread as resumed.
In this commit I propose that we add an extra parameter to
remote_target::remote_add_thread. This new parameter will force the
new thread to be added with a call to add_thread_silent. We can now
call remote_add_thread from the ::attach method, the extra
actions (listed above) will now be performed, and the thread will be
left in the correct state.
Additionally, in PR gdb/28405, a segfault is reported. This segfault
triggers when 'set debug remote 1' is used before trying to reproduce
the original assertion failure. The cause of this is in
remote_target::select_thread_for_ambiguous_stop_reply, where we do
this:
remote_debug_printf ("first resumed thread is %s",
pid_to_str (first_resumed_thread->ptid).c_str ());
remote_debug_printf ("is this guess ambiguous? = %d", ambiguous);
gdb_assert (first_resumed_thread != nullptr);
Notice that when debug printing is on we dereference
first_resumed_thread before we assert that the pointer is not
nullptr. This is the cause of the segfault, and is resolved by moving
the assert before the debug printing code.
I've extended an existing test, ext-attach.exp, so that the original
test is run multiple times; we run in the original mode, as normal,
but also, we now run with different packets disabled in gdbserver. In
particular, disabling Tthread would trigger the assertion as it was
reported in the original bug. I also run the test in all-stop and
non-stop modes now for extra coverage, we also run the tests with
target-async enabled, and disabled.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28405
diff --git a/gdb/remote.c b/gdb/remote.c
index 1bb6138a7c7..46cd352ef90 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -760,7 +760,8 @@ class remote_target : public process_stratum_target
void print_one_stopped_thread (thread_info *thread);
void process_initial_stop_replies (int from_tty);
- thread_info *remote_add_thread (ptid_t ptid, bool running, bool executing);
+ thread_info *remote_add_thread (ptid_t ptid, bool running, bool executing,
+ bool silent_p);
void btrace_sync_conf (const btrace_config *conf);
@@ -2551,10 +2552,13 @@ static remote_thread_info *get_remote_thread_info (remote_target *target,
ptid_t ptid);
/* Add thread PTID to GDB's thread list. Tag it as executing/running
- according to RUNNING. */
+ according to EXECUTING and RUNNING respectively. If SILENT_P (or the
+ remote_state::starting_up flag) is true then the new thread is added
+ silently, otherwise the new thread will be announced to the user. */
thread_info *
-remote_target::remote_add_thread (ptid_t ptid, bool running, bool executing)
+remote_target::remote_add_thread (ptid_t ptid, bool running, bool executing,
+ bool silent_p)
{
struct remote_state *rs = get_remote_state ();
struct thread_info *thread;
@@ -2565,7 +2569,7 @@ remote_target::remote_add_thread (ptid_t ptid, bool running, bool executing)
consider that a single-threaded target, mentioning a new thread
might be confusing to the user. Be silent then, preserving the
age old behavior. */
- if (rs->starting_up)
+ if (rs->starting_up || silent_p)
thread = add_thread_silent (this, ptid);
else
thread = add_thread (this, ptid);
@@ -2603,7 +2607,7 @@ remote_target::remote_notice_new_inferior (ptid_t currthread, bool executing)
{
/* We're seeing an event on a thread id we knew had exited.
This has to be a new thread reusing the old id. Add it. */
- remote_add_thread (currthread, running, executing);
+ remote_add_thread (currthread, running, executing, false);
return;
}
@@ -2625,7 +2629,7 @@ remote_target::remote_notice_new_inferior (ptid_t currthread, bool executing)
else
{
thread_info *thr
- = remote_add_thread (currthread, running, executing);
+ = remote_add_thread (currthread, running, executing, false);
switch_to_thread (thr);
}
return;
@@ -2657,7 +2661,7 @@ remote_target::remote_notice_new_inferior (ptid_t currthread, bool executing)
/* This is really a new thread. Add it. */
thread_info *new_thr
- = remote_add_thread (currthread, running, executing);
+ = remote_add_thread (currthread, running, executing, false);
/* If we found a new inferior, let the common code do whatever
it needs to with it (e.g., read shared libraries, insert
@@ -6171,14 +6175,11 @@ extended_remote_target::attach (const char *args, int from_tty)
ptid. */
ptid_t curr_ptid = remote_current_thread (ptid_t (pid));
- /* Add the main thread to the thread list. */
- thread_info *thr = add_thread_silent (this, curr_ptid);
+ /* Add the main thread to the thread list. We add the thread
+ silently in this case (the final true parameter). */
+ thread_info *thr = remote_add_thread (curr_ptid, true, true, true);
switch_to_thread (thr);
-
- /* Don't consider the thread stopped until we've processed the
- saved stop reply. */
- set_executing (this, thr->ptid, true);
}
/* Next, if the target can specify a description, read it. We do
@@ -8010,12 +8011,12 @@ remote_target::select_thread_for_ambiguous_stop_reply
ambiguous = true;
}
+ gdb_assert (first_resumed_thread != nullptr);
+
remote_debug_printf ("first resumed thread is %s",
pid_to_str (first_resumed_thread->ptid).c_str ());
remote_debug_printf ("is this guess ambiguous? = %d", ambiguous);
- gdb_assert (first_resumed_thread != nullptr);
-
/* Warn if the remote target is sending ambiguous stop replies. */
if (ambiguous)
{
diff --git a/gdb/testsuite/gdb.server/ext-attach.exp b/gdb/testsuite/gdb.server/ext-attach.exp
index c9766e35317..0babae2ebd0 100644
--- a/gdb/testsuite/gdb.server/ext-attach.exp
+++ b/gdb/testsuite/gdb.server/ext-attach.exp
@@ -30,53 +30,77 @@ if {![can_spawn_for_attach]} {
return 0
}
-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 {[build_executable "failed to prepare" $testfile $srcfile debug]} {
+ return -1
+}
- if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
- return -1
+# Run the test. TARGET_NON_STOP and TARGET_ASYNC should be 'on'
+# or 'off'. TO_DISABLE should be either the empty string, or
+# something that can be passed to gdbserver's --disable-packet command
+# line option.
+proc run_test { target_async target_non_stop to_disable } {
+ save_vars { ::GDBFLAGS } {
+ append ::GDBFLAGS " -ex \"maint set target-non-stop $target_non_stop\""
+ append ::GDBFLAGS " -ex \"maintenance set target-async ${target_async}\""
+
+ # 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\""
+ }
+
+ clean_restart $::binfile
}
-}
-# Make sure we're disconnected, in case we're testing with an
-# extended-remote board, therefore already connected.
-gdb_test "disconnect" ".*"
+ # 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]
-gdbserver_start_extended
+ if { [gdb_target_supports_trace] } then {
+ # Test predefined TSVs are uploaded.
+ gdb_test_sequence "info tvariables" "check uploaded tsv" {
+ "\[\r\n\]+Name\[\t \]+Initial\[\t \]+Current"
+ "\[\r\n\]+\\\$trace_timestamp 0"
+ }
+ }
-gdb_test_no_output "set remote exec-file $target_exec" "set remote exec-file"
+ set target_exec [gdbserver_download_current_prog]
+ if { $to_disable != "" } {
+ set gdbserver_opts "--disable-packet=${to_disable}"
+ } else {
+ set gdbserver_opts ""
+ }
+ gdbserver_start_extended $gdbserver_opts
-set test_spawn_id [spawn_wait_for_attach $binfile]
-set testpid [spawn_id_get_pid $test_spawn_id]
+ gdb_test_no_output "set remote exec-file $target_exec" "set remote exec-file"
-gdb_test "attach $testpid" \
- "Attaching to program: .*, process $testpid.*(in|at).*" \
- "attach to remote program 1"
+ set test_spawn_id [spawn_wait_for_attach $::binfile]
+ set testpid [spawn_id_get_pid $test_spawn_id]
-if { [gdb_target_supports_trace] } then {
- # Test predefined TSVs are uploaded.
- gdb_test_sequence "info tvariables" "check uploaded tsv" {
- "\[\r\n\]+Name\[\t \]+Initial\[\t \]+Current"
- "\[\r\n\]+\\\$trace_timestamp 0"
- }
-}
+ gdb_test "attach $testpid" \
+ "Attaching to program: .*, process $testpid.*(in|at).*" \
+ "attach to remote program 1"
+
+ gdb_test "backtrace" ".*main.*" "backtrace 1"
-gdb_test "backtrace" ".*main.*" "backtrace 1"
+ gdb_test "detach" "Detaching from program.*process.*"
+ gdb_test "backtrace" "No stack\\." "backtrace with no program"
-gdb_test "detach" "Detaching from program.*process.*"
-gdb_test "backtrace" "No stack\\." "backtrace with no program"
+ gdb_test "attach $testpid" \
+ "Attaching to program: .*, process $testpid.*(in|at).*" \
+ "attach to remote program 2"
+ gdb_test "backtrace" ".*main.*" "backtrace 2"
-gdb_test "attach $testpid" \
- "Attaching to program: .*, process $testpid.*(in|at).*" \
- "attach to remote program 2"
-gdb_test "backtrace" ".*main.*" "backtrace 2"
+ gdb_test "kill" "" "kill" "Kill the program being debugged. .y or n. " "y"
+ gdb_test_no_output "monitor exit"
-gdb_test "kill" "" "kill" "Kill the program being debugged. .y or n. " "y"
-gdb_test_no_output "monitor exit"
+ kill_wait_spawned_process $test_spawn_id
+}
-kill_wait_spawned_process $test_spawn_id
+foreach_with_prefix target_async {"on" "off" } {
+ foreach_with_prefix target_non_stop {"off" "on"} {
+ foreach_with_prefix to_disable { "" Tthread T } {
+ run_test ${target_async} ${target_non_stop} $to_disable
+ }
+ }
+}
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCHv3] gdb/remote: handle attach when stop packet lacks thread-id
2021-12-18 11:15 ` [PATCHv3] " Andrew Burgess
@ 2021-12-23 12:22 ` Andrew Burgess
0 siblings, 0 replies; 8+ messages in thread
From: Andrew Burgess @ 2021-12-23 12:22 UTC (permalink / raw)
To: gdb-patches
Given that Simon's feedback on v1 was mostly positive, with a few
minor tweaks requested, which I've addressed in v2/v3, I've now gone
ahead and pushed v3 of this patch.
I also plan to back-port this patch to the gdb-11-branch, I'll follow
up once that is done.
Thanks,
Andrew
* Andrew Burgess <aburgess@redhat.com> [2021-12-18 11:15:39 +0000]:
> * Andrew Burgess <aburgess@redhat.com> [2021-11-23 14:24:43 +0000]:
>
> > * Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2021-11-14 19:50:01 -0500]:
> >
> > >
> > >
> > > On 2021-10-05 05:04, Andrew Burgess wrote:
> > > > Bug PR gdb/28405 reports a regression when using attach with an
> > > > extended-remote target. In this case the target is not including a
> > > > thread-id in the stop packet it sends back after the attach.
> > > >
> > > > The regression was introduced with this commit:
> > > >
> > > > commit 8f66807b98f7634c43149ea62e454ea8f877691d
> > > > Date: Wed Jan 13 20:26:58 2021 -0500
> > > >
> > > > gdb: better handling of 'S' packets
> > > >
> > > > The problem is that when GDB processes the stop packet, it sees that
> > > > there is no thread-id and so has to "guess" which thread the stop
> > > > should apply to.
> > > >
> > > > In this case the target only has one thread, so really, there's no
> > > > guessing needed, but GDB still runs through the same process, this
> > > > shouldn't cause us any problems.
> > > >
> > > > However, after the above commit, GDB now expects itself to be more
> > > > internally consistent, specifically, only a thread that GDB thinks is
> > > > resumed, can be a candidate for having stopped.
> > > >
> > > > It turns out that, when GDB attaches to a process through an
> > > > extended-remote target, the threads of the process being attached too,
> > > > are not, initially, marked as resumed.
> > > >
> > > > And so, when GDB tries to figure out which thread the stop might apply
> > > > too, it finds no threads in the processes marked resumed, and so an
> > > > assert triggers.
> > > >
> > > > In extended_remote_target::attach we create a new thread with a call
> > > > to add_thread_silent, rather than remote_target::remote_add_thread,
> > > > the reason is that calling the latter will result in a call to
> > > > 'add_thread' rather than 'add_thread_silent'. However,
> > > > remote_target::remote_add_thread includes additional
> > > > actions (i.e. calling remote_thread_info::set_resumed and set_running)
> > > > which are missing from extended_remote_target::attach. These missing
> > > > calls are what would serve to mark the new thread as resumed.
> > > >
> > > > In this commit, I propose that we add the extra calls into
> > > > extended_remote_target::attach, this solves the problem at hand.
> > >
> > > To avoid duplication, did you consider calling remote_add_thread, but
> > > add some more parameters to have the caller have control over the
> > > thread's initial state?
> >
> > I've made this change in the patch below.
> >
> > >
> > > But the way to resolve the problem (marking the initial thread as
> > > resumed) sounds fine.
> > >
> > > While reading the code, I saw there is an additional code path for
> > > "target_can_async_p false". I suppose this can be exercised with "maint
> > > set target-async off", it might be useful to add an axis for that in
> > > your test?
> >
> > Sigh! So I tried running with 'maint set target-async off'. And went
> > down the rabbit hole of debugging non-async mode. See:
> >
> > https://sourceware.org/pipermail/gdb-patches/2021-November/183712.html
> >
> > I've added this to the test for now:
> >
> > # Currently, this test is only run with 'target-async on' as setting
> > # this off highlights some test failures.
> > foreach_with_prefix target_async {"on"} {
> > ...
> > }
> >
> > If/when the series I linked above is merged then that foreach loop can
> > be expanded to include both "off" and "on".
>
> The above patch to fix the "target-async off" issues has now been
> merged. Below is an updated version of the patch that includes the
> full test coverage.
>
> The changes within GDB itself are unchanged since v2.
>
> Thanks,
> Andrew
>
> ---
>
> commit 24654176f35e189575bb4488cb44676ca48f62a1
> Author: Andrew Burgess <andrew.burgess@embecosm.com>
> Date: Mon Oct 4 15:48:11 2021 +0100
>
> gdb/remote: handle attach when stop packet lacks thread-id
>
> Bug PR gdb/28405 reports a regression when using attach with an
> extended-remote target. In this case the target is not including a
> thread-id in the stop packet it sends back after the attach.
>
> The regression was introduced with this commit:
>
> commit 8f66807b98f7634c43149ea62e454ea8f877691d
> Date: Wed Jan 13 20:26:58 2021 -0500
>
> gdb: better handling of 'S' packets
>
> The problem is that when GDB processes the stop packet, it sees that
> there is no thread-id and so has to "guess" which thread the stop
> should apply to.
>
> In this case the target only has one thread, so really, there's no
> guessing needed, but GDB still runs through the same process, this
> shouldn't cause us any problems.
>
> However, after the above commit, GDB now expects itself to be more
> internally consistent, specifically, only a thread that GDB thinks is
> resumed, can be a candidate for having stopped.
>
> It turns out that, when GDB attaches to a process through an
> extended-remote target, the threads of the process being attached too,
> are not, initially, marked as resumed.
>
> And so, when GDB tries to figure out which thread the stop might apply
> too, it finds no threads in the processes marked resumed, and so an
> assert triggers.
>
> In extended_remote_target::attach we create a new thread with a call
> to add_thread_silent, rather than remote_target::remote_add_thread,
> the reason is that calling the latter will result in a call to
> 'add_thread' rather than 'add_thread_silent'. However,
> remote_target::remote_add_thread includes additional
> actions (i.e. calling remote_thread_info::set_resumed and set_running)
> which are missing from extended_remote_target::attach. These missing
> calls are what would serve to mark the new thread as resumed.
>
> In this commit I propose that we add an extra parameter to
> remote_target::remote_add_thread. This new parameter will force the
> new thread to be added with a call to add_thread_silent. We can now
> call remote_add_thread from the ::attach method, the extra
> actions (listed above) will now be performed, and the thread will be
> left in the correct state.
>
> Additionally, in PR gdb/28405, a segfault is reported. This segfault
> triggers when 'set debug remote 1' is used before trying to reproduce
> the original assertion failure. The cause of this is in
> remote_target::select_thread_for_ambiguous_stop_reply, where we do
> this:
>
> remote_debug_printf ("first resumed thread is %s",
> pid_to_str (first_resumed_thread->ptid).c_str ());
> remote_debug_printf ("is this guess ambiguous? = %d", ambiguous);
>
> gdb_assert (first_resumed_thread != nullptr);
>
> Notice that when debug printing is on we dereference
> first_resumed_thread before we assert that the pointer is not
> nullptr. This is the cause of the segfault, and is resolved by moving
> the assert before the debug printing code.
>
> I've extended an existing test, ext-attach.exp, so that the original
> test is run multiple times; we run in the original mode, as normal,
> but also, we now run with different packets disabled in gdbserver. In
> particular, disabling Tthread would trigger the assertion as it was
> reported in the original bug. I also run the test in all-stop and
> non-stop modes now for extra coverage, we also run the tests with
> target-async enabled, and disabled.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28405
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 1bb6138a7c7..46cd352ef90 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -760,7 +760,8 @@ class remote_target : public process_stratum_target
> void print_one_stopped_thread (thread_info *thread);
> void process_initial_stop_replies (int from_tty);
>
> - thread_info *remote_add_thread (ptid_t ptid, bool running, bool executing);
> + thread_info *remote_add_thread (ptid_t ptid, bool running, bool executing,
> + bool silent_p);
>
> void btrace_sync_conf (const btrace_config *conf);
>
> @@ -2551,10 +2552,13 @@ static remote_thread_info *get_remote_thread_info (remote_target *target,
> ptid_t ptid);
>
> /* Add thread PTID to GDB's thread list. Tag it as executing/running
> - according to RUNNING. */
> + according to EXECUTING and RUNNING respectively. If SILENT_P (or the
> + remote_state::starting_up flag) is true then the new thread is added
> + silently, otherwise the new thread will be announced to the user. */
>
> thread_info *
> -remote_target::remote_add_thread (ptid_t ptid, bool running, bool executing)
> +remote_target::remote_add_thread (ptid_t ptid, bool running, bool executing,
> + bool silent_p)
> {
> struct remote_state *rs = get_remote_state ();
> struct thread_info *thread;
> @@ -2565,7 +2569,7 @@ remote_target::remote_add_thread (ptid_t ptid, bool running, bool executing)
> consider that a single-threaded target, mentioning a new thread
> might be confusing to the user. Be silent then, preserving the
> age old behavior. */
> - if (rs->starting_up)
> + if (rs->starting_up || silent_p)
> thread = add_thread_silent (this, ptid);
> else
> thread = add_thread (this, ptid);
> @@ -2603,7 +2607,7 @@ remote_target::remote_notice_new_inferior (ptid_t currthread, bool executing)
> {
> /* We're seeing an event on a thread id we knew had exited.
> This has to be a new thread reusing the old id. Add it. */
> - remote_add_thread (currthread, running, executing);
> + remote_add_thread (currthread, running, executing, false);
> return;
> }
>
> @@ -2625,7 +2629,7 @@ remote_target::remote_notice_new_inferior (ptid_t currthread, bool executing)
> else
> {
> thread_info *thr
> - = remote_add_thread (currthread, running, executing);
> + = remote_add_thread (currthread, running, executing, false);
> switch_to_thread (thr);
> }
> return;
> @@ -2657,7 +2661,7 @@ remote_target::remote_notice_new_inferior (ptid_t currthread, bool executing)
>
> /* This is really a new thread. Add it. */
> thread_info *new_thr
> - = remote_add_thread (currthread, running, executing);
> + = remote_add_thread (currthread, running, executing, false);
>
> /* If we found a new inferior, let the common code do whatever
> it needs to with it (e.g., read shared libraries, insert
> @@ -6171,14 +6175,11 @@ extended_remote_target::attach (const char *args, int from_tty)
> ptid. */
> ptid_t curr_ptid = remote_current_thread (ptid_t (pid));
>
> - /* Add the main thread to the thread list. */
> - thread_info *thr = add_thread_silent (this, curr_ptid);
> + /* Add the main thread to the thread list. We add the thread
> + silently in this case (the final true parameter). */
> + thread_info *thr = remote_add_thread (curr_ptid, true, true, true);
>
> switch_to_thread (thr);
> -
> - /* Don't consider the thread stopped until we've processed the
> - saved stop reply. */
> - set_executing (this, thr->ptid, true);
> }
>
> /* Next, if the target can specify a description, read it. We do
> @@ -8010,12 +8011,12 @@ remote_target::select_thread_for_ambiguous_stop_reply
> ambiguous = true;
> }
>
> + gdb_assert (first_resumed_thread != nullptr);
> +
> remote_debug_printf ("first resumed thread is %s",
> pid_to_str (first_resumed_thread->ptid).c_str ());
> remote_debug_printf ("is this guess ambiguous? = %d", ambiguous);
>
> - gdb_assert (first_resumed_thread != nullptr);
> -
> /* Warn if the remote target is sending ambiguous stop replies. */
> if (ambiguous)
> {
> diff --git a/gdb/testsuite/gdb.server/ext-attach.exp b/gdb/testsuite/gdb.server/ext-attach.exp
> index c9766e35317..0babae2ebd0 100644
> --- a/gdb/testsuite/gdb.server/ext-attach.exp
> +++ b/gdb/testsuite/gdb.server/ext-attach.exp
> @@ -30,53 +30,77 @@ if {![can_spawn_for_attach]} {
> return 0
> }
>
> -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 {[build_executable "failed to prepare" $testfile $srcfile debug]} {
> + return -1
> +}
>
> - if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
> - return -1
> +# Run the test. TARGET_NON_STOP and TARGET_ASYNC should be 'on'
> +# or 'off'. TO_DISABLE should be either the empty string, or
> +# something that can be passed to gdbserver's --disable-packet command
> +# line option.
> +proc run_test { target_async target_non_stop to_disable } {
> + save_vars { ::GDBFLAGS } {
> + append ::GDBFLAGS " -ex \"maint set target-non-stop $target_non_stop\""
> + append ::GDBFLAGS " -ex \"maintenance set target-async ${target_async}\""
> +
> + # 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\""
> + }
> +
> + clean_restart $::binfile
> }
> -}
>
> -# Make sure we're disconnected, in case we're testing with an
> -# extended-remote board, therefore already connected.
> -gdb_test "disconnect" ".*"
> + # 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]
> -gdbserver_start_extended
> + if { [gdb_target_supports_trace] } then {
> + # Test predefined TSVs are uploaded.
> + gdb_test_sequence "info tvariables" "check uploaded tsv" {
> + "\[\r\n\]+Name\[\t \]+Initial\[\t \]+Current"
> + "\[\r\n\]+\\\$trace_timestamp 0"
> + }
> + }
>
> -gdb_test_no_output "set remote exec-file $target_exec" "set remote exec-file"
> + set target_exec [gdbserver_download_current_prog]
> + if { $to_disable != "" } {
> + set gdbserver_opts "--disable-packet=${to_disable}"
> + } else {
> + set gdbserver_opts ""
> + }
> + gdbserver_start_extended $gdbserver_opts
>
> -set test_spawn_id [spawn_wait_for_attach $binfile]
> -set testpid [spawn_id_get_pid $test_spawn_id]
> + gdb_test_no_output "set remote exec-file $target_exec" "set remote exec-file"
>
> -gdb_test "attach $testpid" \
> - "Attaching to program: .*, process $testpid.*(in|at).*" \
> - "attach to remote program 1"
> + set test_spawn_id [spawn_wait_for_attach $::binfile]
> + set testpid [spawn_id_get_pid $test_spawn_id]
>
> -if { [gdb_target_supports_trace] } then {
> - # Test predefined TSVs are uploaded.
> - gdb_test_sequence "info tvariables" "check uploaded tsv" {
> - "\[\r\n\]+Name\[\t \]+Initial\[\t \]+Current"
> - "\[\r\n\]+\\\$trace_timestamp 0"
> - }
> -}
> + gdb_test "attach $testpid" \
> + "Attaching to program: .*, process $testpid.*(in|at).*" \
> + "attach to remote program 1"
> +
> + gdb_test "backtrace" ".*main.*" "backtrace 1"
>
> -gdb_test "backtrace" ".*main.*" "backtrace 1"
> + gdb_test "detach" "Detaching from program.*process.*"
> + gdb_test "backtrace" "No stack\\." "backtrace with no program"
>
> -gdb_test "detach" "Detaching from program.*process.*"
> -gdb_test "backtrace" "No stack\\." "backtrace with no program"
> + gdb_test "attach $testpid" \
> + "Attaching to program: .*, process $testpid.*(in|at).*" \
> + "attach to remote program 2"
> + gdb_test "backtrace" ".*main.*" "backtrace 2"
>
> -gdb_test "attach $testpid" \
> - "Attaching to program: .*, process $testpid.*(in|at).*" \
> - "attach to remote program 2"
> -gdb_test "backtrace" ".*main.*" "backtrace 2"
> + gdb_test "kill" "" "kill" "Kill the program being debugged. .y or n. " "y"
> + gdb_test_no_output "monitor exit"
>
> -gdb_test "kill" "" "kill" "Kill the program being debugged. .y or n. " "y"
> -gdb_test_no_output "monitor exit"
> + kill_wait_spawned_process $test_spawn_id
> +}
>
> -kill_wait_spawned_process $test_spawn_id
> +foreach_with_prefix target_async {"on" "off" } {
> + foreach_with_prefix target_non_stop {"off" "on"} {
> + foreach_with_prefix to_disable { "" Tthread T } {
> + run_test ${target_async} ${target_non_stop} $to_disable
> + }
> + }
> +}
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-12-23 12:22 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-05 9:04 [PATCH] gdb/remote: handle attach when stop packet lacks thread-id Andrew Burgess
2021-11-09 10:06 ` Andrew Burgess
2021-11-15 0:50 ` Simon Marchi
2021-11-23 14:24 ` [PATCHv2] " Andrew Burgess
2021-12-04 11:40 ` Joel Brobecker
2021-12-18 10:48 ` Joel Brobecker
2021-12-18 11:15 ` [PATCHv3] " Andrew Burgess
2021-12-23 12:22 ` Andrew Burgess
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).