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

[Tiezhu Yang <yangtiezhu@loongson.cn>]

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