Move MemoryImageSource::map_at to mmap module #9687
Conversation
| // Note that this code would be incorrect if clear-on-drop | ||
| // were enabled. That's because in the struct definition, | ||
| // `memory_image` above is listed after `alloc`. Rust drops | ||
| // fields in the order they're defined, so `memory_image` | ||
| // would be dropped after `alloc`. If clear-on-drop were | ||
| // enabled, `memory_image` would end up remapping a bunch of | ||
| // memory after it was unmapped by the `alloc` drop. |
There was a problem hiding this comment.
This was a subtle dependency that took me a bit to understand.
There was a problem hiding this comment.
(And this is only dangerous when alloc represents an owned memory allocation. It's fine if alloc is borrowed. This is a subtle point.)
There was a problem hiding this comment.
Do you think it'd perhaps be better to remove this no_clear_on_drop and instead perform it in Drop for LocalMemory? (where that could .take() and explicitly do the image stuff before dropping the mmap)
There was a problem hiding this comment.
Oh that's a good idea, similar to MemoryPool. I think it's correct.
There was a problem hiding this comment.
Oh actually there's another subtle issue here, I think if slot.instantiate below fails, we should clear on drop (that's part of the point of clear-on-drop, right?) So I think it would be best to make this change in a followup since it's a behavior change, not a refactor.
There was a problem hiding this comment.
I've expanded this comment -- it would definitely be good to move this to the destructor, but it would be a behavior change so it should be a followup. This PR itself should not contain any behavior changes.
There was a problem hiding this comment.
Oh, and adding a Drop impl identified the problem with unwrap_static_image, haha, since you can't move out of a type when there's a Drop impl.
There was a problem hiding this comment.
Ah ok that all makes sense to me yeah, thanks for checking!
github-actions
bot
added
the
wasmtime:api
| // Note that this code would be incorrect if clear-on-drop | ||
| // were enabled. That's because in the struct definition, | ||
| // `memory_image` above is listed after `alloc`. Rust drops | ||
| // fields in the order they're defined, so `memory_image` | ||
| // would be dropped after `alloc`. If clear-on-drop were | ||
| // enabled, `memory_image` would end up remapping a bunch of | ||
| // memory after it was unmapped by the `alloc` drop. |
There was a problem hiding this comment.
Do you think it'd perhaps be better to remove this no_clear_on_drop and instead perform it in Drop for LocalMemory? (where that could .take() and explicitly do the image stuff before dropping the mmap)
sunshowers
force-pushed
the
map-at-mmap-2
branch
from
ec01aa6 to
7169a05
Compare
sunshowers
force-pushed
the
map-at-mmap-2
branch
from
7169a05 to
c3fa778
Compare
sunshowers
changed the title
sunshowers
force-pushed
the
map-at-mmap-2
branch
from
c3fa778 to
477b3bd
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
Fixing the issues |
sunshowers
force-pushed
the
map-at-mmap-2
branch
from
477b3bd to
d9d910e
Compare
|
Hmm, actually I want to rename the os-specific Mmap |
Return a single `SendSyncPtr` -- the platform-independent context converts to the various raw pointer types as desired. This simplifies upcoming work where I wanted to return a `SendSyncPtr`.
This is part of the work to centralize memory management into the `mmap` module. This commit introduces a few structures which aid in that process, and starts converting one of the functions (`MemoryImageSource::map_at`) into this module. The structures introduced are: * `MemoryBase`: `RuntimeLinearMemory::base_ptr` is now `RuntimeLinearMemory::base`, which returns a `MemoryBase`. This is either a raw pointer or an `MmapOffset` as described below. * `MmapOffset`: A combination of a reference to an mmap and an offset into it. Logically represents a pointer into a mapped section of memory. In future work, we'll move more image-mapping code over to `Mmap` instances.
sunshowers
force-pushed
the
map-at-mmap-2
branch
from
d9d910e to
d24a2e7
Compare
| if VirtualProtect( | ||
| ret.as_send_sync_ptr().as_ptr().cast(), | ||
| ret.len(), | ||
| PAGE_WRITECOPY, | ||
| &mut old, | ||
| ) == 0 | ||
| { |
There was a problem hiding this comment.
Changed some of this code's pointer typology (though not the arithmetic) -- would be worth a look.
Fuzzing found a small issue with bytecodealliance#9687 and this commit relaxes a few off-by-one checks to allow addressing one-byte-beyond-the-end of a linear memory.
* Fix some off-by-one comparisons in assertions Fuzzing found a small issue with #9687 and this commit relaxes a few off-by-one checks to allow addressing one-byte-beyond-the-end of a linear memory. * Review comments
This is part of the work to centralize memory management into the
mmapmodule. This commit introduces a few structures which aid in that process, and
starts converting one of the functions (
MemoryImageSource::map_at) into thismodule.
The structures introduced are:
MemoryBase:RuntimeLinearMemory::base_ptris nowRuntimeLinearMemory::base, which returns aMemoryBase. This is either araw pointer or an
MmapOffsetas described below.MmapOffset: A combination of a reference to an mmap and an offset into it.Logically represents a pointer into a mapped section of memory.
In future work, we'll move more image-mapping code over to
Mmapinstances.