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

[Simon Marchi <simon.marchi@polymtl.ca>]

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