From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2144 invoked by alias); 19 Sep 2019 19:01:07 -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 2012 invoked by uid 89); 19 Sep 2019 19:01:06 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.6 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_PASS autolearn=ham version=3.3.1 spammy= X-HELO: mx1.suse.de Received: from mx2.suse.de (HELO mx1.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 19 Sep 2019 19:01:04 +0000 Received: from relay2.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 287E9ABBD; Thu, 19 Sep 2019 19:01:02 +0000 (UTC) Subject: Re: [PATCH][gdb/testsuite] Introduce gdb_test_ext To: Andrew Burgess Cc: gdb-patches@sourceware.org References: <20190919111322.GA29391@delia> <20190919161846.GC4962@embecosm.com> From: Tom de Vries Openpgp: preference=signencrypt Message-ID: <62b20c8f-6792-c17e-621a-946002df6df9@suse.de> Date: Thu, 19 Sep 2019 19:01:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: <20190919161846.GC4962@embecosm.com> Content-Type: multipart/mixed; boundary="------------F2830D0F6D95DE7715BB7FD0" X-IsSubscribed: yes X-SW-Source: 2019-09/txt/msg00376.txt.bz2 This is a multi-part message in MIME format. --------------F2830D0F6D95DE7715BB7FD0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Content-length: 3791 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. 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. > 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? Thanks, - Tom --------------F2830D0F6D95DE7715BB7FD0 Content-Type: text/x-patch; name="0001-gdb-testsuite-Introduce-gdb_test_ext.patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="0001-gdb-testsuite-Introduce-gdb_test_ext.patch" Content-length: 5942 [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 } { --------------F2830D0F6D95DE7715BB7FD0--