Make any_sender_of<> play nicer with MSVC #619
Merged
Conversation
This file contains 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
facebook-github-bot
added
the
CLA Signed
|
I think this is breaking an internal build. |
janondrusek
approved these changes
Jul 16, 2024
ispeters
added a commit
that referenced
this pull request
Jul 16, 2024
The Unifex unit test suite won't build for Windows with async stack injection enabled *unless* PR #619 (Make any_sender_of<> play nicer with MSVC) is also merged, but that PR causes Windows + Clang + ASAN errors in Meta-internal builds. This diff works around the above conflict by disabling async stack injection in Windows builds by default so we don't need PR #619. We can change the default once we figure out a proper resolution to the ASAN problem.
ispeters
added a commit
that referenced
this pull request
Jul 16, 2024
The Unifex unit test suite won't build for Windows with async stack injection enabled *unless* PR #619 (Make any_sender_of<> play nicer with MSVC) is also merged, but that PR causes Windows + Clang + ASAN errors in Meta-internal builds. This diff works around the above conflict by disabling async stack injection in Windows builds by default so we don't need PR #619. We can change the default once we figure out a proper resolution to the ASAN problem.
ispeters
added a commit
that referenced
this pull request
Jul 17, 2024
The Unifex unit test suite won't build for Windows with async stack injection enabled *unless* PR #619 (Make any_sender_of<> play nicer with MSVC) is also merged, but that PR causes Windows + Clang + ASAN errors in Meta-internal builds. This diff works around the above conflict by disabling async stack injection in Windows builds by default so we don't need PR #619. We can change the default once we figure out a proper resolution to the ASAN problem.
ispeters
added a commit
that referenced
this pull request
Jul 18, 2024
This PR copies the core of Folly's [async stack trace support](https://github.com/facebook/folly/tree/main/folly/tracing) into `include/unifex/tracing` and builds on it to add support for generalized *Senders*. When `UNIFEX_NO_ASYNC_STACKS` is falsey, `unifex::connect` returns a wrapped operation state that injects async stack tracing into the operation tree. - The wrapper operation: - stores an `AsyncStackFrame` for the wrapped operation; and - wraps the receiver. - In the wrapper operation's customization of `unifex::start` we: - create an `AsyncStackRoot` on the stack; - push the wrapper operation's `AsyncStackFrame` onto the current async stack; - activate the wrapper operation's `AsyncStackFrame` on the current `AsyncStackRoot`; and - start the wrapped operation. - In the wrapper receiver's completion methods we: - create an `AsyncStackRoot` on the stack; - copy the *parent* operation's `AsyncStackFrame` to the stack; - activate the parent `AsyncStackFrame` on the current `AsyncStackRoot`; and - invoke the parent operation's receiver. The effect is that we build up a linked list (technically a DAG) of `AsyncStackFrame`s pointing "up" toward the start of the operation as `unifex::start` recurses into the nested operation state and then unwind it on the way back out as the receiver completion methods are invoked. At any given time, the current thread's `AsyncStackRoot` is sitting on the most recently-activated "normal" stack frame that is participating in async stack management, allowing Folly's `co_bt.py` debugger extension to figure out when it should stop walking normal stack frames and start walking async stack frames. As alluded to above, the behaviour of the async stack tracing machinery is controlled by the `UNIFEX_NO_ASYNC_STACKS` preprocessor macro. If it's truthy, async stacks are not traced; if it's falsey, they are traced. The default in `unifex/config.hpp` is to enable async stack tracing in non-Windows debug builds. - Why not Windows builds? - Because there's something weird about how `any_sender_of<>` builds on Windows (both Clang and MSVC); the resolution is to land PR #619, but that PR breaks an internal Meta build so I'll have to come back to it. - Why only debug builds? - The additional work done to track async stacks adds non-trivial binary size to the output so I figure it should default to off for release builds. You can turn it on by defining `UNIFEX_NO_ASYNC_STACKS=0` in your release build script if the extra debuggability is worth the extra binary size in production. This iteration is an MVP: - only general senders are supported, not coroutines - the "return addresses" captured for each sender point to `unifex::_get_return_address::default_return_address<T>()`, where `T` is the type of the sender - this is better than nothing because the resulting symbol includes the sender's fully-qualified name, but it's not great Futures PRs will: - add support for tracing the async stacks of coroutines - improve the rendering of async stack traces by making senders capture a pointer to the call site of their factory - maybe shrink the binary size overhead of enabling this feature if I can figure out how to eliminate some of the recursion Original diff descriptions: * Import Folly's async stack library This diff, originally by @janondrusek and @jesswong, copies the core of Folly's [async stack trace support](https://github.com/facebook/folly/tree/main/folly/tracing) into `include/unifex/tracing`. * Add type-safe instruction and frame pointer types Stop using `void*` to represent both instruction pointers and stack frame pointers and start using `unifex::instruction_ptr` and `unifex::frame_ptr`. * Add ScopedAsyncStackRoot::ensureFrameDeactivated() We need a way to restore a `ScopedAsyncStackRoot` to the "no active frame" state before destroying it on the way out of a customization of `unifex::start` but the frame we want to deactivate is a member of the operation state, which means it's likely already been destroyed. This diff adds `ScopedAsyncStackRoot::ensureFrameDeactivated()`, which performs most of the same actions as `deactivateAsyncStackFrame()` but without touching the frame. I think this still technically invokes UB by copying and comparing a zapped pointer, but it's better than what we had before. * Add get_async_stack_frame This diff adds a new receiver query CPO that is expected to return the address of the `AsyncStackFrame` associated with the receiver's operation. * Add get_return_address This diff adds a new sender query CPO that is expected to return the instruction pointer best representing the "return address" for the sender; the default implementation returns the return address of a function template instantiation that includes the sender's type in its signature as a kind of "better than nothing" result. * Add a comment re: lldb type summaries The `instruction_ptr` type is best rendered by the debugger as an "address", which will render as a symbol + offset rather than an arbitrary hexadecimal value. This diff adds a comment to the type documenting this fact. * Establish an AsyncStackRoot in sync_wait() This diff modifies `unifex::sync_wait()` to establish an `AsyncStackRoot` on the stack while the awaited operation is running. * Modify unifex::connect to inject async stacks This diff modifies `unifex::connect` to inject async stack tracking into every operation state is it's built. * Work around Windows-only problems The Unifex unit test suite won't build for Windows with async stack injection enabled *unless* PR #619 (Make any_sender_of<> play nicer with MSVC) is also merged, but that PR causes Windows + Clang + ASAN errors in Meta-internal builds. This diff works around the above conflict by disabling async stack injection in Windows builds by default so we don't need PR #619. We can change the default once we figure out a proper resolution to the ASAN problem. Co-authored-by: Ján Ondrušek <[email protected]> Co-authored-by: Jessica Wong <[email protected]> Co-authored-by: Deniz Evrenci <[email protected]>
ispeters
force-pushed
the
tweak_any_sender_of_opstate
branch
from
f9f1072
to
a752dc6
Compare
ispeters
force-pushed
the
coroutine_async_stack_support
branch
from
76f3bc0
to
48f411c
Compare
ispeters
force-pushed
the
tweak_any_sender_of_opstate
branch
from
a752dc6
to
e95f9df
Compare
ispeters
force-pushed
the
coroutine_async_stack_support
branch
from
48f411c
to
e8a18b9
Compare
ispeters
force-pushed
the
tweak_any_sender_of_opstate
branch
2 times, most recently
from
a4dcc5f
to
009bf11
Compare
ispeters
added a commit
that referenced
this pull request
Sep 1, 2024
After #619, async stack support is almost possible with MSVC; this PR makes the final changes to support the feature.
ispeters
force-pushed
the
tweak_any_sender_of_opstate
branch
from
009bf11
to
b63a902
Compare
ispeters
changed the base branch from
coroutine_async_stack_support
to
add_gcc_15_and_clang_18
ispeters
added a commit
that referenced
this pull request
Sep 11, 2024
After #619, async stack support is almost possible with MSVC; this PR makes the final changes to support the feature.
`unifex::any_sender_of<>` needs a type-erased operation state, which it creates with `unifex::any_unique<concrete-op>`. The tricky bit is that operation states are expected to be immovable so constructing that `any_unique` requires care. The old approach was to use a type called `_rvo<>` that implicitly converted to the desired type, but it was brittle. This change invents a new opstate type with two jobs: 1. wrap some other opstate, and 2. construct that other opstate by invoking a callable.
ispeters
force-pushed
the
tweak_any_sender_of_opstate
branch
from
b63a902
to
4e4f5e9
Compare
ispeters
added a commit
that referenced
this pull request
Sep 11, 2024
After #619, async stack support is almost possible with MSVC; this PR makes the final changes to support the feature.
ispeters
added a commit
that referenced
this pull request
Oct 1, 2024
After #619, async stack support is almost possible with MSVC; this PR makes the final changes to support the feature.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
unifex::any_sender_of<>needs a type-erased operation state, which it creates withunifex::any_unique<concrete-op>. The tricky bit is that operation states are expected to be immovable so constructing thatany_uniquerequires care. The old approach was to use a type called_rvo<>that implicitly converted to the desired type, but it was brittle. This change invents a new opstate type with two jobs: