[PATCH v2 0/4] gdb: add gdb_attach to fix failed testcases

[PATCH v2 0/4] gdb: add gdb_attach to fix failed testcases

[Tiezhu Yang]

Thank Simon very much.

Tiezhu Yang (4):
  gdb: testsuite: remove attach test from can_spawn_for_attach
  gdb: testsuite: add new gdb_attach to check "attach" command
  gdb: testsuite: use gdb_attach to fix attach-pie-noexec.exp
  gdb: testsuite: use 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                    | 68 +++++++++++-----------------
 3 files changed, 34 insertions(+), 47 deletions(-)

-- 
2.1.0

[PATCH v2 1/4] gdb: testsuite: remove attach test from can_spawn_for_attach

[Tiezhu Yang]

As Pedro Alves said, caching procs should not issue pass/fail [1],
this commit removes attach test from can_spawn_for_attach, at the
same time, use "verbose -log" instead of "unsupported" to get a
trace about why a test run doesn't support spawning 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 | 52 ++++++++---------------------------------------
 1 file changed, 9 insertions(+), 43 deletions(-)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 08726f7..f508ac6 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -5090,12 +5090,12 @@ 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.
     if [is_remote target] then {
-	unsupported "skip attach tests (target is remote)"
+	verbose -log "skip attach tests (target is remote)"
 	return 0
     }
 
@@ -5103,50 +5103,10 @@ gdb_caching_proc can_spawn_for_attach {
     # stub-like, where GDB finds the program already started on
     # initial connection.
     if {[target_info exists use_gdb_stub]} {
-	unsupported "skip attach tests (target is stub)"
+	verbose -log "skip attach tests (target is stub)"
 	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
 }
@@ -5196,6 +5156,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

Re: [PATCH v2 1/4] gdb: testsuite: remove attach test from can_spawn_for_attach

[Simon Marchi]

> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index 08726f7..f508ac6 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -5090,12 +5090,12 @@ 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.
>      if [is_remote target] then {
> -	unsupported "skip attach tests (target is remote)"
> +	verbose -log "skip attach tests (target is remote)"

Since we're not actually skipping any tests here, maybe change the
actual message to

  "can't spawn for attach (target is remote)"

Same below.

LGTM with that fixed.

Simon

[PATCH v2 2/4] gdb: testsuite: add new gdb_attach to check "attach" command

[Tiezhu Yang]

This commit adds new gdb_attach to centralize the failure checking of
"attach" command. Return 0 if attach failed, otherwise return 1.

Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 gdb/testsuite/lib/gdb.exp | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index f508ac6..823828f 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -5111,6 +5111,26 @@ proc can_spawn_for_attach { } {
     return 1
 }
 
+# Centralize the failure checking of "attach" command.
+# Return 0 if attach failed, otherwise return 1.
+
+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
+	}
+    }
+}
+
 # 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

Re: [PATCH v2 2/4] gdb: testsuite: add new gdb_attach to check "attach" command

[Simon Marchi]

On 2022-03-17 00:43, Tiezhu Yang wrote:
> This commit adds new gdb_attach to centralize the failure checking of
> "attach" command. Return 0 if attach failed, otherwise return 1.
> 
> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>

LGTM.

Simon

Re: [PATCH v2 2/4] gdb: testsuite: add new gdb_attach to check "attach" command

[Simon Marchi]

On 2022-03-17 07:51, Simon Marchi via Gdb-patches wrote:
> 
> 
> On 2022-03-17 00:43, Tiezhu Yang wrote:
>> This commit adds new gdb_attach to centralize the failure checking of
>> "attach" command. Return 0 if attach failed, otherwise return 1.
>>
>> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> 
> LGTM.
> 
> Simon

Just a precision, please wait before pushing this until Pedro has time
to review the series.

Simon

Re: [PATCH v2 2/4] gdb: testsuite: add new gdb_attach to check "attach" command

[Tiezhu Yang]

On 03/17/2022 08:02 PM, Simon Marchi wrote:
>
>
> On 2022-03-17 07:51, Simon Marchi via Gdb-patches wrote:
>>
>>
>> On 2022-03-17 00:43, Tiezhu Yang wrote:
>>> This commit adds new gdb_attach to centralize the failure checking of
>>> "attach" command. Return 0 if attach failed, otherwise return 1.
>>>
>>> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
>>
>> LGTM.
>>
>> Simon
>
> Just a precision, please wait before pushing this until Pedro has time
> to review the series.

OK, thank you, I will send v3 later.

Patch #1 and #3 need to do a little changes, patch #4 need to do some 
more changes as you said:

"I think that the clean_reattach proc should return a
value to indicate whether it worked, and the callers should return early
as well on failure, all the way up to the top-level of the test."

Thanks,
Tiezhu

[PATCH v2 3/4] gdb: testsuite: use gdb_attach to fix attach-pie-noexec.exp

[Tiezhu Yang]

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 +++-
 1 file changed, 3 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.*"
 
-- 
2.1.0

Re: [PATCH v2 3/4] gdb: testsuite: use gdb_attach to fix attach-pie-noexec.exp

[Simon Marchi]

On 2022-03-17 00:43, 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/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 +++-
>  1 file changed, 3 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
> +}

You can do just "return", I don't think that return value is used
anywhere.

LGTM with that.

Simon

[PATCH v2 4/4] gdb: testsuite: use gdb_attach to fix jit-elf.exp

[Tiezhu Yang]

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

use gdb_attach to fix the above issue.

Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 gdb/testsuite/gdb.base/jit-elf.exp | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/gdb/testsuite/gdb.base/jit-elf.exp b/gdb/testsuite/gdb.base/jit-elf.exp
index 8a4c5b7..61ddb2a 100644
--- a/gdb/testsuite/gdb.base/jit-elf.exp
+++ b/gdb/testsuite/gdb.base/jit-elf.exp
@@ -57,11 +57,10 @@ 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"
-	}
+    if {![gdb_attach $testpid {
+	"pattern" "main.*at .*$::main_srcfile:.*"
+    }]} {
+	return
     }
 
     gdb_test_no_output "set var wait_for_gdb = 0"
-- 
2.1.0

Re: [PATCH v2 4/4] gdb: testsuite: use gdb_attach to fix jit-elf.exp

[Simon Marchi]

On 2022-03-17 00:43, 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
> 
> use gdb_attach to fix the above issue.
> 
> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> ---
>  gdb/testsuite/gdb.base/jit-elf.exp | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.base/jit-elf.exp b/gdb/testsuite/gdb.base/jit-elf.exp
> index 8a4c5b7..61ddb2a 100644
> --- a/gdb/testsuite/gdb.base/jit-elf.exp
> +++ b/gdb/testsuite/gdb.base/jit-elf.exp
> @@ -57,11 +57,10 @@ 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"
> -	}
> +    if {![gdb_attach $testpid {
> +	"pattern" "main.*at .*$::main_srcfile:.*"
> +    }]} {
> +	return
>      }

I suggest writing it like this, it will be more readable:


    set attach_opts {
	"pattern" "main.*at .*$::main_srcfile:.*"
    }

    if { ![gdb_attach $testpid $attach_opts] } {
	return
    }

I still get some failures when I run with ptrace_scope=1, meaning we are
missing something.  I think that the clean_reattach proc should return a
value to indicate whether it worked, and the callers should return early
as well on failure, all the way up to the top-level of the test.

Simon