Details
- Reviewers
ldionne Mordante var-const - Group Reviewers
Restricted Project - Commits
- rG46db8d822ecd: [libc++] Granularize <atomic>
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
LGTM!
Since the patch already most likely breaks possible patches, I would suggest to do a followup patch to change some code to our current preferred style. WDYT?
libcxx/include/__atomic/aliases.h | ||
---|---|---|
28 | Suggestion, change this to using in a NFC follow up patch. | |
libcxx/include/__atomic/atomic.h | ||
37 | Followup suggestion s/_LIBCPP_INLINE_VISIBILITY/_LIBCPP_HIDE_FROM_ABI/ | |
libcxx/include/__atomic/atomic_sync.h | ||
87 | Followup suggestion s/_VSTD/std/ |
libcxx/include/__atomic/cxx_atomic_impl.h | ||
---|---|---|
774 | I think the use of __libcpp_is_always_lock_free requires a new #include <__atomic/is_always_lock_free.h>, doesn't it? We're seeing some build failures after this change that appear to be caused by a missing include. If that is the case can you provide a forward fix? Here's our error message, and a link to our failing bot. [38461/300130](185) CXX kernel.efi_x64/obj/src/lib/utf_conversion/libutf_conversion.utf_conversion.cc.o FAILED: kernel.efi_x64/obj/src/lib/utf_conversion/libutf_conversion.utf_conversion.cc.o ../../../recipe_cleanup/clangr8n6ja_c/bin/clang++ -MD -MF kernel.efi_x64/obj/src/lib/utf_conversion/libutf_conversion.utf_conversion.cc.o.d -o kernel.efi_x64/obj/src/lib/utf_conversion/libutf_conversion.utf_conversion.cc.o -D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS -D_LIBCPP_ENABLE_THREAD_SAFETY_ANNOTATIONS=1 -DZX_ASSERT_LEVEL=2 -DWITH_FRAME_POINTERS=1 -I../../zircon/system/public -I../../sdk/lib... In file included from ../../src/lib/utf_conversion/utf_conversion.cc:7: In file included from ../../sdk/lib/stdcompat/include/lib/stdcompat/bit.h:12: In file included from ../../sdk/lib/stdcompat/include/lib/stdcompat/memory.h:8: In file included from ../../../recipe_cleanup/clangr8n6ja_c/include/c++/v1/memory:898: In file included from ../../../recipe_cleanup/clangr8n6ja_c/include/c++/v1/__memory/shared_ptr.h:42: In file included from ../../../recipe_cleanup/clangr8n6ja_c/include/c++/v1/atomic:522: In file included from ../../../recipe_cleanup/clangr8n6ja_c/include/c++/v1/__atomic/aliases.h:12: In file included from ../../../recipe_cleanup/clangr8n6ja_c/include/c++/v1/__atomic/atomic.h:12: In file included from ../../../recipe_cleanup/clangr8n6ja_c/include/c++/v1/__atomic/atomic_base.h:12: In file included from ../../../recipe_cleanup/clangr8n6ja_c/include/c++/v1/__atomic/atomic_sync.h:12: In file included from ../../../recipe_cleanup/clangr8n6ja_c/include/c++/v1/__atomic/contention_t.h:12: ../../../recipe_cleanup/clangr8n6ja_c/include/c++/v1/__atomic/cxx_atomic_impl.h:773:49: error: no template named '__libcpp_is_always_lock_free' typename _Base = typename conditional<__libcpp_is_always_lock_free<_Tp>::__value, ^ ../../../recipe_cleanup/clangr8n6ja_c/include/c++/v1/__atomic/cxx_atomic_impl.h:775:80: error: no type named 'type' in the global namespace __cxx_atomic_lock_impl<_Tp> >::type> ~~^ ../../../recipe_cleanup/clangr8n6ja_c/include/c++/v1/__atomic/cxx_atomic_impl.h:775:84: error: expected unqualified-id __cxx_atomic_lock_impl<_Tp> >::type> |
libcxx/include/__atomic/cxx_atomic_impl.h | ||
---|---|---|
774 | Thanks for the report! I fixed it in 21a543656cf4840023078359a6c7e0db7d5391b2. We should definitely get a runner for a minimal freestanding implementation to catch these kinds of problems. |
libcxx/include/__atomic/cxx_atomic_impl.h | ||
---|---|---|
774 | Thanks for the quick fix. Fuchsia's build is a bit complex, so I wasn't 100% sure there wasn't' something wacky going on. |
libcxx/include/__atomic/cxx_atomic_impl.h | ||
---|---|---|
774 | I think this is still missing #include <__type_traits/conditional.h> Do you have an intuition as to why these wouldn't fail in presubmit testing? AFAICT this should always be an issue w/ the new header structure. Well, at least in this code path, which seems to be a common/tested case. Are other platforms getting definitions pulled in earlier for some reason? I didn't see anything in the preprocessor logic that stood out, so I'm a bit puzzled as to why this crops up in our build more than others. The code being built is a user-land library that just pulls in standard headers, and isn't overly complex (counter to my earlier statement). [53272/300225](640) CXX kernel.efi_x64/obj/zircon/kernel/lib/libc/libc.rand.cc.o FAILED: kernel.efi_x64/obj/zircon/kernel/lib/libc/libc.rand.cc.o ../../../recipe_cleanup/clangmfsa2yky/bin/clang++ -MD -MF kernel.efi_x64/obj/zircon/kernel/lib/libc/libc.rand.cc.o.d -o kernel.efi_x64/obj/zircon/kernel/lib/libc/libc.rand.cc.o -D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS -D_LIBCPP_ENABLE_THREAD_SAFETY_ANNOTATIONS=1 -DZX_ASSERT_LEVEL=2 -DWITH_FRAME_POINTERS=1 -I../../zircon/system/public -I../../zircon/kernel/lib/libc/include -I../../zircon/kernel/... In file included from ../../zircon/kernel/lib/libc/rand.cc:10: In file included from ../../zircon/kernel/lib/ktl/include/ktl/atomic.h:10: In file included from ../../sdk/lib/stdcompat/include/lib/stdcompat/atomic.h:10: In file included from ../../../recipe_cleanup/clangmfsa2yky/include/c++/v1/atomic:522: In file included from ../../../recipe_cleanup/clangmfsa2yky/include/c++/v1/__atomic/aliases.h:12: In file included from ../../../recipe_cleanup/clangmfsa2yky/include/c++/v1/__atomic/atomic.h:12: In file included from ../../../recipe_cleanup/clangmfsa2yky/include/c++/v1/__atomic/atomic_base.h:12: In file included from ../../../recipe_cleanup/clangmfsa2yky/include/c++/v1/__atomic/atomic_sync.h:12: In file included from ../../../recipe_cleanup/clangmfsa2yky/include/c++/v1/__atomic/contention_t.h:12: ../../../recipe_cleanup/clangmfsa2yky/include/c++/v1/__atomic/cxx_atomic_impl.h:774:37: error: no template named 'conditional' typename _Base = typename conditional<__libcpp_is_always_lock_free<_Tp>::__value, |
libcxx/include/__atomic/cxx_atomic_impl.h | ||
---|---|---|
774 | sorry was commuting. it may take me a bit to spin something up and check locally, but I can give it a whirl. |
Suggestion, change this to using in a NFC follow up patch.