Emit check for memory intrinsics for WebAssembly #16345
Conversation
There was a problem hiding this comment.
According to the LLVM langref, both memset and memcpy have this:
If is 0, it is no-op modulo the behavior of attributes attached to the arguments. If is not a well-defined value, the behavior is undefined. If is not zero, should be well-defined, otherwise the behavior is undefined.
This means we are actually emitting correct LLVM code, and this is an upstream bug in LLVM, which needs to do this logic for wasm.
I would ask that you please work with LLVM maintainers upstream before adding this workaround in the Zig LLVM backend.
|
I'll be filing a ticket upstream, this weekend, and linking it here when done so. I expect it to take some time to be fixed upstream and before it eventually lands in Zig when we upgrade to the next LLVM version. Are we willing to carry this workaround until then? If not, I'll close this PR and only focus on the upstream ticket. |
|
Yes, in general, the policy is to carry LLVM workarounds along with a link to an upstream bug report so that we can follow-up on the workaround in the future. |
|
Understood, I'll also open an issue in the Zig repository then so we can track the upstream ticket. |
Luukdegram
changed the title
When lowering the `memcpy` instruction, LLVM will lower it to WebAssembly's `memory.copy` instruction when the bulk-memory feature is enabled. This instruction will trap when the destination or source pointer is out-of-bounds. By Zig's semantics, it is valid to have an invalid pointer when the length is 0. To prevent runtimes from trapping, we add a safety-check for slices to only lower to a memcpy instruction when the length is larger than 0.
When lowering the `memset` instruction, LLVM will lower it to WebAssembly's `memory.fill` instruction when the bulk-memory feature is enabled. This instruction will trap when the destination address is out-of-bounds. By Zig's semantics, it is valid to have an invalid pointer when the length is 0. To prevent runtimes from trapping, we add a safety-check for slices to only lower to a memset instruction when the length is larger than 0.
|
Also added a link to the issue in the codebase so we don't forget to remove the logic once #16360 is closed. |
This adds a standalone test case to ensure the runtime does not trap when performing a memory.copy or memory.fill instruction while the destination or source address is out-of-bounds and the length is 0.
|
Merging with the premise the check will be removed once the linked issue above has been resolved. |
Originally, in WebAssembly, a zero-length
memory.copyormemory.fillwould not result in a trap for zero-length out-of-bounds operations. Unfortunately, that decision was reverted, and now any out-of-bounds memory operations regardless of length will trap instead. Attempts were made to convince the committee to not trap on zero-length operations, but due to simplicity reasons, the status quo remains.The intended semantics of Zig is to not panic/UB for
@memcpyand@memsetwhen the length is 0. For this reason, this PR adds a safety check for the WebAssembly target to verify the length is non-zero. This check is only emitted when the CPU featurebulk-memoryis enabled.This PR is a prerequisite to getting all tests passing for the upcoming multi-thread test enablement for WASI. (Due to multi-threading requiring the feature
bulk-memoryto be enabled). For this reason, also, a standalone test was added as we do not yet have a test matrix with additional CPU features enabled. (This will be added in a follow-up PR, in which I will move this test case to a behavior test instead).Closes #15920
For now, this only emits the check in the LLVM backend.