From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 870AB3858D1E for ; Thu, 17 Mar 2022 17:05:05 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 870AB3858D1E Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 22HH4oTT030523 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 17 Mar 2022 13:04:55 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 22HH4oTT030523 Received: from [10.0.0.11] (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 01F8B1F0D2; Thu, 17 Mar 2022 13:04:49 -0400 (EDT) Message-ID: Date: Thu, 17 Mar 2022 13:04:49 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.6.1 Subject: Re: [PATCH v3 2/4] gdb: testsuite: add new gdb_attach to check "attach" command Content-Language: en-US To: Pedro Alves , Tiezhu Yang , gdb-patches@sourceware.org References: <1647525661-8607-1-git-send-email-yangtiezhu@loongson.cn> <1647525661-8607-3-git-send-email-yangtiezhu@loongson.cn> <0d25ccda-c26d-672f-6321-8a39aec9c1c6@palves.net> From: Simon Marchi In-Reply-To: <0d25ccda-c26d-672f-6321-8a39aec9c1c6@palves.net> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Thu, 17 Mar 2022 17:04:50 +0000 X-Spam-Status: No, score=-3038.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_PASS, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 17 Mar 2022 17:05:07 -0000 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 >> --- >> 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 > 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