This is an archive of the discontinued LLVM Phabricator instance.

[libc++] [C++2b] [P0943] Add stdatomic.h header.
ClosedPublic

Authored by ldionne on Feb 19 2021, 4:54 AM.

Diff Detail

Event Timeline

curdeius created this revision.Feb 19 2021, 4:54 AM
curdeius requested review of this revision.Feb 19 2021, 4:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 19 2021, 4:54 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
curdeius edited the summary of this revision. (Show Details)Feb 19 2021, 4:55 AM
curdeius edited the summary of this revision. (Show Details)
curdeius updated this revision to Diff 324954.Feb 19 2021, 4:58 AM

Fix C++ includes in C.

Harbormaster completed remote builds in B89909: Diff 324953.
curdeius updated this revision to Diff 325006.Feb 19 2021, 9:07 AM

Guard stdatomic.h version test on libcpp-has-no-threads.

Mordante added inline comments.
libcxx/include/stdatomic.h
3

Copy-paste of wchar.h.

curdeius added inline comments.Feb 19 2021, 9:14 AM
libcxx/include/stdatomic.h
3

Thanks for pointing this. I'll fix it in the next revision.

jfb added a comment.Feb 19 2021, 10:04 AM

Does it line up property with ./clang/lib/Headers/stdatomic.h ? I think this other header might need some adjustment as well, for example the hosted/freestanding part.

I haven't found anything really different from https://github.com/llvm/llvm-project/blob/main/clang/lib/Headers/stdatomic.h.
There are some differences because e.g. ATOMIC_VAR_INIT, atomic_init, kill_dependency aren't mentioned in the paper, but nothing crucial IMO.
Concerning hosted/freestanding, I'm not sure I know what should be verified/done. The clang/lib/Headers/stdatomic.h just includes system's stdatomic.h in hosted env. when available.

What I am more afraid of is rather incompatibilities between C's _Atomic and C++'s atomic.
I haven't tested it extensively, but there were at least some differences in sizeof. And the standard (strongly) recommends to make them compatible (cf. https://eel.is/c++draft/stdatomic.h.syn#4).

libcxx/include/stdatomic.h
134

Should I use ::_VSTD::atomic<T> or should we go for changing _VSTD to ::std? Or is it not worth the trouble?

libcxx/test/std/atomics/stdatomic.h.syn/types.pass.cpp
21 ↗(On Diff #325006)

Anyone has an idea about this? Is this an omission in the standard (given that it includes atomic_char8_t) or it should match C11 and so omit both ATOMIC_CHAR8_T_LOCK_FREE and atomic_char8_t?

curdeius retitled this revision from [libc++] [c++2b] [P0943] Add stdatomic.h header. to [libc++] [C++2b] [P0943] Add stdatomic.h header..Feb 24 2021, 1:49 AM
ldionne requested changes to this revision.Jul 23 2021, 8:41 AM
ldionne added a subscriber: timshen.

Can you rebase onto main and go through https://libcxx.llvm.org/Contributing.html?

libcxx/include/stdatomic.h
134

Use _VSTD until we decide what we do with that, for consistency.

136

Can you use _LIBCPP_USING_IF_EXISTS everywhere?

libcxx/test/std/atomics/stdatomic.h.syn/types.pass.cpp
21 ↗(On Diff #325006)

GCC does define ATOMIC_CHAR8_T_LOCK_FREE. I think this is likely an oversight in the spec.

@timshen Would you agree?

@curdeius I think we should add both ATOMIC_CHAR8_T_LOCK_FREE and atomic_char8_t.

175 ↗(On Diff #325006)

You don't need double-parens here and elsewhere, I think.

This revision now requires changes to proceed.Jul 23 2021, 8:41 AM
hubert.reinterpretcast added inline comments.
libcxx/include/stdatomic.h
125

Please see thread re: unintended consequences of enabling C++ stdatomic.h for applications currently using the C one with Clang:
https://llvm.discourse.group/t/compiling-with-built-in-stdatomic-vs-stdatomic-h/6225

libcxx/include/stdatomic.h
131

undef takes an identifier (and that's it).

ldionne added inline comments.Feb 7 2022, 1:03 PM
libcxx/include/stdatomic.h
125

After reading the thread, I assume what you mean is that we should be #include_nexting the platform <stdatomic.h> if we're in C++ < 23?

ldionne commandeered this revision.Feb 7 2022, 1:08 PM
ldionne edited reviewers, added: curdeius; removed: ldionne.
ldionne marked 3 inline comments as done.Feb 7 2022, 1:29 PM
ldionne added inline comments.
libcxx/test/std/atomics/stdatomic.h.syn/types.pass.cpp
21 ↗(On Diff #325006)

I'm actually going to implement this naively for now and not check for ATOMIC_CHAR8_T_LOCK_FREE. This will end up being the target of a LWG issue anyway, so we can add the check then.

ldionne updated this revision to Diff 406594.Feb 7 2022, 1:31 PM

Rebase onto main, apply comments.

ldionne added inline comments.Feb 7 2022, 1:32 PM
libcxx/include/stdatomic.h
226

@hubert.reinterpretcast This is my attempt to address your comment about https://discourse.llvm.org/t/compiling-with-built-in-stdatomic-vs-stdatomic-h/6225. Please LMK if you think that doesn't work.

libcxx/include/stdatomic.h
125

Yes.

224–232

This is what I meant (since it reflects the status quo where Clang has a stdatomic.h that "works" in C++ mode (except that <atomic> and anything that uses it cannot be used after the stdatomic.h inclusion).

Maybe the message should be updated:

#ifdef kill_dependency
# error C++ standard library is incompatible with <stdatomic.h>
#endif

Aside: The #else comment style above is not my usual preference (I like using the negation), but it seems to be the more common style in libc++ (although there are examples of both styles).

ldionne updated this revision to Diff 406964.Feb 8 2022, 2:02 PM
ldionne marked 2 inline comments as done.

Address review comments and fix CI.

libcxx/include/stdatomic.h
224–232

I added a test for this.

ldionne updated this revision to Diff 407166.Feb 9 2022, 8:12 AM

Fix failures in GCC with older C++ Standards.

@hubert.reinterpretcast I am wondering whether it's a good idea to fall back
to the Clang provided <stdatomic.h> on older C++ standards. It seems to me that
if you're compiling as C++, we should be providing a strictly conforming behavior
and not trying to provide some maybe-not-entirely-working fall-backs in previous
Standard modes. Any thoughts?

@hubert.reinterpretcast I am wondering whether it's a good idea to fall back
to the Clang provided <stdatomic.h> on older C++ standards. It seems to me that
if you're compiling as C++, we should be providing a strictly conforming behavior
and not trying to provide some maybe-not-entirely-working fall-backs in previous
Standard modes. Any thoughts?

Clang seems to have had the not-entirely-working behaviour as a deliberate extension (with compiler-level support for _Atomic).
It appears that users have made use of it with the older C++ modes.

My understanding is that such cases led to a Clang-only fallback in libstdc++.

ldionne updated this revision to Diff 407566.Feb 10 2022, 9:15 AM

Hopefully fix CI issue on GCC.

@ldionne, thanks for taking this over.
I have no time to work on libc++ these days (and months :) ) unfortunately...

@ldionne, thanks for taking this over.
I have no time to work on libc++ these days (and months :) ) unfortunately...

Yeah, no worries at all. Thanks for kicking off the initial patch.

libcxx/include/stdatomic.h
108

Do we want to preemptively apply the proposed resolution to https://cplusplus.github.io/LWG/issue3671 and add the xor operations too?

ldionne added inline comments.Feb 10 2022, 2:03 PM
libcxx/include/stdatomic.h
108

If we're all happy with the patch as-is, I'd like to land it. I can handle the LWG issue separately via our usual means.

If you're happy with the patch as-is, please let me know and I'll land it.

LGTM with one comment.

libcxx/test/std/atomics/stdatomic.h.syn/types.compile.pass.cpp
146–151

It's simple enough to also check the type and lvalueness.

ldionne updated this revision to Diff 408600.Feb 14 2022, 1:38 PM
ldionne marked an inline comment as done.

Address review comments -- I'll ship this.

libcxx/test/std/atomics/stdatomic.h.syn/types.compile.pass.cpp
146–151

I'll use a slight variation on this by comparing the addresses, since that's simpler.

ldionne accepted this revision.Feb 14 2022, 1:39 PM
This revision is now accepted and ready to land.Feb 14 2022, 1:39 PM
This revision was landed with ongoing or failed builds.Feb 14 2022, 1:39 PM
This revision was automatically updated to reflect the committed changes.

This commit broke the CI, it triggers failures in the std/atomics/stdatomic.h.syn/types.compile.pass.cpp test on the GCC 11 / C++-latest configuration.

shafik added a subscriber: shafik.Feb 15 2022, 8:07 AM

Looks like this commit broke the build for LLDB green dragon bots: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/41395/#showFailuresLink

We are seeing 129 test failures with errors like this:

In file included from /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/bin/../include/c++/v1/__memory/shared_ptr.h:34:
/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/bin/../include/c++/v1/atomic:543:3: error: C++ standard library is incompatible with <stdatomic.h> before C++23. Please compile with -std=c++23.
# error C++ standard library is incompatible with <stdatomic.h> before C++23. Please compile with -std=c++23.
  ^
/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/bin/../include/c++/v1/atomic:1075:25: error: expected ')'
_Tp kill_dependency(_Tp __y) _NOEXCEPT
                        ^
/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/bin/../include/c++/v1/atomic:1075:5: note: to match this '('
_Tp kill_dependency(_Tp __y) _NOEXCEPT
    ^
/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/lib/clang/15.0.99/include/stdatomic.h:65:28: note: expanded from macro 'kill_dependency'
#define kill_dependency(y) (y)
                           ^

This commit broke the CI, it triggers failures in the std/atomics/stdatomic.h.syn/types.compile.pass.cpp test on the GCC 11 / C++-latest configuration.

This was fixed by a30a7948d59470fe9403102b192b3e7ddaf21849.

Looks like this commit broke the build for LLDB green dragon bots: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/41395/#showFailuresLink

We are seeing 129 test failures with errors like this:

In file included from /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/bin/../include/c++/v1/__memory/shared_ptr.h:34:
/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/bin/../include/c++/v1/atomic:543:3: error: C++ standard library is incompatible with <stdatomic.h> before C++23. Please compile with -std=c++23.
# error C++ standard library is incompatible with <stdatomic.h> before C++23. Please compile with -std=c++23.
  ^
/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/bin/../include/c++/v1/atomic:1075:25: error: expected ')'
_Tp kill_dependency(_Tp __y) _NOEXCEPT
                        ^
/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/bin/../include/c++/v1/atomic:1075:5: note: to match this '('
_Tp kill_dependency(_Tp __y) _NOEXCEPT
    ^
/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/lib/clang/15.0.99/include/stdatomic.h:65:28: note: expanded from macro 'kill_dependency'
#define kill_dependency(y) (y)
                           ^

Looking into it. I suspect this is caused by the LLDB build doing something funky with include paths and the C standard library <stdatomic.h> being picked up before the libc++ one, but I'm not sure. Investigating.

Looking into it. I suspect this is caused by the LLDB build doing something funky with include paths and the C standard library <stdatomic.h> being picked up before the libc++ one, but I'm not sure. Investigating.

I can reproduce with

echo "#include <memory>" | clang++ -xc++ - -fmodules -fcxx-modules -std=c++11 -fsyntax-only -nostdinc++ -isystem build/default/include/c++/v1

So that's yet another modules issue. I'll revert this for the time being, however the #1 priority now needs to be to have some sort of LLDB formatters testing as part of our pre-commit CI, because we can't continue like this, it's terrible for both LLDB and libc++.

ldionne reopened this revision.Feb 15 2022, 9:59 AM
This revision is now accepted and ready to land.Feb 15 2022, 9:59 AM
ldionne updated this revision to Diff 408945.Feb 15 2022, 10:03 AM

Uploading again -- I'll land this once I'm confident it doesn't break LLDB.

gulfem added a subscriber: gulfem.Feb 15 2022, 10:57 AM

We started seeing the following issue before this patch got reverted in https://llvm.googlesource.com/llvm-project/+/987c7f407d1414a023f03eb788bb97667b479f27.

[48618/254381] CC efi_x64/obj/src/firmware/gigaboot/src/src.avb.c.o
FAILED: efi_x64/obj/src/firmware/gigaboot/src/src.avb.c.o
../../../recipe_cleanup/clangc7enxobn/bin/clang -MD -MF efi_x64/obj/src/firmware/gigaboot/src/src.avb.c.o.d -o efi_x64/obj/src/firmware/gigaboot/src/src.avb.c.o -D_LIBCPP_DISABLE_VISIBILITY_ANNOTAT...
In file included from ../../src/firmware/gigaboot/src/avb.c:17:
In file included from ../../src/firmware/gigaboot/src/utf_conversion.h:6:
../../zircon/system/public/zircon/types.h:554:9: error: unknown type name 'atomic_int'
typedef atomic_int zx_futex_t;
        ^
1 error generated.

https://luci-milo.appspot.com/ui/p/fuchsia/builders/ci/clang_toolchain.ci.core.x64-release/b8822209109414248273/overview

We started seeing the following issue before this patch got reverted in https://llvm.googlesource.com/llvm-project/+/987c7f407d1414a023f03eb788bb97667b479f27.

[48618/254381] CC efi_x64/obj/src/firmware/gigaboot/src/src.avb.c.o
FAILED: efi_x64/obj/src/firmware/gigaboot/src/src.avb.c.o
../../../recipe_cleanup/clangc7enxobn/bin/clang -MD -MF efi_x64/obj/src/firmware/gigaboot/src/src.avb.c.o.d -o efi_x64/obj/src/firmware/gigaboot/src/src.avb.c.o -D_LIBCPP_DISABLE_VISIBILITY_ANNOTAT...
In file included from ../../src/firmware/gigaboot/src/avb.c:17:
In file included from ../../src/firmware/gigaboot/src/utf_conversion.h:6:
../../zircon/system/public/zircon/types.h:554:9: error: unknown type name 'atomic_int'
typedef atomic_int zx_futex_t;
        ^
1 error generated.

https://luci-milo.appspot.com/ui/p/fuchsia/builders/ci/clang_toolchain.ci.core.x64-release/b8822209109414248273/overview

I can't see the full command-lines in the linked log. Can you try to reduce the issue you're seeing? I don't think this is the same thing as the LLDB issue, so this will break again when I re-commit this after fixing the LLDB issues.

Sorry for the noise, and this was an issue caused from our side.
We fixed the issue, and it should be ok to reland this patch.

Herald added a project: Restricted Project. · View Herald TranscriptApr 7 2022, 1:46 AM
ldionne updated this revision to Diff 421617.Apr 8 2022, 12:47 PM

Rebase onto main. @shafik, would you mind applying this patch locally to confirm that this version doesn't break the LLDB data formatter tests? I've tried running them myself but I am running into various errors.

The modules reproducer I had is now passing, so this should be fine, but I'd like to be real sure.

Rebase onto main. @shafik, would you mind applying this patch locally to confirm that this version doesn't break the LLDB data formatter tests? I've tried running them myself but I am running into various errors.

The modules reproducer I had is now passing, so this should be fine, but I'd like to be real sure.

@ldionne I'm wondering, how do you feel about adding LLDB and execute its tests in the Bootstrap build. (We've broken LLDB before and might have again D122598). Shall I provide a patch?

Rebase onto main. @shafik, would you mind applying this patch locally to confirm that this version doesn't break the LLDB data formatter tests? I've tried running them myself but I am running into various errors.

The modules reproducer I had is now passing, so this should be fine, but I'd like to be real sure.

@ldionne I'm wondering, how do you feel about adding LLDB and execute its tests in the Bootstrap build. (We've broken LLDB before and might have again D122598). Shall I provide a patch?

Yes, I would love that. I think we should only run the data formatters though, not all the tests. If those are not too expensive to run, I would run them in a separate job. I'm not sure we need to build all of Clang anyways, so including that in the Bootstrapping build might not save us that much work.

ldionne updated this revision to Diff 421924.Apr 11 2022, 7:35 AM

Address CI failures.

ldionne updated this revision to Diff 421925.Apr 11 2022, 7:43 AM

Rebase onto main to get the new feature-test macro testing.

ldionne updated this revision to Diff 427364.May 5 2022, 9:51 AM

Rebase onto main. I'll ship this tomorrow if there is no new information.

This revision was landed with ongoing or failed builds.May 6 2022, 6:53 AM
This revision was automatically updated to reflect the committed changes.
rnk added a subscriber: rnk.May 12 2022, 10:47 AM

I wanted to give some early feedback that this broke our internal build of CPython, and we put together a local workaround, so we aren't blocked. I don't fully understand what's going on or have time to follow up, but I wanted to give a heads up that this seems like it could be a disruptive change for users.

In D97044#3509541, @rnk wrote:

I wanted to give some early feedback that this broke our internal build of CPython, and we put together a local workaround, so we aren't blocked. I don't fully understand what's going on or have time to follow up, but I wanted to give a heads up that this seems like it could be a disruptive change for users.

Seeing more fallout from this internally - existing code that seems to have been including <stdatomic.h> after <atomic> successfully is now broken/needs fixing.

@ldionne is this sort of new breakage expected? Should it be mitigated in some way if it's fairly widespread?

(ah, might not be causing existing breakage because we're building the libc++ as a module, so stdatomic before/after doesn't interfere with the module, perhaps... )

@dblaikie @rnk

Thanks both for your heads up. Could you perhaps share the error messages you were seeing (and ideally how you were building these TUs)?

libcxx/include/stdatomic.h
234–236

I suspect this is the issue. This might be too aggressive.

Before C++23, if you include <stdatomic.h> and then <atomic>, things break inside <atomic> in horrible ways. We have an error message in <atomic> to catch that. However, if you include <atomic> AND THEN <stdatomic.h>, perhaps things just work, and we don't need to flag anything like we try to do here.