alexcrichton opened issue #4890:
This input module:
(module (func (param i32) (result f32) f32.const 0 local.get 0 f32.load offset=1 f32.copysign ) (memory 1) (export "" (func 0)) )
yields:
$ cargo run -q testcase0.shrunken.wat --invoke '' 0 warning: using `--invoke` with a function that takes arguments is experimental and may break in the future Error: failed to run main module `testcase0.shrunken.wat` Caused by: 0: failed to invoke `` 1: wasm trap: out of bounds memory access wasm backtrace: 0: 0x2e - <unknown>!<wasm function 0>
but the trap reported here is incorrect because there is no out-of-bounds memory access in this module. Instead what's happening is that in the compile function:
0000000000000000 <_wasm_function_0>: 0: 55 push %rbp 1: 48 89 e5 mov %rsp,%rbp 4: 8b c2 mov %edx,%eax 6: 48 8b 4f 50 mov 0x50(%rdi),%rcx a: 41 ba 00 00 00 80 mov $0x80000000,%r10d 10: 66 45 0f 6e da movd %r10d,%xmm11 15: 66 41 0f 6f c3 movdqa %xmm11,%xmm0 1a: 0f 55 05 0f 00 00 00 andnps 0xf(%rip),%xmm0 # 30 <_wasm_function_0+0x30> 21: 44 0f 54 5c 01 01 andps 0x1(%rcx,%rax,1),%xmm11 27: 41 0f 56 c3 orps %xmm11,%xmm0 2b: 48 89 ec mov %rbp,%rsp 2e: 5d pop %rbp 2f: c3 retq
The instruction at 0x21 is segfaulting due to a misaligned address. The segfault is also registered as a trap point in Wasmtime since I believe this is a folding of the
f32.load
into thef32.copysign
and so this could also segfault due to an out-of-bounds memory access.Bisection reveals that this issue become a segfault in #4730 and then became a trap in #4790 (cc @elliottt). We'll want to be sure to backport the fix for this to the release-1.0.0 branch as well.
alexcrichton labeled issue #4890:
This input module:
(module (func (param i32) (result f32) f32.const 0 local.get 0 f32.load offset=1 f32.copysign ) (memory 1) (export "" (func 0)) )
yields:
$ cargo run -q testcase0.shrunken.wat --invoke '' 0 warning: using `--invoke` with a function that takes arguments is experimental and may break in the future Error: failed to run main module `testcase0.shrunken.wat` Caused by: 0: failed to invoke `` 1: wasm trap: out of bounds memory access wasm backtrace: 0: 0x2e - <unknown>!<wasm function 0>
but the trap reported here is incorrect because there is no out-of-bounds memory access in this module. Instead what's happening is that in the compile function:
0000000000000000 <_wasm_function_0>: 0: 55 push %rbp 1: 48 89 e5 mov %rsp,%rbp 4: 8b c2 mov %edx,%eax 6: 48 8b 4f 50 mov 0x50(%rdi),%rcx a: 41 ba 00 00 00 80 mov $0x80000000,%r10d 10: 66 45 0f 6e da movd %r10d,%xmm11 15: 66 41 0f 6f c3 movdqa %xmm11,%xmm0 1a: 0f 55 05 0f 00 00 00 andnps 0xf(%rip),%xmm0 # 30 <_wasm_function_0+0x30> 21: 44 0f 54 5c 01 01 andps 0x1(%rcx,%rax,1),%xmm11 27: 41 0f 56 c3 orps %xmm11,%xmm0 2b: 48 89 ec mov %rbp,%rsp 2e: 5d pop %rbp 2f: c3 retq
The instruction at 0x21 is segfaulting due to a misaligned address. The segfault is also registered as a trap point in Wasmtime since I believe this is a folding of the
f32.load
into thef32.copysign
and so this could also segfault due to an out-of-bounds memory access.Bisection reveals that this issue become a segfault in #4730 and then became a trap in #4790 (cc @elliottt). We'll want to be sure to backport the fix for this to the release-1.0.0 branch as well.
elliottt commented on issue #4890:
It seems like we want a stronger condition in
put_in_xmm_mem
, as we need to know that it's a mergeable load and 16-byte aligned to merge the load:
cfallin commented on issue #4890:
I sort of suspect that we will find most loads encountered by
put_in_xmm_mem
won't have any known alignment (greater than 1 byte); at least, in CLIF lowered from Wasm, that will be the case, since Wasm SIMD loads must work with unaligned addresses. The simplest fix here may just be to remove load-op merging in the XMM case, and always force to a register (i.e., delete exactly the lines you highlighted). Thoughts?
elliottt commented on issue #4890:
I was just experimenting with that fix and was going to suggest it :) I'll make a PR!
bjorn3 commented on issue #4890:
In cg_clif we do almost always know an alignment bigger than 1.
cfallin commented on issue #4890:
In cg_clif we do almost always know an alignment bigger than 1.
Fair enough; we can eventually look at adding this back, while respecting alignment, but IMHO we should fix the fuzzbug first in the most straightforward way, especially given we want to cherry-pick this to the 1.0 branch too :-)
elliottt commented on issue #4890:
Here's another option: in
is_mergable_load
we currently only require alignment of the load if the type being loaded is a vector type. Why don't we extend this to floating point types as well?
elliottt edited a comment on issue #4890:
Here's another option: in
is_mergable_load
we currently only require the address to be aligned if the type being loaded is a vector type. Why don't we extend this to floating point types as well?
elliottt commented on issue #4890:
We could also pass a flag to
is_mergeable_load
that indicates if the destination would be an xmm register, and use that in place of the check forload_ty.is_vector()
.
cfallin commented on issue #4890:
Possibly, yeah, though it's less obviously clear to me that this covers all cases (e.g. what if some lowering were to somehow load an integer-typed value into an XMM register?). The proximate cause here is that loads into XMM registers merged into SSE instructions need to be aligned, so I'd prefer to take the more risk-averse option for the initial fix and turn that path off altogether, I think...
elliottt commented on issue #4890:
For now, never merging the loads is obviously correct, and we can always revisit this later :)
elliottt closed issue #4890:
This input module:
(module (func (param i32) (result f32) f32.const 0 local.get 0 f32.load offset=1 f32.copysign ) (memory 1) (export "" (func 0)) )
yields:
$ cargo run -q testcase0.shrunken.wat --invoke '' 0 warning: using `--invoke` with a function that takes arguments is experimental and may break in the future Error: failed to run main module `testcase0.shrunken.wat` Caused by: 0: failed to invoke `` 1: wasm trap: out of bounds memory access wasm backtrace: 0: 0x2e - <unknown>!<wasm function 0>
but the trap reported here is incorrect because there is no out-of-bounds memory access in this module. Instead what's happening is that in the compile function:
0000000000000000 <_wasm_function_0>: 0: 55 push %rbp 1: 48 89 e5 mov %rsp,%rbp 4: 8b c2 mov %edx,%eax 6: 48 8b 4f 50 mov 0x50(%rdi),%rcx a: 41 ba 00 00 00 80 mov $0x80000000,%r10d 10: 66 45 0f 6e da movd %r10d,%xmm11 15: 66 41 0f 6f c3 movdqa %xmm11,%xmm0 1a: 0f 55 05 0f 00 00 00 andnps 0xf(%rip),%xmm0 # 30 <_wasm_function_0+0x30> 21: 44 0f 54 5c 01 01 andps 0x1(%rcx,%rax,1),%xmm11 27: 41 0f 56 c3 orps %xmm11,%xmm0 2b: 48 89 ec mov %rbp,%rsp 2e: 5d pop %rbp 2f: c3 retq
The instruction at 0x21 is segfaulting due to a misaligned address. The segfault is also registered as a trap point in Wasmtime since I believe this is a folding of the
f32.load
into thef32.copysign
and so this could also segfault due to an out-of-bounds memory access.Bisection reveals that this issue become a segfault in #4730 and then became a trap in #4790 (cc @elliottt). We'll want to be sure to backport the fix for this to the release-1.0.0 branch as well.
abrown commented on issue #4890:
Perhaps there should be an issue to revisit this later? @elliottt brought up what seemed to me like good avenues for fixing this.
alexcrichton commented on issue #4890:
@elliottt also would you be up for backporting this to the release-1.0.0 branch? If not no worries and I can do that as well.
elliottt commented on issue #4890:
Absolutely, I'll make a PR today and send it to you for review.
Last updated: Dec 23 2024 at 12:05 UTC