diff mbox

[testsuite] Clean up effective_target cache

Message ID CAKdteObvT_g=usMxP_G0AyUWqFrXCj2AhQSj=YUGvi8v3=F1Rg@mail.gmail.com
State New
Headers show

Commit Message

Christophe Lyon Aug. 25, 2015, 8:14 a.m. UTC
Hi,

Some subsets of the tests override ALWAYS_CXXFLAGS or
TEST_ALWAYS_FLAGS and perform effective_target support tests using
these modified flags.

In case these flags conflict with the effective_target tests, it means
that subsequent tests will be UNSUPPORTED even though
ALWAYS_CXXFLAGS/TEST_ALWAYS_FLAGS have been reset and no longer
conflict.

In practice, we noticed this when running validation under 'ulimit -v
XXX', which can conflict with ASAN. We observed that sse2 and
stack_protector tests would randomly fail when tested from asan.exp,
making non-asan tests UNSUPPORTED.

This patch adds a new function 'clear_effective_target_cache', which
is called at the end of every .exp file which overrides
ALWAYS_CXXFLAGS or TEST_ALWAYS_FLAGS.

I tested it works well for asan.exp on x86_64 but the changes in other
.exp files seem mechanical.

However, I noticed that lib/g++.exp changes ALWAYS_CXXFLAGS, but does
not appear to restore it. In doubt, I didn't change it.

OK?

Christophe.

Comments

Mike Stump Aug. 25, 2015, 3:31 p.m. UTC | #1
On Aug 25, 2015, at 1:14 AM, Christophe Lyon <christophe.lyon@linaro.org> wrote:
> Some subsets of the tests override ALWAYS_CXXFLAGS or
> TEST_ALWAYS_FLAGS and perform effective_target support tests using
> these modified flags.

> This patch adds a new function 'clear_effective_target_cache', which
> is called at the end of every .exp file which overrides
> ALWAYS_CXXFLAGS or TEST_ALWAYS_FLAGS.

So, a simple English directive somewhere that says, if one changes ALWAYS_CXXFLAGS or TEST_ALWAYS_FLAGS then they should do a clear_effective_target_cache at the end as the target cache can make decisions based upon the flags, and those decisions need to be redone when the flags change would be nice.

I do wonder, do we need to reexamine when setting the flags?  I’m thinking of a sequence like: non-thumb default, is_thumb, set flags (thumb), is_thumb.  Anyway, safe to punt this until someone discovers it or is reasonable sure it happens.

Anyway, all looks good.  Ok.

> However, I noticed that lib/g++.exp changes ALWAYS_CXXFLAGS, but does
> not appear to restore it. In doubt, I didn't change it.

Yeah, I examined it.  It seems like it might not matter, as anyone setting and unsetting would come in cleared, and if they didn’t, it should be roughly the same exact state, meaning, no clearing necessary.  I think it is safe to punt this until someone finds a bug or can see a way that it would matter.  I also don’t think it would hurt to clear, if someone wanted to refactor the code a bit and make the clearing and the cleanup a little more automatic.  I’m thinking of a RAII style code in which the dtor runs the clear.  Not sure if that is even possible in tcl.  [ checking ] Nope, maybe not.  Oh well.
Jeff Law Aug. 25, 2015, 8:22 p.m. UTC | #2
On 08/25/2015 02:14 AM, Christophe Lyon wrote:
> Hi,
>
> Some subsets of the tests override ALWAYS_CXXFLAGS or
> TEST_ALWAYS_FLAGS and perform effective_target support tests using
> these modified flags.
>
> In case these flags conflict with the effective_target tests, it means
> that subsequent tests will be UNSUPPORTED even though
> ALWAYS_CXXFLAGS/TEST_ALWAYS_FLAGS have been reset and no longer
> conflict.
>
> In practice, we noticed this when running validation under 'ulimit -v
> XXX', which can conflict with ASAN. We observed that sse2 and
> stack_protector tests would randomly fail when tested from asan.exp,
> making non-asan tests UNSUPPORTED.
>
> This patch adds a new function 'clear_effective_target_cache', which
> is called at the end of every .exp file which overrides
> ALWAYS_CXXFLAGS or TEST_ALWAYS_FLAGS.
>
> I tested it works well for asan.exp on x86_64 but the changes in other
> .exp files seem mechanical.
>
> However, I noticed that lib/g++.exp changes ALWAYS_CXXFLAGS, but does
> not appear to restore it. In doubt, I didn't change it.
>
> OK?
OK after a full regression test.  While I agree the change is 
mechanical, there may be interactions that are non-obvious.

Jeff
diff mbox

Patch

2015-08-25  Christophe Lyon  <christophe.lyon@linaro.org>

	* lib/target-supports.exp (clear_effective_target_cache): New.
	(check_cached_effective_target): Update et_prop_list.
	* lib/asan-dg.exp (asan_finish): Call clear_effective_target_cache.
	* g++.dg/compat/compat.exp: Likewise.
	* g++.dg/compat/struct-layout-1.exp: Likewise.
	* lib/asan-dg.exp: Likewise.
	* lib/atomic-dg.exp: Likewise.
	* lib/cilk-plus-dg.exp: Likewise.
	* lib/clearcap.exp: Likewise.
	* lib/mpx-dg.exp: Likewise.
	* lib/target-supports.exp: Likewise.
	* lib/tsan-dg.exp: Likewise.
	* lib/ubsan-dg.exp: Likewise.

diff --git a/gcc/testsuite/g++.dg/compat/compat.exp b/gcc/testsuite/g++.dg/compat/compat.exp
index 1272289..4c4b25f 100644
--- a/gcc/testsuite/g++.dg/compat/compat.exp
+++ b/gcc/testsuite/g++.dg/compat/compat.exp
@@ -78,6 +78,7 @@  proc compat-use-tst-compiler { } {
 	set ALWAYS_CXXFLAGS $save_always_cxxflags
 	set ld_library_path $save_ld_library_path
 	set_ld_library_path_env_vars
+	clear_effective_target_cache
     }
 }
 
diff --git a/gcc/testsuite/g++.dg/compat/struct-layout-1.exp b/gcc/testsuite/g++.dg/compat/struct-layout-1.exp
index 7777d98..097a731 100644
--- a/gcc/testsuite/g++.dg/compat/struct-layout-1.exp
+++ b/gcc/testsuite/g++.dg/compat/struct-layout-1.exp
@@ -61,6 +61,7 @@  proc compat-use-alt-compiler { } {
 	set ld_library_path $alt_ld_library_path
 	set_ld_library_path_env_vars
 	restore_gcc_exec_prefix_env_var
+	clear_effective_target_cache
     }
 }
 
diff --git a/gcc/testsuite/lib/asan-dg.exp b/gcc/testsuite/lib/asan-dg.exp
index 141a479..3ce264e 100644
--- a/gcc/testsuite/lib/asan-dg.exp
+++ b/gcc/testsuite/lib/asan-dg.exp
@@ -138,6 +138,7 @@  proc asan_finish { args } {
     }
     set ld_library_path $asan_saved_library_path
     set_ld_library_path_env_vars
+    clear_effective_target_cache
 }
 
 # Symbolize lines like
diff --git a/gcc/testsuite/lib/atomic-dg.exp b/gcc/testsuite/lib/atomic-dg.exp
index d9df227..fe24127 100644
--- a/gcc/testsuite/lib/atomic-dg.exp
+++ b/gcc/testsuite/lib/atomic-dg.exp
@@ -101,4 +101,5 @@  proc atomic_finish { args } {
     } else {
 	unset TEST_ALWAYS_FLAGS
     }
+    clear_effective_target_cache
 }
diff --git a/gcc/testsuite/lib/cilk-plus-dg.exp b/gcc/testsuite/lib/cilk-plus-dg.exp
index 38e5400..7f38f37 100644
--- a/gcc/testsuite/lib/cilk-plus-dg.exp
+++ b/gcc/testsuite/lib/cilk-plus-dg.exp
@@ -101,4 +101,5 @@  proc cilkplus_finish { args } {
     } else {
 	unset TEST_ALWAYS_FLAGS
     }
+    clear_effective_target_cache
 }
diff --git a/gcc/testsuite/lib/clearcap.exp b/gcc/testsuite/lib/clearcap.exp
index d41aa1e..3e2a88c 100644
--- a/gcc/testsuite/lib/clearcap.exp
+++ b/gcc/testsuite/lib/clearcap.exp
@@ -55,4 +55,5 @@  proc clearcap-finish { args } {
     } else {
 	unset TEST_ALWAYS_FLAGS
     }
+    clear_effective_target_cache
 }
diff --git a/gcc/testsuite/lib/mpx-dg.exp b/gcc/testsuite/lib/mpx-dg.exp
index c8f64cd..b2bd40c 100644
--- a/gcc/testsuite/lib/mpx-dg.exp
+++ b/gcc/testsuite/lib/mpx-dg.exp
@@ -142,4 +142,5 @@  proc mpx_finish { args } {
     }
     set ld_library_path $mpx_saved_library_path
     set_ld_library_path_env_vars
+    clear_effective_target_cache
 }
diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp
index 1988301..e2084bb 100644
--- a/gcc/testsuite/lib/target-supports.exp
+++ b/gcc/testsuite/lib/target-supports.exp
@@ -117,6 +117,7 @@  proc current_target_name { } {
 
 proc check_cached_effective_target { prop args } {
     global et_cache
+    global et_prop_list
 
     set target [current_target_name]
     if {![info exists et_cache($prop,target)]
@@ -124,12 +125,30 @@  proc check_cached_effective_target { prop args } {
 	verbose "check_cached_effective_target $prop: checking $target" 2
 	set et_cache($prop,target) $target
 	set et_cache($prop,value) [uplevel eval $args]
+	lappend et_prop_list $prop
+	verbose "check_cached_effective_target cached list is now: $et_prop_list" 2
     }
     set value $et_cache($prop,value)
     verbose "check_cached_effective_target $prop: returning $value for $target" 2
     return $value
 }
 
+# Clear effective-target cache. This is useful after testing
+# effective-target features and overriding TEST_ALWAYS_FLAGS and/or
+# ALWAYS_CXXFLAGS.
+
+proc clear_effective_target_cache { } {
+    global et_cache
+    global et_prop_list
+
+    verbose "clear_effective_target_cache: $et_prop_list" 2
+    foreach prop $et_prop_list {
+	unset et_cache($prop,value)
+	unset et_cache($prop,target)
+    }
+    unset et_prop_list
+}
+
 # Like check_compile, but delete the output file and return true if the
 # compiler printed no messages.
 proc check_no_compiler_messages_nocache {args} {
diff --git a/gcc/testsuite/lib/tsan-dg.exp b/gcc/testsuite/lib/tsan-dg.exp
index eb528f8..ff51fdf 100644
--- a/gcc/testsuite/lib/tsan-dg.exp
+++ b/gcc/testsuite/lib/tsan-dg.exp
@@ -149,4 +149,5 @@  proc tsan_finish { args } {
     }
     set ld_library_path $tsan_saved_library_path
     set_ld_library_path_env_vars
+    clear_effective_target_cache
 }
diff --git a/gcc/testsuite/lib/ubsan-dg.exp b/gcc/testsuite/lib/ubsan-dg.exp
index 81934bb..65799db 100644
--- a/gcc/testsuite/lib/ubsan-dg.exp
+++ b/gcc/testsuite/lib/ubsan-dg.exp
@@ -121,4 +121,5 @@  proc ubsan_finish { args } {
     }
     set ld_library_path $ubsan_saved_library_path
     set_ld_library_path_env_vars
+    clear_effective_target_cache
 }