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

Fri Feb 17 16:29:25 PST 2023

philnik marked an inline comment as done.
philnik 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>,
----------------
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?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142972