This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Granularize <atomic>
ClosedPublic

Authored by philnik on Jan 31 2023, 4:00 AM.

Details

Reviewers
ldionne
Mordante
var-const
Group Reviewers
Restricted Project
Commits
rG46db8d822ecd: [libc++] Granularize <atomic>

Diff Detail

Event Timeline

philnik created this revision.Jan 31 2023, 4:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 31 2023, 4:00 AM
Herald added a subscriber: krytarowski. · View Herald Transcript
philnik updated this revision to Diff 493567.Jan 31 2023, 4:37 AM

Generate files

philnik updated this revision to Diff 493591.Jan 31 2023, 6:34 AM

Try to fix CI

philnik updated this revision to Diff 494873.Feb 5 2023, 1:42 AM

Try to fix CI

philnik updated this revision to Diff 494876.Feb 5 2023, 1:53 AM

Try to fix CI

philnik updated this revision to Diff 494891.Feb 5 2023, 2:58 AM

Try to fix CI

philnik updated this revision to Diff 497088.Feb 13 2023, 1:24 PM

Try to fix CI

philnik published this revision for review.Feb 13 2023, 4:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 13 2023, 4:05 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante accepted this revision.Feb 16 2023, 1:09 PM

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
29

Suggestion, change this to using in a NFC follow up patch.

libcxx/include/__atomic/atomic.h
38

Followup suggestion s/_LIBCPP_INLINE_VISIBILITY/_LIBCPP_HIDE_FROM_ABI/

libcxx/include/__atomic/atomic_sync.h
88

Followup suggestion s/_VSTD/std/

This revision is now accepted and ready to land.Feb 16 2023, 1:09 PM

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?

Sounds good. I'll make a few NFC patches.

philnik updated this revision to Diff 498305.Feb 17 2023, 3:09 AM

ignore format

This revision was landed with ongoing or failed builds.Feb 17 2023, 3:44 AM
This revision was automatically updated to reflect the committed changes.
paulkirth added inline comments.
libcxx/include/__atomic/cxx_atomic_impl.h
773

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

philnik marked an inline comment as done.Feb 17 2023, 10:35 AM
philnik added inline comments.
libcxx/include/__atomic/cxx_atomic_impl.h
773

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.

paulkirth added inline comments.Feb 17 2023, 10:40 AM
libcxx/include/__atomic/cxx_atomic_impl.h
773

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.

paulkirth added inline comments.Feb 17 2023, 4:23 PM
libcxx/include/__atomic/cxx_atomic_impl.h
773

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,
philnik marked an inline comment as done.Feb 17 2023, 4:29 PM
philnik added inline comments.
libcxx/include/__atomic/cxx_atomic_impl.h
773

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?

paulkirth added inline comments.Feb 17 2023, 5:57 PM
libcxx/include/__atomic/cxx_atomic_impl.h
773

sorry was commuting. it may take me a bit to spin something up and check locally, but I can give it a whirl.

paulkirth added inline comments.Feb 17 2023, 8:20 PM
libcxx/include/__atomic/cxx_atomic_impl.h
773

Seems like D144307 addresses the last of these issues from what I can see.