x64: Incorrect out-of-bounds access reported for a misaligned float load

[alexcrichton]

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 the f32.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]

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:

wasmtime/cranelift/codegen/src/isa/x64/lower/isle.rs

Lines 169 to 175 in 2986f6b

	if let InputSourceInst::UniqueUse(src_insn, 0) = inputs.inst {
	if let Some((addr_input, offset)) = is_mergeable_load(self.lower_ctx, src_insn) {
	self.lower_ctx.sink_inst(src_insn);
	let amode = lower_to_amode(self.lower_ctx, addr_input, offset);
	return XmmMem::new(RegMem::mem(amode)).unwrap();
	}
	}

[cfallin]

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]

I was just experimenting with that fix and was going to suggest it :) I'll make a PR!

[bjorn3]

In cg_clif we do almost always know an alignment bigger than 1.

[cfallin]

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]

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?

wasmtime/cranelift/codegen/src/isa/x64/lower.rs

Lines 107 to 111 in 2986f6b

	// SIMD instructions can only be load-coalesced when the loaded value comes
	// from an aligned address.
	if load_ty.is_vector() && !insn_data.memflags().map_or(false, |f| f.aligned()) {
	return None;
	}

[elliottt]

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 for load_ty.is_vector().

[cfallin]

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]

For now, never merging the loads is obviously correct, and we can always revisit this later :)

[abrown]

Perhaps there should be an issue to revisit this later? @elliottt brought up what seemed to me like good avenues for fixing this.

[alexcrichton]

@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]

Absolutely, I'll make a PR today and send it to you for review.