From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 130305 invoked by alias); 19 Sep 2019 19:24:41 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 130270 invoked by uid 89); 19 Sep 2019 19:24:34 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-14.4 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy=desk, convinced X-HELO: mail-wr1-f67.google.com Received: from mail-wr1-f67.google.com (HELO mail-wr1-f67.google.com) (209.85.221.67) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 19 Sep 2019 19:24:31 +0000 Received: by mail-wr1-f67.google.com with SMTP id q17so4291404wrx.10 for ; Thu, 19 Sep 2019 12:24:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=tjMAIFw0BYMT2Roe94Q/bMZXEFpQqd91i/g0+s5QspY=; b=aNB+rl5eFEG4lH6cWm9gd2CE6wLpoSmpJiGeq/tXD2gP93MalKiX5Vk2jT9i9eCuf9 1EuFz3yPZrPZwyJUUFas3Z2zFI4F2Dr5WlO+2wkkITLjp2LrWBcuUX40yAQ3rxrZQHzv BwYc4xpgZqV9KbXRzjx1fH9lGOarWb3hCLaZL32XYCYKNJtlWdBGDGfWJDiXPKM3N7NC LS+ihJYCm0/OVU2SHOM7KexFS+HkYvcFVbtpChBQqVljI0Tlou21PNsZJl/gN+U166/x rzSO3ajF2Xe0rtmXHITu1yddo0TUM4Gfl4Ubfm5of5PgSJOC3ebdkjpO2KZA6Wvikbzu Yd6g== Return-Path: Received: from localhost ([185.69.145.40]) by smtp.gmail.com with ESMTPSA id d28sm14425969wrb.95.2019.09.19.12.24.25 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 19 Sep 2019 12:24:26 -0700 (PDT) Date: Thu, 19 Sep 2019 19:24:00 -0000 From: Andrew Burgess To: Tom de Vries Cc: gdb-patches@sourceware.org Subject: Re: [PATCH][gdb/testsuite] Introduce gdb_test_ext Message-ID: <20190919192423.GF4962@embecosm.com> References: <20190919111322.GA29391@delia> <20190919161846.GC4962@embecosm.com> <62b20c8f-6792-c17e-621a-946002df6df9@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <62b20c8f-6792-c17e-621a-946002df6df9@suse.de> X-Fortune: I'm not laughing with you, I'm laughing at you. X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes X-SW-Source: 2019-09/txt/msg00378.txt.bz2 * Tom de Vries [2019-09-19 21:01:01 +0200]: > On 19-09-19 18:18, Andrew Burgess wrote: > > * Tom de Vries [2019-09-19 13:13:23 +0200]: > > > >> Hi, > >> > >> In commit 25e5c20918 "[gdb/testsuite] Allow some tests in gdb.base/store.exp > >> to be unsupported" we replace a gdb_test: > >> ... > >> gdb_test "print l" " = ${l}" \ > >> "${prefix}; print old l, expecting ${l}" > >> ... > >> with a gdb_test_multiple: > >> ... > >> set supported 1 > >> set test "${prefix}; print old l, expecting ${l}" > >> gdb_test_multiple "print l" "$test" { > >> -re " = \r\n$gdb_prompt $" { > >> unsupported $test > >> set supported 0 > >> } > >> -re " = ${l}\r\n$gdb_prompt $" { > >> pass $test > >> } > >> } > >> ... > >> in order to handle the UNSUPPORTED case. > >> > >> This has the drawback that we have to be explicit about the gdb_prompt, and > >> move the gdb_test arguments around to fit the gdb_test_multiple format. > >> > >> Introduce a new proc gdb_test_ext that behaves as gdb_test, but also allows > >> extension, allowing us to rewrite the gdb_test_multiple above in a form > >> resembling the original gdb_test: > >> ... > >> set supported 1 > >> gdb_test_ext "print l" " = ${l}" \ > >> "${prefix}; print old l, expecting ${l}" \ > >> -- [list "unsupported" " = " "set supported 0"] > > > > I've also thought about this sort of problem in the past, and would > > like to propose a similar, but slightly different solution. > > > > My idea is more like a trimmed down gdb_test_multiple, so for your > > example above you would write this: > > > > gdb_test_ext "print l" "${prefix}; print old l, expecting ${l}" { > > " = ${l}" { > > pass $gdb_test_ext_name > > } > > " = " { > > unsupported $gdb_test_ext_name > > set supported 0 > > } > > } > > > > You don't put '-re' before each pattern, this is because they aren't > > full patterns, gdb_test_ext will be extending them. > > > > Unlike your solution the 'pass' case is not created automatically, you > > need to write it yourself, so maybe that's a negative. The advantages > > I see of this approach is that there's not special handling for > > different "types" of alternatives as in your original code, the action > > block can contain anything 'unsupported', 'fail', etc. Plus it's > > formatted as a code body, which I like. > > > > The solution as I proposed it doesn't limit itself to require each > alternative to be handled as either supported or pass or fail or > somesuch. It just adds a means to extend gdb_test using a keyword that > determines how the keyword arguments are handled. I don't really understand what you're trying to say here, sorry. Looking at the code it still appears that each case would need to be added specifically, so if tomorrow I want a 'fail' case, I'd need to add it to lib/gdb.exp - or am I not understanding? > > So, I've added the style you propose here as "generic", and rewrote one > of the two places I update in store.exp using the "generic" style for > demonstration purposes. > > I envision the usage like this: you'd usually use "unsupported" or > similar to skip all the repetitive code and focus on the bits that > actually contain content, and for special cases where that doesn't fit > you'd use "generic". You can go further and add a "freeform" or some > such where you'd have to write out the entire regexp. > > The nice thing is that you can add keywords and corresponding handling > as you go, whereas the gdb_test_multiple-like solution you propose only > has one way of handling its arguments, which of course does makes things > consistent and clear, but is not very extensible. I'm still not seeing which arguments can only be handled in one way. Could you give an example maybe? I do see that you're suggestion improves the existing test - removing the need to set an extra 'test' variable, and not having to add $gdb_prompt, but when all is said and done, the pattern is still just a pattern, and the code is still just code - how is this any different to gdb_test_multiple? > > > One other thing which I've wanted for _ages_ is to avoid having to set > > the test name into a separate variable, which your solution offers > > too. The solution I offer is '$gdb_test_ext_name', this variable is > > set auto-magically by the call to 'gdb_test_ext, and is available in > > all of the action bodies for calls to pass/fail/unsupported/etc. > > > > Nice trick, I've copied that for usage in the "generic" method. > > So, WDYT? I'm still not convinced. On further thought, I actually think there's no need for an extra function at all, we can get all the benefit (as I see it) by possibly updating gdb_test_multiple. I'm travelling right now so can't code this up, but I think a solution that does something like this: gdb_test_multiple "command" "test name" { -re "full regexp here$gdb_prompt" { pass $gdb_test_multiple_name } -output "pattern without prompt" { fail $gdb_test_multiple_name } } So using '-re' and '-output' to specialise the behaviour of gdb_test_multiple, and adding in the $gdb_test_multiple_name variable. When I get back to my desk I'll try to code this up. I'm know the above isn't going to satisfy you though - it's basically an iteration on what I already proposed - maybe you could expand on the benefits of you solution a bit more. Thanks, Andrew > > Thanks, > - Tom > > [gdb/testsuite] Introduce gdb_test_ext > > In commit 25e5c20918 "[gdb/testsuite] Allow some tests in gdb.base/store.exp > to be unsupported" we replace a gdb_test: > ... > gdb_test "print l" " = ${l}" \ > "${prefix}; print old l, expecting ${l}" > ... > with a gdb_test_multiple: > ... > set supported 1 > set test "${prefix}; print old l, expecting ${l}" > gdb_test_multiple "print l" "$test" { > -re " = \r\n$gdb_prompt $" { > unsupported $test > set supported 0 > } > -re " = ${l}\r\n$gdb_prompt $" { > pass $test > } > } > ... > in order to handle the UNSUPPORTED case. > > This has the drawback that we have to be explicit about the gdb_prompt, and > move the gdb_test arguments around to fit the gdb_test_multiple format. > > Introduce a new proc gdb_test_ext that behaves as gdb_test, but also allows > extension, allowing us to rewrite the gdb_test_multiple above in a form > resembling the original gdb_test: > ... > set supported 1 > gdb_test_ext "print l" " = ${l}" \ > "${prefix}; print old l, expecting ${l}" \ > -- [list "unsupported" " = " "set supported 0"] > ... > > Tested on x86_64-linux. > > gdb/testsuite/ChangeLog: > > 2019-09-19 Tom de Vries > > * lib/gdb.exp (gdb_test_ext): New proc. > * gdb.base/store.exp: Use gdb_test_ext. > > --- > gdb/testsuite/gdb.base/store.exp | 27 ++++-------- > gdb/testsuite/lib/gdb.exp | 95 ++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 103 insertions(+), 19 deletions(-) > > diff --git a/gdb/testsuite/gdb.base/store.exp b/gdb/testsuite/gdb.base/store.exp > index 9c19ce15a7..89a594f96a 100644 > --- a/gdb/testsuite/gdb.base/store.exp > +++ b/gdb/testsuite/gdb.base/store.exp > @@ -56,16 +56,8 @@ proc check_set { t l r new add } { > } > > set supported 1 > - set test "${prefix}; print old l, expecting ${l}" > - gdb_test_multiple "print l" "$test" { > - -re " = \r\n$gdb_prompt $" { > - unsupported $test > - set supported 0 > - } > - -re " = ${l}\r\n$gdb_prompt $" { > - pass $test > - } > - } > + gdb_test_ext "print l" " = ${l}" "${prefix}; print old l, expecting ${l}" \ > + -- [list "unsupported" " = " "set supported 0"] > if { $supported } { > gdb_test "print r" " = ${r}" \ > "${prefix}; print old r, expecting ${r}" > @@ -102,16 +94,13 @@ proc up_set { t l r new } { > "${prefix}; up" > > set supported 1 > - set test "${prefix}; print old l, expecting ${l}" > - gdb_test_multiple "print l" "$test" { > - -re " = \r\n$gdb_prompt $" { > - unsupported $test > - set supported 0 > + gdb_test_ext "print l" " = ${l}" "${prefix}; print old l, expecting ${l}" \ > + -- { > + "generic" " = " { > + set supported 0 > + unsupported $gdb_test_ext_name > + } > } > - -re " = ${l}\r\n$gdb_prompt $" { > - pass $test > - } > - } > if { $supported } { > gdb_test "print r" " = ${r}" \ > "${prefix}; print old r, expecting ${r}" > diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp > index acbeb01376..d01ca25ef7 100644 > --- a/gdb/testsuite/lib/gdb.exp > +++ b/gdb/testsuite/lib/gdb.exp > @@ -1103,6 +1103,101 @@ proc gdb_test { args } { > }] > } > > +# As gdb_test, but with additional parameters, listed after a "--" separator. > +# Handled extra parameters: > +# - [list "unsupported" []] > +# The idea is to prevent the need to rewrite gdb_test into gdb_test_multiple > +# if some modification is needed. > +proc gdb_test_ext { args } { > + global gdb_prompt > + upvar timeout timeout > + > + # Find the '--' separator. > + set pos -1 > + set index 0 > + while { $index < [llength $args] } { > + if { [lindex $args $index] == "--" } { > + set pos $index > + break > + } > + set index [expr $index + 1] > + } > + if { $pos == -1 } { > + error "No -- argument found" > + } > + > + if { $pos > 2 } then { > + set message [lindex $args 2] > + } else { > + set message [lindex $args 0] > + } > + set command [lindex $args 0] > + set pattern [lindex $args 1] > + > + set user_code {} > + lappend user_code { > + -re "\[\r\n\]*(?:$pattern)\[\r\n\]+$gdb_prompt $" { > + if ![string match "" $message] then { > + pass "$message" > + } > + } > + } > + > + if { $pos == 5 } { > + set question_string [lindex $args 3] > + set response_string [lindex $args 4] > + lappend user_code { > + -re "(${question_string})$" { > + send_gdb "$response_string\n" > + exp_continue > + } > + } > + } > + > + set index [expr $pos + 1] > + while { $index < [llength $args] } { > + set arg [lindex $args $index] > + set index [expr $index + 1] > + set kind [lindex $arg 0] > + switch $kind { > + "unsupported" { > + set unsupported_pattern [lindex $arg 1] > + set unsupported_code [lindex $arg 2] > + if { $unsupported_code == "" } { > + set unsupported_code "expr true" > + } > + lappend user_code { > + -re "\[\r\n\]*(?:$unsupported_pattern)\[\r\n\]+$gdb_prompt $" { > + unsupported $message > + uplevel $unsupported_code > + } > + } > + } > + "generic" { > + # In order to support the gdb_test_ext_name variable we need to > + # push the variable into the parent scope. Before we blindly do > + # that check the user hasn't already defined that variable. If > + # they haven't, go ahead and create it for them. > + if { [uplevel { info exists gdb_test_ext_name }] } { > + error "variable gdb_test_ext_name unexpectedly exists" > + return -1 > + } > + uplevel set gdb_test_ext_name \"$message\" > + set generic_pattern [lindex $arg 1] > + set generic_code [lindex $arg 2] > + lappend user_code { > + -re "\[\r\n\]*(?:$generic_pattern)\[\r\n\]+$gdb_prompt $" { > + uplevel $generic_code > + } > + } > + } > + } > + } > + > + set user_code [join $user_code " "] > + return [gdb_test_multiple $command $message $user_code] > +} > + > # Return 1 if version MAJOR.MINOR is at least AT_LEAST_MAJOR.AT_LEAST_MINOR. > proc version_at_least { major minor at_least_major at_least_minor} { > if { $major > $at_least_major } {