Wasmtime GitHub notifications bot (Jun 11 2026 at 05:59): Wasmtime GitHub notifications bot (Jun 11 2026 at 05:59):fitzgen requested alexcrichton for a review on PR #13610.
Wasmtime GitHub notifications bot (Jun 11 2026 at 05:59):fitzgen opened PR #13610 from fitzgen:inline-copy-alias-regions to bytecodealliance:main:
The small-constant-length inline fast path for
{memory,table,array}.copyemitted region-less loads/stores, while every other access to the same bytes is region-tagged. This led to stale values being forwarded in store-to-load forwarding.We probably need to backport this to the unreleased 46 branch as well, but I'll wait to open that PR until after this is reviewed and merges.
Wasmtime GitHub notifications bot (Jun 11 2026 at 05:59):fitzgen requested wasmtime-compiler-reviewers for a review on PR #13610.
Wasmtime GitHub notifications bot (Jun 11 2026 at 05:59):fitzgen requested wasmtime-core-reviewers for a review on PR #13610.
Wasmtime GitHub notifications bot (Jun 11 2026 at 13:57)::memo: alexcrichton submitted PR review:
This seems fine, but also pretty unfortunate. Forgetting to add these seems extremely likely, and resulting in what's possibly-a-CVE seems like a pretty severe consequence for forgetting to do something. Could this be something we can post-verify or something like that? For example should we debug-assert in wasmtime-cranelift that all loads/stores have regions? Or could cranelift be more conservative and assume that region-les loads/stores alias everything?
Wasmtime GitHub notifications bot (Jun 11 2026 at 19:23):fitzgen commented on PR #13610:
For example should we debug-assert in wasmtime-cranelift that all loads/stores have regions?
We can eventually do this, but not yet because not all loads and stores are region-tagged yet (which is fine, as long as all accesses to that data are either region-tagged or not, the miscompiles happen when we sometimes tag and sometimes don't). Filling out region-tagging is one of the work streams I am currently focused on, however, and we could certainly add a debug-assert to walk over the IR before
finalizeand assert this property when I'm done.Or could cranelift be more conservative and assume that region-les loads/stores alias everything?
This is half the case today:
- Regionless stores act as a fence and are considered to potentially alias everything, so they clear out last-store data for all regions in our alias analysis.
- Regionless loads, however, are not considered as potentially aliasing with previous region-tagged stores (that is, a region-tagged store does not clear the last-store data for the lack-of-region slot in the alias analysis).
I'm not totally sure what the implications of changing the latter behavior would be, and whether it would be sound or not. I guess clearing last-store data is always sound because it only makes our analysis more conservative? cc @cfallin for thoughts on this subject
Wasmtime GitHub notifications bot (Jun 11 2026 at 20:24):cfallin commented on PR #13610:
I'm not totally sure what the implications of changing the latter behavior would be, and whether it would be sound or not. I guess clearing last-store data is always sound because it only makes our analysis more conservative? cc @cfallin for thoughts on this subject
The sound/conservative fallback for loads is indeed may-alias. In essence "no region" is a parent region of all others (contains all others); for untagged stores it would also be fine to update all writers for all regions (rather than clearing), though clearing is sound; and for untagged loads we know nothing, so they could alias anything, so we soundly avoid moving across any writer and may forward from any store if the concrete address (SSA value + offset) matches.
Wasmtime GitHub notifications bot (Jun 11 2026 at 21:29)::thumbs_up: alexcrichton submitted PR review:
Talked with @fitzgen about this on Zulip in DMs a bit -- my thinking is that we want some sort of safeguard here and the two that I can think of are either asserting we have regions everywhere or handicapping Cranelift's current analysis. Nick's convinced me that the handicapping approach is not the right one here since we don't want to tamper with Cranelift's current assertion of "if it's got an alias region then it doesn't alias anything else, including untagged regions" (this is my understanding correct me if I'm wrong Nick, and AFAIK this is a bit unrelated to the change you talked aboust just above?).
In lieu of this though Nick's got all the remaining regions tagged in local PRs and will have them landed shortly, so it'll be viable in short order to have such an assertion.
Wasmtime GitHub notifications bot (Jun 11 2026 at 21:32):cfallin commented on PR #13610:
"if it's got an alias region then it doesn't alias anything else, including untagged regions" (this is my understanding correct me if I'm wrong Nick, and AFAIK this is a bit unrelated to the change you talked aboust just above?).
See comment just above yours? It would be sound to change to untagged-is-parent-region-of-all (so may-alias all), I think.
Wasmtime GitHub notifications bot (Jun 11 2026 at 21:37):alexcrichton commented on PR #13610:
My understanding is, yes, it's sound, but Nick was thinking that perf-wise we don't want to do that as it'd change the meaning of alias regions from what Cranelift has historically had and risk de-optimizing things too much. (correct me though if I'm wrong Nick)
Wasmtime GitHub notifications bot (Jun 11 2026 at 21:38):fitzgen added PR #13610 Properly tag inline bulk-copy loads/stores with alias regions to the merge queue.
Wasmtime GitHub notifications bot (Jun 11 2026 at 21:39):fitzgen commented on PR #13610:
I think doing that would probably be okay. Certainly, after I have everything region-tagged in
crates/craneliftthen it won't affect Wasmtime at that point.
Wasmtime GitHub notifications bot (Jun 11 2026 at 21:41):alexcrichton commented on PR #13610:
Oh, in that case, would that be a less-risky-but-still-correct backport option?
Wasmtime GitHub notifications bot (Jun 11 2026 at 21:46):fitzgen commented on PR #13610:
Yeah potentially. I can look into it.
Wasmtime GitHub notifications bot (Jun 11 2026 at 22:02)::check: fitzgen merged PR #13610.
Wasmtime GitHub notifications bot (Jun 11 2026 at 22:02):fitzgen removed PR #13610 Properly tag inline bulk-copy loads/stores with alias regions from the merge queue.
Last updated: Jul 29 2026 at 05:03 UTC