* [PATCH 1/2] gdb.threads/step-over-thread-exit.exp improvements
2023-11-21 15:05 [PATCH 0/2] Step over thread exit improvements/fixes Pedro Alves
@ 2023-11-21 15:05 ` Pedro Alves
2023-11-21 15:05 ` [PATCH 2/2] Ensure selected thread after thread exit stop Pedro Alves
2023-12-14 18:37 ` [PATCH 0/2] Step over thread exit improvements/fixes Pedro Alves
2 siblings, 0 replies; 4+ messages in thread
From: Pedro Alves @ 2023-11-21 15:05 UTC (permalink / raw)
To: gdb-patches
This commit makes the following improvements to
gdb.threads/step-over-thread-exit.exp:
- Add a third axis to stepping over the breakpoint with displaced vs
inline stepping -- also test with no breakpoint at all.
- Check that when GDB reports "Command aborted, thread exited.", the
selected thread is the thread that exited. This is always true
currently on GNU/Linux by coincidence, but a similar testcase on AMD
GPU exposed a problem here. Better make the testcase catch any
potential regression.
- Fixes a race that Simon ran into with GDBserver testing.
(gdb) next
[New Thread 2143071.2143438]
Thread 3 "step-over-threa" hit Breakpoint 2, 0x000055555555524e in my_exit_syscall () at .../testsuite/lib/my-syscalls.S:74
74 SYSCALL (my_exit, __NR_exit)
(gdb) FAIL: gdb.threads/step-over-thread-exit.exp: displaced-stepping=auto: non-stop=on: target-non-stop=on: schedlock=off: cmd=next: ns_stop_all=0: command aborts when thread exits
I was not able to reproduce it, but I believe that what happens is
the following:
Once we continue, the thread 2 exits, and the main thread thus
unblocks from its pthread_join, and spawns a new thread. That new
thread may hit the breakpoint at my_exit_syscall very quickly. GDB
could then see/process that breakpoint event before the thread exit
event for the thread we care about, which would result in the
failure seen above.
The fix here is to not loop and start a new thread at all in the
scenario where the race can happen. We only need to loop and spawn
new threads when testing with "cmd=continue" and schedlock off, in
which case GDB doesn't abort the command when the thread exits.
Change-Id: I90c95c32f00630a3f682b1541c23aff52451f9b6
---
.../gdb.threads/step-over-thread-exit.c | 16 ++-
.../gdb.threads/step-over-thread-exit.exp | 107 +++++++++++++++---
2 files changed, 102 insertions(+), 21 deletions(-)
diff --git a/gdb/testsuite/gdb.threads/step-over-thread-exit.c b/gdb/testsuite/gdb.threads/step-over-thread-exit.c
index 878e5924c5c..218f003b205 100644
--- a/gdb/testsuite/gdb.threads/step-over-thread-exit.c
+++ b/gdb/testsuite/gdb.threads/step-over-thread-exit.c
@@ -18,6 +18,7 @@
#include <pthread.h>
#include <assert.h>
#include <stdlib.h>
+#include <unistd.h>
#include "../lib/my-syscalls.h"
static void *
@@ -30,13 +31,19 @@ thread_func (void *arg)
abort ();
}
+/* Number of threads we'll create. */
+int n_threads = 100;
+
int
-main (void)
+main (int argc, char **argv)
{
int i;
- /* Spawn and join a thread, 100 times. */
- for (i = 0; i < 100; i++)
+ if (argc > 1)
+ n_threads = atoi (argv[1]);
+
+ /* Spawn and join a thread, N_THREADS times. */
+ for (i = 0; i < n_threads; i++)
{
pthread_t thread;
int ret;
@@ -48,5 +55,8 @@ main (void)
assert (ret == 0);
}
+ /* Some time to make sure that GDB processes the thread exit event
+ before the whole-process exit. */
+ sleep (3);
return 0;
}
diff --git a/gdb/testsuite/gdb.threads/step-over-thread-exit.exp b/gdb/testsuite/gdb.threads/step-over-thread-exit.exp
index 615bd838763..366e9249357 100644
--- a/gdb/testsuite/gdb.threads/step-over-thread-exit.exp
+++ b/gdb/testsuite/gdb.threads/step-over-thread-exit.exp
@@ -25,11 +25,28 @@ if { [build_executable "failed to prepare" $testfile \
return
}
-# Each argument is a different testing axis, most of them obvious.
+# Test stepping/continuing at an exit syscall instruction.
+#
+# Each argument is a different testing axis.
+#
+# STEP_OVER_MODE can be one of:
+#
+# - none: don't put a breakpoint on the s_endpgm instruction.
+#
+# - inline: put a breakpoint on the s_endpgm instruction, and use
+# in-line stepping to step over it (disable displaced-stepping).
+#
+# - displaced: same, but use displaced stepping.
+#
+# SCHEDLOCK can be "on" or "off".
+#
+# CMD is the GDB command to run when at the exit syscall instruction.
+#
# NS_STOP_ALL is only used if testing "set non-stop on", and indicates
# whether to have GDB explicitly stop all threads before continuing to
# thread exit.
-proc test {displaced-stepping non-stop target-non-stop schedlock cmd ns_stop_all} {
+#
+proc test {step_over_mode non-stop target-non-stop schedlock cmd ns_stop_all} {
if {${non-stop} == "off" && $ns_stop_all} {
error "invalid arguments"
}
@@ -40,19 +57,24 @@ proc test {displaced-stepping non-stop target-non-stop schedlock cmd ns_stop_all
clean_restart $::binfile
}
- gdb_test_no_output "set displaced-stepping ${displaced-stepping}"
-
- if { ![runto_main] } {
- return
+ if { $step_over_mode == "none" } {
+ # Nothing to do.
+ } elseif { $step_over_mode == "inline" } {
+ gdb_test_no_output "set displaced-stepping off"
+ } elseif { $step_over_mode == "displaced" } {
+ gdb_test_no_output "set displaced-stepping on"
+ } else {
+ error "Invalid step_over_mode value: $step_over_mode"
}
- gdb_breakpoint "my_exit_syscall"
-
if {$schedlock
|| (${non-stop} == "on" && $ns_stop_all)} {
- gdb_test "continue" \
- "Thread 2 .*hit Breakpoint $::decimal.* my_exit_syscall .*" \
- "continue until syscall"
+
+ gdb_test_no_output "set args 1"
+
+ if { ![runto my_exit_syscall] } {
+ return
+ }
if {${non-stop} == "on"} {
# The test only spawns one thread at a time, so this just
@@ -72,6 +94,13 @@ proc test {displaced-stepping non-stop target-non-stop schedlock cmd ns_stop_all
gdb_test_no_output "set scheduler-locking ${schedlock}"
+ # If testing a step-over is requested, leave the breakpoint at
+ # the current instruction to force a step-over; otherwise,
+ # remove it.
+ if { $step_over_mode == "none" } {
+ delete_breakpoints
+ }
+
if {$cmd == "continue"} {
gdb_test "continue" \
"No unwaited-for children left." \
@@ -84,9 +113,23 @@ proc test {displaced-stepping non-stop target-non-stop schedlock cmd ns_stop_all
}
}
} else {
+ if {$cmd != "continue" || $step_over_mode == "none"} {
+ set n_threads 1
+ } else {
+ set n_threads 100
+ }
+
+ gdb_test_no_output "set args $n_threads"
+
+ if { ![runto_main] } {
+ return
+ }
+
+ gdb_breakpoint "my_exit_syscall"
+
gdb_test_no_output "set scheduler-locking ${schedlock}"
- if {$cmd != "continue"} {
+ if {$cmd != "continue" || $step_over_mode == "none"} {
set thread "<unknown>"
gdb_test_multiple "continue" "" {
-re -wrap "Thread ($::decimal) .*hit Breakpoint $::decimal.* my_exit_syscall .*" {
@@ -98,12 +141,40 @@ proc test {displaced-stepping non-stop target-non-stop schedlock cmd ns_stop_all
"switch to event thread"
}
- gdb_test_multiple $cmd "command aborts when thread exits" {
- -re "Command aborted, thread exited\\.\r\n$::gdb_prompt " {
- pass $gdb_test_name
+ # If testing a step-over is requested, leave the breakpoint at
+ # the current instruction to force a step-over; otherwise,
+ # remove it.
+ if { $step_over_mode == "none" } {
+ delete_breakpoints
+ }
+
+ if {$cmd == "continue"} {
+ gdb_continue_to_end "continue to end" "continue" 1
+ } else {
+ gdb_test_multiple $cmd "command aborts when thread exits" {
+ -re "Command aborted, thread exited\\.\r\n$::gdb_prompt " {
+ pass $gdb_test_name
+ }
+ }
+
+ set cur_thr_after "<unknown>"
+ gdb_test_multiple "print /d \$_thread" "get selected thread after" {
+ -re -wrap "$::decimal = ($::decimal)" {
+ set cur_thr_after $expect_out(1,string)
+ pass "$gdb_test_name"
+ }
}
+
+ gdb_assert {$thread == $cur_thr_after} \
+ "selected thread didn't change"
}
} else {
+ if {$step_over_mode == "none"} {
+ # If we removed the breakpoint at my_exit_syscall, the
+ # loopping wouldn't work. This case is instead
+ # handled in the then/else branch above.
+ error "unexpected \$step_over_mode==none"
+ }
for { set i 0 } { $i < 100 } { incr i } {
with_test_prefix "iter $i" {
set ok 0
@@ -130,7 +201,7 @@ proc test {displaced-stepping non-stop target-non-stop schedlock cmd ns_stop_all
}
}
-foreach_with_prefix displaced-stepping {off auto} {
+foreach_with_prefix step_over_mode {none inline displaced} {
foreach_with_prefix non-stop {off on} {
foreach_with_prefix target-non-stop {off on} {
if {${non-stop} == "on" && ${target-non-stop} == "off"} {
@@ -142,11 +213,11 @@ foreach_with_prefix displaced-stepping {off auto} {
foreach_with_prefix cmd {"next" "continue"} {
if {${non-stop} == "on"} {
foreach_with_prefix ns_stop_all {0 1} {
- test ${displaced-stepping} ${non-stop} ${target-non-stop} \
+ test ${step_over_mode} ${non-stop} ${target-non-stop} \
${schedlock} ${cmd} ${ns_stop_all}
}
} else {
- test ${displaced-stepping} ${non-stop} ${target-non-stop} ${schedlock} ${cmd} 0
+ test ${step_over_mode} ${non-stop} ${target-non-stop} ${schedlock} ${cmd} 0
}
}
}
--
2.42.0
^ permalink raw reply [flat|nested] 4+ messages in thread* [PATCH 2/2] Ensure selected thread after thread exit stop
2023-11-21 15:05 [PATCH 0/2] Step over thread exit improvements/fixes Pedro Alves
2023-11-21 15:05 ` [PATCH 1/2] gdb.threads/step-over-thread-exit.exp improvements Pedro Alves
@ 2023-11-21 15:05 ` Pedro Alves
2023-12-14 18:37 ` [PATCH 0/2] Step over thread exit improvements/fixes Pedro Alves
2 siblings, 0 replies; 4+ messages in thread
From: Pedro Alves @ 2023-11-21 15:05 UTC (permalink / raw)
To: gdb-patches
While making step over thread exit work properly on AMDGPU, I noticed
that if there's a breakpoint on top of the exit syscall, and,
displaced stepping is off, then when GDB reports "Command aborted,
thread exited.", GDB also switches focus to a random thread, instead
of leaving the exited thread as selected:
(gdb) thread
[Current thread is 6, lane 0 (AMDGPU Lane 1:4:1:1/0 (0,0,0)[0,0,0])]
(gdb) si
[AMDGPU Wave 1:4:1:1 (0,0,0)/0 exited]
Command aborted, thread exited.
(gdb) thread
[Current thread is 5 (Thread 0x7ffff626f640 (LWP 3248392))]
(gdb)
The previous patch extended gdb.threads/step-over-thread-exit.exp to
exercise this on GNU/Linux (on the CPU side), and there, after that
"si", we always end up with the exiting thread as selected even
without this fix, but that's just a concidence. This commit enforces
it, fixing the latent problem for GNU/Linux, and the actual problem on
AMDGPU. I wrote a gdb.rocm/ testcase for this, but I can't be
upstreamed yet, until more pieces of the DWARF machinery are upstream
as well.
Change-Id: I6ff57a79514ac0142bba35c749fe83d53d9e4e51
---
gdb/infrun.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/gdb/infrun.c b/gdb/infrun.c
index a1543ab2443..a74ec824a8e 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -5891,7 +5891,14 @@ handle_thread_exited (execution_control_state *ecs)
if (abort_cmd)
{
+ /* We're stopping for the thread exit event. Switch to the
+ event thread again, as finish_step_over may have switched
+ threads. */
+ switch_to_thread (ecs->event_thread);
+
+ /* Emit [Thread ... exited] notification. */
delete_thread (ecs->event_thread);
+
ecs->event_thread = nullptr;
return false;
}
--
2.42.0
^ permalink raw reply [flat|nested] 4+ messages in thread