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

Fri Feb 17 16:23:09 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>,
----------------
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,
```

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142972