Emit an error when include <atomic> after <stdatomic.h>

[vsapsai]

Summary

C11 defines kill_dependency as a macro in <stdatomic.h>. When you include
<atomic> after <stdatomic.h>, the macro clashes with std::kill_dependency and
causes multiple errors. Explicit error should help in diagnosing those errors.

No change for working code that includes <atomic> before <stdatomic.h>.

rdar://problem/27435938

[vsapsai]

In addition to the currently proposed approach

#if ...
#error ...
#endif
// header content

I also tried

#if ...
#error ...
#else
// header content
#endif

It allows to have fewer error messages but can be more surprising. It can look like the only problem is a missing macro but after defining it you can discover that the situations is more complex. Also in some cases omitting a header content can lead to other errors which can be confusing too.

[vsapsai]

Ping.

[mclow.lists]

clang/test/Headers/stdatomic.cpp

4 ↗

(On Diff #141775)

Is there a reason we want to test this twice - once in clang and once in libc++?
We can use expected-error in libc++ tests to check the error.

[vsapsai]

clang/test/Headers/stdatomic.cpp

4 ↗

(On Diff #141775)

The idea was to test <stdatomic.h> in clang and <atomic> in libc++. Does that answer your question? I'm not sure I understood it correctly.

Good suggestion to use expected-error in libc++ test, will add that.

[vsapsai]

• Tighten up a test with expected-error per review comment.

[vsapsai]

Thanks for the review, Marshall.

In the end I removed XFAIL: with_system_cxx_lib from libc++ test as the change is header-only at compile time. Library behaviour at runtime shouldn't be affected.

[jfb]

This isn't bad, so I'd go with it, but separately I imagine that we could implement the suggestion in http://wg21.link/p0943 and expose it even before C++20? Not sure we do this much, but I'd argue that before that fix stdatomic.h is just useless anyways, so it's a fine breakage. I imagine that the stdatomic.h where it's implemented would be the one injected by clang, not the libc++ one?

[vsapsai]

In D45470#1087026, @jfb wrote:

This isn't bad, so I'd go with it, but separately I imagine that we could implement the suggestion in http://wg21.link/p0943 and expose it even before C++20? Not sure we do this much, but I'd argue that before that fix stdatomic.h is just useless anyways, so it's a fine breakage. I imagine that the stdatomic.h where it's implemented would be the one injected by clang, not the libc++ one?

Change visible here is for the header injected by clang. libc++ change is r331379 but that error triggers only when you consciously decided to opt in C atomics but included C++ <atomic>.

I am concerned that the change adds the error to the valid code that uses C atomics from C++ and doesn't mix them with C++ atomics. It is unfortunate but I think it is a right trade-off. Adding __ALLOW_STDC_ATOMICS_IN_CXX__ to fix such code shouldn't take much time. While having an explicit error can save a lot of time trying to figure out a broken build. Also I expect that with time libc++ will use <atomic> more and mixing currently safe parts of libc++ with <stdatomic.h> is likely to cause problems at some point. So it is better to be prepared and have an explicit error for this case.

[vsapsai]

__ALLOW_STDC_ATOMICS_IN_CXX__ approach didn't work in practice, I've reverted all changes.

[vsapsai]

Here is another approach that should emit an error only when mixing headers
causes compilation problems.

Have no ideas how to test the change. -verify doesn't work with fatal errors
and libcxx doesn't use FileCheck. Performed only manual testing.

[jfb]

In D45470#1092212, @vsapsai wrote:

Here is another approach that should emit an error only when mixing headers
causes compilation problems.

Have no ideas how to test the change. -verify doesn't work with fatal errors
and libcxx doesn't use FileCheck. Performed only manual testing.

This worked with <atomic> before <stdatomic.h> as well as with the order reversed?

LGTM pending testing answer. This seems simpler than implementing http://wg21.link/p0943 outright, though that would still be the best solution IMO.

libcxx/include/atomic

558 ↗

(On Diff #145817)

I checked and C guarantees that this is a macro :)

[vsapsai]

In D45470#1092260, @jfb wrote:

In D45470#1092212, @vsapsai wrote:

Here is another approach that should emit an error only when mixing headers
causes compilation problems.

Have no ideas how to test the change. -verify doesn't work with fatal errors
and libcxx doesn't use FileCheck. Performed only manual testing.

This worked with <atomic> before <stdatomic.h> as well as with the order reversed?

#include <stdatomic.h>
#include <atomic>

fatal error: too many errors emitted, stopping now [-ferror-limit=]

#include <atomic>
#include <stdatomic.h>

no errors

So when <atomic> is after <stdatomic.h>, I add one more error to existing. When <atomic> is before <stdatomic.h>, there are no errors and I don't add anything.

[vsapsai]

Thanks for review.

libcxx/trunk/include/atomic

 #endif
 
 #ifdef _LIBCPP_HAS_NO_THREADS
 #error <atomic> is not supported on this single threaded system
 #endif
 #if !defined(_LIBCPP_HAS_C_ATOMIC_IMP) && !defined(_LIBCPP_HAS_GCC_ATOMIC_IMP)
 #error <atomic> is not implemented
 #endif
+#ifdef kill_dependency
+#error C++ standard library is incompatible with <stdatomic.h>
+#endif
 
 #if _LIBCPP_STD_VER > 14
 # define __cpp_lib_atomic_is_always_lock_free 201603L
 #endif
 
 #define _LIBCPP_CHECK_STORE_MEMORY_ORDER(__m) \
   _LIBCPP_DIAGNOSE_WARNING(__m == memory_order_consume || \
                            __m == memory_order_acquire || \