Re: [PATCH] testsuite/lib/multline.exp: show test name and line numbers

Re: [PATCH] testsuite/lib/multline.exp: show test name and line numbers

[Uros Bizjak]

Hello!

> Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu;
> I compared the results against a control build (of r231445), and
> the results were unchanged, other than the expected changes from
> the above, leading to
> - 92 PASS results changing name within g++.sum
> - 7 PASS results changing name within each of obj-c++.sum
>   and objc.sum, and
> - 125 PASS results changing name within gcc.sum.
>
> OK for trunk for gcc 6?
>
> gcc/testsuite/ChangeLog:
> * lib/multiline.exp (_multiline_expected_outputs): Update comment.
> (dg-end-multiline-output): Capture line numbers within
> _multiline_expected_outputs.
> (handle-multiline-outputs): Access global $testname_with_flags
> and add it as a prefix to pass/fail results.  Extract line numbers
> from $_multiline_expected_outputs and print them within pass/fail
> results, replacing the printing of $index.  Consolidate the
> string prefix shared between pass/fail into a new local: $title.

It looks that this new functionality doesn't handle conditional
compilation, when

/* { dg-do compile { target { ! ia32 } } } */

is added to the testcase, such as in recently changed
gcc.target/i386/pr68473-1.c.

The directive is passed to the next testcase, leading to spurious
testsuite failures [1] in unrelated testcases.

[1] https://gcc.gnu.org/ml/gcc-testresults/2015-12/msg02761.html

Uros.

Re: [PATCH] testsuite/lib/multline.exp: show test name and line numbers

[Bernd Schmidt]

On 12/29/2015 10:17 AM, Uros Bizjak wrote:
> It looks that this new functionality doesn't handle conditional
> compilation, when
>
> /* { dg-do compile { target { ! ia32 } } } */
>
> is added to the testcase, such as in recently changed
> gcc.target/i386/pr68473-1.c.
>
> The directive is passed to the next testcase, leading to spurious
> testsuite failures [1] in unrelated testcases.

Please open a PR if you haven't already. David, could you investigate?


Bernd

Re: [PATCH] testsuite/lib/multline.exp: show test name and line numbers

[Uros Bizjak]

On Thu, Jan 7, 2016 at 1:10 PM, Bernd Schmidt <bschmidt@redhat.com> wrote:
> On 12/29/2015 10:17 AM, Uros Bizjak wrote:
>>
>> It looks that this new functionality doesn't handle conditional
>> compilation, when
>>
>> /* { dg-do compile { target { ! ia32 } } } */
>>
>> is added to the testcase, such as in recently changed
>> gcc.target/i386/pr68473-1.c.
>>
>> The directive is passed to the next testcase, leading to spurious
>> testsuite failures [1] in unrelated testcases.
>
>
> Please open a PR if you haven't already. David, could you investigate?

I have opened PR 69181 [1].

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69181

Uros.

[PATCH] PR testsuite/69181: ensure expected multiline outputs is cleared per-test

[David Malcolm]

The root cause here is that the logic to reset the list of expected
multiline outputs was being run from:
  handle-multiline-outputs, called by
    prune.exp's prune_gcc_output
and none of that happens if the test is skipped by a target exclusion
in dg-do.

This patch moves the clearing of the relevant list to happen in
gcc-dg.exp's wrapper for dg-test, so it happens per-test even if the
test is skipped.

Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu; adds
1 UNSUPPORTED (by design: gcc.dg/pr69181-1.c) and 1 PASS to gcc.sum.

OK for trunk?

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): Reset global
	multiline_expected_outputs to the empty list after each
	call to the real dg-test.
	* 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     |  2 ++
 gcc/testsuite/lib/multiline.exp  | 22 +++++++++-------------
 4 files changed, 22 insertions(+), 13 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..f778bca 100644
--- a/gcc/testsuite/lib/gcc-dg.exp
+++ b/gcc/testsuite/lib/gcc-dg.exp
@@ -836,6 +836,7 @@ if { [info procs saved-dg-test] == [list] } {
 	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
@@ -871,6 +872,7 @@ if { [info procs saved-dg-test] == [list] } {
 	if [info exists testname_with_flags] {
 	    unset testname_with_flags
 	}
+	set multiline_expected_outputs []
     }
 }
 
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

Re: [PATCH] PR testsuite/69181: ensure expected multiline outputs is cleared per-test

[Bernd Schmidt]

On 01/09/2016 01:51 AM, David Malcolm wrote:
> The root cause here is that the logic to reset the list of expected
> multiline outputs was being run from:
>    handle-multiline-outputs, called by
>      prune.exp's prune_gcc_output
> and none of that happens if the test is skipped by a target exclusion
> in dg-do.

Thanks for tackling this.

> diff --git a/gcc/testsuite/lib/gcc-dg.exp b/gcc/testsuite/lib/gcc-dg.exp
> index f9ec206..f778bca 100644
> --- a/gcc/testsuite/lib/gcc-dg.exp
> +++ b/gcc/testsuite/lib/gcc-dg.exp
> @@ -836,6 +836,7 @@ if { [info procs saved-dg-test] == [list] } {
>   	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
> @@ -871,6 +872,7 @@ if { [info procs saved-dg-test] == [list] } {
>   	if [info exists testname_with_flags] {
>   	    unset testname_with_flags
>   	}
> +	set multiline_expected_outputs []
>       }
>   }

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?

Otherwise this looks reasonable IMO.


Bernd

Re: [PATCH] PR testsuite/69181: ensure expected multiline outputs is cleared per-test

[David Malcolm]

On Sat, 2016-01-09 at 03:07 +0100, Bernd Schmidt wrote:
> On 01/09/2016 01:51 AM, David Malcolm wrote:
> > The root cause here is that the logic to reset the list of expected
> > multiline outputs was being run from:
> >    handle-multiline-outputs, called by
> >      prune.exp's prune_gcc_output
> > and none of that happens if the test is skipped by a target exclusion
> > in dg-do.
> 
> Thanks for tackling this.
> 
> > diff --git a/gcc/testsuite/lib/gcc-dg.exp b/gcc/testsuite/lib/gcc-dg.exp
> > index f9ec206..f778bca 100644
> > --- a/gcc/testsuite/lib/gcc-dg.exp
> > +++ b/gcc/testsuite/lib/gcc-dg.exp
> > @@ -836,6 +836,7 @@ if { [info procs saved-dg-test] == [list] } {
> >   	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
> > @@ -871,6 +872,7 @@ if { [info procs saved-dg-test] == [list] } {
> >   	if [info exists testname_with_flags] {
> >   	    unset testname_with_flags
> >   	}
> > +	set multiline_expected_outputs []
> >       }
> >   }
> 
> 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")

I see that this pattern was introduced back in r67696 aka
91a385a522a94154f9e0cd940c5937177737af02:

diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 39ccaf6..c660eca 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2003-06-09  Mark Mitchell  <mark@codesourcery.com>
+
+       * lib/gcc-dg.exp (dg-test): Clear additional_files and
+       additional_sources.
+
 2003-05-21  David Taylor  <dtaylor@emc.com>
 
        * gcc.dg/Wpadded.c: New file.
diff --git a/gcc/testsuite/lib/gcc-dg.exp b/gcc/testsuite/lib/gcc-dg.exp
index aade663..1feadc4 100644
--- a/gcc/testsuite/lib/gcc-dg.exp
+++ b/gcc/testsuite/lib/gcc-dg.exp
@@ -294,3 +294,31 @@ proc dg-require-gc-sections { args } {
        return
     }
 }
+
+# We need to make sure that additional_files and additional_sources
+# are both cleared out after every test.  It is not enough to clear
+# them out *before* the next test run because gcc-target-compile gets
+# run directly from some .exp files (outside of any test).  (Those
+# uses should eventually be eliminated.) 
+
+# Because the DG framework doesn't provide a hook that is run at the
+# end of a test, we must replace dg-test with a wrapper.
+
+if { [info procs saved-dg-test] == [list] } {
+    rename dg-test saved-dg-test
+
+    proc dg-test { args } {
+       global additional_files
+       global additional_sources
+       global errorInfo
+
+       if { [ catch { eval saved-dg-test $args } errmsg ] } {
+           set saved_info $errorInfo
+           set additional_files ""
+           set additional_sources ""
+           error $errmsg $saved_info
+       }
+       set additional_files ""
+       set additional_sources ""
+    }
+}

and this pattern has been extended over the years.

I *could* add the
  set multiline_expected_outputs []
to the block guarded by the if {}, but it feels like cargo-culting to
me.  Am I missing something?


> Otherwise this looks reasonable IMO.
> 
> 
> Bernd

Re: [PATCH] PR testsuite/69181: ensure expected multiline outputs is cleared per-test

[Jeff Law]

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

Jeff