cfallin opened PR #12566 from cfallin:a-new-framing-for-stack-frames to bytecodealliance:main:
This addresses some of the issues described #12486: we need the ability to keep a handle to a stack frame as long as execution is frozen, and keep multiple of these handles around, alongside the
Store, without any handle directly holding a borrow of the store.The frame handles work by means of an "execution version" scheme: the idea is that whenever any execution resumes in a given store, all handles to existing frames could be invalidated, but if no such execution occurs, all handles should still be valid. A tuple of (globally unique for process lifetime) store ID, and execution version within that store, should be sufficient to uniquely identify any frozen-stack period during execution. This accomplishes cheap handle invalidation without the need to track existing handles.
This PR also implements a cache of parsed frame-table data. Previously this was lazily parsed by the cursor as it walked up a stack, but with multiple handles hanging around, and with handles meant to be cheap to hold and clone, and with handles being invalidated eagerly, it makes much more sense to persist this parsed metadata at the
Storelevel. (It cannot persist at theEnginelevel because PCs are local per store.)<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
cfallin requested fitzgen for a review on PR #12566.
cfallin requested wasmtime-compiler-reviewers for a review on PR #12566.
cfallin requested wasmtime-core-reviewers for a review on PR #12566.
cfallin commented on PR #12566:
(I plan to add tests for various handle invalidation cases tomorrow -- sorry, missed including those here)
github-actions[bot] added the label wasmtime:api on PR #12566.
cfallin updated PR #12566.
alexcrichton created PR review comment:
"hte"
alexcrichton created PR review comment:
The
impl Into<...>strategy may not be necessary here, can this beAsContextMutinstead? This is also applicable to a lot of methods below too, basically anything that doesn't need to return a thing-with-'acan useAsContextMutinstead ofInto<...>
alexcrichton created PR review comment:
Can you add
// SAFETY: ...comments here and below?
alexcrichton created PR review comment:
Could this perhaps take just a
pcinstead of a fullFrameto emphasize how it's a per-pc calculation and can't take anything into account like fp/sp/etc?
alexcrichton created PR review comment:
Can you expand the docs here with a
# Panicssection for theis_validassertion?
alexcrichton created PR review comment:
Can you add a
// SAFETY: ...comment here?
alexcrichton created PR review comment:
Can this, and all methods below, document
# Panicswithis_validassertions?
alexcrichton submitted PR review:
In retrospect we probably should have had this earlier, but given all the
unsafehere I think it'd be good to get Miri testing for this. Could you extend thepulley_provenance_testtest intests/all/pulley.rs(or something like that) to be able to exercise all this under Miri?
cfallin updated PR #12566.
cfallin updated PR #12566.
cfallin submitted PR review.
cfallin created PR review comment:
Fixed, thanks!
cfallin submitted PR review.
cfallin created PR review comment:
Done in e1e24025927adb47b463159ab707951554bd6cd3.
cfallin submitted PR review.
cfallin created PR review comment:
Done in e1e24025927adb47b463159ab707951554bd6cd3.
cfallin submitted PR review.
cfallin created PR review comment:
Switched the API to return
Resultanywhere a handle is used that needs to be valid, instead, after discussion with @fitzgen; we'll need checks at some level anyway when we wrap this in a WIT and it makes more sense to produce theErrfrom the builtin check than do a double-check. Implemented in e1e24025927adb47b463159ab707951554bd6cd3.
cfallin submitted PR review.
cfallin created PR review comment:
Updated to return a
Resultinstead (details below) in e1e24025927adb47b463159ab707951554bd6cd3.
cfallin updated PR #12566.
cfallin submitted PR review.
cfallin created PR review comment:
Good idea -- done in dedd88b297.
cfallin submitted PR review.
cfallin created PR review comment:
Ah, yes, that's much cleaner -- possible everywhere but for
fn module(...) -> Option<&'a Module>. Done in dedd88b297.
cfallin commented on PR #12566:
In retrospect we probably should have had this earlier, but given all the
unsafehere I think it'd be good to get Miri testing for this. Could you extend thepulley_provenance_testtest intests/all/pulley.rs(or something like that) to be able to exercise all this under Miri?I'm getting some UB errors in miri deep in the
MmapVeccode -- perhaps something to do with the private-code-copy support? In any case, I'd like to defer this miri testing to a followup if you don't mind as it's looking like it won't be trivial.
alexcrichton submitted PR review:
That sounds reasonable to me yeah, and thanks!
alexcrichton added PR #12566 Debugging: refactor stack frame cursor into frame handle abstraction. to the merge queue
github-merge-queue[bot] removed PR #12566 Debugging: refactor stack frame cursor into frame handle abstraction. from the merge queue
cfallin updated PR #12566.
cfallin has enabled auto merge for PR #12566.
fitzgen submitted PR review:
(realizing I forgot to hit submit on my pending review comments)
fitzgen created PR review comment:
Seems like this could be
FrameCursor::newnow?
fitzgen created PR review comment:
This seems like it should be an
Activation::frame_cursormethod, so that we don't need to make the activation's register statepub(crate)(which means we need to audit the whole crate to check the safety of passing their values intoframe_cursor, making sure that nothing else is setting them to invalid values).
fitzgen created PR review comment:
Can we avoid making these
pub(crate)?Because if
frame_cursor/FrameCursor::newis going to continue to beunsafe(meaning that we are trusting its pc/fp/trampoline_fp arguments), then we should probably not make thesepub(crate)to limit the scope of how much code needs to be trusted to maintain these values correctly.Alternatively, we could also make
frame_cursor/FrameCursor::newa safe function and have all theunsafeinvariants bottleneck atFrameCursor::advance.
cfallin has disabled auto merge for PR #12566.
cfallin updated PR #12566.
cfallin submitted PR review.
cfallin created PR review comment:
Done in 40f26f8fa4.
cfallin submitted PR review.
cfallin created PR review comment:
Ah, yep, done in 40f26f8fa4 now that we have a method in
Activationto use them directly.
cfallin submitted PR review.
cfallin created PR review comment:
Done in 40f26f8fa4.
cfallin has enabled auto merge for PR #12566.
cfallin edited PR #12566:
This addresses some of the issues described #12486: we need the ability to keep a handle to a stack frame as long as execution is frozen, and keep multiple of these handles around, alongside the
Store, without any handle directly holding a borrow of the store.The frame handles work by means of an "execution version" scheme: the idea is that whenever any execution resumes in a given store, all handles to existing frames could be invalidated, but if no such execution occurs, all handles should still be valid. A tuple of (globally unique for process lifetime) store ID, and execution version within that store, should be sufficient to uniquely identify any frozen-stack period during execution. This accomplishes cheap handle invalidation without the need to track existing handles.
This PR also implements a cache of parsed frame-table data. Previously this was lazily parsed by the cursor as it walked up a stack, but with multiple handles hanging around, and with handles meant to be cheap to hold and clone, and with handles being invalidated eagerly, it makes much more sense to persist this parsed metadata at the
Storelevel. (It cannot persist at theEnginelevel because PCs are local per store.)Fixes #12486.
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
cfallin updated PR #12566.
cfallin updated PR #12566.
cfallin added PR #12566 Debugging: refactor stack frame cursor into frame handle abstraction. to the merge queue
cfallin updated PR #12566.
cfallin removed PR #12566 Debugging: refactor stack frame cursor into frame handle abstraction. from the merge queue
cfallin has enabled auto merge for PR #12566.
cfallin added PR #12566 Debugging: refactor stack frame cursor into frame handle abstraction. to the merge queue
cfallin merged PR #12566.
cfallin removed PR #12566 Debugging: refactor stack frame cursor into frame handle abstraction. from the merge queue
Last updated: Feb 24 2026 at 04:36 UTC