Stream: git-wasmtime

Topic: wasmtime / Issue #2875 Cranelift: store with iadd i32 add...

Wasmtime GitHub notifications bot (May 05 2021 at 12:26):

Mrmaxmeier edited Issue #2875:

I'm not sure what causes this, so I'll stick to a generic title..

.clif Test Case

function u0:1(i64 vmctx) {
    gv0 = vmctx
    heap0 = static gv0

block0(v3: i64):
    v1 = iconst.i32 0
    v2 = iadd v1, v1
    store little v1, v2
    return
}

Steps to Reproduce

⋊> cargo run --manifest-path=cranelift/Cargo.toml -- compile --target=x86_64 /tmp/test.clif
    Finished dev [unoptimized + debuginfo] target(s) in 0.08s
     Running `target/debug/clif-util compile --target=x86_64 /tmp/test.clif`
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `types::I32`,
 right: `types::I64`', cranelift/codegen/src/isa/x64/lower.rs:1289:9
stack backtrace:
   0: rust_begin_unwind
   [...]
   3: core::panicking::assert_failed
   4: cranelift_codegen::isa::x64::lower::lower_to_amode
             at ./cranelift/codegen/src/isa/x64/lower.rs:1289:9
   5: cranelift_codegen::isa::x64::lower::lower_insn_to_regs
             at ./cranelift/codegen/src/isa/x64/lower.rs:4805:21
   6: cranelift_codegen::isa::x64::lower::<impl cranelift_codegen::machinst::lower::LowerBackend for cranelift_codegen::isa::x64::X64Backend>::lower
             at ./cranelift/codegen/src/isa/x64/lower.rs:5755:9
   7: cranelift_codegen::machinst::lower::Lower<I>::lower_clif_block
             at ./cranelift/codegen/src/machinst/lower.rs:736:17
   8: cranelift_codegen::machinst::lower::Lower<I>::lower
             at ./cranelift/codegen/src/machinst/lower.rs:973:17
   9: cranelift_codegen::machinst::compile::compile
             at ./cranelift/codegen/src/machinst/compile.rs:30:9
  10: cranelift_codegen::isa::x64::X64Backend::compile_vcode
             at ./cranelift/codegen/src/isa/x64/mod.rs:50:9
  11: <cranelift_codegen::isa::x64::X64Backend as cranelift_codegen::machinst::MachBackend>::compile_function
             at ./cranelift/codegen/src/isa/x64/mod.rs:61:21
  12: cranelift_codegen::context::Context::compile
             at ./cranelift/codegen/src/context.rs:197:26
  13: cranelift_codegen::context::Context::compile_and_emit
             at ./cranelift/codegen/src/context.rs:137:20
  14: clif_util::compile::handle_module
             at ./cranelift/src/compile.rs:81:29
  15: clif_util::compile::run
             at ./cranelift/src/compile.rs:52:9
  16: clif_util::main
             at ./cranelift/src/clif-util.rs:130:33

Versions and Environment

Cranelift version or commit: 0.73.0 and current main

Operating system: Linux

Architecture: x86_64

Extra Info

A quick git bisect takes me to cb48ea406eea535b3108853b7b680d9aa21eb14b. That's probably not too useful though.

Wasmtime GitHub notifications bot (May 05 2021 at 12:33):

Mrmaxmeier commented on Issue #2875:

Right, thanks! Seems like I'm missing a few heap_addrs in my codegen.
It seems like 32bit pointers are supported / extended when not in the form of adds though.

Wasmtime GitHub notifications bot (May 05 2021 at 12:34):

Mrmaxmeier edited a comment on Issue #2875:

Right, thanks! Seems like I'm missing a few heap_addrs in my codegen.
It looks like 32bit pointers are supported / extended when not in the form of adds though.

Wasmtime GitHub notifications bot (May 05 2021 at 12:46):

Mrmaxmeier commented on Issue #2875:

Here's a crude patch that disables the special handling of non-i64 iadd pointers:

diff --git a/cranelift/codegen/src/isa/x64/lower.rs b/cranelift/codegen/src/isa/x64/lower.rs
index 6f675b923..ba00b8a61 100644
--- a/cranelift/codegen/src/isa/x64/lower.rs
+++ b/cranelift/codegen/src/isa/x64/lower.rs
@@ -1286,6 +1286,7 @@ fn lower_to_amode<C: LowerCtx<I = Inst>>(ctx: &mut C, spec: InsnInput, offset: i
     // We now either have an add that we must materialize, or some other input; as well as the
     // final offset.
     if let Some(add) = matches_input(ctx, spec, Opcode::Iadd) {
+        if ctx.output_ty(add, 0) == types::I64 {
         debug_assert_eq!(ctx.output_ty(add, 0), types::I64);
         let add_inputs = &[
             InsnInput {
@@ -1359,6 +1360,7 @@ fn lower_to_amode<C: LowerCtx<I = Inst>>(ctx: &mut C, spec: InsnInput, offset: i
         };

         return Amode::imm_reg_reg_shift(offset as u32, base, index, shift).with_flags(flags);
+        }
     }

     let input = put_input_in_reg(ctx, spec);

Wasmtime GitHub notifications bot (May 05 2021 at 12:50):

bjorn3 commented on Issue #2875:

Non-64bit pointers shouldn't work at all with load and store on 64bit systems. They are fine with heap_load and heap_store if the heap is itself declared as 32bit.

Wasmtime GitHub notifications bot (May 05 2021 at 12:51):

Mrmaxmeier edited a comment on Issue #2875:

Right, thanks! Seems like I'm missing a few heap_addrs in my (untested..) codegen. I got the order of the store arguments wrong.
It looks like 32bit pointers are supported / extended when not in the form of adds though.

Wasmtime GitHub notifications bot (May 05 2021 at 22:12):

cfallin commented on Issue #2875:

@Mrmaxmeier, thanks for this report! As @bjorn3 noted, the fundamental issue is the use of 32-bit pointer types here (on a 64-bit platform); rather than try to make it somehow work, I think the right answer is to reject the type mismatch. In particular, depending on the OS and system configuration, it might not even be possible to represent a valid pointer in 32 bits; e.g. with ASLR it's unlikely that a native pointer will be in the low 4GB of the address space. So such a pattern in CLIF is almost certainly a bug and I think it would be safer to detect it and panic explicitly.

The developer-UX you experienced is certainly suboptimal -- we should have a proper panic/error message here that indicates what's wrong. I think we can use this issue to track that -- thanks!

Last updated: Nov 22 2024 at 16:03 UTC