Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
| libcxx/include/stdatomic.h | ||
|---|---|---|
| 3 | Copy-paste of wchar.h. | |
| libcxx/include/stdatomic.h | ||
|---|---|---|
| 3 | Thanks for pointing this. I'll fix it in the next revision. | |
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? |
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) | |
| 175 ↗ | (On Diff #325006) | You don't need double-parens here and elsewhere, I think. |
| 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: | |
| libcxx/include/stdatomic.h | ||
|---|---|---|
| 131 | undef takes an identifier (and that's it). | |
| 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? | |
| 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. |
| libcxx/include/stdatomic.h | ||
|---|---|---|
| 225 | @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. | |
| 223–231 | 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). | |
Address review comments and fix CI.
| libcxx/include/stdatomic.h | ||
|---|---|---|
| 223–231 | I added a test for this. | |
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?
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, thanks for taking this over.
I have no time to work on libc++ these days (and months :) ) unfortunately...
| 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? | |
| 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. | |
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. | |
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.
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 was fixed by a30a7948d59470fe9403102b192b3e7ddaf21849.
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++.
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.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.
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.
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.
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... )
| libcxx/include/stdatomic.h | ||
|---|---|---|
| 235–237 | 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. | |
Copy-paste of wchar.h.