[libcxx-commits] [PATCH] D131435: [libcxx] Make stdatomic.h work when included from a C source file

[Shoaib Meenai via Phabricator via libcxx-commits]

smeenai added a comment.

In D131435#3713835 <https://reviews.llvm.org/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++ <https://developer.android.com/ndk/guides/cpp-support#cs>. At Meta, we build our Android apps with Buck <https://buck.build/>, 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 <https://buck.build/rule/cxx_library.html#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 <https://buck.build/rule/cxx_library.html#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 <https://buck.build/rule/cxx_library.html#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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131435/new/

https://reviews.llvm.org/D131435