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

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

[Tiezhu Yang]

Hi Simon and Pedro,

Thank you very much for your suggestions.
Please review this v3 patchset.
Any comments will be much appreciated.

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           | 27 +++++++----
 gdb/testsuite/lib/gdb.exp                    | 68 +++++++++++-----------------
 3 files changed, 49 insertions(+), 50 deletions(-)

-- 
2.1.0

[PATCH v3 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..89dcd0a 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 "can't spawn for attach (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 "can't spawn for attach (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

[PATCH v3 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 89dcd0a..abbe79e 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

[PATCH v3 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..a916841 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
+}
 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

[PATCH v3 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, at the same time, the clean_reattach
proc should return a value to indicate whether it worked, and the callers
should return early as well on failure.

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

diff --git a/gdb/testsuite/gdb.base/jit-elf.exp b/gdb/testsuite/gdb.base/jit-elf.exp
index 8a4c5b7..e63f086 100644
--- a/gdb/testsuite/gdb.base/jit-elf.exp
+++ b/gdb/testsuite/gdb.base/jit-elf.exp
@@ -38,6 +38,7 @@ set jit_solib_basename jit-elf-solib
 set jit_solib_srcfile ${srcdir}/${subdir}/${jit_solib_basename}.c
 
 # Detach, restart GDB, and re-attach to the program.
+# Return 0 if attach failed, otherwise return 1.
 proc clean_reattach {} {
     global decimal gdb_prompt
     global main_binfile main_srcfile
@@ -57,18 +58,21 @@ 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"
-	}
+    set attach_opts {
+	"pattern" "main.*at .*$::main_srcfile:.*"
+    }
+
+    if { ![gdb_attach $testpid $attach_opts] } {
+	return 0
     }
 
     gdb_test_no_output "set var wait_for_gdb = 0"
+    return 1
 }
 
 # Continue to LOCATION in the program.  If REATTACH, detach and
 # re-attach to the program from scratch.
+# Return 0 if clean_reattach failed, otherwise return 1.
 proc continue_to_test_location {location reattach} {
     global main_srcfile
 
@@ -76,9 +80,12 @@ proc continue_to_test_location {location reattach} {
     gdb_continue_to_breakpoint $location
     if {$reattach} {
 	with_test_prefix "$location" {
-	    clean_reattach
+	    if { ![clean_reattach] } {
+		return 0
+	    }
 	}
     }
+    return 1
 }
 
 proc one_jit_test {jit_solibs_target match_str reattach} {
@@ -114,7 +121,9 @@ proc one_jit_test {jit_solibs_target match_str reattach} {
 	gdb_continue_to_breakpoint "break here 0"
 
 
-	continue_to_test_location "break here 1" $reattach
+	if { ![continue_to_test_location "break here 1" $reattach] } {
+	    return
+	}
 
 	gdb_test "info function ^jit_function" "$match_str"
 
@@ -124,7 +133,9 @@ proc one_jit_test {jit_solibs_target match_str reattach} {
 	    gdb_test "maintenance info break"
 	}
 
-	continue_to_test_location "break here 2" $reattach
+	if { ![continue_to_test_location "break here 2" $reattach] } {
+	    return
+	}
 
 	# All jit librares must have been unregistered
 	gdb_test "info function jit_function" \
-- 
2.1.0

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

[Pedro Alves]

On 2022-03-17 14:00, 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>
> ---
>  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 89dcd0a..abbe79e 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 ""}
> +    }

I'm surprised to see parse_options used instead of parse_args.

Like, in patch #4:

+    set attach_opts {
+	"pattern" "main.*at .*$::main_srcfile:.*"
+    }
+
+    if { ![gdb_attach $testpid $attach_opts] } {
+	return 0
     }

That looks odd to me -- it's not obvious that "pattern" is the option name.

If we instead use parse_args, the option would be specified with "-pattern" (leading dash),
and we'd use the magic TCL args array, so we wouldn't need to wrap the options in a list.
Like so (on top of the whole series):

From 86aa3249e0d6362fef7f784982949fb6422acf18 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Thu, 17 Mar 2022 16:35:49 +0000
Subject: [PATCH] parse_args

Change-Id: Ia7234f34efbd6bcdad3aa7e39075df47abfbe24e
---
 gdb/testsuite/gdb.base/jit-elf.exp | 7 ++-----
 gdb/testsuite/lib/gdb.exp          | 8 ++++++--
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/gdb/testsuite/gdb.base/jit-elf.exp b/gdb/testsuite/gdb.base/jit-elf.exp
index e63f0863e99..efa1dc036b3 100644
--- a/gdb/testsuite/gdb.base/jit-elf.exp
+++ b/gdb/testsuite/gdb.base/jit-elf.exp
@@ -58,11 +58,8 @@ proc clean_reattach {} {
 
     clean_restart ${main_binfile}
 
-    set attach_opts {
-	"pattern" "main.*at .*$::main_srcfile:.*"
-    }
-
-    if { ![gdb_attach $testpid $attach_opts] } {
+    if { ![gdb_attach $testpid \
+	       -pattern "main.*at .*$::main_srcfile:.*"] } {
 	return 0
     }
 
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index abbe79e1a88..149a1c31776 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -5114,11 +5114,15 @@ proc can_spawn_for_attach { } {
 # Centralize the failure checking of "attach" command.
 # Return 0 if attach failed, otherwise return 1.
 
-proc gdb_attach { testpid {options {}} } {
-    parse_options {
+proc gdb_attach { testpid args } {
+    parse_args {
 	{pattern ""}
     }
 
+    if { [llength $args] != 0 } {
+	error "Unexpected arguments: $args"
+    }
+
     gdb_test_multiple "attach $testpid" "attach" {
 	-re -wrap "Attaching to.*ptrace: Operation not permitted\\." {
 	    unsupported "$gdb_test_name (Operation not permitted)"

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

[Pedro Alves]

On 2022-03-17 14:01, Tiezhu Yang wrote:

> diff --git a/gdb/testsuite/gdb.base/attach-pie-noexec.exp b/gdb/testsuite/gdb.base/attach-pie-noexec.exp
> index 4712824..a916841 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] } {

Should call

  kill_wait_spawned_process $test_spawn_id

here before returning.

> +    return
> +}

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

[Pedro Alves]

On 2022-03-17 14:00, Tiezhu Yang wrote:
> Hi Simon and Pedro,
> 
> Thank you very much for your suggestions.
> Please review this v3 patchset.
> Any comments will be much appreciated.

Thanks for doing this.

Are these really the only .exp files that need to use gdb_attach?  Or is it just
that you're tackling this incrementally?

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

[Simon Marchi]

On 2022-03-17 12:50, Pedro Alves wrote:
> On 2022-03-17 14:00, 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>
>> ---
>>  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 89dcd0a..abbe79e 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 ""}
>> +    }
> 
> I'm surprised to see parse_options used instead of parse_args.
> 
> Like, in patch #4:
> 
> +    set attach_opts {
> +	"pattern" "main.*at .*$::main_srcfile:.*"
> +    }
> +
> +    if { ![gdb_attach $testpid $attach_opts] } {
> +	return 0
>      }
> 
> That looks odd to me -- it's not obvious that "pattern" is the option name.
> 
> If we instead use parse_args, the option would be specified with "-pattern" (leading dash),
> and we'd use the magic TCL args array, so we wouldn't need to wrap the options in a list.
> Like so (on top of the whole series):

I used parse_options because back when doing the rnglists support in the
DWARF assembler, I had used parse_args and Tom Tromey mentioned he
didn't really like that use of `args`, because it didn't make it clear
what positional arguments were expected..  Unlike what you've done
below, it was something like:

  proc rnglists { args } {
  	# call parse_args, and then use the remaining args as positional
	# arguments
  }

So we switched to using parse_opts.  But there is also that the DWARF
assembler already used the parse_opts pattern, so it clashed.

> 
> From 86aa3249e0d6362fef7f784982949fb6422acf18 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <pedro@palves.net>
> Date: Thu, 17 Mar 2022 16:35:49 +0000
> Subject: [PATCH] parse_args
> 
> Change-Id: Ia7234f34efbd6bcdad3aa7e39075df47abfbe24e
> ---
>  gdb/testsuite/gdb.base/jit-elf.exp | 7 ++-----
>  gdb/testsuite/lib/gdb.exp          | 8 ++++++--
>  2 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.base/jit-elf.exp b/gdb/testsuite/gdb.base/jit-elf.exp
> index e63f0863e99..efa1dc036b3 100644
> --- a/gdb/testsuite/gdb.base/jit-elf.exp
> +++ b/gdb/testsuite/gdb.base/jit-elf.exp
> @@ -58,11 +58,8 @@ proc clean_reattach {} {
>  
>      clean_restart ${main_binfile}
>  
> -    set attach_opts {
> -	"pattern" "main.*at .*$::main_srcfile:.*"
> -    }
> -
> -    if { ![gdb_attach $testpid $attach_opts] } {
> +    if { ![gdb_attach $testpid \
> +	       -pattern "main.*at .*$::main_srcfile:.*"] } {
>  	return 0
>      }
>  
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index abbe79e1a88..149a1c31776 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -5114,11 +5114,15 @@ proc can_spawn_for_attach { } {
>  # Centralize the failure checking of "attach" command.
>  # Return 0 if attach failed, otherwise return 1.
>  
> -proc gdb_attach { testpid {options {}} } {
> -    parse_options {
> +proc gdb_attach { testpid args } {
> +    parse_args {
>  	{pattern ""}
>      }
>  
> +    if { [llength $args] != 0 } {
> +	error "Unexpected arguments: $args"
> +    }
> +

The way you did it here has the advantage that it's clear which
positional arguments are expected.  The downside is that flag arguments
can't be put before positional arguments (like you usually can with
shell or some tcl / expect procs).  But I think that's an acceptable
tradeoff, I'm fine with it.

Could we add a way to tell parse_args that unexpected arguments are not
allowed, so it could throw the error itself (and therefore be re-used
elsewhere)?

Simon

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

[Pedro Alves]

On 2022-03-17 17:04, Simon Marchi via Gdb-patches wrote:

> On 2022-03-17 12:50, Pedro Alves wrote:

>> That looks odd to me -- it's not obvious that "pattern" is the option name.
>>
>> If we instead use parse_args, the option would be specified with "-pattern" (leading dash),
>> and we'd use the magic TCL args array, so we wouldn't need to wrap the options in a list.
>> Like so (on top of the whole series):
> 
> I used parse_options because back when doing the rnglists support in the
> DWARF assembler, I had used parse_args and Tom Tromey mentioned he
> didn't really like that use of `args`, because it didn't make it clear
> what positional arguments were expected..  Unlike what you've done
> below, it was something like:
> 
>   proc rnglists { args } {
>   	# call parse_args, and then use the remaining args as positional
> 	# arguments
>   }

OK.  I'd be fine with that pattern myself, FWIW, as I'd assume the real arguments would
be described in a comment, which you always have to look at anyhow, even if you have
positional arguments:

   # Usage: func FOO BAR [-opt OPT]
   #
   proc func { args } {


Maybe we could instead come up with a proc alternative that would
let you specify the options in the args list directly, like:

   proc_with_opts func { foo bar } { {opt1 "default1"}
                                     {opt2 "default2"} } {
     ...
  }

... and we'd make it support users passing options before or after
positional args.

Maybe that's going a bit too far, dunno.


> The way you did it here has the advantage that it's clear which
> positional arguments are expected.  The downside is that flag arguments
> can't be put before positional arguments (like you usually can with
> shell or some tcl / expect procs).  But I think that's an acceptable
> tradeoff, I'm fine with it.

Yeah, it's what we do with gdb_test_multiple too, for example, even
thought we don't use parse_args there for some reason.

 # gdb_test_multiple COMMAND MESSAGE [ -prompt PROMPT_REGEXP] [ -lbl ]
 #                   EXPECT_ARGUMENTS
 ...
 proc gdb_test_multiple { command message args } {
 ... 

> 
> Could we add a way to tell parse_args that unexpected arguments are not
> allowed, so it could throw the error itself (and therefore be re-used
> elsewhere)?

I'd guess we could.  I wouldn't make that a requirement for this patch, though.

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

[Tiezhu Yang]

On 03/18/2022 12:55 AM, Pedro Alves wrote:
> On 2022-03-17 14:00, Tiezhu Yang wrote:
>> Hi Simon and Pedro,
>>
>> Thank you very much for your suggestions.
>> Please review this v3 patchset.
>> Any comments will be much appreciated.
>
> Thanks for doing this.
>
> Are these really the only .exp files that need to use gdb_attach?  Or is it just
> that you're tackling this incrementally?
>

At least for now, attach-pie-noexec.exp and jit-elf.exp need to use
gdb_attach to fix the failed testcases, if we find the other .exp
files also have similar issues in the future, I will tackling them
incrementally.

Thanks,
Tiezhu

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

[Tiezhu Yang]

On 03/18/2022 01:31 AM, Pedro Alves wrote:
> On 2022-03-17 17:04, Simon Marchi via Gdb-patches wrote:
>
>> On 2022-03-17 12:50, Pedro Alves wrote:
>
>>> That looks odd to me -- it's not obvious that "pattern" is the option name.
>>>
>>> If we instead use parse_args, the option would be specified with "-pattern" (leading dash),
>>> and we'd use the magic TCL args array, so we wouldn't need to wrap the options in a list.
>>> Like so (on top of the whole series):
>>
>> I used parse_options because back when doing the rnglists support in the
>> DWARF assembler, I had used parse_args and Tom Tromey mentioned he
>> didn't really like that use of `args`, because it didn't make it clear
>> what positional arguments were expected..  Unlike what you've done
>> below, it was something like:
>>
>>   proc rnglists { args } {
>>   	# call parse_args, and then use the remaining args as positional
>> 	# arguments
>>   }
>
> OK.  I'd be fine with that pattern myself, FWIW, as I'd assume the real arguments would
> be described in a comment, which you always have to look at anyhow, even if you have
> positional arguments:
>
>    # Usage: func FOO BAR [-opt OPT]
>    #
>    proc func { args } {
>
>
> Maybe we could instead come up with a proc alternative that would
> let you specify the options in the args list directly, like:
>
>    proc_with_opts func { foo bar } { {opt1 "default1"}
>                                      {opt2 "default2"} } {
>      ...
>   }
>
> ... and we'd make it support users passing options before or after
> positional args.
>
> Maybe that's going a bit too far, dunno.
>
>
>> The way you did it here has the advantage that it's clear which
>> positional arguments are expected.  The downside is that flag arguments
>> can't be put before positional arguments (like you usually can with
>> shell or some tcl / expect procs).  But I think that's an acceptable
>> tradeoff, I'm fine with it.
>
> Yeah, it's what we do with gdb_test_multiple too, for example, even
> thought we don't use parse_args there for some reason.
>
>  # gdb_test_multiple COMMAND MESSAGE [ -prompt PROMPT_REGEXP] [ -lbl ]
>  #                   EXPECT_ARGUMENTS
>  ...
>  proc gdb_test_multiple { command message args } {
>  ...
>
>>
>> Could we add a way to tell parse_args that unexpected arguments are not
>> allowed, so it could throw the error itself (and therefore be re-used
>> elsewhere)?
>
> I'd guess we could.  I wouldn't make that a requirement for this patch, though.
>

Hi,

Thank you very much for your discussions.
I am not quite sure my understanding is correct.
Which way is proper? parse_options or parse_args?
Could you please clarify it for me?

(1) parse_options
gdb/testsuite/lib/gdb.exp
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
         }
     }
}

gdb/testsuite/gdb.base/attach-pie-noexec.exp
if { ![gdb_attach $testpid] } {
     kill_wait_spawned_process $test_spawn_id
     return
}

gdb/testsuite/gdb.base/jit-elf.exp
set attach_opts {
     "pattern" "main.*at .*$::main_srcfile:.*"
}

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

(2) parse_args
gdb/testsuite/lib/gdb.exp
proc gdb_attach { testpid args } {
     parse_args {
         {pattern ""}
     }

     if { [llength $args] != 0 } {
         error "Unexpected arguments: $args"
     }

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

gdb/testsuite/gdb.base/attach-pie-noexec.exp
if { ![gdb_attach $testpid] } {
     kill_wait_spawned_process $test_spawn_id
     return
}

gdb/testsuite/gdb.base/jit-elf.exp
if { ![gdb_attach $testpid \
					-pattern "main.*at .*$::main_srcfile:.*"] } {
		return 0
}

I will wait for your feedback and then send v4 later.

Thanks,
Tiezhu

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

[Simon Marchi]

> (1) parse_options
> gdb/testsuite/lib/gdb.exp
> 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
>         }
>     }
> }
>
> gdb/testsuite/gdb.base/attach-pie-noexec.exp
> if { ![gdb_attach $testpid] } {
>     kill_wait_spawned_process $test_spawn_id
>     return
> }
>
> gdb/testsuite/gdb.base/jit-elf.exp
> set attach_opts {
>     "pattern" "main.*at .*$::main_srcfile:.*"
> }
>
> if { ![gdb_attach $testpid $attach_opts] } {
>     return 0
> }
>
> (2) parse_args
> gdb/testsuite/lib/gdb.exp
> proc gdb_attach { testpid args } {
>     parse_args {
>         {pattern ""}
>     }
>
>     if { [llength $args] != 0 } {
>         error "Unexpected arguments: $args"
>     }
>
>     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
>         }
>     }
> }
>
> gdb/testsuite/gdb.base/attach-pie-noexec.exp
> if { ![gdb_attach $testpid] } {
>     kill_wait_spawned_process $test_spawn_id
>     return
> }
>
> gdb/testsuite/gdb.base/jit-elf.exp
> if { ![gdb_attach $testpid \
>                     -pattern "main.*at .*$::main_srcfile:.*"] } {
>         return 0
> }
>
> I will wait for your feedback and then send v4 later.

Go with parse_args (the version Pedro suggested).

Thanks,

Simon

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

[Pedro Alves]

On 2022-03-18 01:53, Tiezhu Yang wrote:
> 
> 
> On 03/18/2022 12:55 AM, Pedro Alves wrote:
>> On 2022-03-17 14:00, Tiezhu Yang wrote:
>>> Hi Simon and Pedro,
>>>
>>> Thank you very much for your suggestions.
>>> Please review this v3 patchset.
>>> Any comments will be much appreciated.
>>
>> Thanks for doing this.
>>
>> Are these really the only .exp files that need to use gdb_attach?  Or is it just
>> that you're tackling this incrementally?
>>
> 
> At least for now, attach-pie-noexec.exp and jit-elf.exp need to use
> gdb_attach to fix the failed testcases, if we find the other .exp
> files also have similar issues in the future, I will tackling them
> incrementally.

On a first approximation, I'd think most of the tests that use can_spawn_for_attach
would need to be adjusted.

 gdb.threads/detach-step-over.exp:51:if {![can_spawn_for_attach]} {
 gdb.threads/attach-many-short-lived-threads.exp:52:if {![can_spawn_for_attach]} {
 gdb.threads/clone-attach-detach.exp:26:if {![can_spawn_for_attach]} {
 gdb.threads/attach-non-stop.exp:22:if {![can_spawn_for_attach]} {
 gdb.base/attach.exp:16:if {![can_spawn_for_attach]} {
 gdb.base/kill-detach-inferiors-cmd.exp:21:if ![can_spawn_for_attach] {
 gdb.base/watchpoint-hw-attach.exp:23:if {![can_spawn_for_attach]} {
 gdb.base/solib-overlap.exp:34:if {![can_spawn_for_attach]} {
 gdb.base/random-signal.exp:70:    if {[can_spawn_for_attach]} {
 gdb.base/jit-elf.exp:151:if {[can_spawn_for_attach]} {
 gdb.base/interrupt-daemon-attach.exp:27:if { ![can_spawn_for_attach] } {
 gdb.base/attach-non-pgrp-leader.exp:21:if {![can_spawn_for_attach]} {
 gdb.base/attach-pie-noexec.exp:16:if {![can_spawn_for_attach]} {
 gdb.base/corefile.exp:290:    if [can_spawn_for_attach] {
 gdb.base/run-attach-while-running.exp:38:    if { ${run-or-attach} == "attach" && ![can_spawn_for_attach] } {
 gdb.base/quit-live.exp:168:    if {$appear_how != "run" && ![can_spawn_for_attach]} {
 gdb.base/attach-twice.exp:16:if {![can_spawn_for_attach]} {
 gdb.base/bp-cmds-continue-ctrl-c.exp:137:    if {[can_spawn_for_attach]} {
 gdb.base/run-after-attach.exp:19:if ![can_spawn_for_attach] {
 gdb.base/jit-attach-pie.exp:16:if {![can_spawn_for_attach]} {
 gdb.python/py-sync-interp.exp:23:if {![can_spawn_for_attach]} {
 gdb.python/py-prompt.exp:81:if {![can_spawn_for_attach]} {
 gdb.server/attach-flag.exp:28:if {![can_spawn_for_attach]} {
 gdb.server/ext-attach.exp:29:if {![can_spawn_for_attach]} {
 gdb.multi/multi-attach.exp:22:if {![can_spawn_for_attach]} {
 gdb.multi/multi-term-settings.exp:28:if ![can_spawn_for_attach] {
 gdb.mi/list-thread-groups-available.exp:38:if ![can_spawn_for_attach] {

My main "worry" (not very serious), is that we discover that some test
is currently using gdb_test_multiple to expect more than one potential
output after "attach ...", which can't be mapped to gdb_attach as is.  That
would suggest that gdb_attach would be better redesigned as a wrapper around
gdb_test_multiple with some internal matches, rather than passing down
the expected pattern with "-pattern".

if ![!gdb_test_attach_multiple "attach 123" "testname" {
   -re -wrap "foo bar output 1" {
       pass "$gdb_test_name (v1)"
    }
   -re -wrap "foo bar output 2" {
       pass "$gdb_test_name (v2)"
    }
}] {
  return
}

Maybe we'd call it gdb_test_attach_multiple, and then add a gdb_test_attach
wrapper around it that would be just like gdb_test vs gdb_test_multiple.  That
would suggest that gdb_test_attach wouldn't be using options like "-pattern"
at all.