libc++: test lock-free atomic alignment

[jfb]

Summary

libc++ implements std::atomic<_Tp> using atomic_base<_Tp> with
mutable _Atomic(_Tp) __a_. That member must be suitably aligned on
relevant ISAs for instructions such as cmpxchg to work properly, but
this alignment isn't checked anywhere. atomic_base's implementation
relies on _Atomic doing "the right thing" since it's under the
compiler's control, and only the compiler knows about lock-freedom and
instruction generation. This test makes sure that the compiler isn't
breaking libc++'s expectations.

I'm looking at a few odd things in the C++ standard, and will have a few
other fixes around this area in the future.

This requires building with -DLIBCXX_HAS_ATOMIC_LIB=True.

[EricWF]

• The test should be moved to test/libcxx/atomics/atomics.align since it's libc++ specific.
• Please give the anonymous struct declarations unique names. T1, T2, ..., TN is fine. Currently they all mangle to main::type in diagnostic output.
• The test fails to link on my machine unless I manually links -latomic. The tests currently don't link -latomic and I don't want to turn it on by default. I'll try and fix this.

[jfb]

• Move atomics.align to libcxx-specific
• Give names to anonymous structs

[jfb]

In D22073#481402, @EricWF wrote:

• The test should be moved to test/libcxx/atomics/atomics.align since it's libc++ specific.

Done.

• Please give the anonymous struct declarations unique names. T1, T2, ..., TN is fine. Currently they all mangle to main::type in diagnostic output.

Done. I'll update test/std/atomics/atomics.lockfree/isalwayslockfree.pass.cpp in a separate change to do the same (I wrote that one as well, didn't name the structs at the time).

• The test fails to link on my machine unless I manually links -latomic. The tests currently don't link -latomic and I don't want to turn it on by default. I'll try and fix this.

OK, LMK if I can do anything. I build with:

cmake -G Ninja ../llvm \
  -DCMAKE_EXPORT_COMPILE_COMMANDS=ON \
  -DLLVM_BUILD_TESTS=ON \
  -DCMAKE_BUILD_TYPE=RelWithDebInfo \
  -DLLVM_ENABLE_ASSERTIONS=ON \
  -DCMAKE_C_COMPILER=/path/to/bin/clang \
  -DCMAKE_CXX_COMPILER=/path/to/bin/clang++ \
  -DLLVM_PATH=/path/to/llvm \
  -DLIBCXX_CXX_ABI=libcxxabi \
  -DLIBCXX_CXX_ABI_INCLUDE_PATHS=/path/to/llvm/projects/libcxxabi/include \
  -DLIBCXX_HAS_ATOMIC_LIB=True

I found it weird to have to specify -DLIBCXX_HAS_ATOMIC_LIB=True because it tells me that libc++ isn't tested *at all* with GCCMM atomic runtime functions! I was going to look into that afterwards.

IMO we may want to always link to it.

[EricWF]

In D22073#482120, @jfb wrote:

In D22073#481402, @EricWF wrote:

• The test should be moved to test/libcxx/atomics/atomics.align since it's libc++ specific.

Done.

• Please give the anonymous struct declarations unique names. T1, T2, ..., TN is fine. Currently they all mangle to main::type in diagnostic output.

Done. I'll update test/std/atomics/atomics.lockfree/isalwayslockfree.pass.cpp in a separate change to do the same (I wrote that one as well, didn't name the structs at the time).

• The test fails to link on my machine unless I manually links -latomic. The tests currently don't link -latomic and I don't want to turn it on by default. I'll try and fix this.

OK, LMK if I can do anything. I build with:

cmake -G Ninja ../llvm \
  -DCMAKE_EXPORT_COMPILE_COMMANDS=ON \
  -DLLVM_BUILD_TESTS=ON \
  -DCMAKE_BUILD_TYPE=RelWithDebInfo \
  -DLLVM_ENABLE_ASSERTIONS=ON \
  -DCMAKE_C_COMPILER=/path/to/bin/clang \
  -DCMAKE_CXX_COMPILER=/path/to/bin/clang++ \
  -DLLVM_PATH=/path/to/llvm \
  -DLIBCXX_CXX_ABI=libcxxabi \
  -DLIBCXX_CXX_ABI_INCLUDE_PATHS=/path/to/llvm/projects/libcxxabi/include \
  -DLIBCXX_HAS_ATOMIC_LIB=True

I found it weird to have to specify -DLIBCXX_HAS_ATOMIC_LIB=True because it tells me that libc++ isn't tested *at all* with GCCMM atomic runtime functions! I was going to look into that afterwards.

Right, but I'm not concerned by that. Our <atomic> implementation is a very shallow wrapper over compiler builtins and I would expect the compiler to test those builtins properly.
What do you expect libc++ to gain from testing this?

IMO we may want to always link to it.

IMHO we don't want to link it unless we need it internally. By not linking it the tests prove that all internally used atomic operations are lock free. I don't see what we gain from linking it unconditionally, except for maybe ease of use when users have "oversized" types.

[EricWF]

OK, IMO the way to handle this test is to have it manually link -latomic. This can be done by renaming the test to <test>.sh.cpp and adding the following lines:

// REQUIRES: libatomic
// RUN: %build -latomic
// RUN: %run

After that this LGTM.

[jfb]

• Move atomics.align to libcxx-specific
• Give names to anonymous structs
• Rename test, use REQUIRES / RUN commands

[jfb]

In D22073#486636, @EricWF wrote:

OK, IMO the way to handle this test is to have it manually link -latomic. This can be done by renaming the test to <test>.sh.cpp and adding the following lines:

// REQUIRES: libatomic
// RUN: %build -latomic
// RUN: %run

After that this LGTM.

Sorry for the delay in getting back to this. It's now done :)
Will land after lunch (so I'm not AFK).

test/std/atomics/atomics.align/align.pass.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
+
+// <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
+// lock-freedom (e.g. load-linked / store-conditional, or cmpxchg).
+
+#include <atomic>
+#include <cassert>
+
+template <typename T> struct atomic_test : public std::__atomic_base<T> {
+  atomic_test() {
+    if (this->is_lock_free())
+      assert(alignof(this->__a_) >= sizeof(this->__a_) &&
+             "expected natural alignment for lock-free type");
+  }
+};
+
+int main() {
+
+// structs and unions can't be defined in the template invocation.
+// Work around this with a typedef.
+#define CHECK_ALIGNMENT(T)                                                     \
+  do {                                                                         \
+    typedef T type;                                                            \
+    atomic_test<type> t;                                                       \
+  } while (0)
+
+  CHECK_ALIGNMENT(bool);
+  CHECK_ALIGNMENT(char);
+  CHECK_ALIGNMENT(signed char);
+  CHECK_ALIGNMENT(unsigned char);
+  CHECK_ALIGNMENT(char16_t);
+  CHECK_ALIGNMENT(char32_t);
+  CHECK_ALIGNMENT(wchar_t);
+  CHECK_ALIGNMENT(short);
+  CHECK_ALIGNMENT(unsigned short);
+  CHECK_ALIGNMENT(int);
+  CHECK_ALIGNMENT(unsigned int);
+  CHECK_ALIGNMENT(long);
+  CHECK_ALIGNMENT(unsigned long);
+  CHECK_ALIGNMENT(long long);
+  CHECK_ALIGNMENT(unsigned long long);
+  CHECK_ALIGNMENT(std::nullptr_t);
+  CHECK_ALIGNMENT(void *);
+  CHECK_ALIGNMENT(float);
+  CHECK_ALIGNMENT(double);
+  CHECK_ALIGNMENT(long double);
+  CHECK_ALIGNMENT(int __attribute__((vector_size(1 * sizeof(int)))));
+  CHECK_ALIGNMENT(int __attribute__((vector_size(2 * sizeof(int)))));
+  CHECK_ALIGNMENT(int __attribute__((vector_size(4 * sizeof(int)))));
+  CHECK_ALIGNMENT(int __attribute__((vector_size(16 * sizeof(int)))));
+  CHECK_ALIGNMENT(int __attribute__((vector_size(32 * sizeof(int)))));
+  CHECK_ALIGNMENT(float __attribute__((vector_size(1 * sizeof(float)))));
+  CHECK_ALIGNMENT(float __attribute__((vector_size(2 * sizeof(float)))));
+  CHECK_ALIGNMENT(float __attribute__((vector_size(4 * sizeof(float)))));
+  CHECK_ALIGNMENT(float __attribute__((vector_size(16 * sizeof(float)))));
+  CHECK_ALIGNMENT(float __attribute__((vector_size(32 * sizeof(float)))));
+  CHECK_ALIGNMENT(double __attribute__((vector_size(1 * sizeof(double)))));
+  CHECK_ALIGNMENT(double __attribute__((vector_size(2 * sizeof(double)))));
+  CHECK_ALIGNMENT(double __attribute__((vector_size(4 * sizeof(double)))));
+  CHECK_ALIGNMENT(double __attribute__((vector_size(16 * sizeof(double)))));
+  CHECK_ALIGNMENT(double __attribute__((vector_size(32 * sizeof(double)))));
+  CHECK_ALIGNMENT(struct {});
+  CHECK_ALIGNMENT(struct { int i; });
+  CHECK_ALIGNMENT(struct { int i[2]; });
+  CHECK_ALIGNMENT(struct { long long int i[2]; });
+  CHECK_ALIGNMENT(struct { long long int i[4]; });
+  CHECK_ALIGNMENT(struct { long long int i[8]; });
+  CHECK_ALIGNMENT(struct { long long int i[16]; });
+  CHECK_ALIGNMENT(struct { char c; /* padding */ long long int i; });
+  CHECK_ALIGNMENT(union { int i; float f; });
+}