sunfishcode commented on Issue #1237:
The part about this that makes me nervous is being able to access a module's memory without it being exported.
Obviously, requiring modules to export their memory is unfortunate, and the simple name "memory" isn't ideal, and we should do something better, but for now, it does help to model what's going on. You're calling out with
i32
values which are pointers to a memory accessed via the "memory" export. We don't want Wasmtime or any tools assuming that a non-exported memory isn't mutated by anything outside the module. The export also helps in the case of multiple memories, because you can pick which one to export as "memory".Would it still achieve your goal here if you reinstated the
lookup("memory")
logic infrom_abi
, keeping the rest of the feature?
lostman commented on Issue #1237:
Typically you'll have to set up an
Rc<RefCell<Option<Memory>>>
, close over that in your function imports, and then fill it in once the instance is created.When an instance is created it automatically runs the
(start)
function. If theRc<...>
was the only way to access memory then it wouldn't be possible to use a memory-accessing host call inside(start)
.Is that the intended behavior?
Alternatively, if one could make an instance, tie the knot, and only then explicitly run
(start)
, theRc<...>
approach would work.How would interface types solve this problem?
alexcrichton commented on Issue #1237:
@sunfishcode yeah we could definitely go that route, only giving access to exported memories. I would prefer to not hardcode the string
"memory"
, though, so we could switch to something like:Func::wrap1(&store, |mem: wasmtime::Caller| { let mem: Option<&Memory> = mem.get_export("memory").and_then(|e| e.memory()); // ... });That way the string
"memory"
would still be passed in by the caller, and we could enhanceCallerMemory
over time with other stuff if we really wanted. For now it'd just be a thin wrapper around*mut VMContext
.@lostman yes the current way to implement this today with
Rc
doesn't support calling imports in thestart
function, but this is basically the same limitation of the JS API and is one of the known caveats of thestart
function.Interface types would solve this issue because callers would simply say "I need a string", and it's the job of
wasmtime
to figure out how to give you that string, not the caller to figure out how to read the string out of linear memory.
sunfishcode commented on Issue #1237:
Passing in the "memory" string sounds good. Maybe we could also add a
pub const WASM_MEMORY_EXPORT_NAME = "memory"
or some such thing that we could recommend people use with this API, so that when we do change it, we have more options for helping them avoid silent breakage.
lostman commented on Issue #1237:
What about multiple memories? Is that anywhere on the road map? If it is, any API that relies on specific names will lead to trouble.
The current situation is already a bit messy. What if a module exports memory but calls it
"mem"
? I recall compiling C++ to Wasm while back and there was no option to chose the name. Not sure what the current state of Rust toolchain is either.
alexcrichton commented on Issue #1237:
@lostman with @sunfishcode's suggestion nothing will be hardcoded here any more, so we'll have an easy way to specify which memory is being accessed (via an exported name). I'm working on an update to the various traits here to support all this.
sunfishcode commented on Issue #1237:
As additional background, the "memory" export name is currently hard-wired into lld, and it's become a de-facto convention that engines assume now. This is obviously not ideal, but I am optimistic that we'll have chances to move away from programs exporting their entire memory to the world, which will be opportunities to fix this.
alexcrichton commented on Issue #1237:
I'm gonna end up taking a pretty different approach to doing this, so I'm going to close this.
alexcrichton commented on Issue #1237:
Ok I've followed up with https://github.com/bytecodealliance/wasmtime/pull/1290 which I think is a better way to tackle this, thanks for the suggestion @sunfishcode!
Last updated: Dec 23 2024 at 12:05 UTC