* [PATCH 0/3] gdb: add gdb_attach to fix failed testcases @ 2022-03-16 9:58 Tiezhu Yang 2022-03-16 9:58 ` [PATCH 1/3] gdb: testsuite: leave can_spawn_for_attach alone Tiezhu Yang ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: Tiezhu Yang @ 2022-03-16 9:58 UTC (permalink / raw) To: gdb-patches Tiezhu Yang (3): gdb: testsuite: leave can_spawn_for_attach alone gdb: testsuite: add new gdb_attach to fix attach-pie-noexec.exp gdb: testsuite: enhance gdb_attach to fix jit-elf.exp gdb/testsuite/gdb.base/attach-pie-noexec.exp | 4 +- gdb/testsuite/gdb.base/jit-elf.exp | 9 +--- gdb/testsuite/lib/gdb.exp | 69 ++++++++++++---------------- 3 files changed, 34 insertions(+), 48 deletions(-) -- 2.1.0 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/3] gdb: testsuite: leave can_spawn_for_attach alone 2022-03-16 9:58 [PATCH 0/3] gdb: add gdb_attach to fix failed testcases Tiezhu Yang @ 2022-03-16 9:58 ` Tiezhu Yang 2022-03-16 13:24 ` Simon Marchi 2022-03-16 9:58 ` [PATCH 2/3] gdb: testsuite: add new gdb_attach to fix attach-pie-noexec.exp Tiezhu Yang 2022-03-16 9:58 ` [PATCH 3/3] gdb: testsuite: enhance gdb_attach to fix jit-elf.exp Tiezhu Yang 2 siblings, 1 reply; 7+ messages in thread From: Tiezhu Yang @ 2022-03-16 9:58 UTC (permalink / raw) To: gdb-patches As Pedro Alves said, caching procs should not issue pass/fail [1], this commit removes gdb_test_multiple in can_spawn_for_attach. [1] https://sourceware.org/pipermail/gdb-patches/2022-March/186311.html Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> --- gdb/testsuite/lib/gdb.exp | 48 +++++++---------------------------------------- 1 file changed, 7 insertions(+), 41 deletions(-) diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp index a35d08a..8fafcc0 100644 --- a/gdb/testsuite/lib/gdb.exp +++ b/gdb/testsuite/lib/gdb.exp @@ -5081,7 +5081,7 @@ proc gdb_exit { } { # Return true if we can spawn a program on the target and attach to # it. -gdb_caching_proc can_spawn_for_attach { +proc can_spawn_for_attach { } { # We use exp_pid to get the inferior's pid, assuming that gives # back the pid of the program. On remote boards, that would give # us instead the PID of e.g., the ssh client, etc. @@ -5098,46 +5098,6 @@ gdb_caching_proc can_spawn_for_attach { return 0 } - set me "can_spawn_for_attach" - set src { int main (void) { sleep (600); return 0; } } - if {![gdb_simple_compile $me $src executable]} { - return 0 - } - - set test_spawn_id [spawn_wait_for_attach $obj] - set test_pid [spawn_id_get_pid $test_spawn_id] - - gdb_start - file delete $obj - gdb_test_multiple "attach $test_pid" "can spawn for attach" { - -re -wrap "Attaching to process $test_pid\r\n.*No executable file now.*" { - pass $gdb_test_name - kill_wait_spawned_process $test_spawn_id - return 1 - } - -re -wrap "Attaching to process $test_pid\r\n.*ptrace: Operation not permitted\\." { - unsupported "$gdb_test_name (Operation not permitted)" - kill_wait_spawned_process $test_spawn_id - return 0 - } - -re -wrap "Attaching to process $test_pid\r\n.*Attaching to process $test_pid failed" { - unsupported "$gdb_test_name (Attaching to process failed)" - kill_wait_spawned_process $test_spawn_id - return 0 - } - -re -wrap "Attaching to process $test_pid\r\n.*XML support was disabled at compile time.*" { - pass $gdb_test_name - kill_wait_spawned_process $test_spawn_id - return 1 - } - -re "A program is being debugged already. Kill it. .y or n. " { - send_gdb "y\n" - exp_continue - } - } - - kill_wait_spawned_process $test_spawn_id - # Assume yes. return 1 } @@ -5187,6 +5147,12 @@ proc spawn_id_get_pid { spawn_id } { proc spawn_wait_for_attach { executable_list } { set spawn_id_list {} + if ![can_spawn_for_attach] { + # The caller should have checked can_spawn_for_attach itself + # before getting here. + error "can't spawn for attach with this target/board" + } + foreach {executable} $executable_list { # Note we use Expect's spawn, not Tcl's exec, because with # spawn we control when to wait for/reap the process. That -- 2.1.0 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/3] gdb: testsuite: leave can_spawn_for_attach alone 2022-03-16 9:58 ` [PATCH 1/3] gdb: testsuite: leave can_spawn_for_attach alone Tiezhu Yang @ 2022-03-16 13:24 ` Simon Marchi 0 siblings, 0 replies; 7+ messages in thread From: Simon Marchi @ 2022-03-16 13:24 UTC (permalink / raw) To: Tiezhu Yang, gdb-patches On 2022-03-16 05:58, Tiezhu Yang wrote: > As Pedro Alves said, caching procs should not issue pass/fail [1], > this commit removes gdb_test_multiple in can_spawn_for_attach. > > [1] https://sourceware.org/pipermail/gdb-patches/2022-March/186311.html > > Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> > --- > gdb/testsuite/lib/gdb.exp | 48 +++++++---------------------------------------- > 1 file changed, 7 insertions(+), 41 deletions(-) > > diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp > index a35d08a..8fafcc0 100644 > --- a/gdb/testsuite/lib/gdb.exp > +++ b/gdb/testsuite/lib/gdb.exp > @@ -5081,7 +5081,7 @@ proc gdb_exit { } { > # Return true if we can spawn a program on the target and attach to > # it. > > -gdb_caching_proc can_spawn_for_attach { > +proc can_spawn_for_attach { } { > # We use exp_pid to get the inferior's pid, assuming that gives > # back the pid of the program. On remote boards, that would give > # us instead the PID of e.g., the ssh client, etc. > @@ -5098,46 +5098,6 @@ gdb_caching_proc can_spawn_for_attach { > return 0 > } If we don't want can_spawn_for_attach to emit test results, we should also remove the "unsupported" calls. If you'd like to still get a trace about why a test run doesn't support spawning for attach, you can replace those with "verbose -log ...", instead of removing them. Maybe change the subject to "remove attach test from can_spawn_for_attach", that would be more precise than "leave can_spawn_for_attach alone". With those changed, this patch LGTM, it essentially reverts to the original state. Simon ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/3] gdb: testsuite: add new gdb_attach to fix attach-pie-noexec.exp 2022-03-16 9:58 [PATCH 0/3] gdb: add gdb_attach to fix failed testcases Tiezhu Yang 2022-03-16 9:58 ` [PATCH 1/3] gdb: testsuite: leave can_spawn_for_attach alone Tiezhu Yang @ 2022-03-16 9:58 ` Tiezhu Yang 2022-03-16 14:03 ` Simon Marchi 2022-03-16 9:58 ` [PATCH 3/3] gdb: testsuite: enhance gdb_attach to fix jit-elf.exp Tiezhu Yang 2 siblings, 1 reply; 7+ messages in thread From: Tiezhu Yang @ 2022-03-16 9:58 UTC (permalink / raw) To: gdb-patches This commit adds gdb_attach to check various output of "attach" command, and then use gdb_attach to fix the following issue. If /proc/sys/kernel/yama/ptrace_scope is 1, when execute the following command without superuser: make check-gdb TESTS="gdb.base/attach-pie-noexec.exp" we can see the following messages in gdb/testsuite/gdb.log: (gdb) attach 6500 Attaching to process 6500 ptrace: Operation not permitted. (gdb) PASS: gdb.base/attach-pie-noexec.exp: attach It is obviously wrong, the expected result should be UNSUPPORTED in such a case. With this patch, we can see "Operation not permitted" in the log info, and then we can do the following processes to test: (1) set ptrace_scope as 0 $ echo 0 | sudo tee /proc/sys/kernel/yama/ptrace_scope $ make check-gdb TESTS="gdb.base/attach-pie-noexec.exp" (2) use sudo $ sudo make check-gdb TESTS="gdb.base/attach-pie-noexec.exp" Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> --- gdb/testsuite/gdb.base/attach-pie-noexec.exp | 4 +++- gdb/testsuite/lib/gdb.exp | 15 +++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/gdb/testsuite/gdb.base/attach-pie-noexec.exp b/gdb/testsuite/gdb.base/attach-pie-noexec.exp index 4712824..9a1bafe 100644 --- a/gdb/testsuite/gdb.base/attach-pie-noexec.exp +++ b/gdb/testsuite/gdb.base/attach-pie-noexec.exp @@ -59,7 +59,9 @@ set testpid [spawn_id_get_pid $test_spawn_id] gdb_start file delete -- $binfile -gdb_test "attach $testpid" "Attaching to process $testpid\r\n.*" "attach" +if {![gdb_attach $testpid]} { + return 0 +} gdb_test "set architecture $arch" "The target architecture is set to \"$arch\"\\." gdb_test "info shared" "From\[ \t\]+To\[ \t\]+Syms Read\[ \t\]+Shared Object Library\r\n0x.*" diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp index 8fafcc0..599e8cc 100644 --- a/gdb/testsuite/lib/gdb.exp +++ b/gdb/testsuite/lib/gdb.exp @@ -5102,6 +5102,21 @@ proc can_spawn_for_attach { } { return 1 } +# Check various output of "attach" command. + +proc gdb_attach { testpid } { + gdb_test_multiple "attach $testpid" "gdb attach" { + -re -wrap "Attaching to process $testpid\r\n.*No executable file now.*" { + pass $gdb_test_name + return 1 + } + -re -wrap "Attaching to process $testpid\r\n.*ptrace: Operation not permitted\\." { + unsupported "$gdb_test_name (Operation not permitted)" + return 0 + } + } +} + # Kill a progress previously started with spawn_wait_for_attach, and # reap its wait status. PROC_SPAWN_ID is the spawn id associated with # the process. -- 2.1.0 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/3] gdb: testsuite: add new gdb_attach to fix attach-pie-noexec.exp 2022-03-16 9:58 ` [PATCH 2/3] gdb: testsuite: add new gdb_attach to fix attach-pie-noexec.exp Tiezhu Yang @ 2022-03-16 14:03 ` Simon Marchi 0 siblings, 0 replies; 7+ messages in thread From: Simon Marchi @ 2022-03-16 14:03 UTC (permalink / raw) To: Tiezhu Yang, gdb-patches On 2022-03-16 05:58, Tiezhu Yang wrote: > This commit adds gdb_attach to check various output of "attach" command, > and then use gdb_attach to fix the following issue. > > If /proc/sys/kernel/yama/ptrace_scope is 1, when execute the following > command without superuser: > > make check-gdb TESTS="gdb.base/attach-pie-noexec.exp" > > we can see the following messages in gdb/testsuite/gdb.log: > > (gdb) attach 6500 > Attaching to process 6500 > ptrace: Operation not permitted. > (gdb) PASS: gdb.base/attach-pie-noexec.exp: attach > > It is obviously wrong, the expected result should be UNSUPPORTED in such > a case. > > With this patch, we can see "Operation not permitted" in the log info, > and then we can do the following processes to test: > (1) set ptrace_scope as 0 > $ echo 0 | sudo tee /proc/sys/kernel/yama/ptrace_scope > $ make check-gdb TESTS="gdb.base/attach-pie-noexec.exp" > (2) use sudo > $ sudo make check-gdb TESTS="gdb.base/attach-pie-noexec.exp" > > Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> > --- > gdb/testsuite/gdb.base/attach-pie-noexec.exp | 4 +++- > gdb/testsuite/lib/gdb.exp | 15 +++++++++++++++ > 2 files changed, 18 insertions(+), 1 deletion(-) > > diff --git a/gdb/testsuite/gdb.base/attach-pie-noexec.exp b/gdb/testsuite/gdb.base/attach-pie-noexec.exp > index 4712824..9a1bafe 100644 > --- a/gdb/testsuite/gdb.base/attach-pie-noexec.exp > +++ b/gdb/testsuite/gdb.base/attach-pie-noexec.exp > @@ -59,7 +59,9 @@ set testpid [spawn_id_get_pid $test_spawn_id] > > gdb_start > file delete -- $binfile > -gdb_test "attach $testpid" "Attaching to process $testpid\r\n.*" "attach" > +if {![gdb_attach $testpid]} { > + return 0 > +} > gdb_test "set architecture $arch" "The target architecture is set to \"$arch\"\\." > gdb_test "info shared" "From\[ \t\]+To\[ \t\]+Syms Read\[ \t\]+Shared Object Library\r\n0x.*" > > diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp > index 8fafcc0..599e8cc 100644 > --- a/gdb/testsuite/lib/gdb.exp > +++ b/gdb/testsuite/lib/gdb.exp > @@ -5102,6 +5102,21 @@ proc can_spawn_for_attach { } { > return 1 > } > > +# Check various output of "attach" command. Please document the possible return values and what they mean. > + > +proc gdb_attach { testpid } { > + gdb_test_multiple "attach $testpid" "gdb attach" { I think the test name can be just "attach" (we know it's gdb). Although I presume that we will eventually want to make the test name customizable. "attach" can still be the default though. > + -re -wrap "Attaching to process $testpid\r\n.*No executable file now.*" { The "No executable file now" message is just one particular message that can be printed. And it's kind of specific to the test you are updated, where the executable is deleted before attaching. If there was actually an executable to be found, the message would be different. I would suggest trying to catch the known error cases with some "-re -wrap" clauses, and then have a catch-all clause that assumes things went fine: gdb_test_multiple "attach $testpid" "attach" { -re -wrap "ptrace: Operation not permitted\\." { unsupported "$gdb_test_name (Operation not permitted)" return 0 } -re -wrap "" { pass $gdb_test_name return 1 } } Note that the "process $testpid" part is target-specific, so it's better if we don't match it. I don't see the need to match the "Attaching to" part either, although it wouldn't do any harm. As we find other ways attach can fail, we can add more patterns. It's hard to write it the other way around, because attach doesn't really give an "OK, attach worked!" message that we can match. I think that's fine... if gdb_attach reports that the attach worked even though it didn't, the followings checks in the test calling gdb_attach will likely fail themselves, so the problem will not go unnoticed. Simon ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 3/3] gdb: testsuite: enhance gdb_attach to fix jit-elf.exp 2022-03-16 9:58 [PATCH 0/3] gdb: add gdb_attach to fix failed testcases Tiezhu Yang 2022-03-16 9:58 ` [PATCH 1/3] gdb: testsuite: leave can_spawn_for_attach alone Tiezhu Yang 2022-03-16 9:58 ` [PATCH 2/3] gdb: testsuite: add new gdb_attach to fix attach-pie-noexec.exp Tiezhu Yang @ 2022-03-16 9:58 ` Tiezhu Yang 2022-03-16 14:16 ` Simon Marchi 2 siblings, 1 reply; 7+ messages in thread From: Tiezhu Yang @ 2022-03-16 9:58 UTC (permalink / raw) To: gdb-patches If /proc/sys/kernel/yama/ptrace_scope is 1, when execute the following command without superuser: make check-gdb TESTS="gdb.base/jit-elf.exp" we can see the following messages in gdb/testsuite/gdb.log: (gdb) attach 1650108 Attaching to program: /home/yangtiezhu/build/gdb/testsuite/outputs/gdb.base/jit-elf/jit-elf-main, process 1650108 ptrace: Operation not permitted. (gdb) FAIL: gdb.base/jit-elf.exp: attach: one_jit_test-2: break here 1: attach handle the above case in gdb_attach, and then use gdb_attach to fix the above issue. Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> --- gdb/testsuite/gdb.base/jit-elf.exp | 9 ++------- gdb/testsuite/lib/gdb.exp | 8 ++++++++ 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/gdb/testsuite/gdb.base/jit-elf.exp b/gdb/testsuite/gdb.base/jit-elf.exp index 8a4c5b7..61a51a2 100644 --- a/gdb/testsuite/gdb.base/jit-elf.exp +++ b/gdb/testsuite/gdb.base/jit-elf.exp @@ -40,7 +40,7 @@ set jit_solib_srcfile ${srcdir}/${subdir}/${jit_solib_basename}.c # Detach, restart GDB, and re-attach to the program. proc clean_reattach {} { global decimal gdb_prompt - global main_binfile main_srcfile + global main_binfile # Get PID of test program. set testpid -1 @@ -57,12 +57,7 @@ proc clean_reattach {} { clean_restart ${main_binfile} - set test "attach" - gdb_test_multiple "attach $testpid" "$test" { - -re "Attaching to program.*.*main.*at .*$main_srcfile:.*$gdb_prompt $" { - pass "$test" - } - } + gdb_attach $testpid gdb_test_no_output "set var wait_for_gdb = 0" } diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp index 599e8cc..5751bde 100644 --- a/gdb/testsuite/lib/gdb.exp +++ b/gdb/testsuite/lib/gdb.exp @@ -5114,6 +5114,14 @@ proc gdb_attach { testpid } { unsupported "$gdb_test_name (Operation not permitted)" return 0 } + -re -wrap "Attaching to program.*.*main.*at .*:.*" { + pass $gdb_test_name + return 1 + } + -re -wrap "Attaching to program: .*, process $testpid\r\n.*ptrace: Operation not permitted\\." { + unsupported "$gdb_test_name (Operation not permitted)" + return 0 + } } } -- 2.1.0 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 3/3] gdb: testsuite: enhance gdb_attach to fix jit-elf.exp 2022-03-16 9:58 ` [PATCH 3/3] gdb: testsuite: enhance gdb_attach to fix jit-elf.exp Tiezhu Yang @ 2022-03-16 14:16 ` Simon Marchi 0 siblings, 0 replies; 7+ messages in thread From: Simon Marchi @ 2022-03-16 14:16 UTC (permalink / raw) To: Tiezhu Yang, gdb-patches On 2022-03-16 05:58, Tiezhu Yang wrote: > If /proc/sys/kernel/yama/ptrace_scope is 1, when execute the following > command without superuser: > > make check-gdb TESTS="gdb.base/jit-elf.exp" > > we can see the following messages in gdb/testsuite/gdb.log: > > (gdb) attach 1650108 > Attaching to program: /home/yangtiezhu/build/gdb/testsuite/outputs/gdb.base/jit-elf/jit-elf-main, process 1650108 > ptrace: Operation not permitted. > (gdb) FAIL: gdb.base/jit-elf.exp: attach: one_jit_test-2: break here 1: attach > > handle the above case in gdb_attach, and then use gdb_attach to fix > the above issue. > > Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> > --- > gdb/testsuite/gdb.base/jit-elf.exp | 9 ++------- > gdb/testsuite/lib/gdb.exp | 8 ++++++++ > 2 files changed, 10 insertions(+), 7 deletions(-) > > diff --git a/gdb/testsuite/gdb.base/jit-elf.exp b/gdb/testsuite/gdb.base/jit-elf.exp > index 8a4c5b7..61a51a2 100644 > --- a/gdb/testsuite/gdb.base/jit-elf.exp > +++ b/gdb/testsuite/gdb.base/jit-elf.exp > @@ -40,7 +40,7 @@ set jit_solib_srcfile ${srcdir}/${subdir}/${jit_solib_basename}.c > # Detach, restart GDB, and re-attach to the program. > proc clean_reattach {} { > global decimal gdb_prompt > - global main_binfile main_srcfile > + global main_binfile > > # Get PID of test program. > set testpid -1 > @@ -57,12 +57,7 @@ proc clean_reattach {} { > > clean_restart ${main_binfile} > > - set test "attach" > - gdb_test_multiple "attach $testpid" "$test" { > - -re "Attaching to program.*.*main.*at .*$main_srcfile:.*$gdb_prompt $" { > - pass "$test" > - } > - } > + gdb_attach $testpid Ok, to expand on my previous message: in order to allow matching for some specific output, like this test does (to avoid losing some test coverage), I propose to let the caller pass an extra pattern that will be used in the catch-all success clause: gdb_attach can be: proc gdb_attach { testpid {options {}} } { parse_options { {pattern ""} } gdb_test_multiple "attach $testpid" "attach" { -re -wrap "Attaching to.*ptrace: Operation not permitted\\." { unsupported "$gdb_test_name (Operation not permitted)" return 0 } -re -wrap "$pattern" { pass $gdb_test_name return 1 } } } And the caller can do: gdb_attach $testpid { "pattern" "main.*at .*$::main_srcfile:.*" } A simpler caller that doesn't care about the output can still do just: gdb_attach $testpid Note that you probably want to make the test return if gdb_attach returns 0 (as you did in the previous test), so that we don't continue and generate some FAILs. > > gdb_test_no_output "set var wait_for_gdb = 0" > } > diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp > index 599e8cc..5751bde 100644 > --- a/gdb/testsuite/lib/gdb.exp > +++ b/gdb/testsuite/lib/gdb.exp > @@ -5114,6 +5114,14 @@ proc gdb_attach { testpid } { > unsupported "$gdb_test_name (Operation not permitted)" > return 0 > } > + -re -wrap "Attaching to program.*.*main.*at .*:.*" { > + pass $gdb_test_name > + return 1 > + } > + -re -wrap "Attaching to program: .*, process $testpid\r\n.*ptrace: Operation not permitted\\." { > + unsupported "$gdb_test_name (Operation not permitted)" > + return 0 > + } > } > } This way we don't need to add many specific patterns like this. Simon ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-03-16 14:16 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-03-16 9:58 [PATCH 0/3] gdb: add gdb_attach to fix failed testcases Tiezhu Yang 2022-03-16 9:58 ` [PATCH 1/3] gdb: testsuite: leave can_spawn_for_attach alone Tiezhu Yang 2022-03-16 13:24 ` Simon Marchi 2022-03-16 9:58 ` [PATCH 2/3] gdb: testsuite: add new gdb_attach to fix attach-pie-noexec.exp Tiezhu Yang 2022-03-16 14:03 ` Simon Marchi 2022-03-16 9:58 ` [PATCH 3/3] gdb: testsuite: enhance gdb_attach to fix jit-elf.exp Tiezhu Yang 2022-03-16 14:16 ` Simon Marchi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).