V0ldek opened PR #14885 from V0ldek:v0ldek/custom-linear-memories to bytecodealliance:main:
<!--
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 review the Bytecode Alliance's AI tool usage policy at
https://github.com/bytecodealliance/governance/blob/main/AI_TOOL_POLICY.mdPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->Following up on the discussion on #10740 I propose a rework of the memory API that centralizes the requirements for linear memory implementation in the public
LinearMemorytrait.The core challenge is resolving the issue from #10740 -- custom memories did not work with the CoW mechanism. This is because the code mounting memory images was hardwired to the internal mmap implementation. The idea is to move the responsibility for supporting CoW into the
LinearMemoryimplementation with the new functioninit_from_image. It receives an opaque publicMemoryImagethat informs the implementor where it needs to be mounted (offset and length). The predicted path is that a memory that supports mapping the image over it (backed by mmap) calls themap_atcallback and acknowledges that the pages required for the image have been remapped (returnstrue). If this is not supported, the implementation (default) returnsfalseand wasmtime abandons CoW mapping in favour of a full copy of the data pages. TheMemoryImagestruct also exposes the underlying bytes of the memory and equality semantics, so custom implementors can, in theory, implement their own lazy copying or their own pooling.The second change is that
LocalMemorywas holding anOption<MemoryImageSlot>to support theStaticMemoryand pooling allocators. It also manually called theset_heap_limiton the slot during grow. I did two things:
- The
MemoryImageSlotis now owned by theStaticMemoryandLocalMemoryonly records if the memory needs to be initialized from an image (for the purposes ofneeds_init()required by instantiation code). This makes it so thatLocalMemorydepends only on theLinearMemoryimplementation and needs no knowledge of the pooling allocator machinery.LinearMemorynow has theAnysupertrait. This is a) required so that the pooling allocator can get the innerBox<dyn LinearMemory>fromLocalMemoryand unwrap it to what it knows to beStaticMemoryand recover theMemoryImageSlot, b) useful in the potential future where we add the ability to retrieve the memory impl from an instance, as a custom memory could be downcast fromBox<dyn LinearMemory>.== Overview of code changes
Public API changes:
LinearMemorygetsinit_from_imageMemoryImage(arg to the above) is publicThese are non-breaking: existing LinearMemories will not support CoW, but instead of crashing they'll just force wasmtime to copy.
Removed:
- the internal
RuntimeLinearMemorytrait- the
LinearMemoryProxythat wrapped aBox<dyn LinearMemory>and provided theRuntimeLinearMemoryimplThis functionality is now centralized in the
LinearMemorytrait.The
MmapMemorywas refactored to accommodate the changes, it relied onRuntimeLinearMemory. Now it uses the standardised path throughinit_from_imageto map a CoW image, although with a hatch through an internalMemoryImage::vm()to keep all of the mmap code as it was with the nice static Mmap types that were already there. Technically one could get rid of thevm()function, but changing all the mmap code felt like more hassle than it's worth.
MemoryImageSlotis now only used by the pooling code. In fact, its definition could be gated behind#[cfg(feature = 'pooling-allocator')]', but that would trigger a whole cascade of cfgs because a lot ofuse` statements become dead code with the feature disabled and trigger warnings. I wasn't sure if we want those cfgs splattered everywhere, but I can introduce them if we do (judgement call for the code owners).I fixed what I think was a bug (see Zulip discussion #wasmtime > Custom memory creators for host memories @ 💬) in
create_memorywhere custom memory creators were bypassed and the default creator was always used. This meanMemory::newalways created a default memory and not a custom one. I added a regression test for that.The new functionality is tested in
tests/all/memory_creator.rswith focus on CoW behaviour for custom memories. The new publicMemoryImageis thoroughly tested in unit tests inruntime/memory.rs. I also added a few tests I felt were missing:MmapOffset(to make sure the bounds checks are correct, since they must be for unsafe code to be sound) and a few tests forLocalMemorygrowth.I also tried to make the
LinearMemorysafety documentation more useful for implementors. Organised the requirements into bullets so that an implementor can reason about their safety contract and "tick-off" the requirements.
V0ldek removed the review request for wasmtime-core-reviewers from PR #14885.
V0ldek requested dicej for a review on PR #14885.
V0ldek requested wasmtime-core-reviewers for a review on PR #14885.
V0ldek edited PR #14885:
<!--
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 review the Bytecode Alliance's AI tool usage policy at
https://github.com/bytecodealliance/governance/blob/main/AI_TOOL_POLICY.mdPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->Following up on the discussion on #10740 I propose a rework of the memory API that centralizes the requirements for linear memory implementation in the public
LinearMemorytrait.The core challenge is resolving the issue from #10740 -- custom memories did not work with the CoW mechanism. This is because the code mounting memory images was hardwired to the internal mmap implementation. The idea is to move the responsibility for supporting CoW into the
LinearMemoryimplementation with the new functioninit_from_image. It receives an opaque publicMemoryImagethat informs the implementor where it needs to be mounted (offset and length). The predicted path is that a memory that supports mapping the image over it (backed by mmap) calls themap_atcallback and acknowledges that the pages required for the image have been remapped (returnstrue). If this is not supported, the implementation (default) returnsfalseand wasmtime abandons CoW mapping in favour of a full copy of the data pages. TheMemoryImagestruct also exposes the underlying bytes of the memory and equality semantics, so custom implementors can, in theory, implement their own lazy copying or their own pooling.The second change is that
LocalMemorywas holding anOption<MemoryImageSlot>to support theStaticMemoryand pooling allocators. It also manually called theset_heap_limiton the slot during grow. I did two things:
- The
MemoryImageSlotis now owned by theStaticMemoryandLocalMemoryonly records if the memory needs to be initialized from an image (for the purposes ofneeds_init()required by instantiation code). This makes it so thatLocalMemorydepends only on theLinearMemoryimplementation and needs no knowledge of the pooling allocator machinery.LinearMemorynow has theAnysupertrait. This is a) required so that the pooling allocator can get the innerBox<dyn LinearMemory>fromLocalMemoryand unwrap it to what it knows to beStaticMemoryand recover theMemoryImageSlot, b) useful in the potential future where we add the ability to retrieve the memory impl from an instance, as a custom memory could be downcast fromBox<dyn LinearMemory>.Overview of code changes
Public API changes:
LinearMemorygetsinit_from_imageMemoryImage(arg to the above) is publicThese are non-breaking: existing LinearMemories will not support CoW, but instead of crashing they'll just force wasmtime to copy.
Removed:
- the internal
RuntimeLinearMemorytrait- the
LinearMemoryProxythat wrapped aBox<dyn LinearMemory>and provided theRuntimeLinearMemoryimplThis functionality is now centralized in the
LinearMemorytrait.The
MmapMemorywas refactored to accommodate the changes, it relied onRuntimeLinearMemory. Now it uses the standardised path throughinit_from_imageto map a CoW image, although with a hatch through an internalMemoryImage::vm()to keep all of the mmap code as it was with the nice static Mmap types that were already there. Technically one could get rid of thevm()function, but changing all the mmap code felt like more hassle than it's worth.
MemoryImageSlotis now only used by the pooling code. In fact, its definition could be gated behind#[cfg(feature = 'pooling-allocator')]', but that would trigger a whole cascade of cfgs because a lot ofuse` statements become dead code with the feature disabled and trigger warnings. I wasn't sure if we want those cfgs splattered everywhere, but I can introduce them if we do (judgement call for the code owners).I fixed what I think was a bug (see Zulip discussion #wasmtime > Custom memory creators for host memories @ 💬) in
create_memorywhere custom memory creators were bypassed and the default creator was always used. This meanMemory::newalways created a default memory and not a custom one. I added a regression test for that.The new functionality is tested in
tests/all/memory_creator.rswith focus on CoW behaviour for custom memories. The new publicMemoryImageis thoroughly tested in unit tests inruntime/memory.rs. I also added a few tests I felt were missing:MmapOffset(to make sure the bounds checks are correct, since they must be for unsafe code to be sound) and a few tests forLocalMemorygrowth.I also tried to make the
LinearMemorysafety documentation more useful for implementors. Organised the requirements into bullets so that an implementor can reason about their safety contract and "tick-off" the requirements.
V0ldek edited PR #14885:
<!--
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 review the Bytecode Alliance's AI tool usage policy at
https://github.com/bytecodealliance/governance/blob/main/AI_TOOL_POLICY.mdPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->Following up on the discussion on #10740 I propose a rework of the memory API that centralizes the requirements for linear memory implementation in the public
LinearMemorytrait.The core challenge is resolving the issue from #10740 -- custom memories did not work with the CoW mechanism. This is because the code mounting memory images was hardwired to the internal mmap implementation. The idea is to move the responsibility for supporting CoW into the
LinearMemoryimplementation with the new functioninit_from_image. It receives an opaque publicMemoryImagethat informs the implementor where it needs to be mounted (offset and length). The predicted path is that a memory that supports mapping the image over it (backed by mmap) calls themap_atcallback and acknowledges that the pages required for the image have been remapped (returnstrue). If this is not supported, the implementation (default) returnsfalseand wasmtime abandons CoW mapping in favour of a full copy of the data pages. TheMemoryImagestruct also exposes the underlying bytes of the memory and equality semantics, so custom implementors can, in theory, implement their own lazy copying or their own pooling.The second change is that
LocalMemorywas holding anOption<MemoryImageSlot>to support theStaticMemoryand pooling allocators. It also manually called theset_heap_limiton the slot during grow. I did two things:
- The
MemoryImageSlotis now owned by theStaticMemoryandLocalMemoryonly records if the memory needs to be initialized from an image (for the purposes ofneeds_init()required by instantiation code). This makes it so thatLocalMemorydepends only on theLinearMemoryimplementation and needs no knowledge of the pooling allocator machinery.LinearMemorynow has theAnysupertrait. This is a) required so that the pooling allocator can get the innerBox<dyn LinearMemory>fromLocalMemoryand unwrap it to what it knows to beStaticMemoryand recover theMemoryImageSlot, b) useful in the potential future where we add the ability to retrieve the memory impl from an instance, as a custom memory could be downcast fromBox<dyn LinearMemory>.Overview of code changes
Public API changes:
LinearMemorygetsinit_from_imageMemoryImage(arg to the above) is publicThese are non-breaking: existing LinearMemories will not support CoW, but instead of crashing they'll just force wasmtime to copy via the default impl of
init_from_image.Removed:
- the internal
RuntimeLinearMemorytrait- the
LinearMemoryProxythat wrapped aBox<dyn LinearMemory>and provided theRuntimeLinearMemoryimplThis functionality is now centralized in the
LinearMemorytrait.The
MmapMemorywas refactored to accommodate the changes, it relied onRuntimeLinearMemory. Now it uses the standardised path throughinit_from_imageto map a CoW image, although with a hatch through an internalMemoryImage::vm()to keep all of the mmap code as it was with the nice static Mmap types that were already there. Technically one could get rid of thevm()function, but changing all the mmap code felt like more hassle than it's worth.
MemoryImageSlotis now only used by the pooling code. In fact, its definition could be gated behind#[cfg(feature = 'pooling-allocator')]', but that would trigger a whole cascade of cfgs because a lot ofuse` statements become dead code with the feature disabled and trigger warnings. I wasn't sure if we want those cfgs splattered everywhere, but I can introduce them if we do (judgement call for the code owners).I fixed what I think was a bug (see Zulip discussion #wasmtime > Custom memory creators for host memories @ 💬) in
create_memorywhere custom memory creators were bypassed and the default creator was always used. This meanMemory::newalways created a default memory and not a custom one. I added a regression test for that.The new functionality is tested in
tests/all/memory_creator.rswith focus on CoW behaviour for custom memories. The new publicMemoryImageis thoroughly tested in unit tests inruntime/memory.rs. I also added a few tests I felt were missing:MmapOffset(to make sure the bounds checks are correct, since they must be for unsafe code to be sound) and a few tests forLocalMemorygrowth.I also tried to make the
LinearMemorysafety documentation more useful for implementors. Organised the requirements into bullets so that an implementor can reason about their safety contract and "tick-off" the requirements.
V0ldek edited PR #14885:
<!--
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 review the Bytecode Alliance's AI tool usage policy at
https://github.com/bytecodealliance/governance/blob/main/AI_TOOL_POLICY.mdPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->Following up on the discussion on #10740 I propose a rework of the memory API that centralizes the requirements for linear memory implementation in the public
LinearMemorytrait.The core challenge is resolving the issue from #10740 -- custom memories did not work with the CoW mechanism. This is because the code mounting memory images was hardwired to the internal mmap implementation. The idea is to move the responsibility for supporting CoW into the
LinearMemoryimplementation with the new functioninit_from_image. It receives an opaque publicMemoryImagethat informs the implementor where it needs to be mounted (offset and length). The predicted path is that a memory that supports mapping the image over it (backed by mmap) calls themap_atcallback and acknowledges that the pages required for the image have been remapped (returnstrue). If this is not supported, the implementation (default) returnsfalseand wasmtime abandons CoW mapping in favour of a full copy of the data pages. TheMemoryImagestruct also exposes the underlying bytes of the memory and equality semantics, so custom implementors can, in theory, implement their own lazy copying or their own pooling.The second change is that
LocalMemorywas holding anOption<MemoryImageSlot>to support theStaticMemoryand pooling allocators. It also manually called theset_heap_limiton the slot during grow. I did two things:
- The
MemoryImageSlotis now owned by theStaticMemoryandLocalMemoryonly records if the memory needs to be initialized from an image (for the purposes ofneeds_init()required by instantiation code). This makes it so thatLocalMemorydepends only on theLinearMemoryimplementation and needs no knowledge of the pooling allocator machinery.LinearMemorynow has theAnysupertrait. This is a) required so that the pooling allocator can get the innerBox<dyn LinearMemory>fromLocalMemoryand unwrap it to what it knows to beStaticMemoryand recover theMemoryImageSlot, b) useful in the potential future where we add the ability to retrieve the memory impl from an instance, as a custom memory could be downcast fromBox<dyn LinearMemory>.Overview of code changes
Public API changes:
LinearMemorygetsinit_from_imageMemoryImage(arg to the above) is publicThese are non-breaking: existing LinearMemories will not support CoW, but instead of crashing they'll just force wasmtime to copy via the default impl of
init_from_image.Removed:
- the internal
RuntimeLinearMemorytrait- the
LinearMemoryProxythat wrapped aBox<dyn LinearMemory>and provided theRuntimeLinearMemoryimplThis functionality is now centralized in the
LinearMemorytrait.The
MmapMemorywas refactored to accommodate the changes, it relied onRuntimeLinearMemory. Now it uses the standardised path throughinit_from_imageto map a CoW image, although with a hatch through an internalMemoryImage::vm()to keep all of the mmap code as it was with the nice static Mmap types that were already there. Technically one could get rid of thevm()function, but changing all the mmap code felt like more hassle than it's worth.
MemoryImageSlotis now only used by the pooling code. In fact, its definition could be gated behind#[cfg(feature = 'pooling-allocator')], but that would trigger a whole cascade of cfgs because a lot ofusestatements become dead code with the feature disabled and trigger warnings. I wasn't sure if we want those cfgs splattered everywhere, but I can introduce them if we do (judgement call for the code owners).I fixed what I think was a bug (see Zulip discussion #wasmtime > Custom memory creators for host memories @ 💬) in
create_memorywhere custom memory creators were bypassed and the default creator was always used. This meanMemory::newalways created a default memory and not a custom one. I added a regression test for that.The new functionality is tested in
tests/all/memory_creator.rswith focus on CoW behaviour for custom memories. The new publicMemoryImageis thoroughly tested in unit tests inruntime/memory.rs. I also added a few tests I felt were missing:MmapOffset(to make sure the bounds checks are correct, since they must be for unsafe code to be sound) and a few tests forLocalMemorygrowth.I also tried to make the
LinearMemorysafety documentation more useful for implementors. Organised the requirements into bullets so that an implementor can reason about their safety contract and "tick-off" the requirements.
github-actions[bot] added the label wasmtime:api on PR #14885.
github-actions[bot] added the label wasmtime:ref-types on PR #14885.
github-actions[bot] commented on PR #14885:
Subscribe to Label Action
cc @fitzgen
<details>
This issue or pull request has been labeled: "wasmtime:api", "wasmtime:ref-types"Thus the following users have been cc'd because of the following labels:
- fitzgen: wasmtime:ref-types
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
alexcrichton removed the review request for dicej from PR #14885.
alexcrichton requested alexcrichton for a review on PR #14885.
:memo: alexcrichton submitted PR review:
Thank you so much for this! Overall this looks like great work to me. I've left some minor comments about tests/docs/organization, but nothing major. Happy to merge with them addressed and/or as-is if you feel things should stay
:speech_balloon: alexcrichton created PR review comment:
Are the new tests here testing much beyond what's already testable form the public API? We try to prefer
tests/all/*.rswhere possible since that's a more stable surface to test against. If these have specific coverage of internals though it's fine to keep, but wanted to ask. (on review to me these look pretty similar to what's already tested elsewhere)
:speech_balloon: alexcrichton created PR review comment:
One thing I'd recommend adding to this is a clause saying that this documentation strives to be thorough but is not guaranteed to be exhaustive. Basically if we forgot something here that's a bug, not a CVE
:speech_balloon: alexcrichton created PR review comment:
I think it's reasonable to keep this (as opposed to making it truly conditional and cascading everything) -- but could the
allowchange toexpect?
:speech_balloon: alexcrichton created PR review comment:
In my experience
should_panictests are pretty finnicky and also typically indicative of something that should be transmitted through aResult. Perhaps themap_image_atAPI could return a result instead of panicking on these conditions? Makes it easier to test and additionally lessens the severity of accidentally tripping over the assertions later (e.g. a panic is a CVE, but a spurious error not so)
:speech_balloon: alexcrichton created PR review comment:
I think with simd bits we actually require 16-byte alignment.
:memo: V0ldek submitted PR review.
:speech_balloon: V0ldek created PR review comment:
I'm relatively sure SIMD doesn't need it,
v128.loadis valid regardless of alignment, but 16-byte aligned would potentially be faster (avoiding cache-line splits).In any case, we can always require more than needed from the implementations "just in case", the internal ones align to 16 already.
V0ldek updated PR #14885.
:memo: V0ldek submitted PR review.
:speech_balloon: V0ldek created PR review comment:
I added the following "disclaimer":
/// Note that while we strive to be thorough in this documentation, we do not guarantee that the /// following points are exhaustive. /// When using a custom memory backend in a security-sensitive context auditing just the external /// implementation of this trait is not sufficient.
:memo: V0ldek submitted PR review.
:speech_balloon: V0ldek created PR review comment:
Changed
MmapOffset::map_image_atto return an error instead of panicking, tests adjusted.
:memo: V0ldek submitted PR review.
:speech_balloon: V0ldek created PR review comment:
Hey, that's a neat feature I didn't know of! While at it, I adjusted a bunch of similar
allowstatements in the same file to beexpectwhen pooling allocator is disabled.
:memo: V0ldek submitted PR review.
:speech_balloon: V0ldek created PR review comment:
No, these are specifically tests of the public API. I'll move them to the integration tests and double-check if I'm not duplicating work.
:speech_balloon: V0ldek edited PR review comment.
V0ldek updated PR #14885.
V0ldek updated PR #14885.
:memo: V0ldek submitted PR review.
V0ldek updated PR #14885.
:speech_balloon: V0ldek created PR review comment:
I moved the tests to tests/all/memory_creator.rs and then essentially rewrote that test module:
- It now runs most tests on both unix and windows, except for CoW images that require memfd
- Removed the old simple tests and covered the MemoryCreator and LinearMemory public API expectations in new tests.
One of them uses an
expect_panicwhich you said you dislike, but I think it's justified there. It's a cheap defense-in-depth effort by the engine to prevent wrong implementations from doing something stupid. I don't think it's reasonable to Err out of there, because whenmemory.growfails the user might attempt to still use the memory for something, but in this case critical invariants were violated so the correct decision is to explode. I also changed it to have an actual panic message.
:memo: alexcrichton submitted PR review.
:speech_balloon: alexcrichton created PR review comment:
Oh right good point I was misremembering -- it's atomics that require alignment not simd. In any case I think documenting 16 for now is reasonable, thanks!
:thumbs_up: alexcrichton submitted PR review:
Thanks again!
alexcrichton added PR #14885 Moving to LinearMemory trait and fixing custom memories to the merge queue.
:check: alexcrichton merged PR #14885.
alexcrichton removed PR #14885 Moving to LinearMemory trait and fixing custom memories from the merge queue.
Last updated: Oct 31 2026 at 23:03 UTC