Page MenuHomePhabricator

libc++: test lock-free atomic alignment
ClosedPublic

Authored by jfb on Jul 6 2016, 4:45 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

jfb updated this revision to Diff 62997.Jul 6 2016, 4:45 PM
jfb retitled this revision from to libc++: test lock-free atomic alignment.
jfb updated this object.
jfb added a reviewer: cfe-commits.
jfb updated this object.Jul 6 2016, 4:46 PM
jfb added subscribers: mclow.lists, 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 updated this revision to Diff 63723.Jul 12 2016, 1:13 PM
  • Move atomics.align to libcxx-specific
  • Give names to anonymous structs
jfb added a comment.Jul 12 2016, 1:17 PM
  • 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.

In D22073#482120, @jfb 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.

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 updated this revision to Diff 66342.Aug 1 2016, 11:24 AM
  • Move atomics.align to libcxx-specific
  • Give names to anonymous structs
  • Rename test, use REQUIRES / RUN commands
jfb added a comment.Aug 1 2016, 11:25 AM

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

This revision was automatically updated to reflect the committed changes.