fitzgen opened PR #14115 from fitzgen:known-memories-tables-globals-alias-regions to bytecodealliance:main:
Accesses of statically-known globals, memories, and tables now use
AliasRegionKey::Defined{Global,Memory,Table}rather than the conservative
Public{Global,Memory,Table}region shared by every entity of that kind which
crosses a module boundary.However, unlike
known_imported_functions, this requires an extra condition:
every module that ever imports an entity must always import that same
entity. Otherwise a function that accesses, e.g., a memory via the conservative
region could be inlined into one that uses the precise region, and accessing the
same bytes through two different alias regions is invalid and leads to
miscompiles. That is, all of the importing modules and the defining module must
agree on the alias region.Depends on https://github.com/bytecodealliance/wasmtime/pull/14114
fitzgen requested alexcrichton for a review on PR #14115.
fitzgen requested wasmtime-core-reviewers for a review on PR #14115.
fitzgen requested wasmtime-compiler-reviewers for a review on PR #14115.
github-actions[bot] added the label cranelift on PR #14115.
github-actions[bot] added the label cranelift:area:machinst on PR #14115.
:memo: alexcrichton submitted PR review:
Thinking about this analysis done here I'm worried about the case where a module imports something and reexports it, although I can't quite place my finger on why so I wanted to ask about that. It seems like an import could be considered unambiguous but then an export could be considered ambiguous, but in that scenario we'd want the import to additionally be considered ambiguous. Does this handle that sort of case already?
:speech_balloon: alexcrichton created PR review comment:
Could the comment here, or somewhere in this function, indicate that the global/table version of this function below is a copy/paste of this function and they should all stay in sync?
fitzgen updated PR #14115.
fitzgen commented on PR #14115:
@alexcrichton good catch, there was indeed a bug with re-exports. I've fixed it and added
.wasttests that will fail when run with inlining enabled (e.g. by the wast-file fuzzer) if we mess up our alias regions in this way again (and checked locally withwasmtime wast -Cinlining=ythat they failed before the fix and pass afterwards).
:thumbs_up: alexcrichton submitted PR review.
:speech_balloon: alexcrichton created PR review comment:
Reading over this again I personally find this pretty confusing. I understand why this is the way it is, but in consider this I could imaging an alternative design where a module statically knows it imports a particular global and then has separate information on whether it's appropriate to use a statically known alias region for that global. Basically I could imagine a design where one axis of imports is "is it always this thing" and then another axis is "is that thing always known to all other modules as well".
One example optimization with this is that if we know an import of a non-mutable global is always a particular global we can just inline the value everywhere. That's got nothing to do with alias regions, however, and by tying alias regions to this known-imports set we're unable to optimize some situations.
I don't mean to place more work on this PR, but I want to explain my confusion with the phrasing/naming here. The "XXX" here seems to indicate that this is a strong requirement that these sets must always be different from the functions set, but I don't feel that fully describes the situation.
Given all that, two questions:
- Could this actually be relaxed where "known imports" are unconditionally "this module is only ever instantiated with this thing"? In such a situation I'd imagine that when deducing the alias region for an imported global, for example, it'd see the known import and then lookup in that defining
ModuleTranslationif the global is in theglobals_known_to_importersset. I'm not sure if we've got all the siblingModuleTranslations at compile time available to make this deduction.- Failing that, could the comment here be expanded with some of the commentary I have here? Basically that this is an open issue we could consider fixing in the future and the "XXX" here isn't a hard requirement, just an artifact of the current implementation.
:speech_balloon: alexcrichton created PR review comment:
Since this is compile-time an
assert!would do I think here -- it's a bit odd to see a debug assertion followed by a case handling when the debug assertion is false and silently handling when the debug assert otherwise fails. What I'd otherwise expect for example is something likebail_bug!which isn't quite appropriate here
:speech_balloon: alexcrichton created PR review comment:
Could this drop the
Try*part of the collection since this is at compile time?
:memo: fitzgen submitted PR review.
:speech_balloon: fitzgen created PR review comment:
That would require moving a some maps that currently only exist on the translation into the module itself and doing a bit of additional plumbing that we don't do today, which is all doable, but I don't want to spin on that in this PR, so I opened #14164 to track it.
fitzgen updated PR #14115.
fitzgen has enabled auto merge for PR #14115.
fitzgen added PR #14115 Use precise alias regions for statically-known entity imports to the merge queue.
:check: fitzgen merged PR #14115.
fitzgen removed PR #14115 Use precise alias regions for statically-known entity imports from the merge queue.
Last updated: Aug 30 2026 at 09:07 UTC