[libc++] Use C11 for atomics check

[smeenai]

Summary

The purpose of the check is to see if atomics are present and if they
require an explicit -latomic to link. We can achieve this equally well
by testing for C11 atomics.

The main motivation is to ease bootstrapping slightly, by cutting down
on the number of configuration steps which require an existing C++
library. It also eases cross compilation against non-standard C++
libraries, which would otherwise require passing a -nostdlib to the
driver and adjusting CMAKE_REQUIRED_LIBRARIES accordingly.

[bcraig]

I like the rationale here, but can we avoid pulling in headers at all?
You could test _ _STDC_NO_ATOMICS_ _. You could also have some code like this...

_Atomic int x;
int main() {
  x += 1;
  return x;
}

[rsmith]

Are we really guaranteed that the C and C++ compiler behave the same way here? I don't see why that would necessarily be the case.

[smeenai]

@bcraig: __STDC_NO_ATOMICS__ wouldn't be defined for pre-C11 compilers either, right?

From what I understand of the original code sample, one of the purposes was to check for 64-bit atomic support on 32-bit systems (hence the use of uintmax_t and the check for __atomic_fetch_add_8). I don't really know of a portable way of accomplishing that without at least including <stdint.h>.

[bcraig]

In D23719#520952, @rsmith wrote:

Are we really guaranteed that the C and C++ compiler behave the same way here? I don't see why that would necessarily be the case.

For libc++, std::atomic is implemented in terms of _Atomic. So as long as the C++ compiler doesn't butcher _Atomic, it seems that the behavior would be the same.

[bcraig]

In D23719#520953, @smeenai wrote:

@bcraig: __STDC_NO_ATOMICS__ wouldn't be defined for pre-C11 compilers either, right?

From what I understand of the original code sample, one of the purposes was to check for 64-bit atomic support on 32-bit systems (hence the use of uintmax_t and the check for __atomic_fetch_add_8). I don't really know of a portable way of accomplishing that without at least including <stdint.h>.

Given the choice of including stdatomic.h or stdint.h, I'll gladly take stdint.h. libc++ shouldn't require a C11 enabled C library. C++14 only requires C99.

[rsmith]

In D23719#520954, @bcraig wrote:

In D23719#520952, @rsmith wrote:

Are we really guaranteed that the C and C++ compiler behave the same way here? I don't see why that would necessarily be the case.

For libc++, std::atomic is implemented in terms of _Atomic. So as long as the C++ compiler doesn't butcher _Atomic, it seems that the behavior would be the same.

I don't see any good reason to assume that's the case. GCC, for instance, does not define _Atomic *at all* in C++ mode; the implementation used by libc++ in that case is completely different. Also, as far as I know, libc++ did not previously require the host to have a C11 compiler. And there's no reason to assume that the C compiler picked up by cmake is in any way related to the C++ compiler.

Bottom line: if you want to know how the C++ compiler behaves, you need to test the C++ compiler not the C compiler.

[bcraig]

In D23719#520969, @rsmith wrote:

In D23719#520954, @bcraig wrote:

In D23719#520952, @rsmith wrote:

Are we really guaranteed that the C and C++ compiler behave the same way here? I don't see why that would necessarily be the case.

For libc++, std::atomic is implemented in terms of _Atomic. So as long as the C++ compiler doesn't butcher _Atomic, it seems that the behavior would be the same.

I don't see any good reason to assume that's the case. GCC, for instance, does not define _Atomic *at all* in C++ mode; the implementation used by libc++ in that case is completely different. Also, as far as I know, libc++ did not previously require the host to have a C11 compiler. And there's no reason to assume that the C compiler picked up by cmake is in any way related to the C++ compiler.

Bottom line: if you want to know how the C++ compiler behaves, you need to test the C++ compiler not the C compiler.

I managed to completely miss "#define _Atomic(x) gcc_atomic::gcc_atomic_t<x>" in the atomic header.

On a different note, the general class of checks that LLVM and LIBCXX have in place to detect atomics generally don't work for cross-compilation. The set(CMAKE_REQUIRED_FLAGS ...) calls overwrite important settings. I end up setting HAVE_CXX_ATOMICS_WITHOUT_LIB=ON, HAVE_CXX_ATOMICS64_WITHOUT_LIB=ON, and LIBCXX_HAVE_CXX_ATOMICS_WITHOUT_LIB=ON explicitly, because the automatic check just doesn't work. So I look forward to the day when updates to these checks stop breaking my build. Given Richard's comments, I don't think this check is the one.

[rsmith]

I have to admit to not understanding the motivation for this change. It claims that this is removing a reliance on having an existing C++ standard library, but it doesn't appear to affect that: this test explicitly passes -nostdinc++ to avoid using a system standard library, and instead adds the (currently-being-configured) libc++ headers to the include path.

If that approach is a problem somehow, how about this alternative: instead of indirectly trying to get the compiler to emit a use of a function provided by libatomic, we create a (C++) compilation test that declares a function from libatomic and calls it. That should tell us whether the C++ compiler implicitly links in libatomic or a suitable alternative.

[smeenai]

@rsmith: I think your points are valid, and I should have been more explicit with the motivation :)

The issue is with linking, not with compiling. When we call check_cxx_source_compiles, the compiler driver passes all the standard C++ libraries for the platform to the link step, which can lead to spurious link failures when cross-compiling during the configure step, which then manifest as a bad configuration.

When we're compiling the library itself, we [pass -nodefaultlibs to the driver](https://reviews.llvm.org/diffusion/L/browse/libcxx/trunk/lib/CMakeLists.txt;279330$76) to avoid the standard libraries from creeping in, and then explicitly add any required libraries. Conceptually, I want to do the same for any check_cxx_source_compiles calls, so that the configuration linking behavior matches the library linking behavior. I'm thinking the best way to accomplish this is to globally add -nodefaultlib to CMAKE_REQUIRED_FLAGS, and then globally set CMAKE_REQUIRED_LIBRARIES to the required set of libraries, similar to what the library build is doing. Does that make sense?

[rsmith]

In D23719#521233, @smeenai wrote:

I'm thinking the best way to accomplish this is to globally add -nodefaultlib to CMAKE_REQUIRED_FLAGS, and then globally set CMAKE_REQUIRED_LIBRARIES to the required set of libraries, similar to what the library build is doing. Does that make sense?

I'm not sure that will test the right thing, but if it works then it seems reasonable. My concern is that -nodefaultlib may suppress the addition of -latomic to the link line, and if so, the configure-time check would always fail, even though an actual link using libc++'s <atomic> would work. Is there a -nodefaultlib analogue of -nostdinc++ to turn off just the C++ standard library portion of the link?

[smeenai]

In D23719#521250, @rsmith wrote:

Is there a -nodefaultlib analogue of -nostdinc++ to turn off just the C++ standard library portion of the link?

There's -nostdlib, but if I'm parsing the GCC documentation correctly, it's even more restrictive than -nodefaultlibs, since it omits the startup files as well.

My concern is that -nodefaultlib may suppress the addition of -latomic to the link line, and if so, the configure-time check would always fail, even though an actual link using libc++'s <atomic> would work.

Hmm. I understand what you mean, but this configure-time check is for compiling libc++ itself, and if a driver adds -latomic automatically, I'd argue this check is actually incorrect as it stands right now:

• We run check_cxx_source_compiles to check if -latomic is required
• This links without -nodefaultlibs, and the driver adds the -latomic in that case, so we report that we don't need the flag
• We try compiling libc+, which passes -nodefaultlibs, and now the driver doesn't give us -latomic, and our configuration said we didn't need the flag explicitly, so compilation fails

If the driver doesn't add -latomic automatically, the above wouldn't be a problem, but then my proposed change wouldn't be a problem either.

[smeenai]

Will create a different change with the other config change, since it's completely different conceptually.

cmake/Modules/CheckLibcxxAtomic.cmake

-INCLUDE(CheckCXXSourceCompiles)
+INCLUDE(CheckCSourceCompiles)
 
 # 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 "-std=c++11 -nostdinc++ -isystem ${LIBCXX_SOURCE_DIR}/include")
+  set(CMAKE_REQUIRED_FLAGS "-std=c11")
   if (${LIBCXX_GCC_TOOLCHAIN})
     set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} --gcc-toolchain=${LIBCXX_GCC_TOOLCHAIN}")
   endif()
-  check_cxx_source_compiles("
-#include <cstdint>
-#include <atomic>
-std::atomic<uintptr_t> x;
-std::atomic<uintmax_t> y;
+  check_c_source_compiles("
+#include <stdatomic.h>
+atomic_uintptr_t x;
+atomic_uintmax_t y;
 int main() {
-  return x + y;
+  return atomic_load(&x) + atomic_load(&y);
 }
 " ${varname})
   set(CMAKE_REQUIRED_FLAGS ${OLD_CMAKE_REQUIRED_FLAGS})
 endfunction(check_cxx_atomics)
 
 check_cxx_atomics(LIBCXX_HAVE_CXX_ATOMICS_WITHOUT_LIB)
 check_library_exists(atomic __atomic_fetch_add_8 "" LIBCXX_HAS_ATOMIC_LIB)
 # If not, check if the library exists, and atomics work with it.