Wasmtime GitHub notifications bot (Jun 22 2021 at 14:24): Wasmtime GitHub notifications bot (Jun 22 2021 at 14:24):alexcrichton commented on issue #3013:
This makes me much more worried than the patch fix here implies I think, I would have figured that the solution would be to either change generated wasm code to load this field with native endianness or it would be to always store the field in little-endian. In any case though it definitely seems like wasm and native should at least agree on the size of the field?
How does the current patch fix tests? Would it be possible to implement a deeper fix?
Wasmtime GitHub notifications bot (Jun 22 2021 at 14:30):uweigand commented on issue #3013:
This makes me much more worried than the patch fix here implies I think, I would have figured that the solution would be to either change generated wasm code to load this field with native endianness or it would be to always store the field in little-endian. In any case though it definitely seems like wasm and native should at least agree on the size of the field?
The current code stores the field in native endianness as 8 bytes, and loads the field in native endianness as 4 bytes (taking the first 4 bytes of the 8 byte field). That works if native endian is little. If native endian is big, it works with this patch where the 4 bytes are now taken to be the last 4 bytes (instead of the first 4 bytes) of the 8 byte field.
I do not know why the sizes do not match, that is really the underlying problem, I'd say. I didn't try to fix this since this was already flagged as a TODO (with the test that the sized match commented out), so I assumed a fix would be non-trivial.
Wasmtime GitHub notifications bot (Jun 22 2021 at 14:32):alexcrichton commented on issue #3013:
Oh I see, yeah that makes sense how the fix works. I think though there's no fundamental reason why there's a disagreement here, so the best fix would be to align the sizes of the loads/stores between Rust & wasm
Wasmtime GitHub notifications bot (Jun 22 2021 at 14:34):uweigand commented on issue #3013:
Oh I see, yeah that makes sense how the fix works. I think though there's no fundamental reason why there's a disagreement here, so the best fix would be to align the sizes of the loads/stores between Rust & wasm
So what _should_ the size be? It seems Rust wants to use usize since that fits better into the Rust type system (natural type for lengths/sizes), while the wasm code wants to use u32 for more efficient guard code generation?
Wasmtime GitHub notifications bot (Jun 22 2021 at 14:39):alexcrichton commented on issue #3013:
I would naively say that the JIT code should guide us here since that's probably the most performance sensitive, but at the same time if this is a
u32then it also means that there's not actually support for 4GB memory if the value stored here needs to be the length of the memory + 1 (which I thought it was?)This means that we may need to store a
usizefor full 4gb memory support, but that might have a greater effect on the performance of jit code, I don't know how much theu32aspect is relied on for bounds checks.
Wasmtime GitHub notifications bot (Jun 22 2021 at 16:49):uweigand commented on issue #3013:
So from what I can see, the cranelift JIT makes the hard-coded assumption that the gv used as bound (which is where
current_lengthends up) must be of the same type as theindex_typeof the (dynamic) heap in question. And the index type of all heaps generated by current code (at least inmake_heapincrates/cranelift/src/func_environ.rs, not sure if there are other places) is always hardcoded totypes::I32. It does seem a bad idea to change this, as that would force all the index computations to be performed as 64-bit operations ...Of course, this means that the dynamic bound checks are currently simply wrong for a heap size of 4GB or more. However, it seems that the main use case of the 4GB heap (SpiderMonkey) uses the static heap type anyway?
On the other hand, I noticed the same
current_lengthfield also seems to used by the lightbeam back-end, which apparently treats it always as a 64-bit value if I understand the magicdynasm!macro correctly:
https://github.com/bytecodealliance/wasmtime/blob/main/crates/lightbeam/src/backend.rs#L1902All this makes me more hesitant to attempt to change this logic ...
Wasmtime GitHub notifications bot (Jun 22 2021 at 16:58):alexcrichton commented on issue #3013:
Ah ok that makes sense. Could this be updated to be a u32 stored, and an issue filed about how 4gb heaps have the wrong bounds checks?
Wasmtime GitHub notifications bot (Jun 22 2021 at 17:10):uweigand commented on issue #3013:
Ah ok that makes sense. Could this be updated to be a u32 stored, and an issue filed about how 4gb heaps have the wrong bounds checks?
But that would break lightbeam, right? The current code I pointed to above seems to rely on the value having been stored as u64 (with the high bits zero) ...
Wasmtime GitHub notifications bot (Jun 22 2021 at 17:20):alexcrichton commented on issue #3013:
Personally, I think that's ok. Lightbeam isn't tested at all and hasn't received maintenance in many months, I don't think this is the only part about it which is broken.
Wasmtime GitHub notifications bot (Jun 22 2021 at 17:51):uweigand commented on issue #3013:
I gave it a quick try. If current_length is u32, this will now cause an assertion to be raised whenever someone attempts to set the field to 4GB or larger. However, this already triggered in the test suite (in instance::linear_memory_limits) ...
Unfortunately, I don't think I can just ignore the assertion, since current_length is also used for length checks in Rust code, so even if the dynamic heap method is used (so the problem with JITed code doesn't occur), having a u32 current_length field would then cause those Rust checks to be incorrect.
Wasmtime GitHub notifications bot (Jun 22 2021 at 17:56):bjorn3 commented on issue #3013:
For wasm32 the linear memory can be at most 4GB as it uses 32bit pointers. Wasmtime doesn't yet support wasm64 and once it does will have to change codegen between wasm32 and wasm64 anyway as a static heap is impossible even on 64bit systems, so changing the current_length from 32bit to 64bit in that case shouldn't be much of a problem I think.
Wasmtime GitHub notifications bot (Jun 22 2021 at 18:00):alexcrichton commented on issue #3013:
Ah yes the test that specifically tries to grow to 4GB should be updated. It should grow to 4gb minus one page and then assert that growing the extra page fails. Basically Wasmtime is buggy right now with 4gb heaps so I don't think we should pretend they work by simply allowing the buggy code to also run on big-endian platforms, ideally we'd fix the issue outright here. We can have a documented limitation that Wasmtime supports 4gb heaps minus a page, and an issue tracking on fixing that limitation.
For length checks in Rust code I'm not sure I understand? I understand that
u32andusizeare different types but I would imagine that the conversion needs to happen and we'd handle it wherevercurrent_lengthwas read/written.
Wasmtime GitHub notifications bot (Jun 22 2021 at 18:09):uweigand commented on issue #3013:
My understanding is that a 4GB _dynamic_ heap is currently broken due to incorrect checks in JITed code. However, a 4GB _static_ heap works correctly (and is apparently used by SpiderMonkey if I'm reading the docs correctly).
In that latter case, current_length is currently set to 4GB, and that value is used in Rust code for various length checks. If we change current_length to a u32, it can no longer hold that value. If we instead set set u32 current_length to some other value, then the Rust length checks will be incorrect. If we simply disallow the 4GB case, then 4GB static heaps will also stop working, which I guess may break SpiderMonkey then ...
Wasmtime GitHub notifications bot (Jun 22 2021 at 18:13):alexcrichton commented on issue #3013:
Can you clarify what you mean by spidermonkey? Do you mean the Cranelift integration in SpiderMonkey? Or something else?
(I'm not aware what this is in reference to so hard to comment on breaking it...)
Wasmtime GitHub notifications bot (Jun 22 2021 at 18:16):uweigand commented on issue #3013:
I'm refering to this text: https://github.com/bytecodealliance/wasmtime/blob/main/cranelift/docs/ir.md#heap-examples
The SpiderMonkey VM prefers to use fixed heaps with a 4 GB bound and 2 GB of offset-guard pages when running WebAssembly code on 64-bit CPUs.
Wasmtime GitHub notifications bot (Jun 22 2021 at 18:24):alexcrichton commented on issue #3013:
Ah ok, changing this in Wasmtime would have no effect on that. This isn't really a cranelift-level fix but rather a Wasmtime-level fix.
Wasmtime GitHub notifications bot (Jun 22 2021 at 19:28):uweigand commented on issue #3013:
Ah ok, changing this in Wasmtime would have no effect on that. This isn't really a cranelift-level fix but rather a Wasmtime-level fix.
I see, makes sense.
I've now tried to implement this:
It should grow to 4gb minus one page and then assert that growing the extra page fails.
by setting WASM_MAX_PAGES to 65535 instead of 65536, but then spec tests start to fail as they use (memory 65536) -- is this required by the WebAssembly spec?
Wasmtime GitHub notifications bot (Jun 22 2021 at 20:29):alexcrichton commented on issue #3013:
Oh right yeah we don't want to change the type-level maximum, only the runtime-level maximum. Modules which declare a max size of 65536 should still validate, just that when they request that much memory we say "sorry, no go". This'll I think want to change
crates/runtime/src/memory.rsto rejectgrowandnewcalls with 65536 pages.
Wasmtime GitHub notifications bot (Jun 22 2021 at 22:25):uweigand commented on issue #3013:
Oh right yeah we don't want to change the type-level maximum, only the runtime-level maximum. Modules which declare a max size of 65536 should still validate, just that when they request that much memory we say "sorry, no go". This'll I think want to change
crates/runtime/src/memory.rsto rejectgrowandnewcalls with 65536 pages.That approach seems to work. With the updated patch all tests pass on s390x as well. Thanks!
Wasmtime GitHub notifications bot (Jun 22 2021 at 22:44):github-actions[bot] commented on issue #3013:
Subscribe to Label Action
cc @peterhuene
<details>
This issue or pull request has been labeled: "wasmtime:api"Thus the following users have been cc'd because of the following labels:
- peterhuene: wasmtime:api
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
Last updated: Oct 23 2024 at 20:03 UTC