PR testsuite/69181: ensure expected multiline outputs is cleared per-test (v2)

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&regrtested 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

PR testsuite/69181: ensure expected multiline outputs is cleared per-test (v2)

Commit Message

Comments

Patch