|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.
@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.
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).
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++.
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.
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) ^
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.
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.
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.