[libcxx-commits] [PATCH] D142972: [libc++] Granularize <atomic>

Fri Feb 17 17:57:16 PST 2023

paulkirth added inline comments.

================
Comment at: libcxx/include/__atomic/cxx_atomic_impl.h:773
+template <typename _Tp,
+          typename _Base = typename conditional<__libcpp_is_always_lock_free<_Tp>::__value,
+                                                __cxx_atomic_base_impl<_Tp>,
----------------
philnik wrote:
> paulkirth wrote:
> > paulkirth wrote:
> > > philnik wrote:
> > > > paulkirth wrote:
> > > > > 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>
> > > > > ```
> > > > > 
> > > > > https://ci.chromium.org/ui/p/fuchsia/builders/ci/clang_toolchain.ci.core.x64-debug-tot-build_only/b8788907734054093697/overview
> > > > > 
> > > > 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.
> > > 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.
> > 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,
> > ```
> I think we don't test `_LIBCPP_ATOMIC_ONLY_USE_BUILTINS` in our CI, which means this code gets never executed. I've uploaded D144307, which should hopefully fix the remaining problems. Could you apply it locally and check?
sorry was commuting. it may take me a bit to spin something up and check locally, but I can give it a whirl.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142972/new/

https://reviews.llvm.org/D142972