[gdb/testsuite] Introduce gdb_test_ext

Message ID 20190919111322.GA29391@delia
State New
Headers show
Series
  • [gdb/testsuite] Introduce gdb_test_ext
Related show

Commit Message

Tom de Vries Sept. 19, 2019, 11:13 a.m.
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 " = <optimized out>\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" " = <optimized out>" "set supported 0"]
...

Tested on x86_64-linux.

OK for trunk?

Thanks,
- Tom

[gdb/testsuite] Introduce gdb_test_ext

gdb/testsuite/ChangeLog:

2019-09-19  Tom de Vries  <tdevries@suse.de>

	* lib/gdb.exp (gdb_test_ext): New proc.
	* gdb.base/store.exp: Use gdb_test_ext.

---
 gdb/testsuite/gdb.base/store.exp | 24 +++----------
 gdb/testsuite/lib/gdb.exp        | 77 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 81 insertions(+), 20 deletions(-)

Comments

Andrew Burgess Sept. 19, 2019, 4:18 p.m. | #1
* Tom de Vries <tdevries@suse.de> [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 " = <optimized out>\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" " = <optimized out>" "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
	}
	" = <optimized out>" {
	    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 " = <optimized out>\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
+	" = <optimized out>" {
+	    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 " = <optimized out>\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
+	" = <optimized out>" {
+	    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" <pattern> [<code>]]
+# 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 } {
-- 
2.14.5
Tom de Vries Sept. 19, 2019, 7:01 p.m. | #2
On 19-09-19 18:18, Andrew Burgess wrote:
> * Tom de Vries <tdevries@suse.de> [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 " = <optimized out>\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" " = <optimized out>" "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

> 	}

> 	" = <optimized out>" {

> 	    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
[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 " = <optimized out>\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" " = <optimized out>" "set supported 0"]
...

Tested on x86_64-linux.

gdb/testsuite/ChangeLog:

2019-09-19  Tom de Vries  <tdevries@suse.de>

	* 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 " = <optimized out>\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" " = <optimized out>" "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 " = <optimized out>\r\n$gdb_prompt $" {
-	    unsupported $test
-	    set supported 0
+    gdb_test_ext "print l" " = ${l}" "${prefix}; print old l, expecting ${l}" \
+	-- {
+	    "generic" " = <optimized out>" {
+		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" <pattern> [<code>]]
+# 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 } {
Andrew Burgess Sept. 19, 2019, 7:24 p.m. | #3
* Tom de Vries <tdevries@suse.de> [2019-09-19 21:01:01 +0200]:

> On 19-09-19 18:18, Andrew Burgess wrote:

> > * Tom de Vries <tdevries@suse.de> [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 " = <optimized out>\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" " = <optimized out>" "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

> > 	}

> > 	" = <optimized out>" {

> > 	    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 " = <optimized out>\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" " = <optimized out>" "set supported 0"]

> ...

> 

> Tested on x86_64-linux.

> 

> gdb/testsuite/ChangeLog:

> 

> 2019-09-19  Tom de Vries  <tdevries@suse.de>

> 

> 	* 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 " = <optimized out>\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" " = <optimized out>" "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 " = <optimized out>\r\n$gdb_prompt $" {

> -	    unsupported $test

> -	    set supported 0

> +    gdb_test_ext "print l" " = ${l}" "${prefix}; print old l, expecting ${l}" \

> +	-- {

> +	    "generic" " = <optimized out>" {

> +		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" <pattern> [<code>]]

> +# 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 } {
Tom de Vries Sept. 19, 2019, 9:50 p.m. | #4
On 19-09-19 21:24, Andrew Burgess wrote:
> * Tom de Vries <tdevries@suse.de> [2019-09-19 21:01:01 +0200]:

> 

>> On 19-09-19 18:18, Andrew Burgess wrote:

>>> * Tom de Vries <tdevries@suse.de> [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 " = <optimized out>\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" " = <optimized out>" "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

>>> 	}

>>> 	" = <optimized out>" {

>>> 	    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?

> 


I thought you were arguing that it was not possible handle things
generically with my proposed solution (which is not the case, as I tried
to demonstrate by adding the "generic" method), so I tried to argue
against that. But I guess I misunderstood you there.

So confirmed, adding a fail case requires adding to gdb_test_ext in
gdb.exp. [ Although in the latest version you could handle it using the
"generic" method, but I'd prefer to add a "fail" method instead. ]

>>

>> 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?


Well, you gave this example:
...
gdb_test_ext "print l" "${prefix}; print old l, expecting ${l}" {
  " = ${l}" {
    pass $gdb_test_ext_name
  }
  " = <optimized out>" {
    unsupported $gdb_test_ext_name
    set supported 0
  }
}
...
It has (ignoring the first two) two arguments, one containing the 'pass'
and one containing the 'unsupported'. Each is handled in the exact same way.

> 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?

> 


It's much less verbose, and eliminates as much as possible code that is
commonly occurring, in order to avoid errors and omissions there.

>>

>>> 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.

> 


Agreed, I think it makes sense to fold that into gdb_test_multiple.

> I'm know the above isn't going to satisfy you though - it's basically

> an iteration on what I already proposed


Yeah, I think we're looking for different things here: less verbosity at
the cost of specialization vs. generality at the cost of more verbosity.

> - maybe you could expand on

> the benefits of you solution a bit more.

> 


Sure. The benefits of my solution as I see them:
- uses gdb_test semantics for args before '--', minimizing the effort to
  rewrite from gdb_test to gdb_test_ext
- maximally eliminates repetitive code, to prevent error and omissions
  in there and reduce verbosity
- extensible. Each method is handled completely independent, and new
  methods can be added as needed.

Thanks,
- Tom

Patch

diff --git a/gdb/testsuite/gdb.base/store.exp b/gdb/testsuite/gdb.base/store.exp
index 9c19ce15a7..d97b4cb962 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 " = <optimized out>\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" " = <optimized out>" "set supported 0"]
     if { $supported } {
 	gdb_test "print r" " = ${r}" \
 	    "${prefix}; print old r, expecting ${r}"
@@ -102,16 +94,8 @@  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 " = <optimized out>\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" " = <optimized out>" "set supported 0"]
     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..dc990b370e 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -1103,6 +1103,83 @@  proc gdb_test { args } {
      }]
 }
 
+# As gdb_test, but with additional parameters, listed after a "--" separator.
+# Handled extra parameters:
+# - [list "unsupported" <pattern> [<code>]]
+# 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
+			eval uplevel $unsupported_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 } {