[PATCH] gdb/testsuite: Introduce and use gdb_spawn_attach

[PATCH] gdb/testsuite: Introduce and use gdb_spawn_attach

[Lancelot SIX]

Hi,

In a downstream port of GDB, I have tests which run `gdb --pid=$PID`,
and have to support ptrace restrictions as it was introduced in
a7e6a19e87f3d719ea23c65b580a6d9bca4ccab3 for the attach command.  

As there is a similar testcase in gdb.base/attach.exp, this patch
proposes to add the gdb_spawn_attach helper method to factorize the
error handling.

All feedbacks welcome,
Best,
Lancelot.

---

Following a7e6a19e87f3d719ea23c65b580a6d9bca4ccab3 "gdb: testsuite: add
new gdb_attach to check "attach" command", this commit proposes to
introduce the gdb_spawn_attach helper and use it in gdb.base/attach.exp.

This helper starts GDB and addr the "--pid=$PID" argument.

Tested on x86_64-gnu-linux

Change-Id: I1fdcdb71c86d9c5d34bb28fc86fac68bcec37358
---
 gdb/testsuite/gdb.base/attach.exp |  8 ++----
 gdb/testsuite/lib/gdb.exp         | 45 +++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 6 deletions(-)

diff --git a/gdb/testsuite/gdb.base/attach.exp b/gdb/testsuite/gdb.base/attach.exp
index d01060aba53..3869a7b06e8 100644
--- a/gdb/testsuite/gdb.base/attach.exp
+++ b/gdb/testsuite/gdb.base/attach.exp
@@ -467,13 +467,9 @@ proc_with_prefix do_command_attach_tests {} {
 
     gdb_exit
 
-    set res [gdb_spawn_with_cmdline_opts \
-		 "-quiet -iex \"set height 0\" -iex \"set width 0\" --pid=$testpid"]
     set test "starting with --pid"
-    gdb_test_multiple "" $test {
-	-re "Reading symbols from.*$gdb_prompt $" {
-	    pass "$test"
-	}
+    if { [gdb_spawn_attach $testpid] } {
+	pass $test
     }
 
     # Get rid of the process
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 7c35fbb18c4..fbab5a930b5 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -5172,6 +5172,51 @@ proc gdb_attach { testpid args } {
     return 0
 }
 
+# Start gdb with --pid $TESTPID and wait for the prompt.  Return 1 if GDB
+# managed to start and attach to the process, 0 otherwise.
+
+proc gdb_spawn_attach { testpid } {
+    global gdb_prompt
+
+    if { ![isnative] } {
+	unsupported "spawn with --pid"
+	return 0
+    }
+
+    set res [gdb_spawn_with_cmdline_opts "--pid=$testpid"]
+    if { $res != 0 } {
+	return 0
+    }
+
+    gdb_test_multiple "" "spawn --pid=\$pid" {
+	-re "ptrace: Operation not permitted\\." {
+	    untested "$gdb_test_name (operation not permitted)"
+	    return 0
+	}
+	-re "ptrace: No such process\\." {
+	    fail "$gdb_test_name"
+	    return 0
+	}
+	-re "Attaching to process $testpid\r\n.*\r\n$gdb_prompt $" {
+	    pass $gdb_test_name
+	}
+    }
+
+    # Check that we actually attached to a process, in case the
+    # error message is not catched by the patterns above.
+    gdb_test_multiple "info thread" "" {
+	-re "No threads\\.\r\n$gdb_prompt $" {
+	    fail $gdb_test_name
+	}
+	-re "Id.*$gdb_prompt $" {
+	    pass $gdb_test_name
+	    return 1
+	}
+    }
+
+    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.

base-commit: 75b2a443d515f531299ce1ed942810edd5bf5d84
-- 
2.25.1

Re: [PATCH] gdb/testsuite: Introduce and use gdb_spawn_attach

[Simon Marchi]

On 2022-04-14 07:32, Lancelot SIX via Gdb-patches wrote:
> Hi,
> 
> In a downstream port of GDB, I have tests which run `gdb --pid=$PID`,
> and have to support ptrace restrictions as it was introduced in
> a7e6a19e87f3d719ea23c65b580a6d9bca4ccab3 for the attach command.  
> 
> As there is a similar testcase in gdb.base/attach.exp, this patch
> proposes to add the gdb_spawn_attach helper method to factorize the
> error handling.
> 
> All feedbacks welcome,
> Best,
> Lancelot.
> 
> ---
> 
> Following a7e6a19e87f3d719ea23c65b580a6d9bca4ccab3 "gdb: testsuite: add
> new gdb_attach to check "attach" command", this commit proposes to
> introduce the gdb_spawn_attach helper and use it in gdb.base/attach.exp.
> 
> This helper starts GDB and addr the "--pid=$PID" argument.
> 
> Tested on x86_64-gnu-linux
> 
> Change-Id: I1fdcdb71c86d9c5d34bb28fc86fac68bcec37358
> ---
>  gdb/testsuite/gdb.base/attach.exp |  8 ++----
>  gdb/testsuite/lib/gdb.exp         | 45 +++++++++++++++++++++++++++++++
>  2 files changed, 47 insertions(+), 6 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.base/attach.exp b/gdb/testsuite/gdb.base/attach.exp
> index d01060aba53..3869a7b06e8 100644
> --- a/gdb/testsuite/gdb.base/attach.exp
> +++ b/gdb/testsuite/gdb.base/attach.exp
> @@ -467,13 +467,9 @@ proc_with_prefix do_command_attach_tests {} {
>  
>      gdb_exit
>  
> -    set res [gdb_spawn_with_cmdline_opts \
> -		 "-quiet -iex \"set height 0\" -iex \"set width 0\" --pid=$testpid"]
>      set test "starting with --pid"
> -    gdb_test_multiple "" $test {
> -	-re "Reading symbols from.*$gdb_prompt $" {
> -	    pass "$test"
> -	}
> +    if { [gdb_spawn_attach $testpid] } {
> +	pass $test
>      }
>  
>      # Get rid of the process
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index 7c35fbb18c4..fbab5a930b5 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -5172,6 +5172,51 @@ proc gdb_attach { testpid args } {
>      return 0
>  }
>  
> +# Start gdb with --pid $TESTPID and wait for the prompt.  Return 1 if GDB
> +# managed to start and attach to the process, 0 otherwise.
> +
> +proc gdb_spawn_attach { testpid } {

I'd use proc_with_prefix.  This way, the tests done in here are in their
own namespace, and the names don't risk to clash with the tests done by
the caller.  Also, if the test name is:

    PASS: gdb.base/attach.exp: do_command_attach_tests: gdb_spawn_attach: info thread

... it makes it a bit more obvious that the "info thread" test was done
by the gdb_spawn_attach proc, if you are searching for it.

> +    global gdb_prompt

I suggest to use $::gdb_prompt, instead of the global keyword.  That
makes it more obvious at the point the variable is used that it is from
the global scope.  Also, it avoids leaving unnecessary global
declarations when refactoring.

> +
> +    if { ![isnative] } {
> +	unsupported "spawn with --pid"
> +	return 0
> +    }

I am not sure isnative is the right check here.  What is it we want to
guard against exactly?

isnative does this:


    proc isnative { } {
        global target_triplet
        global build_triplet

        return [string equal $build_triplet $target_triplet]
    }

I think we want to allow this to run if GDB (which runs on the host
board) runs on the same machine as the target program (which runs on the
target board)?  I am not sure how to check that.

If the host and target boards are both non-remote, we can run the test.
If the host and target boards are both remote, but on the same machine,
we could run the test, in theory.

can_spawn_for_attach does this:

     # 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.
     if [is_remote target] then {
         verbose -log "can't spawn for attach (target is remote)"
         return 0
     }

So it bails out as soon as the target is on a different system than the
build system.  This means it does not supoprt the "both host and target
boards are on the same remote machine".  I think it would be fair to do
the same in gdb_spawn_attach.  If somebody ever figures out how to make
it work for remote target boards, then they can adjust gdb_spawn_attach
too.

Presumably, a test using gdb_spawn_attach would have called
can_spawn_for_attach first, then spawn_wait_for_attach.  So if the setup
was not supported, can_spawn_for_attach would have returned false and we
would not be here.  So actually, I think that in gdb_spawn_attach, you
can do the same as spawn_wait_for_attach does:


     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"
     }


> +
> +    set res [gdb_spawn_with_cmdline_opts "--pid=$testpid"]
> +    if { $res != 0 } {
> +	return 0
> +    }

Heh, the different return conventions are making this really
complicated.  gdb_spawn returns 0 on success, 1 on failure.  But I think
that how you did makes more sense, when the return value is boolean, 0
for failure and 1 for success makes sense.  If the return value is an
integer status code, then 0 for success and non-0 for failure (with
different values meaning different things) make sense.  So, I think what
you did is fine, and I think that we should consider changing gdb_spawn
to reverse its return value.

If we take the error path here, do you know if it will have logged an
error or failure that will make discoverable that the test has failed?

> +    gdb_test_multiple "" "spawn --pid=\$pid" {

Avoid putting the pid in the test name, that causes different test runs
to have different names.  That makes it more difficult to compare test
names.  "start gdb with --pid" would be a good name, I think.

> +	-re "ptrace: Operation not permitted\\." {
> +	    untested "$gdb_test_name (operation not permitted)"
> +	    return 0
> +	}
> +	-re "ptrace: No such process\\." {
> +	    fail "$gdb_test_name"

You could append " (no such process)" to the test name.  That would make
it easier to spot which failure path was taken.

> +	    return 0
> +	}

In the two cases above, I think it would be good to consume up to the
prompt.  If the caller wants to do more tests / input more commands
after that, it's good practice to not leave data (especially a prompt)
in the expect buffer, as that would make a subsequent gdb_test probably
fail.

I think it would work with:

    -re -wrap "ptrace: Operation not permitted\\." {

    -re -wrap "ptrace: No such process\\." {


> +	-re "Attaching to process $testpid\r\n.*\r\n$gdb_prompt $" {

You could use -wrap instead of appending the prompt (I think it's
preferable):

    -re -wrap "Attaching to process $testpid\r\n.*" {

> +	    pass $gdb_test_name
> +	}
> +    }
> +
> +    # Check that we actually attached to a process, in case the
> +    # error message is not catched by the patterns above.

catched -> caught

> +    gdb_test_multiple "info thread" "" {
> +	-re "No threads\\.\r\n$gdb_prompt $" {
> +	    fail $gdb_test_name

Let's append " (no threads)" to the test name here.

> +	}
> +	-re "Id.*$gdb_prompt $" {
> +	    pass $gdb_test_name
> +	    return 1
> +	}
> +    }
> +
> +    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.
> 
> base-commit: 75b2a443d515f531299ce1ed942810edd5bf5d84

Re: [PATCH] gdb/testsuite: Introduce and use gdb_spawn_attach

[Pedro Alves]

On 2022-04-14 12:32, Lancelot SIX via Gdb-patches wrote:
> +# Start gdb with --pid $TESTPID and wait for the prompt.  Return 1 if GDB
> +# managed to start and attach to the process, 0 otherwise.
> +
> +proc gdb_spawn_attach { testpid } {

How about we name this gdb_spawn_attach_cmdline?  The that this this attaches
from the command line is important for the callers, and we wouldn't want someone
in the future to change the implementation to just spawn and then "attach".

Re: [PATCH] gdb/testsuite: Introduce and use gdb_spawn_attach

[Lancelot SIX]

Hi,

Thanks for the feedbacks.  I included answers / feedbacks below.  The 
parts I did not reply directly here will be modified as you suggest in 
the V2.

As per suggested by Pedro in a different mail, I'll also rename the proc 
to gdb_spawn_attach_cmdline to ensure that someone does not try to 
refactor this proc using gdb_attach at some point in the future.



>> +
>> +    if { ![isnative] } {
>> +     unsupported "spawn with --pid"
>> +     return 0
>> +    }
> 
> I am not sure isnative is the right check here.  What is it we want to
> guard against exactly?
> 
> isnative does this:
> 
> 
>      proc isnative { } {
>          global target_triplet
>          global build_triplet
> 
>          return [string equal $build_triplet $target_triplet]
>      }
> 
> I think we want to allow this to run if GDB (which runs on the host
> board) runs on the same machine as the target program (which runs on the
> target board)?  I am not sure how to check that.

This is what I thought isnative gave (and I also used it because 
isnative is currently what guards the testcase I refactor in 
gdb.base/attach.exp).

> 
> If the host and target boards are both non-remote, we can run the test.
> If the host and target boards are both remote, but on the same machine,
> we could run the test, in theory.
> 
> can_spawn_for_attach does this:
> 
>       # 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.
>       if [is_remote target] then {
>           verbose -log "can't spawn for attach (target is remote)"
>           return 0
>       }
> 
> So it bails out as soon as the target is on a different system than the
> build system.  This means it does not supoprt the "both host and target
> boards are on the same remote machine".  I think it would be fair to do
> the same in gdb_spawn_attach.  If somebody ever figures out how to make
> it work for remote target boards, then they can adjust gdb_spawn_attach
> too.
> 
> Presumably, a test using gdb_spawn_attach would have called
> can_spawn_for_attach first, then spawn_wait_for_attach.  So if the setup
> was not supported, can_spawn_for_attach would have returned false and we
> would not be here.  So actually, I think that in gdb_spawn_attach, you
> can do the same as spawn_wait_for_attach does:
> 
> 
>       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"
>       }
>

Fair enough.  Thanks, I'll do that.
> 
>> +
>> +    set res [gdb_spawn_with_cmdline_opts "--pid=$testpid"]
>> +    if { $res != 0 } {
>> +     return 0
>> +    }
> 
> Heh, the different return conventions are making this really
> complicated.  gdb_spawn returns 0 on success, 1 on failure.  But I think
> that how you did makes more sense, when the return value is boolean, 0
> for failure and 1 for success makes sense.  If the return value is an
> integer status code, then 0 for success and non-0 for failure (with
> different values meaning different things) make sense.  So, I think what
> you did is fine, and I think that we should consider changing gdb_spawn
> to reverse its return value.
> 
> If we take the error path here, do you know if it will have logged an
> error or failure that will make discoverable that the test has failed?

It comes down to default_gdb_spawn which already use some perror MSG 
before it return non-0.  So from what I can see, it is already covered.

> 
>> +    gdb_test_multiple "" "spawn --pid=\$pid" {
> 
> Avoid putting the pid in the test name, that causes different test runs
> to have different names.  That makes it more difficult to compare test
> names.  "start gdb with --pid" would be a good name, I think.

Here, the the $ is escaped, so '$pid' is part of the test name, not the 
actual pid number.  I am ok with the test name you propose anyway.

Best,
Lancelot.

Re: [PATCH] gdb/testsuite: Introduce and use gdb_spawn_attach

[Simon Marchi]

On 2022-04-14 09:32, Lancelot SIX wrote:
> Hi,
> 
> Thanks for the feedbacks.  I included answers / feedbacks below.  The parts I did not reply directly here will be modified as you suggest in the V2.
> 
> As per suggested by Pedro in a different mail, I'll also rename the proc to gdb_spawn_attach_cmdline to ensure that someone does not try to refactor this proc using gdb_attach at some point in the future.
> 
> 
> 
>>> +
>>> +    if { ![isnative] } {
>>> +     unsupported "spawn with --pid"
>>> +     return 0
>>> +    }
>>
>> I am not sure isnative is the right check here.  What is it we want to
>> guard against exactly?
>>
>> isnative does this:
>>
>>
>>      proc isnative { } {
>>          global target_triplet
>>          global build_triplet
>>
>>          return [string equal $build_triplet $target_triplet]
>>      }
>>
>> I think we want to allow this to run if GDB (which runs on the host
>> board) runs on the same machine as the target program (which runs on the
>> target board)?  I am not sure how to check that.
> 
> This is what I thought isnative gave (and I also used it because isnative is currently what guards the testcase I refactor in gdb.base/attach.exp).
> 
>>
>> If the host and target boards are both non-remote, we can run the test.
>> If the host and target boards are both remote, but on the same machine,
>> we could run the test, in theory.
>>
>> can_spawn_for_attach does this:
>>
>>       # 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.
>>       if [is_remote target] then {
>>           verbose -log "can't spawn for attach (target is remote)"
>>           return 0
>>       }
>>
>> So it bails out as soon as the target is on a different system than the
>> build system.  This means it does not supoprt the "both host and target
>> boards are on the same remote machine".  I think it would be fair to do
>> the same in gdb_spawn_attach.  If somebody ever figures out how to make
>> it work for remote target boards, then they can adjust gdb_spawn_attach
>> too.
>>
>> Presumably, a test using gdb_spawn_attach would have called
>> can_spawn_for_attach first, then spawn_wait_for_attach.  So if the setup
>> was not supported, can_spawn_for_attach would have returned false and we
>> would not be here.  So actually, I think that in gdb_spawn_attach, you
>> can do the same as spawn_wait_for_attach does:
>>
>>
>>       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"
>>       }
>>
> 
> Fair enough.  Thanks, I'll do that.
>>
>>> +
>>> +    set res [gdb_spawn_with_cmdline_opts "--pid=$testpid"]
>>> +    if { $res != 0 } {
>>> +     return 0
>>> +    }
>>
>> Heh, the different return conventions are making this really
>> complicated.  gdb_spawn returns 0 on success, 1 on failure.  But I think
>> that how you did makes more sense, when the return value is boolean, 0
>> for failure and 1 for success makes sense.  If the return value is an
>> integer status code, then 0 for success and non-0 for failure (with
>> different values meaning different things) make sense.  So, I think what
>> you did is fine, and I think that we should consider changing gdb_spawn
>> to reverse its return value.
>>
>> If we take the error path here, do you know if it will have logged an
>> error or failure that will make discoverable that the test has failed?
> 
> It comes down to default_gdb_spawn which already use some perror MSG before it return non-0.  So from what I can see, it is already covered.

Hmm, I think that perror just influences the result of the following
test to be recorded, right?  So if there is not subsequent gdb_test,
because the callers just return and do nothing, will the failure be
visible?  Maybe it is, I just want to be sure.

> 
>>
>>> +    gdb_test_multiple "" "spawn --pid=\$pid" {
>>
>> Avoid putting the pid in the test name, that causes different test runs
>> to have different names.  That makes it more difficult to compare test
>> names.  "start gdb with --pid" would be a good name, I think.
> 
> Here, the the $ is escaped, so '$pid' is part of the test name, not the actual pid number.  I am ok with the test name you propose anyway.

Ah, good point, missed that.  Then that's fine.

Simon

Re: [PATCH] gdb/testsuite: Introduce and use gdb_spawn_attach

[Lancelot SIX]

>>
>> It comes down to default_gdb_spawn which already use some perror MSG before it return non-0.  So from what I can see, it is already covered.
> 
> Hmm, I think that perror just influences the result of the following
> test to be recorded, right?  So if there is not subsequent gdb_test,
> because the callers just return and do nothing, will the failure be
> visible?  Maybe it is, I just want to be sure.
> 

It actually does both[1].  It prints the message and affect the next 
pass/fail.

In such case, I guess I should also issue a fail before returning, so it 
does not affect subsequent tests.  The fail fill report as UNRESOLVED.

Something like

     set res [gdb_spawn_with_cmdline_opts "--pid=$testpid"]
     if { $res != 0 } {
	fail "start gdb with --pid"
	return 0
     }


[1] https://www.gnu.org/software/dejagnu/manual/perror-procedure.html