Un-XFAIL GCC atomics.align

[jfb]

Summary

The ABI version flag should fix the error.

[jfb]

@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<T>::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.

[jfb]

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

[EricWF]

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.

[jfb]

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

[EricWF]

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?

test/libcxx/atomics/atomics.align/align.pass.sh.cpp

 //===----------------------------------------------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is dual licensed under the MIT and the University of Illinois Open
 // Source Licenses. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // UNSUPPORTED: libcpp-has-no-threads, c++98, c++03
 // 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
 
 // <atomic>
 
 // Verify that the content of atomic<T> is properly aligned if the type is
 // lock-free. This can't be observed through the atomic<T> API. It is
 // nonetheless required for correctness of the implementation: lock-free implies
 // that ISA instructions are used, and these instructions assume "suitable
 // alignment". Supported architectures all require natural alignment for

test/libcxx/test/config.py

def configure_compile_flags(self):
             self.configure_default_compile_flags()
         # This include is always needed so add so add it regardless of
         # 'no_default_flags'.
         support_path = os.path.join(self.libcxx_src_root, 'test/support')
         self.cxx.compile_flags += ['-I' + support_path]
         # 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
         # default given in lit.site.cfg is not present. If default is not
         # present then force c++11.
         std = self.get_lit_conf('std')
         if not std:
             # Choose the newest possible language dialect if none is given.