Repository navigation
[libc++] Use Fuchsia futex operations #133571
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
8026b86
[libc++] Use Fuchsia futex operations
frobtech ba63615
Merge branch 'main' into p/libcxx-fuchsia-futex
frobtech 5643728
Merge remote-tracking branch 'fork/main' into p/libcxx-fuchsia-futex
frobtech 3f74907
Update for current libc++ internals
frobtech e9a703b
Merge branch 'main' into p/libcxx-fuchsia-futex
frobtech 70b03d6
Remove unnecessary cast
frobtech b9ea27d
Merge branch 'main' into p/libcxx-fuchsia-futex
frobtech File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just wanted to bring you attention of
libcxx/include/__atomic/atomic_waitable_traits.h
and here is the design intent of how things work.
atomic_waitable_traits.h defines a concept
__has_native_atomic_waitthat decides which types can directly call the platform futex wait, and which types needs to go through the global contention table proxy for platform wait.And there are two ABI modes.
For the default ABI,
__has_native_atomic_waitbasically says only the__cxx_contention_tsatisfies, which isint32_tin your case, can directly go to platform futex wait. I assume here zx_futex_t is exactly int32_t otherwise we might have UBFor another ABI which has
_LIBCPP_ABI_ATOMIC_WAIT_NATIVE_BY_SIZEdefined, which is controlled by the platform owner. in this case, i believe your platform does not enable it. but say, one day you decide to enable this ABI, the way it works is that. you can define__has_native_atomic_waitthe way you want, the default concept in this mode is: any trivially copyable type with unique value representations, whose sizes matches__cxx_contention_t. in this case,__ptrand__valcan be pointers to a wide range of types, e.g. auint32_t,struct S { int32_t i;};etc... In which case, I am not sure howreinterpret_cast<zx_futex_t const*>can be well defined. (anyway you don't need to worry about this if you don't plan to enable this ABI)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do have
_LIBCPP_ABI_ATOMIC_WAIT_NATIVE_BY_SIZEenabled by dint of the fact that we always use ABI version 2. I'd be happy for it to be unconditionally enabled for Fuchsia, but I didn't see any precedent for that setting being controlled by anything but_LIBCPP_ABI_VERSION. In what sense is it controlled by the platform owner?AFAICT there is no change to
__atomic/atomic_waitable_traits.hrequired because__cxx_contention_tis already alwaysint32_t(as my changes here to__atomic/contention_t.h) so the default case usessizeof(__cxx_contention_t). For the same reason, the#ifdef __linux__branch there seems superfluous.I'm not sure what the concern is about the
reinterpret_cast.zx_futex_tis just an integer type with the same size asint32_t, so it's layout compatible with any integer type or struct of integer type of that size.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vendors can configure those individual ABI macros in the
__config_site. See the doc herehttps://libcxx.llvm.org/ABIGuarantees.html#abi-flags
And yes if you opt in
_LIBCPP_ABI_VERSIONto2, you get all of these individual macros.I think in your case, yes no action required. I mentioned that because some platforms can support multiple sizes for the platform wait. For example, Apple's ulock wait support both 4 bytes and 8 bytes wait.
I think it is kind of "it works in practice" but I think it is a language UB if you
reinterpret_castfrom memory location when there is actually no object of that type has started life time (implicitly or explicitly)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, so by "platform owner" you meant "person who sets the libc++ build parameters", not the owner of a particular platform's platform-specific code in libc++ sources. These are not always the same, and for Fuchsia it varies where I am both of those people and where I am only the latter. :-)
There is no language-based access here. The cast is just getting the pointer value to a system call, so regardless of the complexities of trivial object lifetime that might actually apply here, it's really outside the scope of those language rules anyway.
Thanks for the explanations! I think this change is good to go as is now. It just needs the approval from @philnik777 to be mergeable.