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

[Pedro Alves <pedro@palves.net>]

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