From patchwork Thu Sep 19 16:18:46 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 34593 Received: (qmail 57150 invoked by alias); 19 Sep 2019 16:18:52 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 57136 invoked by uid 89); 19 Sep 2019 16:18:52 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.5 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= X-HELO: mail-qk1-f194.google.com Received: from mail-qk1-f194.google.com (HELO mail-qk1-f194.google.com) (209.85.222.194) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 19 Sep 2019 16:18:50 +0000 Received: by mail-qk1-f194.google.com with SMTP id x134so4022598qkb.0 for ; Thu, 19 Sep 2019 09:18:50 -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=l6oMHOZhSTGSGhp6JumfPJdsvi4uz3InuLGZlsLWCmc=; b=A71fdohECKfTF1OWnKy9ANV9F5QEI2AC4yvqy9F1jIbyYh+pFhN/wg42C+UvYCcz7u ZBDzdMVe/lOXPc70WszizImxffwRjKRj+8QocDsSrcT8+JB8K+hi19SE7FOltqJcvwrB 0hi3Ej95iW7bi76ZDgpFnDlgDgitM6l56zwCNG5t03iYMN5hs9JMV/5lSOtGuweqipMB 3YM93eilwbO8PeIN/OfohnY78ivZYPzYhz5OyedGwQTPTUqZFWV6iNOG6U24dAhk6fIW dVv08VQLeVdIUX0YTK8ycHz9YOZa38b40gpZPo6IZTVGOdNL2bLXY/SugiGwSBeFOabD tW7A== Return-Path: Received: from localhost ([172.110.70.5]) by smtp.gmail.com with ESMTPSA id i30sm4960611qte.27.2019.09.19.09.18.47 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 19 Sep 2019 09:18:47 -0700 (PDT) Date: Thu, 19 Sep 2019 12:18:46 -0400 From: Andrew Burgess To: Tom de Vries Cc: gdb-patches@sourceware.org Subject: Re: [PATCH][gdb/testsuite] Introduce gdb_test_ext Message-ID: <20190919161846.GC4962@embecosm.com> References: <20190919111322.GA29391@delia> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20190919111322.GA29391@delia> X-Fortune: Did YOU find a DIGITAL WATCH in YOUR box of VELVEETA? X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes * 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. 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. The patch below isn't complete yet - I've rewritten gdb_test_ext, and updated store.exp, but this would need a commit message and ChangeLog writing, I'm presenting this here for discussion on which solution people prefer. Thanks, Andrew diff --git a/gdb/testsuite/gdb.base/store.exp b/gdb/testsuite/gdb.base/store.exp index 9c19ce15a7b..61d65127e8c 100644 --- a/gdb/testsuite/gdb.base/store.exp +++ b/gdb/testsuite/gdb.base/store.exp @@ -43,27 +43,25 @@ proc check_set { t l r new add } { set prefix "var ${t} l" gdb_test "tbreak wack_${t}" - set test "continue to wack_${t}" - gdb_test_multiple "continue" $test { - -re "register ${t} l = u, r = v;\r\n$gdb_prompt $" { + gdb_test_ext "continue" "continue to wack_${t}" { + "register ${t} l = u, r = v;" { # See GCC PR debug/53948. send_gdb "next\n" exp_continue } - -re "l = add_${t} .l, r.;\r\n$gdb_prompt $" { - pass $test + "l = add_${t} .l, r.;" { + pass $gdb_test_ext_name } } 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" "${prefix}; print old l, expecting ${l}" { + " = ${l}" { + pass $gdb_test_ext_name } - -re " = ${l}\r\n$gdb_prompt $" { - pass $test + " = " { + unsupported $gdb_test_ext_name + set supported 0 } } if { $supported } { @@ -102,14 +100,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" "${prefix}; print old l, expecting ${l}" { + " = ${l}" { + pass $gdb_test_ext_name } - -re " = ${l}\r\n$gdb_prompt $" { - pass $test + " = " { + unsupported $gdb_test_ext_name + set supported 0 } } if { $supported } { diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp index acbeb013768..976ac9d14e0 100644 --- a/gdb/testsuite/lib/gdb.exp +++ b/gdb/testsuite/lib/gdb.exp @@ -1103,6 +1103,78 @@ 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. + + + +# A cross between gdb_test_multiple and gdb_test. The first +# parameter, COMMAND, is the command to send to GDB, and the second +# parameter, TESTNAME, is the name of the test. The ARGS is a list of +# items which should alternate between patterns and action bodies. +# Unlike gdb_test_multiple each pattern will automatically have the +# gdb_prompt pattern added to it. +# +# In addition, within the action bodies a new variable is available +# for use, this is gdb_test_ext_name. This variable will have the +# value of TESTNAME. This can be used like this: +# +# gdb_test_ext "some command" "a helpful test name" { +# "pattern1" { +# pass $gdb_test_ext_name +# } +# "pattern2" { +# fail $gdb_test_ext_name +# } +# } +proc gdb_test_ext { command testname args } { + global gdb_prompt + upvar timeout timeout + + # Setup a default testname if the user was feeling lazy. + if { "$testname" == "" } { + set testname ${cmd} + } + + # 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 }] } { + perror "variable gdb_test_ext_name unexpectedly exists" + return 1 + } + uplevel " set gdb_test_ext_name \"$testname\" " + + # Build up the user code. We need to make use of some extra + # uplevel calls in here as gdb_test_multiple evaluates action + # bodies in the parent scope, so we add the extra uplevel to push + # things back to our parents scope. + set user_code {} + set args [lindex $args 0] + for { set index 0 } { [expr $index + 1] < [llength $args] } { incr index 2 } { + set pattern [uplevel [list subst [lindex $args ${index}]]] + set body [lindex $args [expr $index + 1]] + + lappend user_code " -re " + lappend user_code "\"\\\[\\r\\n\\\]*(?:$pattern)\\\[\\r\\n\\\]+\$gdb_prompt $\"" + lappend user_code " { uplevel { $body } } " + } + set user_code [join $user_code " "] + + # Now we can actually run the test. + set result [gdb_test_multiple $command $testname $user_code] + + # Don't drop litter! Clean up the gdb_test_ext_name variable we + # created in the parent scope. + uplevel " unset gdb_test_ext_name " + + return $result +} + # 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 } {