On Tue, 2016-01-12 at 23:21 -0700, Jeff Law wrote:
On 01/12/2016 12:34 PM, David Malcolm wrote:
> >> I looked at this code, and there are two near-identical blocks which
> >> reset all these variables. You are modifying only one of them, leaving
> >> the one inside the if { catch } thing unchanged - is this intentional?
> >
> > I'm not particularly strong at Tcl, but am I right in thinking that
> > given that we have this:
> >
> > if { [ catch { eval saved-dg-test $args } errmsg ] } {
> > (A) set and unset various things
> > error $errmsg $saved_info
> > }
> > (B) set and unset the same various things as (A)
> >
> > that (B) will always be reached, and that the duplicates in (A) are
> > redundant? (unless they affect "error")
> Seems like it would, but, well it's TCL, so who in the hell knows.
I was wrong: "error" in Tcl is roughly equivalent to throwing an
exception.
Hence the above is actually akin to:
try:
eval saved-dg-test $args
catch:
do cleanup A
re-raise current error
do cleanup B
which is a workaround for the lack of a try-finally construct:
try:
eval saved-dg-test $args
finally:
do cleanup
So we do need error cleanup for both blocks (A) and (B).
> > I see that this pattern was introduced back in r67696 aka
> > 91a385a522a94154f9e0cd940c5937177737af02:
> Strangely, I can't find the patch in the archives nor any discussion for
> the patch. It seems to have appeared from nowhere. My search-fu must
> be weak tonight. It may not have helped understand why this code is the
> way it is anyway.
>
> This duplication screams that it ought to be its own procedure if we're
> going to keep the apparently duplicated behaviour.
The following patch implements this, moving the existing cleanup into
a new "cleanup-after-saved-dg-test" proc, and calling it from both (A)
and (B).
I noticed in doing so that we were missing a:
global additional_sources_used
and hence (if I understand Tcl correctly), the code was merely uselessly
setting a local with that name, rather than clearing the global.
Also, the cleanups weren't quite identical between (A) and (B); these
clauses:
if [info exists set_target_env_var] {
unset set_target_env_var
}
if [info exists keep_saved_temps_suffixes] {
unset keep_saved_temps_suffixes
}
were present in (B) but missing (A); also some of the ordering of
the cleanups varied between (A) and (B).
I assumed that these differences were unintentional, so the patch
consolidates things to make the cleanup identical between (A) and (B).
Successfully bootstrapped®rtested on x86_64-pc-linux-gnu; as before,
adds 1 UNSUPPORTED (by design: gcc.dg/pr69181-1.c) and 1 PASS to gcc.sum.
OK for trunk?
Alternatively, would you prefer the simpler patch to add the cleanup
of multiline_expected_outputs to both (A) and (B), and leave the
consolidation idea for next stage 1? (keeping the new test cases
and the renaming to lose the leading underscore).
gcc/testsuite/ChangeLog:
PR testsuite/69181
* gcc.dg/pr69181-1.c: New test file.
* gcc.dg/pr69181-2.c: New test file.
* lib/gcc-dg.exp (dg-test): Consolidate post-test cleanup of
globals by moving it to...
(cleanup-after-saved-dg-test): ...this new function. Add
"global additional_sources_used". Add reset of global
multiline_expected_outputs to the empty list.
* lib/multiline.exp (_multiline_expected_outputs): Rename this
global to...
(multiline_expected_outputs): ...this, and updated comments to
note that it is modified from gcc-dg.exp.
(dg-end-multiline-output): Update for the above renaming.
(handle-multiline-outputs): Likewise. Remove the clearing
of the expected outputs to the empty list.
---
gcc/testsuite/gcc.dg/pr69181-1.c | 7 +++++++
gcc/testsuite/gcc.dg/pr69181-2.c | 4 ++++
gcc/testsuite/lib/gcc-dg.exp | 36 ++++++++++++++++++------------------
gcc/testsuite/lib/multiline.exp | 22 +++++++++-------------
4 files changed, 38 insertions(+), 31 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/pr69181-1.c
create mode 100644 gcc/testsuite/gcc.dg/pr69181-2.c
diff --git a/gcc/testsuite/gcc.dg/pr69181-1.c b/gcc/testsuite/gcc.dg/pr69181-1.c
new file mode 100644
index 0000000..e851f0c
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr69181-1.c
@@ -0,0 +1,7 @@
+/* { dg-do compile { target this_will_not_be_matched-*-* } } */
+
+/* { dg-begin-multiline-output "" }
+ This message should never be checked for.
+ In particular, it shouldn't be checked for in the *next*
+ test case.
+ { dg-end-multiline-output "" } */
diff --git a/gcc/testsuite/gcc.dg/pr69181-2.c b/gcc/testsuite/gcc.dg/pr69181-2.c
new file mode 100644
index 0000000..dca90dc
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr69181-2.c
@@ -0,0 +1,4 @@
+/* Dummy test case, to verify that the dg-begin-multiline-output directive
+ from pr69181-1.c isn't erroneously expected to be handled in *this*
+ test case. */
+int make_non_empty;
diff --git a/gcc/testsuite/lib/gcc-dg.exp b/gcc/testsuite/lib/gcc-dg.exp
index f9ec206..c003328 100644
--- a/gcc/testsuite/lib/gcc-dg.exp
+++ b/gcc/testsuite/lib/gcc-dg.exp
@@ -826,33 +826,21 @@ proc output-exists-not { args } {
if { [info procs saved-dg-test] == [list] } {
rename dg-test saved-dg-test
- proc dg-test { args } {
+ # Helper function for cleanups that should happen after the call
+ # to the real dg-test, whether or not it returns normally, or
+ # fails with an error.
+ proc cleanup-after-saved-dg-test { } {
global additional_files
global additional_sources
+ global additional_sources_used
global additional_prunes
- global errorInfo
global compiler_conditional_xfail_data
global shouldfail
global testname_with_flags
global set_target_env_var
global keep_saved_temps_suffixes
+ global multiline_expected_outputs
- if { [ catch { eval saved-dg-test $args } errmsg ] } {
- set saved_info $errorInfo
- set additional_files ""
- set additional_sources ""
- set additional_sources_used ""
- set additional_prunes ""
- set shouldfail 0
- if [info exists compiler_conditional_xfail_data] {
- unset compiler_conditional_xfail_data
- }
- if [info exists testname_with_flags] {
- unset testname_with_flags
- }
- unset_timeout_vars
- error $errmsg $saved_info
- }
set additional_files ""
set additional_sources ""
set additional_sources_used ""
@@ -871,6 +859,18 @@ if { [info procs saved-dg-test] == [list] } {
if [info exists testname_with_flags] {
unset testname_with_flags
}
+ set multiline_expected_outputs []
+ }
+
+ proc dg-test { args } {
+ global errorInfo
+
+ if { [ catch { eval saved-dg-test $args } errmsg ] } {
+ set saved_info $errorInfo
+ cleanup-after-saved-dg-test
+ error $errmsg $saved_info
+ }
+ cleanup-after-saved-dg-test
}
}
diff --git a/gcc/testsuite/lib/multiline.exp b/gcc/testsuite/lib/multiline.exp
index 6b2c1da..fd7affc 100644
--- a/gcc/testsuite/lib/multiline.exp
+++ b/gcc/testsuite/lib/multiline.exp
@@ -47,17 +47,18 @@
# to have the testsuite verify the expected output.
############################################################################
-# Global variables. Although global, these are intended to only be used from
-# within multiline.exp.
+# Global variables.
############################################################################
+# This is intended to only be used from within multiline.exp.
# The line number of the last dg-begin-multiline-output directive.
set _multiline_last_beginning_line -1
# A list of
# first-line-number, last-line-number, lines
# where each "lines" is a list of strings.
-set _multiline_expected_outputs []
+# This is cleared at the end of each test by gcc-dg.exp's wrapper for dg-test.
+set multiline_expected_outputs []
############################################################################
# Exported functions.
@@ -94,9 +95,9 @@ proc dg-end-multiline-output { args } {
verbose "lines: $lines" 3
# Create an entry of the form: first-line, last-line, lines
set entry [list $_multiline_last_beginning_line $line $lines]
- global _multiline_expected_outputs
- lappend _multiline_expected_outputs $entry
- verbose "within dg-end-multiline-output: _multiline_expected_outputs:
$_multiline_expected_outputs" 3
+ global multiline_expected_outputs
+ lappend multiline_expected_outputs $entry
+ verbose "within dg-end-multiline-output: multiline_expected_outputs:
$multiline_expected_outputs" 3
set _multiline_last_beginning_line -1
}
@@ -107,14 +108,12 @@ proc dg-end-multiline-output { args } {
# those that weren't found.
#
# It returns a pruned version of its output.
-#
-# It also clears the list of expected multiline outputs.
proc handle-multiline-outputs { text } {
- global _multiline_expected_outputs
+ global multiline_expected_outputs
global testname_with_flags
set index 0
- foreach entry $_multiline_expected_outputs {
+ foreach entry $multiline_expected_outputs {
verbose " entry: $entry" 3
set start_line [lindex $entry 0]
set end_line [lindex $entry 1]
@@ -140,9 +139,6 @@ proc handle-multiline-outputs { text } {
set index [expr $index + 1]
}
- # Clear the list of expected multiline outputs
- set _multiline_expected_outputs []
-
return $text
}
--
1.8.5.3
