Page MenuHomePhabricator

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

Authored by smeenai on Aug 8 2022, 1:32 PM.

Details

Reviewers
philnik
ldionne
Group Reviewers
Restricted Project
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.

Diff Detail

Event Timeline

smeenai created this revision.Aug 8 2022, 1:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2022, 1:33 PM
smeenai requested review of this revision.Aug 8 2022, 1:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2022, 1:33 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik requested changes to this revision.Aug 9 2022, 4:04 AM
philnik added a subscriber: 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.

This revision now requires changes to proceed.Aug 9 2022, 4:04 AM

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.

smeenai requested review of this revision.Aug 9 2022, 5:17 PM

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?

@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.

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.

@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 requested changes to this revision.Aug 10 2022, 12:17 PM

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.

This revision now requires changes to proceed.Aug 10 2022, 12:17 PM

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 requested review of this revision.Aug 16 2022, 10:36 AM

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

ldionne requested changes to this revision.EditedAug 17 2022, 12:44 PM

[...]
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.

This revision now requires changes to proceed.Aug 17 2022, 12:44 PM

@smeenai Can we close this in favour of D134591?

Thanks for bringing this to our attention!

smeenai abandoned this revision.Oct 3 2022, 2:00 PM
smeenai added a subscriber: ngg.

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!