Page MenuHomePhabricator

[libc++] Add missing XFAIL to tests that need __atomic_* libcalls
ClosedPublic

Authored by arichardson on Oct 5 2020, 3:52 AM.

Details

Summary

FreeBSD did not provide the __atomic_* functions as part of the base
system until recently. They were added to libgcc_s in SVN revision r364753
(August 2020), so check for availability of 'non-lockfree-atomics' so that
these tests do not fail unexpectedly on older versions of FreeBSD.

This also removes the #ifndef APPLE from atomic_helpers.h that was used
to work around lack of atomic runtime functions on older Apple platforms
and replaces it with XFAIL: !non-lockfree-atomics.

Diff Detail

Event Timeline

arichardson created this revision.Oct 5 2020, 3:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 5 2020, 3:52 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
arichardson requested review of this revision.Oct 5 2020, 3:52 AM

fine with me

ldionne requested changes to this revision.Oct 13 2020, 10:01 AM

What is the error that you're seeing? How are you running the test suite?

It looks like you are running against the system library, right? If so, we already have a feature to handle these sorts of XFAILs, it's with_system_cxx_lib=<system>. It's only used by macOS for the time being, but it can (and should) be used by other systems as well.

This revision now requires changes to proceed.Oct 13 2020, 10:01 AM
arichardson added a comment.EditedOct 19 2020, 6:19 AM

What is the error that you're seeing? How are you running the test suite?

It looks like you are running against the system library, right? If so, we already have a feature to handle these sorts of XFAILs, it's with_system_cxx_lib=<system>. It's only used by macOS for the time being, but it can (and should) be used by other systems as well.

I am running the tests against a just-build libc++: mkdir -p libcxx/build && cd libcxx/build && cmake -GNinja .. && ninja check-cxx
Most of the atomics tests need libatomic since they perform large atomic operations that are not natively supported and therefore call atomic_store/atomic_load, etc. This works on Linux since LinuxLocalTI adds -latomic to the linker flags but FreeBSD does not ship a libatomic.
As of https://reviews.freebsd.org/rS364753 the __atomic atomic functions are included in libcompiler-rt.a and libgcc_s.so.

Here is an example of a failing test with a full command line:

FreeBSD clang version 8.0.1 (tags/RELEASE_801/final 366581) (based on LLVM 8.0.1)
Target: x86_64-unknown-freebsd12
Thread model: posix
InstalledDir: /usr/bin
 "/usr/bin/c++" -cc1 -triple x86_64-unknown-freebsd12 -emit-obj -mrelax-all -disable-free -disable-llvm-verifier -discard-value-names -main-file-name atomic_load_explicit.pass.cpp -mrelocation-model static -mthread-model posix -mdisable-fp-elim -masm-verbose -mconstructor-aliases -munwind-tables -fuse-init-array -target-cpu x86-64 -dwarf-column-info -debugger-tuning=gdb -v -nostdinc++ -resource-dir /usr/lib/clang/8.0.1 -include /exports/users/alr48/sources/upstream-llvm-project/libcxx/build/__config_site -include /exports/users/alr48/sources/upstream-llvm-project/libcxx/test/support/nasty_macros.h -I /exports/users/alr48/sources/upstream-llvm-project/libcxx/include -I /exports/users/alr48/sources/upstream-llvm-project/libcxx/build/include/c++build -I /exports/users/alr48/sources/upstream-llvm-project/libcxx/test/support -D _LIBCPP_DISABLE_AVAILABILITY -D _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -Werror -Wall -Wextra -Wshadow -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -Werror=thread-safety -Wuser-defined-warnings -std=c++2a -fdeprecated-macro -fdebug-compilation-dir /exports/users/alr48/sources/upstream-llvm-project/libcxx/build/test/std/atomics/atomics.types.operations/atomics.types.operations.req -ferror-limit 19 -fmessage-length 0 -fcoroutines-ts -fobjc-runtime=gnustep -fcxx-exceptions -fexceptions -fdiagnostics-show-option -o /tmp/atomic_load_explicit-edb87d.o -x c++ /exports/users/alr48/sources/upstream-llvm-project/libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/atomic_load_explicit.pass.cpp -faddrsig
clang -cc1 version 8.0.1 based upon LLVM 8.0.1 default target x86_64-unknown-freebsd12.1
#include "..." search starts here:
#include <...> search starts here:
 /exports/users/alr48/sources/upstream-llvm-project/libcxx/include
 /exports/users/alr48/sources/upstream-llvm-project/libcxx/build/include/c++build
 /exports/users/alr48/sources/upstream-llvm-project/libcxx/test/support
 /usr/lib/clang/8.0.1/include
 /usr/include
End of search list.
 "/usr/bin/ld" --eh-frame-hdr -dynamic-linker /libexec/ld-elf.so.1 --hash-style=both --enable-new-dtags -o /exports/users/alr48/sources/upstream-llvm-project/libcxx/build/test/std/atomics/atomics.types.operations/atomics.types.operations.req/Output/atomic_load_explicit.pass.cpp.dir/t.tmp.exe /usr/lib/crt1.o /usr/lib/crti.o /usr/lib/crtbegin.o -L/exports/users/alr48/sources/upstream-llvm-project/libcxx/build/lib -L/usr/lib /tmp/atomic_load_explicit-edb87d.o -rpath /exports/users/alr48/sources/upstream-llvm-project/libcxx/build/lib -lc++ -lc -lm -lpthread -lgcc_s -lcxxrt -lc++experimental /usr/lib/crtend.o /usr/lib/crtn.o
ld: error: undefined symbol: __atomic_load
>>> referenced by atomic_load_explicit.pass.cpp
>>>               /tmp/atomic_load_explicit-edb87d.o:(LargeUserAtomicType std::__1::__cxx_atomic_load<LargeUserAtomicType>(std::__1::__cxx_atomic_base_impl<LargeUserAtomicType> const*, std::__1::memory_order))

ld: error: undefined symbol: __atomic_load
>>> referenced by atomic_load_explicit.pass.cpp
>>>               /tmp/atomic_load_explicit-edb87d.o:(LargeUserAtomicType std::__1::__cxx_atomic_load<LargeUserAtomicType>(std::__1::__cxx_atomic_base_impl<LargeUserAtomicType> const volatile*, std::__1::memory_order))
c++: error: linker command failed with exit code 1 (use -v to see invocation)

As you can see here, there is a -lgcc_s in the linker command, so it will start working on a newer FreeBSD version.

On macOS these functions appear to be provided by /usr/lib/system/libcompiler_rt.dylib, which I believe is automatically added when you link -lSystem.

ldionne added inline comments.Nov 10 2020, 4:36 AM
libcxx/utils/libcxx/test/features.py
122 ↗(On Diff #296145)

This check shouldn't be FreeBSD specific. Instead, we should see whether we can reference these atomic operations generically, by checking the linker output of an appropriate program.

Also, can you explain why the non-lockfree-atomics feature doesn't do what you want?

arichardson planned changes to this revision.Nov 10 2020, 4:45 AM

Will update to use non-lockfree-atomics

libcxx/utils/libcxx/test/features.py
122 ↗(On Diff #296145)

I did not see the "non-lockfree-atomic" feature. That should do the same thing by checking whether the source compiles rather than doing it indirectly using a FreeBSD version check.

arichardson updated this revision to Diff 304141.EditedNov 10 2020, 4:52 AM

Use non-lockfree-atomics instead.

This changes the FreeBSD test results (using system clang, FreeBSD clang version 8.0.1 (tags/RELEASE_801/final 366581) (based on LLVM 8.0.1) from

Unsupported        :  199
Passed             : 6164
Expectedly Failed  :   48
Failed             :   48
Unexpectedly Passed:    2

to

Unsupported        :  199
Passed             : 6164
Expectedly Failed  :   61
Failed             :   35
Unexpectedly Passed:    2
arichardson retitled this revision from [libc++][FreeBSD] XFAIL tests that need __atomic_* libcalls on older versions to [libc++] Add missing XFAIL to tests that need __atomic_* libcalls.Nov 10 2020, 4:53 AM
arichardson edited the summary of this revision. (Show Details)
ldionne requested changes to this revision.Nov 20 2020, 12:13 PM

So... do I understand correctly that you're only seeing issues on older FreeBSDs where libcompiler_rt does not contain __atomic_load & friends? Because macOS is in the same boat as FreeBSD: it doesn't ship libatomic.a (hence the need for the non-lockfree-atomics Lit feature).

You might also want to take a look at libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/atomic_helpers.h -- we seem to exclude large types from some tests on __APPLE__ -- would also excluding them from __FreeBsd__ solve your issues?

Requesting changes since this is breaking CI -- I'm not sure yet what we'll have to change, but we'll have to change something.

libcxx/utils/libcxx/test/features.py
43 ↗(On Diff #304141)

The non-lockfree-atomics feature is defined here, but I assume you found it?

This revision now requires changes to proceed.Nov 20 2020, 12:13 PM
arichardson edited the summary of this revision. (Show Details)

Rebase on top of D91911 and remove the #ifndef APPLE special case from atomic_helpers.

The CI was failing because the non-lockfree-atomics was being detected as false on Apple platforms due to missing __atomic_is_lockfree even though the other ones were available.

arichardson edited the summary of this revision. (Show Details)Nov 21 2020, 4:40 AM
arichardson edited the summary of this revision. (Show Details)

Rebase for CI

CI seems happy now. Does this + D91911 look reasonable @ldionne?

This technically regresses our coverage of atomics on Apple platforms, since those tests are testing both small and large types. I'm OK with that, but I'll make it a priority to investigate shipping support for non-lockfree atomics on Apple platforms.

ldionne accepted this revision.Dec 1 2020, 8:23 AM
This revision is now accepted and ready to land.Dec 1 2020, 8:23 AM

This technically regresses our coverage of atomics on Apple platforms, since those tests are testing both small and large types. I'm OK with that, but I'll make it a priority to investigate shipping support for non-lockfree atomics on Apple platforms.

With these patches the tests still run on my macOS 10.15 laptop, it's just libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/atomic_is_lock_free.pass.cpp that no longer runs. In some ways this is now testing more code on Apple platforms since the TestFunctor<LargeUserAtomicType> is no longer ifdef'd away.

Will commit this once D92302 has landed.