- Notifications
You must be signed in to change notification settings - Fork1.6k
Debugging: allow frame cursor to visit all activations.#12176
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Open
cfallin wants to merge1 commit intobytecodealliance:mainChoose a base branch fromcfallin:debugging-host-sandwich
base:main
Could not load branches
Branch not found:{{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline, and old review comments may become outdated.
+404 −83
Conversation
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
d5edc9c to286fa8dCompareIn the initial design for the `DebugFrameCursor`, I was concernedabout the effects of host async on the stability of visiting earlieractivations (see also the discussion of async combinators inbytecodealliance#11896).The basic hypothesized problem was that when Wasm calls host-codecalls Wasm, the sequence of activations on the stack is not evenstable between async polls; so any debugger hook, which is an asyncfunction, should not be allowed to hold a frame cursor across a yieldpoint since it could become invalidated if the next poll stacks up theactivations differently.In further conversations it's become clear that this is not actually apossibility, for the simple reason that the inner re-entrantactivations into the same store take full ownership (mutably reborrow)that store, and that mut reborrow becomes part of the future; so theexact chain of activations will remain in the same sequence whenre-polled. Said another way, it is impossible at any given level ofasync host-code to create *more than one* future that re-enters thesame store and somehow poll those in different orders at differenttimes. The worst that a host-code async combinator can do is drop thefuture that re-enters the store. This drops and invalidates whateverframes a cursor held over a yield might be referencing, but it *also*drops the async invocation of the debugger hook itself, and due tolifetimes the cursor cannot escape that hook, so everything is stillsound.This PR thus updates the `DebugFrameCursor` to visit allactivations. I've generalized the backtrace code a bit to enable this,and built an internal `StoreBacktrace` that is an iterator over allactivations associated with the store.At the `DebugFrameCursor` (public API) level, the two basic choiceswere to present a sentinel for host frame(s) explicitly and make allWasm-specific accessors return `Option<T>`, or skip over hostframes. I opted for the latter, with `move_to_parent()` returning anenum value now that indicates whether it moved to a new activation.A note regarding the *async* component ABI: once debugging is possiblewithin a `run_concurrent` environment, it will again be the case thata single frame cursor should see only one activation, becauseeach (re)-entry into the store becomes a new task, if my understandingis correct. At that time, we should build an API that lets thedebugger see the activation for each task separately. That's a simplermodel ultimately, and it will be nice when we move to it, but as longas we have the sync component ABI with async host code and the abilityto stack activations as we do today, we need to provide the debuggerthis visibility.(Aside: why does the debugger *need* to see more than one activation?In addition to presenting a weird and incoherent view of the world tothe user if we don't, it is also necessary to implement the"next" (step-over) debugger action, because otherwise a call to a hostfunction that re-enters the store may lead to a state with fewer, butcompletely disjoint, stack frames on the "one latest activation" fromwhich it's not possible to reason about whether we've left thecalled-into function yet.)
286fa8d to5107b56CompareSign up for freeto 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.
In the initial design for the
DebugFrameCursor, I was concerned about the effects of host async on the stability of visiting earlier activations (see also the discussion of async combinators in#11896). The basic hypothesized problem was that when Wasm calls host-code calls Wasm, the sequence of activations on the stack is not even stable between async polls; so any debugger hook, which is an async function, should not be allowed to hold a frame cursor across a yield point since it could become invalidated if the next poll stacks up the activations differently.In further conversations it's become clear that this is not actually a possibility, for the simple reason that the inner re-entrant activations into the same store take full ownership (mutably reborrow) that store, and that mut reborrow becomes part of the future; so the exact chain of activations will remain in the same sequence when re-polled. Said another way, it is impossible at any given level of async host-code to createmore than one future that re-enters the same store and somehow poll those in different orders at different times. The worst that a host-code async combinator can do is drop the future that re-enters the store. This drops and invalidates whatever frames a cursor held over a yield might be referencing, but italso drops the async invocation of the debugger hook itself, and due to lifetimes the cursor cannot escape that hook, so everything is still sound.
This PR thus updates the
DebugFrameCursorto visit all activations. I've generalized the backtrace code a bit to enable this, and built an internalStoreBacktracethat is an iterator over all activations associated with the store.At the
DebugFrameCursor(public API) level, the two basic choices were to present a sentinel for host frame(s) explicitly and make all Wasm-specific accessors returnOption<T>, or skip over host frames. I opted for the latter, withmove_to_parent()returning an enum value now that indicates whether it moved to a new activation.A note regarding theasync component ABI: once debugging is possible within a
run_concurrentenvironment, it will again be the case that a single frame cursor should see only one activation, because each (re)-entry into the store becomes a new task, if my understanding is correct. At that time, we should build an API that lets the debugger see the activation for each task separately. That's a simpler model ultimately, and it will be nice when we move to it, but as long as we have the sync component ABI with async host code and the ability to stack activations as we do today, we need to provide the debugger this visibility.(Aside: why does the debuggerneed to see more than one activation? In addition to presenting a weird and incoherent view of the world to the user if we don't, it is also necessary to implement the "next" (step-over) debugger action, because otherwise a call to a host function that re-enters the store may lead to a state with fewer, but completely disjoint, stack frames on the "one latest activation" from which it's not possible to reason about whether we've left the called-into function yet.)