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.

lib/Headers/stdatomic.h

 
 /* If we're hosted, fall back to the system's stdatomic.h. FreeBSD, for
  * example, already has a Clang-compatible stdatomic.h header.
  */
 #if __STDC_HOSTED__ && __has_include_next(<stdatomic.h>)
 # include_next <stdatomic.h>
 #else
 
+#if !defined(__ALLOW_STDC_ATOMICS_IN_CXX__) && defined(__cplusplus)
+#error "<stdatomic.h> is incompatible with the C++ standard library; define __ALLOW_STDC_ATOMICS_IN_CXX__ to proceed."
+#endif
+
 #include <stddef.h>
 #include <stdint.h>
 
 #ifdef __cplusplus
 extern "C" {
 #endif
 
 /* 7.17.1 Introduction */

test/Headers/stdatomic.cpp

+// RUN: %clang_cc1 %s -verify
+// RUN: %clang_cc1 -D__ALLOW_STDC_ATOMICS_IN_CXX__ %s -verify
+
+#include <stdatomic.h>
+
+#ifndef __ALLOW_STDC_ATOMICS_IN_CXX__
+// expected-error@stdatomic.h:* {{<stdatomic.h> is incompatible with the C++ standard library}}
+#else
+// expected-no-diagnostics
+#endif