[libcxx] Make stdatomic.h work when included from a C source file

[smeenai]

Summary

If a C source file includes the libc++ stdatomic.h, compilation will
break because (a) the C++ standard check will fail (which is expected),
and (b) _LIBCPP_COMPILER_CLANG_BASED won't be defined because the
logic defining it in __config is guarded by a __cplusplus check, so
we'll end up with a blank header. Check for __clang__ directly, which
is equivalent, to make the second check pass even in a C context when
you're using Clang.

In an ideal world, a C source file wouldn't be including the libc++
header directory in its search path, so we'd never have this issue.
Unfortunately, certain build environments make this hard to guarantee,
and in this case it's easy to tweak this header to make it work in a C
context, so I'm hoping this is acceptable.

[philnik]

This won't change anything. All the code is guarded by _LIBCPP_STD_VER > 20. _LIBCPP_STD_VER also isn't defined when compiled for C, so it's #if 0 > 20, which is obviously not the case.

[smeenai]

In D131435#3709364, @philnik wrote:

This won't change anything. All the code is guarded by _LIBCPP_STD_VER > 20. _LIBCPP_STD_VER also isn't defined when compiled for C, so it's #if 0 > 20, which is obviously not the case.

The #elif that I'm changing is for that #if, so we'll fall through to it and include the Clang resource directory's stdatomic.h after my change. I verified that this did fix my actual issue, but I also came up with an alternative solution, so I don't need this patch anymore. I'm happy to abandon if we'd rather not take this change.

[philnik]

In D131435#3711269, @smeenai wrote:

In D131435#3709364, @philnik wrote:

This won't change anything. All the code is guarded by _LIBCPP_STD_VER > 20. _LIBCPP_STD_VER also isn't defined when compiled for C, so it's #if 0 > 20, which is obviously not the case.

The #elif that I'm changing is for that #if, so we'll fall through to it and include the Clang resource directory's stdatomic.h after my change. I verified that this did fix my actual issue, but I also came up with an alternative solution, so I don't need this patch anymore. I'm happy to abandon if we'd rather not take this change.

Ah, I missed that. It's really subtle. Couldn't we just make this an #else and that would do the same thing? Or is there a reason we don't want to #include_next <stdatomic.h> when using GCC?

[Mordante]

@smeenai is this intended to be backported to the LLVM 15 branch?

SGTM but I think it would be good when @ldionne has a look too.

[smeenai]

In D131435#3712212, @philnik wrote:

In D131435#3711269, @smeenai wrote:

In D131435#3709364, @philnik wrote:

This won't change anything. All the code is guarded by _LIBCPP_STD_VER > 20. _LIBCPP_STD_VER also isn't defined when compiled for C, so it's #if 0 > 20, which is obviously not the case.

The #elif that I'm changing is for that #if, so we'll fall through to it and include the Clang resource directory's stdatomic.h after my change. I verified that this did fix my actual issue, but I also came up with an alternative solution, so I don't need this patch anymore. I'm happy to abandon if we'd rather not take this change.

Ah, I missed that. It's really subtle. Couldn't we just make this an #else and that would do the same thing? Or is there a reason we don't want to #include_next <stdatomic.h> when using GCC?

Not sure about that; I was just leaving the existing behavior be, since the comment specifically mentions Clang.

In D131435#3712985, @Mordante wrote:

@smeenai is this intended to be backported to the LLVM 15 branch?

SGTM but I think it would be good when @ldionne has a look too.

Yup, I was planning to backport if this is accepted.

[ldionne]

In an ideal world, a C source file wouldn't be including the libc++ header directory in its search path, so we'd never have this issue.

This.

Unfortunately, certain build environments make this hard to guarantee, and in this case it's easy to tweak this header to make it work in a C context, so I'm hoping this is acceptable.

I also agree that we could try to accommodate these setups, but I'm not super eager to do that because I'm sure there are various other ways in which libc++ can break when used from C.

I think a necessary input to make our mind here would be to better understand the case in which you hit this problem, and how you fixed it.

Finally, an alternative approach to this might be to try to handle C inside __config. We'd have to do something about the way that we define _LIBCPP_STD_VER and we'd probably want to have a special macro like _LIBCPP_C_LANGUAGE or whatever, but it might be workable if we had tests for it.

I'm not closed to this patch, but I'll request changes just so we can have this discussion.

[smeenai]

In D131435#3713835, @ldionne wrote:

In an ideal world, a C source file wouldn't be including the libc++ header directory in its search path, so we'd never have this issue.

This.

Unfortunately, certain build environments make this hard to guarantee, and in this case it's easy to tweak this header to make it work in a C context, so I'm hoping this is acceptable.

I also agree that we could try to accommodate these setups, but I'm not super eager to do that because I'm sure there are various other ways in which libc++ can break when used from C.

I think a necessary input to make our mind here would be to better understand the case in which you hit this problem, and how you fixed it.

Full details follow, but the TL;DR is that our current build system setup results in C libraries getting the libc++ header dependency if they depend on any C++ libraries, and while I do plan to fix that, it's non-trivial and will take some time. I worked around this issue on our end by adding a wrapper stdatomic.h header (in a directory that's guaranteed to be on the include path before the libc++ header directory, so it takes precedence), defining _LIBCPP_COMPILER_CLANG_BASED in there, and then doing an #include_next <stdatomic.h> to get a working stdatomic.h in C mode.

On Android, libc++ isn't a system component (there is a system libc++ but apps aren't supposed to use it); instead, each app is supposed to bundle its own libc++. At Meta, we build our Android apps with Buck, and we also build libc++ (and libc++abi and libunwind, which sometimes exposes us to exciting issues like https://github.com/llvm/llvm-project/issues/56825) with Buck as part of the app build, so that it gets optimized alongside the app (in particular we're able to strip out the parts of the library that aren't used by the app, which is a huge size saving). Other libraries depend on the libc++ target the way they'd depend on any other target, and the libc++ target specifies public_include_directories to expose its headers to the targets that depend on it. (Because we control the full build graph, we're also just able to use exported_preprocessor_flags instead of setting up __config_site, and ensuring that was set up properly was the motivation for my question in https://reviews.llvm.org/D121478#inline-1262415)

The issue stems from another Buck setting called reexport_all_header_dependencies. When it's set to True (which is the default), header dependencies become automatically transitive, i.e. if libA depends on libB (but does not depend on libc++ directly), and libB depends on libc++, then libA automatically includes libc++'s headers. This is the root cause of our issue: we have a C library depending on some C++ libraries, and even though the C library isn't depending on libc++ directly, it's still pulling in the libc++ headers through its transitive dependencies. I want to flip this setting to False to avoid this behavior and the resulting issue, but as you can imagine we have a ton of targets that have inadvertently been depending on the auto-transitive inclusion behavior, and it'll take a while to untangle all of that. In the meantime, adding the wrapper stdatomic.h header works around this well enough. (I could also just modify stdatomic.h directly on our end, of course, but we try to only introduce local patches when absolutely necessary.)

Finally, an alternative approach to this might be to try to handle C inside __config. We'd have to do something about the way that we define _LIBCPP_STD_VER and we'd probably want to have a special macro like _LIBCPP_C_LANGUAGE or whatever, but it might be workable if we had tests for it.

I'm not closed to this patch, but I'll request changes just so we can have this discussion.

Like I said above, I have a local fix for this issue, and that works for us, so I think this comes down to whether it's purely an us problem (in which case keeping the solution on our end is likely also appropriate), or whether others are also affected and would find this useful.

[smeenai]

Requesting review so y'all can decide if we want to take something like this or if I should just abandon it :)

[ldionne]

In D131435#3713983, @smeenai wrote:

[...]
and ensuring that was set up properly was the motivation for my question in https://reviews.llvm.org/D121478#inline-1262415)

Sorry, I never noticed that comment. I just replied.

I'm not closed to this patch, but I'll request changes just so we can have this discussion.

Like I said above, I have a local fix for this issue, and that works for us, so I think this comes down to whether it's purely an us problem (in which case keeping the solution on our end is likely also appropriate), or whether others are also affected and would find this useful.

Thanks a lot for the whole writeup, it is interesting to understand the chain of events that brought you to this.

I am neutral (or slightly against) libc++ headers being includable when compiled as C. On the one hand, it does add flexibility, however it also encourages having poor build system hygiene. I think we're on the same page about this. In fact, I think ideally we would #error out when libc++ headers are used outside of C++ to fail fast and obviously, in the same spirit as D131441.

Another problem with libc++ headers being includable as C is that we'd have to actually test it and support it, and I suspect there's a bunch of stuff that doesn't work as intended in that configuration -- we're just not really aware of those problems.

That being said, above my preference, we do seem to have precedent for supporting this, see libcxx/test/libcxx/include_as_c.sh.cpp. It seems like I basically forgot to update that test when I added <stdatomic.h>.

I would hence suggest that we:

1. Update include_as_c.sh.cpp
2. Move the compiler detection block in __config to outside of #if __cplusplus
3. Not change <stdatomic.h>

And then, if we do want to pursue cleaning up usage of libc++ headers from C language, we can tackle that as a separate effort. But I see this patch as being consistent with the current status-quo.

[ldionne]

@smeenai Can we close this in favour of D134591?

Thanks for bringing this to our attention!

[smeenai]

Sorry for not getting back to this; I had to take a lengthy unplanned leave (which I just got back from). Thanks @ngg for getting it through!

libcxx/include/stdatomic.h

 using std::atomic_load _LIBCPP_USING_IF_EXISTS;
 using std::atomic_load_explicit _LIBCPP_USING_IF_EXISTS;
 using std::atomic_store _LIBCPP_USING_IF_EXISTS;
 using std::atomic_store_explicit _LIBCPP_USING_IF_EXISTS;
 
 using std::atomic_signal_fence _LIBCPP_USING_IF_EXISTS;
 using std::atomic_thread_fence _LIBCPP_USING_IF_EXISTS;
 
-#elif defined(_LIBCPP_COMPILER_CLANG_BASED)
+#elif defined(__clang__)
 
 // Before C++23, we include the next <stdatomic.h> on the path to avoid hijacking
 // the header. We do this because Clang has historically shipped a <stdatomic.h>
 // header that would be available in all Standard modes, and we don't want to
 // break that use case.
 # if __has_include_next(<stdatomic.h>)
 #   include_next <stdatomic.h>
 # endif
 
 #endif // _LIBCPP_STD_VER > 20
 
 #endif // _LIBCPP_STDATOMIC_H