[PATCH] D23041: Un-XFAIL GCC atomics.align
EricWF added a comment. OK, here's what I would do: (1) Split the vector, nullptr, and other weirdly failing tests into their own files. (2) In each file, use `defined(TEST_COMPILER_GCC) && __GXX_ABI_VERSION > 1006` (or w/e version is broken) to define a macro when the test is expected to fail. (3) Either `ifdef` out failing tests, or change the assertion to expect failure. The latter is probably preferable as it will allow us to detect if they test ever starts unexpectedly passing. (4) Add tests for GCC with different `fabi-version` options. I would make these separate test files which wrap the actual test their targeting. For example: // // This file is dual licensed under the MIT and the University of Illinois Open // Source Licenses. See LICENSE.TXT for details. // //===--===// // // REQUIRES: gcc, libatomic // UNSUPPORTED: libcpp-has-no-threads, c++98, c++03 // // RUN: %cxx -o %t.one.exe %s %all_flags -latomic -fabi-version=2 && %t.one.exe // RUN: %cxx -o %t.two.exe %s %all_flags -latomic -fabi-version=6 && %t.two.exe // RUN: %cxx -o %t.three.exe %s %all_flags -latomic -fabi-version=9 && %t.three.exe #include "align-vector-types.sh.cpp" How does that sound? https://reviews.llvm.org/D23041 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D23041: Un-XFAIL GCC atomics.align
jfb added a comment. Herald added subscribers: christof, aheejin. In https://reviews.llvm.org/D23041#632708, @EricWF wrote: > Have you filed a bug against GCC regarding its current behavior? > > Also it seems like a bad idea to add `-fabi-version=6`, since it selects an > older ABI version and not a newer one. Testing the old behavior is not what > we want. > > I think the best plan is to simply split the `vector_size` tests into another > file and XFAIL that for GCC. At least then we get some coverage. Reviving this old thread... Do you think this is still the right approach? I can file the GCC bug and split off the test. https://reviews.llvm.org/D23041 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D23041: Un-XFAIL GCC atomics.align
EricWF added a comment. Have you filed a bug against GCC regarding its current behavior? Also it seems like a bad idea to add `-fabi-version=6`, since it selects an older ABI version and not a newer one. Testing the old behavior is not what we want. I think the best plan is to simply split the `vector_size` tests into another file and XFAIL that for GCC. At least then we get some coverage. https://reviews.llvm.org/D23041 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23041: Un-XFAIL GCC atomics.align
jfb added a comment. I wrote a quick test, and I think this is technically OK for x86 because alignment "Just Works", but I think it's borked on GCC+ARM (I don't have a system to test that here): https://github.com/jfbastien/atomic_nullptr/blob/master/atomic_nullptr.cc https://reviews.llvm.org/D23041 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23041: Un-XFAIL GCC atomics.align
jfb added a comment. @EricWF ran this on configuration `cxx_under_test=g++-4.9`. It generates the following error: libcxx/test/libcxx/atomics/atomics.align/align.pass.sh.cpp:32: atomic_test::atomic_test() [with T = std::nullptr_t]: Assertion `alignof(this->__a_) >= sizeof(this->__a_) && "expected natural alignment for lock-free type"' failed. I think GCC is broken. See the following (semi-relevant) test: https://godbolt.org/g/pFUqVY That's not quite what I'm testing (this used libstdc++ and looks at `std::atomic`, not the value contained), but I'd nonetheless expect the `std::atomic` container to be sufficiently aligned. WDYT? Is that only happening for `nullptr_t`? I can conditionally disable the `nullptr_t` test for GCC, and file a bug on GCC. https://reviews.llvm.org/D23041 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D23041: Un-XFAIL GCC atomics.align
jfb created this revision.
jfb added a reviewer: EricWF.
jfb added a subscriber: cfe-commits.
The ABI version flag should fix the error.
https://reviews.llvm.org/D23041
Files:
test/libcxx/atomics/atomics.align/align.pass.sh.cpp
test/libcxx/test/config.py
Index: test/libcxx/test/config.py
===
--- test/libcxx/test/config.py
+++ test/libcxx/test/config.py
@@ -306,6 +306,11 @@
# Configure extra flags
compile_flags_str = self.get_lit_conf('compile_flags', '')
self.cxx.compile_flags += shlex.split(compile_flags_str)
+# GCC ABI version 6 first appeared in G++ 4.7, corrects the mangling of
+# template argument packs, which is required for the atomics.align
test.
+abi_flag = '-fabi-version=6'
+if self.cxx.hasCompileFlag(abi_flag):
+self.cxx.compile_flags += [abi_flag]
def configure_default_compile_flags(self):
# Try and get the std version from the command line. Fall back to
Index: test/libcxx/atomics/atomics.align/align.pass.sh.cpp
===
--- test/libcxx/atomics/atomics.align/align.pass.sh.cpp
+++ test/libcxx/atomics/atomics.align/align.pass.sh.cpp
@@ -11,10 +11,6 @@
// REQUIRES: libatomic
// RUN: %build -latomic
// RUN: %run
-//
-// GCC currently fails because it needs -fabi-version=6 to fix mangling of
-// std::atomic when used with __attribute__((vector(X))).
-// XFAIL: gcc
//
Index: test/libcxx/test/config.py
===
--- test/libcxx/test/config.py
+++ test/libcxx/test/config.py
@@ -306,6 +306,11 @@
# Configure extra flags
compile_flags_str = self.get_lit_conf('compile_flags', '')
self.cxx.compile_flags += shlex.split(compile_flags_str)
+# GCC ABI version 6 first appeared in G++ 4.7, corrects the mangling of
+# template argument packs, which is required for the atomics.align test.
+abi_flag = '-fabi-version=6'
+if self.cxx.hasCompileFlag(abi_flag):
+self.cxx.compile_flags += [abi_flag]
def configure_default_compile_flags(self):
# Try and get the std version from the command line. Fall back to
Index: test/libcxx/atomics/atomics.align/align.pass.sh.cpp
===
--- test/libcxx/atomics/atomics.align/align.pass.sh.cpp
+++ test/libcxx/atomics/atomics.align/align.pass.sh.cpp
@@ -11,10 +11,6 @@
// REQUIRES: libatomic
// RUN: %build -latomic
// RUN: %run
-//
-// GCC currently fails because it needs -fabi-version=6 to fix mangling of
-// std::atomic when used with __attribute__((vector(X))).
-// XFAIL: gcc
//
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
