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

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

[Tiezhu Yang]

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

[PATCH 1/3] gdb: testsuite: leave can_spawn_for_attach alone

[Tiezhu Yang]

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

[PATCH 2/3] gdb: testsuite: add new gdb_attach to fix attach-pie-noexec.exp

[Tiezhu Yang]

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

[PATCH 3/3] gdb: testsuite: enhance 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

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

Re: [PATCH 1/3] gdb: testsuite: leave can_spawn_for_attach alone

[Simon Marchi]

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

Re: [PATCH 2/3] gdb: testsuite: add new gdb_attach to fix attach-pie-noexec.exp

[Simon Marchi]

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

Re: [PATCH 3/3] gdb: testsuite: enhance gdb_attach to fix jit-elf.exp

[Simon Marchi]

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