[PATCH] gdb/testsuite: implement gdb_spawn_attach_cmdline for the native-extended-gdbserver board

[PATCH] gdb/testsuite: implement gdb_spawn_attach_cmdline for the native-extended-gdbserver board

[Simon Marchi]

I noticed these failures when running gdb.base/attach.exp with the
native-extended-gdbserver board:

    FAIL: gdb.base/attach.exp: do_command_attach_tests: gdb_spawn_attach_cmdline: start gdb with --pid
    FAIL: gdb.base/attach.exp: do_command_attach_tests: gdb_spawn_attach_cmdline: info thread (no thread)

This isn't new, but since the tests changed names, that made me notice them.

The problem is that we run a "gdb -p PID" test, along with "-iex set
auto-connect-native-target off" (this comes from the
native-extended-gdbserver board, to avoid connecting to the native
target by mistake), so there is no target to use for attach:

    builtin_spawn /home/simark/build/binutils-gdb-one-target/gdb/testsuite/../../gdb/gdb -nw -nx -iex set height 0 -iex set width 0 -data-directory /home/simark/build/binutils-gdb-o    ne-target/gdb/testsuite/../data-directory -iex set auto-connect-native-target off -iex set sysroot -quiet --pid=567500^M
    Don't know how to attach.  Try "help target".^M
    (gdb) FAIL: gdb.base/attach.exp: do_command_attach_tests: gdb_spawn_attach_cmdline: start gdb with --pid
    info thread^M
    No threads.^M
    (gdb) FAIL: gdb.base/attach.exp: do_command_attach_tests: gdb_spawn_attach_cmdline: info thread (no thread)

The isnative check in do_command_attach_tests seems to be intended to
avoid running that test when using a board that doesn't use the native
target (like native-extended-gdbserver), but if so that isn't the right
check.

An option would be to just fix that check, probably replacing it with
gdb_is_target_remote or something.

Another idea (implemented by this patch) is to provide an alternative
implementation for gdb_spawn_attach_cmdline, when testing against
GDBserver.  There are two behavios we could implement:

1) start GDB with

     -iex "target extended-remote PORT" --pid PID

   That makes GDB connect to the remote target before attaching, so that
   works.

2) make GDBserver attach with --attach, and then make GDB connect
   normally

I think that both would be in the spirit of gdb_spawn_attach_cmdline,
they would just exercise different things.  But 2) was easier to
implement, so I went with that.

Idea 1) is easy to get going, we just need to override
gdb_spawn_attach_cmdline in boards/native-extended-gdbserver.exp to
append `-iex "target extended-remote ..."` while calling the original
gdb_spawn_attach_cmdline.  But it is harder to detect the "operation not
permitted" case, because that is printed on GDBserver's terminal, and
GDBserver is already dead when returning from the original
gdb_spawn_attach_cmdline.

So implementation details:

 - Add a new gdbserver_start_attach proc, which is a new way to start
   GDBserver (with --attach).  Like gdbserver_start_multi, this proc
   connects GDB to GDBserver.  Its return convention is a bit ugly, but
   I'm not sure how to make things cleaner in TCL.  And the values are
   aligned with the existing gdbserver_start_multi.
 - Override gdb_spawn_attach_cmdline for the native-extended-gdbserver
   board.  It starts GDB and then calls gdbserver_start_attach.
 - To share the "do we have a thread" verification with the original
   gdb_spawn_attach_cmdline, factor this part out in a
   gdb_spawn_attach_cmdline_post helper proc.
 - In do_command_attach_tests, add a gdb_exit.  This isn't necessary,
   but I noticed some warnings about GDBserver not being able to reap
   the child process.  This happens because we otherwise kill the child
   process outside of GDBserver before GDBserver attempts to detach it.
   It seems cleaner to detach the process first, then kill it.
 - To catch the "operation not permitted" case (if ptrace_scope is
   non-0), make gdbserver_start expect that message.  Make it throw an
   "operation not permitted" error in that case.  I chose this because
   gdbserver_start isn't set up to return an error status, and I don't
   want to change its return value, since it is used at a lot of places.
 - Remove the isnative check in do_command_attach_tests, since it's not
   useful.

With ptrace_scope set to 0, I get:

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

With ptrace_scope set to 1, I get:

    UNSUPPORTED: gdb.base/attach.exp: do_command_attach_tests: gdb_spawn_attach_cmdline: start gdbserver with --attach

Note that there are still many other failures in gdb.base/attach.exp if
ptrace_scope is set to 1.

Change-Id: Ie7f35347769381674300e43a124ab68047a27274
---
 .../boards/native-extended-gdbserver.exp      | 24 +++++++++++++
 gdb/testsuite/gdb.base/attach.exp             |  8 ++---
 gdb/testsuite/lib/gdb.exp                     | 29 ++++++++++------
 gdb/testsuite/lib/gdbserver-support.exp       | 34 ++++++++++++++++++-
 4 files changed, 78 insertions(+), 17 deletions(-)

diff --git a/gdb/testsuite/boards/native-extended-gdbserver.exp b/gdb/testsuite/boards/native-extended-gdbserver.exp
index b925c364993f..e2942e435e15 100644
--- a/gdb/testsuite/boards/native-extended-gdbserver.exp
+++ b/gdb/testsuite/boards/native-extended-gdbserver.exp
@@ -143,3 +143,27 @@ proc mi_gdb_load { arg } {
 
     return 0
 }
+
+
+# Alternative implementation of gdb_spawn_attach_cmdline when running with this
+# board.
+#
+# Start GDBserver with --attach to attach to TESTPID, then connect GDB to it.
+
+# We don't need the original gdb_spawn_attach_cmdline.
+rename gdb_spawn_attach_cmdline ""
+
+proc_with_prefix gdb_spawn_attach_cmdline { testpid } {
+    default_gdb_start
+    set res [gdbserver_start_attach $testpid]
+
+    if { $res == 3 } {
+	# Failed to attach due to a permission error.
+	unsupported "start gdbserver with --attach"
+	return 0
+    }
+
+    # This verifies that the attach worked (that we have at least one thread)
+    # and issues a PASS/FAIL.
+    return [gdb_spawn_attach_cmdline_post]
+}
diff --git a/gdb/testsuite/gdb.base/attach.exp b/gdb/testsuite/gdb.base/attach.exp
index 7b661e99a1c8..7f5dd7e7f1b5 100644
--- a/gdb/testsuite/gdb.base/attach.exp
+++ b/gdb/testsuite/gdb.base/attach.exp
@@ -457,11 +457,6 @@ proc_with_prefix do_command_attach_tests {} {
     global gdb_prompt
     global binfile
 
-    if ![isnative] then {
-	unsupported "command attach test"
-	return 0
-    }
-
     set test_spawn_id [spawn_wait_for_attach $binfile]
     set testpid [spawn_id_get_pid $test_spawn_id]
 
@@ -471,6 +466,9 @@ proc_with_prefix do_command_attach_tests {} {
     # call pass/fail here.
     gdb_spawn_attach_cmdline $testpid
 
+    # Let GDB / GDBserver detach cleanly the process before we kill it.
+    gdb_exit
+
     # Get rid of the process
     kill_wait_spawned_process $test_spawn_id
 }
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 47cb2b236766..34b979aa6fe1 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -5186,6 +5186,23 @@ proc gdb_attach { testpid args } {
     return 0
 }
 
+# Helper for gdb_spawn_attach_cmdline implementations, verify that we have a
+# thread.  Issue a PASS/FAIL.  Return 1 on success, 0 on failure.
+
+proc gdb_spawn_attach_cmdline_post { } {
+    gdb_test_multiple "info thread" "" {
+	-re -wrap "No threads\\." {
+	    fail "$gdb_test_name (no thread)"
+	}
+	-re -wrap "Id.*" {
+	    pass $gdb_test_name
+	    return 1
+	}
+    }
+
+    return 0
+}
+
 # Start gdb with "--pid $TESTPID" on the command line and wait for the prompt.
 # Return 1 if GDB managed to start and attach to the process, 0 otherwise.
 
@@ -5219,17 +5236,7 @@ proc_with_prefix gdb_spawn_attach_cmdline { testpid } {
 
     # Check that we actually attached to a process, in case the
     # error message is not caught by the patterns above.
-    gdb_test_multiple "info thread" "" {
-	-re -wrap "No threads\\." {
-	    fail "$gdb_test_name (no thread)"
-	}
-	-re -wrap "Id.*" {
-	    pass $gdb_test_name
-	    return 1
-	}
-    }
-
-    return 0
+    return [gdb_spawn_attach_cmdline_post]
 }
 
 # Kill a progress previously started with spawn_wait_for_attach, and
diff --git a/gdb/testsuite/lib/gdbserver-support.exp b/gdb/testsuite/lib/gdbserver-support.exp
index 08e529fa9857..87cff7ee9b31 100644
--- a/gdb/testsuite/lib/gdbserver-support.exp
+++ b/gdb/testsuite/lib/gdbserver-support.exp
@@ -238,7 +238,10 @@ proc gdbserver_default_get_comm_port { port } {
 # Start a gdbserver process with initial OPTIONS and trailing ARGUMENTS.
 # The port will be filled in between them automatically.
 #
-# Returns the target protocol and socket to connect to.
+# On success, return the target protocol and socket to connect to.
+#
+# On failure, throw an error.  If failing to attach due to a permission error,
+# the error is "operation not permitted".
 
 proc gdbserver_start { options arguments } {
     global portnum
@@ -376,6 +379,11 @@ proc gdbserver_start { options arguments } {
 	    -re ".*: cannot resolve name: .*\r\n" {
 		error "gdbserver cannot resolve name."
 	    }
+
+	    -re "Cannot attach to process\[^\r\n\]+Operation not permitted\[^\r\n\]*\r\n" {
+		error "operation not permitted"
+	    }
+
 	    timeout {
 		error "Timeout waiting for gdbserver response."
 	    }
@@ -692,3 +700,27 @@ proc gdb_debug_init { } {
     # Now call the standard debug init function
     _gdb_debug_init
 }
+
+# Start and connect to a gdbserver with --attach to attach to PID.
+#
+# Return:
+#
+#   - 0 on success.
+#   - 1 if gdb_target_cmd failed
+#   - 2 if GDBserver failed to start for an unknown reason
+#   - 3 if GDBserver failed to start due to an attach permission error
+
+proc gdbserver_start_attach { pid } {
+    if { [catch { gdbserver_start "--attach" "$pid" } res] == 1 } {
+	if { $res == "operation not permitted" } {
+	    return 3
+	}
+
+	return 2
+    }
+
+    set ::gdbserver_protocol [lindex $res 0]
+    set ::gdbserver_gdbport [lindex $res 1]
+
+    return [gdb_target_cmd $::gdbserver_protocol $::gdbserver_gdbport]
+}

base-commit: 6a3c1573cc11c613b2f7509aba3c2ea68c661721
-- 
2.35.2

Re: [PATCH] gdb/testsuite: implement gdb_spawn_attach_cmdline for the native-extended-gdbserver board

[Pedro Alves]

On 2022-04-22 17:21, Simon Marchi via Gdb-patches wrote:
> I noticed these failures when running gdb.base/attach.exp with the
> native-extended-gdbserver board:
> 
>     FAIL: gdb.base/attach.exp: do_command_attach_tests: gdb_spawn_attach_cmdline: start gdb with --pid
>     FAIL: gdb.base/attach.exp: do_command_attach_tests: gdb_spawn_attach_cmdline: info thread (no thread)
> 
> This isn't new, but since the tests changed names, that made me notice them.
> 
> The problem is that we run a "gdb -p PID" test, along with "-iex set
> auto-connect-native-target off" (this comes from the
> native-extended-gdbserver board, to avoid connecting to the native
> target by mistake), so there is no target to use for attach:
> 
>     builtin_spawn /home/simark/build/binutils-gdb-one-target/gdb/testsuite/../../gdb/gdb -nw -nx -iex set height 0 -iex set width 0 -data-directory /home/simark/build/binutils-gdb-o    ne-target/gdb/testsuite/../data-directory -iex set auto-connect-native-target off -iex set sysroot -quiet --pid=567500^M
>     Don't know how to attach.  Try "help target".^M
>     (gdb) FAIL: gdb.base/attach.exp: do_command_attach_tests: gdb_spawn_attach_cmdline: start gdb with --pid
>     info thread^M
>     No threads.^M
>     (gdb) FAIL: gdb.base/attach.exp: do_command_attach_tests: gdb_spawn_attach_cmdline: info thread (no thread)
> 
> The isnative check in do_command_attach_tests seems to be intended to
> avoid running that test when using a board that doesn't use the native
> target (like native-extended-gdbserver), but if so that isn't the right
> check.
> 
> An option would be to just fix that check, probably replacing it with
> gdb_is_target_remote or something.
> 
> Another idea (implemented by this patch) is to provide an alternative
> implementation for gdb_spawn_attach_cmdline, when testing against
> GDBserver.  There are two behavios we could implement:
> 

behavios -> behaviors

> 1) start GDB with
> 
>      -iex "target extended-remote PORT" --pid PID
> 
>    That makes GDB connect to the remote target before attaching, so that
>    works.
> 
> 2) make GDBserver attach with --attach, and then make GDB connect
>    normally
> 
> I think that both would be in the spirit of gdb_spawn_attach_cmdline,
> they would just exercise different things.  But 2) was easier to
> implement, so I went with that.
> 

I really think this is the wrong choice for native-extended-gdbserver.
It might be a good choice for native-gdbserver, though.

The reason I think it is the wrong choice is because in principle, testing
with native-extended-gdbserver should emulate as much as possible
what you'd get if we eliminated GDB's native target and changed GDB to spawn
gdbserver behind the scenes instead.  Tests using gdb_spawn_attach_cmdline
really want to exercise that GDB attaches correctly from the command line,
before the first prompt is reached, as that scenario may run into
badness in GDB's prompt management and readline.  While variant #1 would
exercise this with native-extended-gdbserver too, variant 2 does not, it just
adds more testing of gdbserver --attach, which should be covered by
gdb.server/ tests anyhow.

Re: [PATCH] gdb/testsuite: implement gdb_spawn_attach_cmdline for the native-extended-gdbserver board

[Simon Marchi]

On 2022-05-02 12:16, Pedro Alves wrote:
> On 2022-04-22 17:21, Simon Marchi via Gdb-patches wrote:
>> I noticed these failures when running gdb.base/attach.exp with the
>> native-extended-gdbserver board:
>>
>>     FAIL: gdb.base/attach.exp: do_command_attach_tests: gdb_spawn_attach_cmdline: start gdb with --pid
>>     FAIL: gdb.base/attach.exp: do_command_attach_tests: gdb_spawn_attach_cmdline: info thread (no thread)
>>
>> This isn't new, but since the tests changed names, that made me notice them.
>>
>> The problem is that we run a "gdb -p PID" test, along with "-iex set
>> auto-connect-native-target off" (this comes from the
>> native-extended-gdbserver board, to avoid connecting to the native
>> target by mistake), so there is no target to use for attach:
>>
>>     builtin_spawn /home/simark/build/binutils-gdb-one-target/gdb/testsuite/../../gdb/gdb -nw -nx -iex set height 0 -iex set width 0 -data-directory /home/simark/build/binutils-gdb-o    ne-target/gdb/testsuite/../data-directory -iex set auto-connect-native-target off -iex set sysroot -quiet --pid=567500^M
>>     Don't know how to attach.  Try "help target".^M
>>     (gdb) FAIL: gdb.base/attach.exp: do_command_attach_tests: gdb_spawn_attach_cmdline: start gdb with --pid
>>     info thread^M
>>     No threads.^M
>>     (gdb) FAIL: gdb.base/attach.exp: do_command_attach_tests: gdb_spawn_attach_cmdline: info thread (no thread)
>>
>> The isnative check in do_command_attach_tests seems to be intended to
>> avoid running that test when using a board that doesn't use the native
>> target (like native-extended-gdbserver), but if so that isn't the right
>> check.
>>
>> An option would be to just fix that check, probably replacing it with
>> gdb_is_target_remote or something.
>>
>> Another idea (implemented by this patch) is to provide an alternative
>> implementation for gdb_spawn_attach_cmdline, when testing against
>> GDBserver.  There are two behavios we could implement:
>>
> 
> behavios -> behaviors
> 
>> 1) start GDB with
>>
>>      -iex "target extended-remote PORT" --pid PID
>>
>>    That makes GDB connect to the remote target before attaching, so that
>>    works.
>>
>> 2) make GDBserver attach with --attach, and then make GDB connect
>>    normally
>>
>> I think that both would be in the spirit of gdb_spawn_attach_cmdline,
>> they would just exercise different things.  But 2) was easier to
>> implement, so I went with that.
>>
> 
> I really think this is the wrong choice for native-extended-gdbserver.
> It might be a good choice for native-gdbserver, though.
> 
> The reason I think it is the wrong choice is because in principle, testing
> with native-extended-gdbserver should emulate as much as possible
> what you'd get if we eliminated GDB's native target and changed GDB to spawn
> gdbserver behind the scenes instead.  Tests using gdb_spawn_attach_cmdline
> really want to exercise that GDB attaches correctly from the command line,
> before the first prompt is reached, as that scenario may run into
> badness in GDB's prompt management and readline.  While variant #1 would
> exercise this with native-extended-gdbserver too, variant 2 does not, it just
> adds more testing of gdbserver --attach, which should be covered by
> gdb.server/ tests anyhow.

Yeah, I agree this is probably more useful.  I'll give it another shot
and try to come up with an explanation of what was giving me trouble
when trying to implement this.

Simon