[libc++] Link against libatomic when it is found

[ldionne]

Summary

Before this patch, we tried detecting whether small atomics were available
without linking against libatomic. However, that's not really what we want
to know -- instead, we want to know what's required in order to support
atomics fully, which is to link against libatomic when it's provided.

That is both much simpler, and it doesn't suffer the problem that we would
not link against libatomic when small atomics didn't require it, which
lead to non-lockfree atomics never working.

Furthermore, because we understand that some platforms might not want to
(or be able to) ship non-lockfree atomics, we add that notion to the test
suite, independently of a potential extern library.

After this patch, we therefore:
(1) Link against libatomic when it is provided
(2) Independently detect whether non-lockfree atomics are supported in

the test suite, regardless of whether that means we're linking against
an external library or not (which is an implementation detail).

[ldionne]

libcxx/cmake/config-ix.cmake

101

@phosek Is this right for Fuchsia?

[__simt__]

I'm not an expert in the details but this looks right to me.

When this goes in, I will be adding non-lock-free atomic tests.

These tests would fail today, meaning we have no coverage today.

[griwes]

FWIW this looks good to me as well.

[jfb]

libcxx/test/libcxx/selftest/dsl/dsl.sh.py

105

Here and for the others, int main(int, char**) 😄

libcxx/utils/libcxx/test/dsl.py

71

I think your filenames need to be quoted properly otherwise spaces in filenames will bork stuff (above and below).

libcxx/utils/libcxx/test/features.py

39

int main(int, char**)

[ldionne]

Address JF's comments

libcxx/utils/libcxx/test/dsl.py

71

Lit will expand filenames like %t and directories like %T, the assumption is that it will quote it properly for shell evaluation.

[jfb]

Thanks for this!

[loic.yhuel]

On which platform does this commit work ?

On Linux, the test program would still need an explicit -latomic, since indirect dependencies are not used to resolve undefined symbols from the program.
For this to work, it would need PUBLIC atomic instead of PRIVATE atomic, so that libcxx/cmake/Modules/DefineLinkerScript.cmake would add it to the linker script (libc++.so).
It would be even better with AS_NEEDED in the linker script, and -Wl,--as-needed for the lib, in order to only load the library when it is actually used.

[ldionne]

In D81190#2587483, @loic.yhuel wrote:

On which platform does this commit work ?

On Linux, the test program would still need an explicit -latomic, since indirect dependencies are not used to resolve undefined symbols from the program.
For this to work, it would need PUBLIC atomic instead of PRIVATE atomic, so that libcxx/cmake/Modules/DefineLinkerScript.cmake would add it to the linker script (libc++.so).
It would be even better with AS_NEEDED in the linker script, and -Wl,--as-needed for the lib, in order to only load the library when it is actually used.

Does this commit not work on your system? It has been working on our Linux bots since it was committed.

Regarding adding -latomic to the linker script, I don't know that we actually want to do that. I think the Clang driver should probably be the one to pass the correct -latomic or -l<library-for-builtins> depending on what builtin library we're using.

[loic.yhuel]

In D81190#2896925, @ldionne wrote:

Does this commit not work on your system? It has been working on our Linux bots since it was committed.

I don't remember if I found a case which needed the library.
My question was more : on which system does linking libc++ with libatomic help ?

If the platform doesn't need libatomic for the particular usecase, then the -latomic for libc++.so means the commit triggers a load a libatomic.so at runtime.
So the commit wouldn't break anything, but it would add a useless dependency.

But if libatomic is actually required, I think there must be a -latomic when linking the binary which uses the symbols (then also linking libc++.so with it doesn't change anything).
On Linux, when linking an executable (or a shared library with -Wl,--no-undefined), the linker checks all symbols referenced in the object files are actually resolved by the direct dependencies.
So while loading libatomic.so indirectly would be enough to resolve the symbols at runtime, on Linux at least the linker would mandate a direct link anyway.

Regarding adding -latomic to the linker script, I don't know that we actually want to do that. I think the Clang driver should probably be the one to pass the correct -latomic or -l<library-for-builtins> depending on what builtin library we're using.

The idea of a using the linker script was that any user of libc++.so would automatically link to libatomic.so when it's required (but not if it isn't), as if it were specified as -Wl,--push-state -Wl,--as-needed -latomic -Wl,--pop-state.
This would even apply to gcc using libc++ for example.

libcxx/CMakeLists.txt

if (LIBCXX_BUILTINS_LIBRARY)
       target_link_libraries(${target} PRIVATE "${LIBCXX_BUILTINS_LIBRARY}")
     endif()
   elseif (LIBCXX_HAS_GCC_LIB)
     target_link_libraries(${target} PRIVATE gcc)
   elseif (LIBCXX_HAS_GCC_S_LIB)
     target_link_libraries(${target} PRIVATE gcc_s)
   endif()
 
-  if (LIBCXX_HAVE_CXX_ATOMICS_WITH_LIB)
+  if (LIBCXX_HAS_ATOMIC_LIB)
     target_link_libraries(${target} PRIVATE atomic)
   endif()
 
   if (MINGW)
     target_link_libraries(${target} PRIVATE "${MINGW_LIBRARIES}")
   endif()
 
   if (LIBCXX_TARGETING_MSVC)

libcxx/cmake/Modules/CheckLibcxxAtomic.cmake

-INCLUDE(CheckCXXSourceCompiles)
-
-# Sometimes linking against libatomic is required for atomic ops, if
-# the platform doesn't support lock-free atomics.
-#
-# We could modify LLVM's CheckAtomic module and have it check for 64-bit
-# atomics instead. However, we would like to avoid careless uses of 64-bit
-# atomics inside LLVM over time on 32-bit platforms.
-
-function(check_cxx_atomics varname)
-  set(OLD_CMAKE_REQUIRED_FLAGS ${CMAKE_REQUIRED_FLAGS})
-  set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} -nodefaultlibs -std=c++11 -nostdinc++ -isystem ${LIBCXX_SOURCE_DIR}/include")
-  if (${LIBCXX_GCC_TOOLCHAIN})
-    set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} --gcc-toolchain=${LIBCXX_GCC_TOOLCHAIN}")
-  endif()
-  if (CMAKE_C_FLAGS MATCHES -fsanitize OR CMAKE_CXX_FLAGS MATCHES -fsanitize)
-    set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} -fno-sanitize=all")
-  endif()
-  if (CMAKE_C_FLAGS MATCHES -fsanitize-coverage OR CMAKE_CXX_FLAGS MATCHES -fsanitize-coverage)
-    set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} -fno-sanitize-coverage=edge,trace-cmp,indirect-calls,8bit-counters")
-  endif()
-  check_cxx_source_compiles("
-#include <cstdint>
-#include <atomic>
-std::atomic<uintptr_t> x;
-std::atomic<uintmax_t> y;
-int main(int, char**) {
-  return x + y;
-}
-" ${varname})
-  set(CMAKE_REQUIRED_FLAGS ${OLD_CMAKE_REQUIRED_FLAGS})
-endfunction(check_cxx_atomics)
-
-# Perform the check for 64bit atomics without libatomic. It may have been
-# added to the required libraries during in the configuration of LLVM, which
-# would cause the check for CXX atomics without libatomic to incorrectly pass.
-if (CMAKE_REQUIRED_LIBRARIES)
-  set(OLD_CMAKE_REQUIRED_LIBRARIES ${CMAKE_REQUIRED_LIBRARIES})
-  list(REMOVE_ITEM CMAKE_REQUIRED_LIBRARIES "atomic")
-  check_cxx_atomics(LIBCXX_HAVE_CXX_ATOMICS_WITHOUT_LIB)
-  set(CMAKE_REQUIRED_LIBRARIES ${OLD_CMAKE_REQUIRED_LIBRARIES})
-endif()
-
-check_library_exists(atomic __atomic_fetch_add_8 "" LIBCXX_HAS_ATOMIC_LIB)
-# If not, check if the library exists, and atomics work with it.
-if(NOT LIBCXX_HAVE_CXX_ATOMICS_WITHOUT_LIB)
-  if(LIBCXX_HAS_ATOMIC_LIB)
-    list(APPEND CMAKE_REQUIRED_LIBRARIES "atomic")
-    check_cxx_atomics(LIBCXX_HAVE_CXX_ATOMICS_WITH_LIB)
-    if (NOT LIBCXX_HAVE_CXX_ATOMICS_WITH_LIB)
-      message(WARNING "Host compiler must support std::atomic!")
-    endif()
-  else()
-    message(WARNING "Host compiler appears to require libatomic, but cannot find it.")
-  endif()
-endif()

libcxx/cmake/config-ix.cmake

if(CMAKE_CXX_COMPILER_ID MATCHES "Clang")
   set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} -Werror=unknown-pragmas")
   check_c_source_compiles("
 #pragma comment(lib, \"c\")
 int main() { return 0; }
 " LIBCXX_HAS_COMMENT_LIB_PRAGMA)
   cmake_pop_check_state()
 endif()
 
-if(NOT WIN32 OR MINGW)
-  include(CheckLibcxxAtomic)
-endif()
-
 # Check libraries
 if(WIN32 AND NOT MINGW)
   # TODO(compnerd) do we want to support an emulation layer that allows for the
   # use of pthread-win32 or similar libraries to emulate pthreads on Windows?
   set(LIBCXX_HAS_PTHREAD_LIB NO)
   set(LIBCXX_HAS_M_LIB NO)
   set(LIBCXX_HAS_RT_LIB NO)
   set(LIBCXX_HAS_SYSTEM_LIB NO)
+  set(LIBCXX_HAS_ATOMIC_LIB NO)
 elseif(APPLE)
   check_library_exists(System write "" LIBCXX_HAS_SYSTEM_LIB)
   set(LIBCXX_HAS_PTHREAD_LIB NO)
   set(LIBCXX_HAS_M_LIB NO)
   set(LIBCXX_HAS_RT_LIB NO)
+  set(LIBCXX_HAS_ATOMIC_LIB NO)
 elseif(FUCHSIA)
   set(LIBCXX_HAS_M_LIB NO)
   set(LIBCXX_HAS_PTHREAD_LIB NO)
   set(LIBCXX_HAS_RT_LIB NO)
   set(LIBCXX_HAS_SYSTEM_LIB NO)
+  check_library_exists(atomic __atomic_fetch_add_8 "" LIBCXX_HAS_ATOMIC_LIB)
 else()
   check_library_exists(pthread pthread_create "" LIBCXX_HAS_PTHREAD_LIB)
   check_library_exists(m ccos "" LIBCXX_HAS_M_LIB)
   check_library_exists(rt clock_gettime "" LIBCXX_HAS_RT_LIB)
   set(LIBCXX_HAS_SYSTEM_LIB NO)
+  check_library_exists(atomic __atomic_fetch_add_8 "" LIBCXX_HAS_ATOMIC_LIB)
 endif()

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

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // UNSUPPORTED: libcpp-has-no-threads, c++03
-// REQUIRES: libatomic
-// FILE_DEPENDENCIES: %t.exe
-// RUN: %{build} -latomic
-// RUN: %{run}
-//
+// REQUIRES: non-lockfree-atomics
+
 // 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

libcxx/test/libcxx/selftest/dsl/dsl.sh.py

class TestHasCompileFlag(SetupConfigs):
 
     def test_nonexistent_flag(self):
         self.assertFalse(dsl.hasCompileFlag(self.config, '-this_is_not_a_flag_any_compiler_has'))
 
     def test_multiple_flags(self):
         self.assertTrue(dsl.hasCompileFlag(self.config, '-O1 -Dhello'))
 
 
+class TestSourceBuilds(SetupConfigs):
+    """
+    Tests for libcxx.test.dsl.sourceBuilds
+    """
+    def test_valid_program_builds(self):
+        source = """int main(int, char**) { }"""
+        self.assertTrue(dsl.sourceBuilds(self.config, source))
+
+    def test_compilation_error_fails(self):
+        source = """in main(int, char**) { }"""
+        self.assertFalse(dsl.sourceBuilds(self.config, source))
+
+    def test_link_error_fails(self):
+        source = """extern void this_isnt_defined_anywhere();
+                    int main(int, char**) { this_isnt_defined_anywhere(); }"""
+        self.assertFalse(dsl.sourceBuilds(self.config, source))
+
+
 class TestHasLocale(SetupConfigs):
     """
     Tests for libcxx.test.dsl.hasLocale
     """
     def test_doesnt_explode(self):
         # It's really hard to test that a system has a given locale, so at least
         # make sure we don't explode when we try to check it.
         try:

libcxx/test/lit.site.cfg.in

 config.target_info              = "@LIBCXX_TARGET_INFO@"
 config.test_linker_flags        = "@LIBCXX_TEST_LINKER_FLAGS@"
 config.test_compiler_flags      = "@LIBCXX_TEST_COMPILER_FLAGS@"
 
 config.executor                 = "@LIBCXX_EXECUTOR@"
 config.llvm_unwinder            = @LIBCXXABI_USE_LLVM_UNWINDER@
 config.builtins_library         = "@LIBCXX_BUILTINS_LIBRARY@"
 config.has_libatomic            = @LIBCXX_HAS_ATOMIC_LIB@
-config.use_libatomic            = @LIBCXX_HAVE_CXX_ATOMICS_WITH_LIB@
 config.debug_build              = @LIBCXX_DEBUG_BUILD@
 config.libcxxabi_shared         = @LIBCXX_LINK_TESTS_WITH_SHARED_LIBCXXABI@
 config.cxx_ext_threads          = @LIBCXX_BUILD_EXTERNAL_THREAD_LIBRARY@
 config.pstl_src_root            = "@ParallelSTL_SOURCE_DIR@" if @LIBCXX_ENABLE_PARALLEL_ALGORITHMS@ else None
 config.pstl_obj_root            = "@ParallelSTL_BINARY_DIR@" if @LIBCXX_ENABLE_PARALLEL_ALGORITHMS@ else None
 config.libcxx_gdb               = "@LIBCXX_GDB@"
 
 # Code signing
 config.llvm_codesign_identity   = "@LLVM_CODESIGNING_IDENTITY@"
 
 # Let the main config do the real work.
 config.loaded_site_config = True
 lit_config.load_config(config, "@LIBCXX_SOURCE_DIR@/test/lit.cfg")

libcxx/utils/libcxx/test/config.py

def configure_features(self):
                 "inferred long_tests as: %r" % self.long_tests)
 
         if self.long_tests:
             self.config.available_features.add('long_tests')
 
         if not self.get_lit_bool('enable_filesystem', default=True):
             self.config.available_features.add('c++filesystem-disabled')
 
-        if self.get_lit_bool('has_libatomic', False):
-            self.config.available_features.add('libatomic')
-
         if self.target_info.is_windows():
             self.config.available_features.add('windows')
             if self.cxx_stdlib_under_test == 'libc++':
                 # LIBCXX-WINDOWS-FIXME is the feature name used to XFAIL the
                 # initial Windows failures until they can be properly diagnosed
                 # and fixed. This allows easier detection of new test failures
                 # and regressions. Note: New failures should not be suppressed
                 # using this feature. (Also see llvm.org/PR32730)

libcxx/utils/libcxx/test/dsl.py

def _makeConfigTest(config):
   tmp = tempfile.NamedTemporaryFile(dir=sourceRoot, delete=False)
   tmp.close()
   pathInSuite = [os.path.relpath(tmp.name, sourceRoot)]
   class TestWrapper(lit.Test.Test):
     def __enter__(self):       return self
     def __exit__(self, *args): os.remove(tmp.name)
   return TestWrapper(suite, pathInSuite, config)
 
+def sourceBuilds(config, source):
+  """
+  Return whether the program in the given string builds successfully.
+
+  This is done by compiling and linking a program that consists of the given
+  source with the %{cxx} substitution, and seeing whether that succeeds.
+  """
+  with _makeConfigTest(config) as test:
+    with open(test.getSourcePath(), 'w') as sourceFile:
+      sourceFile.write(source)
+    commands = [
+      "mkdir -p %T",
+      "%{cxx} -xc++ %s %{flags} %{compile_flags} %{link_flags} -o %t.exe"
+    ]
+    commands = libcxx.test.newformat.parseScript(test, preamble=commands, fileDependencies=['%t.exe'])
+    out, err, exitCode, timeoutInfo = _executeScriptInternal(test, commands)
+    cleanup = libcxx.test.newformat.parseScript(test, preamble=['rm %t.exe'], fileDependencies=[])
+    _executeScriptInternal(test, cleanup)
+    return exitCode == 0
+
 def hasCompileFlag(config, flag):
   """
   Return whether the compiler in the configuration supports a given compiler flag.
 
   This is done by executing the %{cxx} substitution with the given flag and
   checking whether that succeeds.
   """
   with _makeConfigTest(config) as test:

libcxx/utils/libcxx/test/features.py

features = [
   Feature(name='libcpp-no-structured-bindings', when=lambda cfg: '__cpp_structured_bindings' not in featureTestMacros(cfg)),
   Feature(name='libcpp-no-deduction-guides',    when=lambda cfg: featureTestMacros(cfg).get('__cpp_deduction_guides', 0) < 201611),
   Feature(name='libcpp-no-concepts',            when=lambda cfg: featureTestMacros(cfg).get('__cpp_concepts', 0) < 201811),
   Feature(name='has-fobjc-arc',                 when=lambda cfg: hasCompileFlag(cfg, '-xobjective-c++ -fobjc-arc') and
                                                                  sys.platform.lower().strip() == 'darwin'), # TODO: this doesn't handle cross-compiling to Apple platforms.
   Feature(name='objective-c++',                 when=lambda cfg: hasCompileFlag(cfg, '-xobjective-c++ -fobjc-arc')),
   Feature(name='diagnose-if-support',           when=lambda cfg: hasCompileFlag(cfg, '-Wuser-defined-warnings'), compileFlag='-Wuser-defined-warnings'),
   Feature(name='modules-support',               when=lambda cfg: hasCompileFlag(cfg, '-fmodules')),
+  Feature(name='non-lockfree-atomics',          when=lambda cfg: sourceBuilds(cfg, """
+                                                                  #include <atomic>
+                                                                  struct Large { int storage[100]; };
+                                                                  std::atomic<Large> x;
+                                                                  int main(int, char**) { return x.load(), x.is_lock_free(); }
+                                                                """)),
 
   Feature(name='apple-clang',                                                                                                      when=_isAppleClang),
   Feature(name=lambda cfg: 'apple-clang-{__clang_major__}'.format(**compilerMacros(cfg)),                                          when=_isAppleClang),
   Feature(name=lambda cfg: 'apple-clang-{__clang_major__}.{__clang_minor__}'.format(**compilerMacros(cfg)),                        when=_isAppleClang),
   Feature(name=lambda cfg: 'apple-clang-{__clang_major__}.{__clang_minor__}.{__clang_patchlevel__}'.format(**compilerMacros(cfg)), when=_isAppleClang),
 
   Feature(name='clang',                                                                                                            when=_isClang),
   Feature(name=lambda cfg: 'clang-{__clang_major__}'.format(**compilerMacros(cfg)),                                                when=_isClang),

libcxx/utils/libcxx/test/target_info.py

def add_cxx_link_flags(self, flags):
             flags += ['-lunwind', '-ldl']
         else:
             flags += ['-lgcc_s']
         builtins_lib = self.full_config.get_lit_conf('builtins_library')
         if builtins_lib:
             flags += [builtins_lib]
         else:
             flags += ['-lgcc']
-        use_libatomic = self.full_config.get_lit_bool('use_libatomic', False)
-        if use_libatomic:
+        has_libatomic = self.full_config.get_lit_bool('has_libatomic', False)
+        if has_libatomic:
             flags += ['-latomic']
         san = self.full_config.get_lit_conf('use_sanitizer', '').strip()
         if san:
             # The libraries and their order are taken from the
             # linkSanitizerRuntimeDeps function in
             # clang/lib/Driver/Tools.cpp
             flags += ['-lpthread', '-lrt', '-lm', '-ldl']