[v2,gdb/testsuite] Handle ptrace operation not permitted in can_spawn_for_attach
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_build--master-arm |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_check--master-arm |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 |
success
|
Testing passed
|
Commit Message
When running the testsuite on a system with kernel.yama.ptrace_scope set to 1,
we run into attach failures.
Fix this by recognizing "ptrace: Operation not permitted" in
can_spawn_for_attach.
Tested on aarch64-linux.
---
gdb/testsuite/gdb.base/break-interp.exp | 4 +
gdb/testsuite/gdb.base/dprintf-detach.exp | 2 +-
.../run-control-while-bg-execution.exp | 10 ++-
.../gdb.multi/attach-while-running.exp | 2 +-
.../gdb.threads/attach-into-signal.exp | 3 +-
.../gdb.threads/attach-slow-waitpid.exp | 3 +-
gdb/testsuite/gdb.threads/attach-stopped.exp | 3 +-
.../gdb.threads/check-libthread-db.exp | 42 ++++-----
gdb/testsuite/lib/gdb.exp | 87 ++++++++++++++++---
9 files changed, 116 insertions(+), 40 deletions(-)
base-commit: ebf18671351d94185823d364b75369abc1baba31
Comments
On 2024-04-18 07:32, Tom de Vries wrote:
> When running the testsuite on a system with kernel.yama.ptrace_scope set to 1,
> we run into attach failures.
>
> Fix this by recognizing "ptrace: Operation not permitted" in
> can_spawn_for_attach.
>
> Tested on aarch64-linux.
I really don't like relying on "attach" 's behavior to decide whether to test attach.
It's an inversion of responsibilities.
can_spawn_for_attach means "can I spawn a process, such that I will attach to it later", a
simple atomic thing.
Also, with this change, it means, "can I spawn a process and does attaching to it work?"
So it ends up misnamed.
Someone tried to add something very much like this a while ago, and I objected then:
https://inbox.sourceware.org/gdb-patches/e5f08136-fa4d-2f21-ff83-8adf4d3a158e@palves.net/
We ended up with gdb_attach, added in a7e6a19e87f3, which handles the "ptrace: Operation not permitted"
scenarios too. Some testcases have meanwhile been converted to use gdb_attach, but there are more,
of course. IMO we should continue that direction. gdb_attach is not unlike "gdb_run_cmd" for example.
See also:
https://inbox.sourceware.org/gdb-patches/3b845985-cbd4-4996-145e-14191338b095@polymtl.ca/
Pedro Alves
On 4/24/24 21:40, Pedro Alves wrote:
> On 2024-04-18 07:32, Tom de Vries wrote:
>> When running the testsuite on a system with kernel.yama.ptrace_scope set to 1,
>> we run into attach failures.
>>
>> Fix this by recognizing "ptrace: Operation not permitted" in
>> can_spawn_for_attach.
>>
>> Tested on aarch64-linux.
>
> I really don't like relying on "attach" 's behavior to decide whether to test attach.
> It's an inversion of responsibilities.
>
Hi Pedro,
I don't see it that way, I think this approach accurately handles that
if there's no permission to attach to a spawned process, you cannot
spawn-for-attach.
> can_spawn_for_attach means "can I spawn a process, such that I will attach to it later", a
> simple atomic thing.
> > Also, with this change, it means, "can I spawn a process and does
attaching to it work?"
> So it ends up misnamed.
>
I see what you mean, though I don't see it as misnamed, but if that
bothers you I'm glad to rename it something else. Can_spawn_and_attach?
You have another suggestion?
> Someone tried to add something very much like this a while ago, and I objected then:
>
> https://inbox.sourceware.org/gdb-patches/e5f08136-fa4d-2f21-ff83-8adf4d3a158e@palves.net/
> > We ended up with gdb_attach, added in a7e6a19e87f3, which handles the
"ptrace: Operation not permitted"
> scenarios too. Some testcases have meanwhile been converted to use gdb_attach, but there are more,
> of course. IMO we should continue that direction. gdb_attach is not unlike "gdb_run_cmd" for example.
>
> See also:
>
> https://inbox.sourceware.org/gdb-patches/3b845985-cbd4-4996-145e-14191338b095@polymtl.ca/
>
I think the gdb_attach approach is problematic for the reasons that:
- it's not versatile enough to handle all the cases where it's needed,
and
- it's required to be used in all the attach sites.
On the contrary, the proposed patch very simply handles all the cases,
and in only one proc which is already used in a trivial way in most
test-cases that need it. So it seems obvious to me that the proposed
solution is preferable.
Thanks,
- Tom
On 2024-04-24 22:35, Tom de Vries wrote:
> On 4/24/24 21:40, Pedro Alves wrote:
>> On 2024-04-18 07:32, Tom de Vries wrote:
>>> When running the testsuite on a system with kernel.yama.ptrace_scope set to 1,
>>> we run into attach failures.
>>>
>>> Fix this by recognizing "ptrace: Operation not permitted" in
>>> can_spawn_for_attach.
>>>
>>> Tested on aarch64-linux.
>>
>> I really don't like relying on "attach" 's behavior to decide whether to test attach.
>> It's an inversion of responsibilities.
>>
>
> Hi Pedro,
>
> I don't see it that way, I think this approach accurately handles that if there's no permission to attach to a spawned process, you cannot spawn-for-attach.
My inversion remark was more about of the risk of attach breaking/regressing, and attach
testcases stop being exercised. PASS -> UNSUPPORTED/UNTESTED regressions aren't that well
noticed. But I suppose that if the procedure is written in a way that is very lax in its attach
check, only really looking for the ptrace output case, and defaulting to return that yes,
we can attach", that is mitigated.
>
>> can_spawn_for_attach means "can I spawn a process, such that I will attach to it later", a
>> simple atomic thing.
>> > Also, with this change, it means, "can I spawn a process and does
> attaching to it work?"
>> So it ends up misnamed.
>>
>
> I see what you mean, though I don't see it as misnamed, but if that bothers you I'm glad to rename it something else. Can_spawn_and_attach? You have another suggestion?
can_spawn_for_attach's name was originally derived from spawn_wait_for_attach.
(git 60b3033e6e2936af6fcc37cf67cade99a89940ad). It was more about
"can I call spawn_wait_for_attach" than "can I attach". But given the
check for use_gdb_stub, I guess you could say that your check doesn't really
change that aspect. It's just my expectations that need to be adjusted.
Let's just leave it as you have it.
>
> I think the gdb_attach approach is problematic for the reasons that:
> - it's not versatile enough to handle all the cases where it's needed,
> and
> - it's required to be used in all the attach sites.
>
> On the contrary, the proposed patch very simply handles all the cases, and in only one proc which is already used in a trivial way in most test-cases that need it. So it seems obvious to me that the proposed solution is preferable.
One issue I was concerned about is that for extended-remote testing, this change
assumes the native-extended-gdbserver board's behavior of gdb_start spawning gdbserver
and making gdb connect to it. I don't know whether actually-remote extended-remote
boards actually do that. But I suppose it's a reasonable thing for them to do.
OK, I'm convinced.
Comments on the patch itself below.
On 2024-04-18 07:32, Tom de Vries wrote:
> When running the testsuite on a system with kernel.yama.ptrace_scope set to 1,
> we run into attach failures.
>
> Fix this by recognizing "ptrace: Operation not permitted" in
> can_spawn_for_attach.
>
> Tested on aarch64-linux.
> ---
> gdb/testsuite/gdb.base/break-interp.exp | 4 +
> gdb/testsuite/gdb.base/dprintf-detach.exp | 2 +-
> .../run-control-while-bg-execution.exp | 10 ++-
> .../gdb.multi/attach-while-running.exp | 2 +-
> .../gdb.threads/attach-into-signal.exp | 3 +-
> .../gdb.threads/attach-slow-waitpid.exp | 3 +-
> gdb/testsuite/gdb.threads/attach-stopped.exp | 3 +-
> .../gdb.threads/check-libthread-db.exp | 42 ++++-----
> gdb/testsuite/lib/gdb.exp | 87 ++++++++++++++++---
> 9 files changed, 116 insertions(+), 40 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.base/break-interp.exp b/gdb/testsuite/gdb.base/break-interp.exp
> index addacde552d..d7f84db4770 100644
> --- a/gdb/testsuite/gdb.base/break-interp.exp
> +++ b/gdb/testsuite/gdb.base/break-interp.exp
> @@ -318,6 +318,10 @@ proc test_attach_gdb {file pid displacement prefix} {
> }
>
> proc test_attach {file displacement {relink_args ""}} {
> + if { ![can_spawn_for_attach] } {
> + return
> + }
> +
> global board_info
> global exec
>
> diff --git a/gdb/testsuite/gdb.base/dprintf-detach.exp b/gdb/testsuite/gdb.base/dprintf-detach.exp
> index 550d319a895..b4184d698df 100644
> --- a/gdb/testsuite/gdb.base/dprintf-detach.exp
> +++ b/gdb/testsuite/gdb.base/dprintf-detach.exp
> @@ -21,7 +21,7 @@
> load_lib gdbserver-support.exp
>
> # The test relies on "detach/attach".
> -require !use_gdb_stub
> +require can_spawn_for_attach
>
> standard_testfile
> set escapedbinfile [string_to_regexp ${binfile}]
> diff --git a/gdb/testsuite/gdb.base/run-control-while-bg-execution.exp b/gdb/testsuite/gdb.base/run-control-while-bg-execution.exp
> index f1cbd9351d3..a36c4ee3614 100644
> --- a/gdb/testsuite/gdb.base/run-control-while-bg-execution.exp
> +++ b/gdb/testsuite/gdb.base/run-control-while-bg-execution.exp
> @@ -108,8 +108,14 @@ proc do_test { action1 action2 } {
> }
> }
>
> -foreach_with_prefix action1 { kill detach add none } {
> - foreach_with_prefix action2 { start run attach } {
> +set actions1 { kill detach add none }
> +set actions2 { start run }
> +if { [can_spawn_for_attach] } {
> + lappend actions2 attach
> +}
> +
I suppose this isn't using the more typical form of:
foreach_with_prefix action1 { kill detach add none } {
foreach_with_prefix action2 { start run attach } {
if {$action2 == "attach" && ![can_spawn_for_attach]} {
continue
}
do_test $action1 $action2
}
}
... to make sure the can_spawn_for_attach call is done early?
Can you add a comment if this is important? Otherwise, I think
the more typical form is a lot more readable.
> +foreach_with_prefix action1 $actions1 {
> + foreach_with_prefix action2 $actions2 {
> do_test $action1 $action2
> }
> }
> diff --git a/gdb/testsuite/gdb.multi/attach-while-running.exp b/gdb/testsuite/gdb.multi/attach-while-running.exp
> index eade8b42a18..ca4fa635467 100644
> --- a/gdb/testsuite/gdb.multi/attach-while-running.exp
> +++ b/gdb/testsuite/gdb.multi/attach-while-running.exp
> @@ -36,7 +36,7 @@
>
> standard_testfile
>
> -require !use_gdb_stub
> +require can_spawn_for_attach
>
> if { [build_executable "failed to prepare" ${testfile} ${srcfile}] } {
> return
> diff --git a/gdb/testsuite/gdb.threads/attach-into-signal.exp b/gdb/testsuite/gdb.threads/attach-into-signal.exp
> index 87e34070548..91da960e09a 100644
> --- a/gdb/testsuite/gdb.threads/attach-into-signal.exp
> +++ b/gdb/testsuite/gdb.threads/attach-into-signal.exp
> @@ -17,7 +17,8 @@
> # This file was created by Jan Kratochvil <jan.kratochvil@redhat.com>.
>
> # This test only works on Linux
> -require !use_gdb_stub isnative
> +require can_spawn_for_attach
> +require isnative
> require {!is_remote host}
> require {istarget *-linux*}
>
> diff --git a/gdb/testsuite/gdb.threads/attach-slow-waitpid.exp b/gdb/testsuite/gdb.threads/attach-slow-waitpid.exp
> index dc3e62a7b7e..28d70daad8c 100644
> --- a/gdb/testsuite/gdb.threads/attach-slow-waitpid.exp
> +++ b/gdb/testsuite/gdb.threads/attach-slow-waitpid.exp
> @@ -37,7 +37,8 @@
> # during the attach phase.
>
> # This test only works on Linux
> -require !use_gdb_stub isnative
> +require can_spawn_for_attach
> +require isnative
> require {!is_remote host}
> require {istarget *-linux*}
>
> diff --git a/gdb/testsuite/gdb.threads/attach-stopped.exp b/gdb/testsuite/gdb.threads/attach-stopped.exp
> index 78e194c992f..0421ffc3794 100644
> --- a/gdb/testsuite/gdb.threads/attach-stopped.exp
> +++ b/gdb/testsuite/gdb.threads/attach-stopped.exp
> @@ -18,7 +18,8 @@
> # This file was updated by Jan Kratochvil <jan.kratochvil@redhat.com>.
>
> # This test only works on Linux
> -require !use_gdb_stub isnative
> +require can_spawn_for_attach
> +require isnative
> require {!is_remote host}
> require {istarget *-linux*}
>
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index d48ea37c0cc..937add7a272 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -6131,6 +6131,43 @@ proc gdb_exit { } {
> catch default_gdb_exit
> }
>
> +# Helper function for can_spawn_for_attach. Try to spawn and attach, and
> +# return 0 only if we cannot attach because it's unsupported.
> +
> +gdb_caching_proc can_spawn_for_attach_1 {} {
> + # Assume yes.
> + set res 1
> +
> + set me "can_spawn_for_attach"
> + set src { int main (void) { sleep (600); return 0; } }
Don't we need a header include for sleep?
> + if {![gdb_simple_compile $me $src executable]} {
> + return $res
> + }
> +
> + set test_spawn_id [spawn_wait_for_attach_1 $obj]
> + file delete $obj
I think this should be a remote_file call, not a build file delete.
can_spawn_wait_for_attach returns false if the target is remote,
not if the host is remote.
> +
> + gdb_start
> +
> + set test_pid [spawn_id_get_pid $test_spawn_id]
> + set attaching_re "Attaching to process $test_pid"
> + gdb_test_multiple "attach $test_pid" "can spawn for attach" {
> + -re -wrap "$attaching_re\r\n.*ptrace: Operation not permitted\\." {
> + # Not permitted.
> + set res 0
> + }
> + -re -wrap "" {
> + # Don't know.
> + }
> + }
> +
> + gdb_exit
The comment below suggests this gdb_exit shouldn't be here.
> +
> + kill_wait_spawned_process $test_spawn_id
> +
> + return $res
> +}
> +
> # Return true if we can spawn a program on the target and attach to
> # it.
>
> @@ -6151,8 +6188,25 @@ proc can_spawn_for_attach { } {
> return 0
> }
>
> - # Assume yes.
> - return 1
> + # By making can_spawn_for_attach_1 a gdb_caching_proc, we make it
> + # unpredictable which test-case will call it first, and consequently a
> + # test-case may pass in say a full test run, but fail when run
> + # individually, due to a can_spawn_for_attach call in location where a
> + # gdb_exit (as can_spawn_for_attach_1 does) breaks things.
> + # To avoid this, we move the initial gdb_exit out of
> + # can_spawn_for_attach_1, guaranteeing that we end up in the same state
> + # regardless of whether can_spawn_for_attach_1 is called. However, that
> + # is only necessary for the first call in a test-case, so cache the result
> + # in a global (which should be reset after each test-case) to keep track
> + # of that.
> + global cache_can_spawn_for_attach_1
> + if { [info exists cache_can_spawn_for_attach_1] } {
> + return $cache_can_spawn_for_attach_1
> + }
> + gdb_exit
There is still a gdb_exit call in can_spawn_for_attach_1.
I guess you meant to remove it?
I think it'll also be good to document that can_spawn_for_attach calls gdb_exit.
> +
> + set cache_can_spawn_for_attach_1 [can_spawn_for_attach_1]
> + return $cache_can_spawn_for_attach_1
> }
>
> # Centralize the failure checking of "attach" command.
> @@ -6265,20 +6319,12 @@ proc spawn_id_get_pid { spawn_id } {
> return $testpid
> }
>
> -# Start a set of programs running and then wait for a bit, to be sure
> -# that they can be attached to. Return a list of processes spawn IDs,
> -# one element for each process spawned. It's a test error to call
> -# this when [can_spawn_for_attach] is false.
> +# Helper function for spawn_wait_for_attach
... and can_spawn_for_attach_1. Just spawn_wait_for_attach by itself
wouldn't need the helper.
> As spawn_wait_for_attach, but
> +# doesn't check for can_spawn_for_attach.
>
On 4/26/24 20:50, Pedro Alves wrote:
> On 2024-04-24 22:35, Tom de Vries wrote:
>> On 4/24/24 21:40, Pedro Alves wrote:
>>> On 2024-04-18 07:32, Tom de Vries wrote:
>>>> When running the testsuite on a system with kernel.yama.ptrace_scope set to 1,
>>>> we run into attach failures.
>>>>
>>>> Fix this by recognizing "ptrace: Operation not permitted" in
>>>> can_spawn_for_attach.
>>>>
>>>> Tested on aarch64-linux.
>>>
>>> I really don't like relying on "attach" 's behavior to decide whether to test attach.
>>> It's an inversion of responsibilities.
>>>
>>
>> Hi Pedro,
>>
>> I don't see it that way, I think this approach accurately handles that if there's no permission to attach to a spawned process, you cannot spawn-for-attach.
>
> My inversion remark was more about of the risk of attach breaking/regressing, and attach
> testcases stop being exercised.
I see.
> PASS -> UNSUPPORTED/UNTESTED regressions aren't that well
> noticed.
Agreed.
> But I suppose that if the procedure is written in a way that is very lax in its attach
> check, only really looking for the ptrace output case, and defaulting to return that yes,
> we can attach", that is mitigated.
>
Yes, that was my intention.
>>
>>> can_spawn_for_attach means "can I spawn a process, such that I will attach to it later", a
>>> simple atomic thing.
>>> > Also, with this change, it means, "can I spawn a process and does
>> attaching to it work?"
>>> So it ends up misnamed.
>>>
>>
>> I see what you mean, though I don't see it as misnamed, but if that bothers you I'm glad to rename it something else. Can_spawn_and_attach? You have another suggestion?
>
>
> can_spawn_for_attach's name was originally derived from spawn_wait_for_attach.
> (git 60b3033e6e2936af6fcc37cf67cade99a89940ad). It was more about
> "can I call spawn_wait_for_attach" than "can I attach". But given the
> check for use_gdb_stub, I guess you could say that your check doesn't really
> change that aspect. It's just my expectations that need to be adjusted.
>
> Let's just leave it as you have it.
>
Ack.
>>
>> I think the gdb_attach approach is problematic for the reasons that:
>> - it's not versatile enough to handle all the cases where it's needed,
>> and
>> - it's required to be used in all the attach sites.
>>
>> On the contrary, the proposed patch very simply handles all the cases, and in only one proc which is already used in a trivial way in most test-cases that need it. So it seems obvious to me that the proposed solution is preferable.
>
> One issue I was concerned about is that for extended-remote testing, this change
> assumes the native-extended-gdbserver board's behavior of gdb_start spawning gdbserver
> and making gdb connect to it. I don't know whether actually-remote extended-remote
> boards actually do that. But I suppose it's a reasonable thing for them to do.
>
> OK, I'm convinced.
>
Great.
> Comments on the patch itself below.
>
> On 2024-04-18 07:32, Tom de Vries wrote:
>> When running the testsuite on a system with kernel.yama.ptrace_scope set to 1,
>> we run into attach failures.
>>
>> Fix this by recognizing "ptrace: Operation not permitted" in
>> can_spawn_for_attach.
>>
>> Tested on aarch64-linux.
>> ---
>> gdb/testsuite/gdb.base/break-interp.exp | 4 +
>> gdb/testsuite/gdb.base/dprintf-detach.exp | 2 +-
>> .../run-control-while-bg-execution.exp | 10 ++-
>> .../gdb.multi/attach-while-running.exp | 2 +-
>> .../gdb.threads/attach-into-signal.exp | 3 +-
>> .../gdb.threads/attach-slow-waitpid.exp | 3 +-
>> gdb/testsuite/gdb.threads/attach-stopped.exp | 3 +-
>> .../gdb.threads/check-libthread-db.exp | 42 ++++-----
>> gdb/testsuite/lib/gdb.exp | 87 ++++++++++++++++---
>> 9 files changed, 116 insertions(+), 40 deletions(-)
>>
>> diff --git a/gdb/testsuite/gdb.base/break-interp.exp b/gdb/testsuite/gdb.base/break-interp.exp
>> index addacde552d..d7f84db4770 100644
>> --- a/gdb/testsuite/gdb.base/break-interp.exp
>> +++ b/gdb/testsuite/gdb.base/break-interp.exp
>> @@ -318,6 +318,10 @@ proc test_attach_gdb {file pid displacement prefix} {
>> }
>>
>> proc test_attach {file displacement {relink_args ""}} {
>> + if { ![can_spawn_for_attach] } {
>> + return
>> + }
>> +
>> global board_info
>> global exec
>>
>> diff --git a/gdb/testsuite/gdb.base/dprintf-detach.exp b/gdb/testsuite/gdb.base/dprintf-detach.exp
>> index 550d319a895..b4184d698df 100644
>> --- a/gdb/testsuite/gdb.base/dprintf-detach.exp
>> +++ b/gdb/testsuite/gdb.base/dprintf-detach.exp
>> @@ -21,7 +21,7 @@
>> load_lib gdbserver-support.exp
>>
>> # The test relies on "detach/attach".
>> -require !use_gdb_stub
>> +require can_spawn_for_attach
>>
>> standard_testfile
>> set escapedbinfile [string_to_regexp ${binfile}]
>> diff --git a/gdb/testsuite/gdb.base/run-control-while-bg-execution.exp b/gdb/testsuite/gdb.base/run-control-while-bg-execution.exp
>> index f1cbd9351d3..a36c4ee3614 100644
>> --- a/gdb/testsuite/gdb.base/run-control-while-bg-execution.exp
>> +++ b/gdb/testsuite/gdb.base/run-control-while-bg-execution.exp
>> @@ -108,8 +108,14 @@ proc do_test { action1 action2 } {
>> }
>> }
>>
>> -foreach_with_prefix action1 { kill detach add none } {
>> - foreach_with_prefix action2 { start run attach } {
>> +set actions1 { kill detach add none }
>> +set actions2 { start run }
>> +if { [can_spawn_for_attach] } {
>> + lappend actions2 attach
>> +}
>> +
>
> I suppose this isn't using the more typical form of:
>
> foreach_with_prefix action1 { kill detach add none } {
> foreach_with_prefix action2 { start run attach } {
> if {$action2 == "attach" && ![can_spawn_for_attach]} {
> continue
> }
> do_test $action1 $action2
> }
> }
>
> ... to make sure the can_spawn_for_attach call is done early?
> Can you add a comment if this is important?
It's not important, since do_test starts with a clean_restart.
> Otherwise, I think
> the more typical form is a lot more readable.
I prefer it as I wrote it (and find that more readable), but ok, I've
reworked it.
>> +foreach_with_prefix action1 $actions1 {
>> + foreach_with_prefix action2 $actions2 {
>> do_test $action1 $action2
>> }
>> }
>> diff --git a/gdb/testsuite/gdb.multi/attach-while-running.exp b/gdb/testsuite/gdb.multi/attach-while-running.exp
>> index eade8b42a18..ca4fa635467 100644
>> --- a/gdb/testsuite/gdb.multi/attach-while-running.exp
>> +++ b/gdb/testsuite/gdb.multi/attach-while-running.exp
>> @@ -36,7 +36,7 @@
>>
>> standard_testfile
>>
>> -require !use_gdb_stub
>> +require can_spawn_for_attach
>>
>> if { [build_executable "failed to prepare" ${testfile} ${srcfile}] } {
>> return
>> diff --git a/gdb/testsuite/gdb.threads/attach-into-signal.exp b/gdb/testsuite/gdb.threads/attach-into-signal.exp
>> index 87e34070548..91da960e09a 100644
>> --- a/gdb/testsuite/gdb.threads/attach-into-signal.exp
>> +++ b/gdb/testsuite/gdb.threads/attach-into-signal.exp
>> @@ -17,7 +17,8 @@
>> # This file was created by Jan Kratochvil <jan.kratochvil@redhat.com>.
>>
>> # This test only works on Linux
>> -require !use_gdb_stub isnative
>> +require can_spawn_for_attach
>> +require isnative
>> require {!is_remote host}
>> require {istarget *-linux*}
>>
>> diff --git a/gdb/testsuite/gdb.threads/attach-slow-waitpid.exp b/gdb/testsuite/gdb.threads/attach-slow-waitpid.exp
>> index dc3e62a7b7e..28d70daad8c 100644
>> --- a/gdb/testsuite/gdb.threads/attach-slow-waitpid.exp
>> +++ b/gdb/testsuite/gdb.threads/attach-slow-waitpid.exp
>> @@ -37,7 +37,8 @@
>> # during the attach phase.
>>
>> # This test only works on Linux
>> -require !use_gdb_stub isnative
>> +require can_spawn_for_attach
>> +require isnative
>> require {!is_remote host}
>> require {istarget *-linux*}
>>
>> diff --git a/gdb/testsuite/gdb.threads/attach-stopped.exp b/gdb/testsuite/gdb.threads/attach-stopped.exp
>> index 78e194c992f..0421ffc3794 100644
>> --- a/gdb/testsuite/gdb.threads/attach-stopped.exp
>> +++ b/gdb/testsuite/gdb.threads/attach-stopped.exp
>> @@ -18,7 +18,8 @@
>> # This file was updated by Jan Kratochvil <jan.kratochvil@redhat.com>.
>>
>> # This test only works on Linux
>> -require !use_gdb_stub isnative
>> +require can_spawn_for_attach
>> +require isnative
>> require {!is_remote host}
>> require {istarget *-linux*}
>>
>
>
>> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
>> index d48ea37c0cc..937add7a272 100644
>> --- a/gdb/testsuite/lib/gdb.exp
>> +++ b/gdb/testsuite/lib/gdb.exp
>> @@ -6131,6 +6131,43 @@ proc gdb_exit { } {
>> catch default_gdb_exit
>> }
>>
>> +# Helper function for can_spawn_for_attach. Try to spawn and attach, and
>> +# return 0 only if we cannot attach because it's unsupported.
>> +
>> +gdb_caching_proc can_spawn_for_attach_1 {} {
>> + # Assume yes.
>> + set res 1
>> +
>> + set me "can_spawn_for_attach"
>> + set src { int main (void) { sleep (600); return 0; } }
>
> Don't we need a header include for sleep?
>
Indeed, added.
>> + if {![gdb_simple_compile $me $src executable]} {
>> + return $res
>> + }
>> +
>> + set test_spawn_id [spawn_wait_for_attach_1 $obj]
>> + file delete $obj
>
> I think this should be a remote_file call, not a build file delete.
> can_spawn_wait_for_attach returns false if the target is remote,
> not if the host is remote.
>
AFAIU, with remote host, compiling mean copying sources to remote host,
compiling on remote host and copying the artifact back to build. This
is all taken care of by dejagnu, and the file we're deleting is the file
on build.
Anyway, I've changed this to use "remote_file build" to make this more
clear.
>> +
>> + gdb_start
>> +
>> + set test_pid [spawn_id_get_pid $test_spawn_id]
>> + set attaching_re "Attaching to process $test_pid"
>> + gdb_test_multiple "attach $test_pid" "can spawn for attach" {
>> + -re -wrap "$attaching_re\r\n.*ptrace: Operation not permitted\\." {
>> + # Not permitted.
>> + set res 0
>> + }
>> + -re -wrap "" {
>> + # Don't know.
>> + }
>> + }
>> +
>> + gdb_exit
>
> The comment below suggests this gdb_exit shouldn't be here.
>
It does belong here. Your comment means my comment is not good enough,
I've updated it to make it more clear.
>> +
>> + kill_wait_spawned_process $test_spawn_id
>> +
>> + return $res
>> +}
>> +
>> # Return true if we can spawn a program on the target and attach to
>> # it.
>>
>
>> @@ -6151,8 +6188,25 @@ proc can_spawn_for_attach { } {
>> return 0
>> }
>>
>> - # Assume yes.
>> - return 1
>> + # By making can_spawn_for_attach_1 a gdb_caching_proc, we make it
>> + # unpredictable which test-case will call it first, and consequently a
>> + # test-case may pass in say a full test run, but fail when run
>> + # individually, due to a can_spawn_for_attach call in location where a
>> + # gdb_exit (as can_spawn_for_attach_1 does) breaks things.
>> + # To avoid this, we move the initial gdb_exit out of
>> + # can_spawn_for_attach_1, guaranteeing that we end up in the same state
>> + # regardless of whether can_spawn_for_attach_1 is called. However, that
>> + # is only necessary for the first call in a test-case, so cache the result
>> + # in a global (which should be reset after each test-case) to keep track
>> + # of that.
>> + global cache_can_spawn_for_attach_1
>> + if { [info exists cache_can_spawn_for_attach_1] } {
>> + return $cache_can_spawn_for_attach_1
>> + }
>> + gdb_exit
>
> There is still a gdb_exit call in can_spawn_for_attach_1.
> I guess you meant to remove it?
>
There is, and it's intentional. Again, I hope that the updated comment
will clarify this.
> I think it'll also be good to document that can_spawn_for_attach calls gdb_exit.
>
Done.
>> +
>> + set cache_can_spawn_for_attach_1 [can_spawn_for_attach_1]
>> + return $cache_can_spawn_for_attach_1
>> }
>>
>> # Centralize the failure checking of "attach" command.
>> @@ -6265,20 +6319,12 @@ proc spawn_id_get_pid { spawn_id } {
>> return $testpid
>> }
>>
>> -# Start a set of programs running and then wait for a bit, to be sure
>> -# that they can be attached to. Return a list of processes spawn IDs,
>> -# one element for each process spawned. It's a test error to call
>> -# this when [can_spawn_for_attach] is false.
>> +# Helper function for spawn_wait_for_attach
>
> ... and can_spawn_for_attach_1. Just spawn_wait_for_attach by itself
> wouldn't need the helper.
>
Ack, updated.
I've submitted a v2 here (
https://sourceware.org/pipermail/gdb-patches/2024-May/208725.html ).
Thanks,
- Tom
>> As spawn_wait_for_attach, but
>> +# doesn't check for can_spawn_for_attach.
>>
>
@@ -318,6 +318,10 @@ proc test_attach_gdb {file pid displacement prefix} {
}
proc test_attach {file displacement {relink_args ""}} {
+ if { ![can_spawn_for_attach] } {
+ return
+ }
+
global board_info
global exec
@@ -21,7 +21,7 @@
load_lib gdbserver-support.exp
# The test relies on "detach/attach".
-require !use_gdb_stub
+require can_spawn_for_attach
standard_testfile
set escapedbinfile [string_to_regexp ${binfile}]
@@ -108,8 +108,14 @@ proc do_test { action1 action2 } {
}
}
-foreach_with_prefix action1 { kill detach add none } {
- foreach_with_prefix action2 { start run attach } {
+set actions1 { kill detach add none }
+set actions2 { start run }
+if { [can_spawn_for_attach] } {
+ lappend actions2 attach
+}
+
+foreach_with_prefix action1 $actions1 {
+ foreach_with_prefix action2 $actions2 {
do_test $action1 $action2
}
}
@@ -36,7 +36,7 @@
standard_testfile
-require !use_gdb_stub
+require can_spawn_for_attach
if { [build_executable "failed to prepare" ${testfile} ${srcfile}] } {
return
@@ -17,7 +17,8 @@
# This file was created by Jan Kratochvil <jan.kratochvil@redhat.com>.
# This test only works on Linux
-require !use_gdb_stub isnative
+require can_spawn_for_attach
+require isnative
require {!is_remote host}
require {istarget *-linux*}
@@ -37,7 +37,8 @@
# during the attach phase.
# This test only works on Linux
-require !use_gdb_stub isnative
+require can_spawn_for_attach
+require isnative
require {!is_remote host}
require {istarget *-linux*}
@@ -18,7 +18,8 @@
# This file was updated by Jan Kratochvil <jan.kratochvil@redhat.com>.
# This test only works on Linux
-require !use_gdb_stub isnative
+require can_spawn_for_attach
+require isnative
require {!is_remote host}
require {istarget *-linux*}
@@ -102,25 +102,27 @@ with_test_prefix "automated load-time check" {
}
# Automated load-time check with NPTL fully operational.
- with_test_prefix "libpthread.so fully initialized" {
- clean_restart ${binfile}
-
- gdb_test_no_output "maint set check-libthread-db 1"
- gdb_test_no_output "set debug libthread-db 1"
-
- set test_spawn_id [spawn_wait_for_attach $binfile]
- set testpid [spawn_id_get_pid $test_spawn_id]
-
- gdb_test_sequence "attach $testpid" \
- "check debug libthread-db output" {
- "\[\r\n\]+Running libthread_db integrity checks:"
- "\[\r\n\]+\[ \]+Got thread 0x\[1-9a-f\]\[0-9a-f\]+ => \[0-9\]+ => 0x\[1-9a-f\]\[0-9a-f\]+ ... OK"
- "\[\r\n\]+\[ \]+Got thread 0x\[1-9a-f\]\[0-9a-f\]+ => \[0-9\]+ => 0x\[1-9a-f\]\[0-9a-f\]+ ... OK"
- "\[\r\n\]+libthread_db integrity checks passed."
- "\[\r\n\]+[Thread debugging using libthread_db enabled]"
- }
-
- gdb_exit
- kill_wait_spawned_process $test_spawn_id
+ if { [can_spawn_for_attach] } {
+ with_test_prefix "libpthread.so fully initialized" {
+ clean_restart ${binfile}
+
+ gdb_test_no_output "maint set check-libthread-db 1"
+ gdb_test_no_output "set debug libthread-db 1"
+
+ set test_spawn_id [spawn_wait_for_attach $binfile]
+ set testpid [spawn_id_get_pid $test_spawn_id]
+
+ gdb_test_sequence "attach $testpid" \
+ "check debug libthread-db output" {
+ "\[\r\n\]+Running libthread_db integrity checks:"
+ "\[\r\n\]+\[ \]+Got thread 0x\[1-9a-f\]\[0-9a-f\]+ => \[0-9\]+ => 0x\[1-9a-f\]\[0-9a-f\]+ ... OK"
+ "\[\r\n\]+\[ \]+Got thread 0x\[1-9a-f\]\[0-9a-f\]+ => \[0-9\]+ => 0x\[1-9a-f\]\[0-9a-f\]+ ... OK"
+ "\[\r\n\]+libthread_db integrity checks passed."
+ "\[\r\n\]+[Thread debugging using libthread_db enabled]"
+ }
+
+ gdb_exit
+ kill_wait_spawned_process $test_spawn_id
+ }
}
}
@@ -6131,6 +6131,43 @@ proc gdb_exit { } {
catch default_gdb_exit
}
+# Helper function for can_spawn_for_attach. Try to spawn and attach, and
+# return 0 only if we cannot attach because it's unsupported.
+
+gdb_caching_proc can_spawn_for_attach_1 {} {
+ # Assume yes.
+ set res 1
+
+ set me "can_spawn_for_attach"
+ set src { int main (void) { sleep (600); return 0; } }
+ if {![gdb_simple_compile $me $src executable]} {
+ return $res
+ }
+
+ set test_spawn_id [spawn_wait_for_attach_1 $obj]
+ file delete $obj
+
+ gdb_start
+
+ set test_pid [spawn_id_get_pid $test_spawn_id]
+ set attaching_re "Attaching to process $test_pid"
+ gdb_test_multiple "attach $test_pid" "can spawn for attach" {
+ -re -wrap "$attaching_re\r\n.*ptrace: Operation not permitted\\." {
+ # Not permitted.
+ set res 0
+ }
+ -re -wrap "" {
+ # Don't know.
+ }
+ }
+
+ gdb_exit
+
+ kill_wait_spawned_process $test_spawn_id
+
+ return $res
+}
+
# Return true if we can spawn a program on the target and attach to
# it.
@@ -6151,8 +6188,25 @@ proc can_spawn_for_attach { } {
return 0
}
- # Assume yes.
- return 1
+ # By making can_spawn_for_attach_1 a gdb_caching_proc, we make it
+ # unpredictable which test-case will call it first, and consequently a
+ # test-case may pass in say a full test run, but fail when run
+ # individually, due to a can_spawn_for_attach call in location where a
+ # gdb_exit (as can_spawn_for_attach_1 does) breaks things.
+ # To avoid this, we move the initial gdb_exit out of
+ # can_spawn_for_attach_1, guaranteeing that we end up in the same state
+ # regardless of whether can_spawn_for_attach_1 is called. However, that
+ # is only necessary for the first call in a test-case, so cache the result
+ # in a global (which should be reset after each test-case) to keep track
+ # of that.
+ global cache_can_spawn_for_attach_1
+ if { [info exists cache_can_spawn_for_attach_1] } {
+ return $cache_can_spawn_for_attach_1
+ }
+ gdb_exit
+
+ set cache_can_spawn_for_attach_1 [can_spawn_for_attach_1]
+ return $cache_can_spawn_for_attach_1
}
# Centralize the failure checking of "attach" command.
@@ -6265,20 +6319,12 @@ proc spawn_id_get_pid { spawn_id } {
return $testpid
}
-# Start a set of programs running and then wait for a bit, to be sure
-# that they can be attached to. Return a list of processes spawn IDs,
-# one element for each process spawned. It's a test error to call
-# this when [can_spawn_for_attach] is false.
+# Helper function for spawn_wait_for_attach. As spawn_wait_for_attach, but
+# doesn't check for can_spawn_for_attach.
-proc spawn_wait_for_attach { executable_list } {
+proc spawn_wait_for_attach_1 { 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
@@ -6292,6 +6338,21 @@ proc spawn_wait_for_attach { executable_list } {
return $spawn_id_list
}
+# Start a set of programs running and then wait for a bit, to be sure
+# that they can be attached to. Return a list of processes spawn IDs,
+# one element for each process spawned. It's a test error to call
+# this when [can_spawn_for_attach] is false.
+
+proc spawn_wait_for_attach { executable_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"
+ }
+
+ return [spawn_wait_for_attach_1 $executable_list]
+}
+
#
# gdb_load_cmd -- load a file into the debugger.
# ARGS - additional args to load command.