Stream: git-wasmtime

Topic: wasmtime / issue #4513 aarch64: Wasm module tripping `ass...

Wasmtime GitHub notifications bot (Jul 22 2022 at 19:52):

alexcrichton opened issue #4513:

Given this input module:

(module
  (type (;0;) (func (param i64)))
  (func (;0;) (type 0) (param i64)
    (local f64 f64 i32)
    i32.const 0
    if  ;; label = @1
      unreachable
    end
    i32.const 0
    i32.const -1
    i32.rem_u
    f64.convert_i32_s
    f64.neg
    f64.neg
    drop
  )
)

this currently panics in codegen with:

$ cargo run -q compile foo.wat
thread '<unnamed>' panicked at 'assertion failed: first_mov_emitted', cranelift/codegen/src/isa/aarch64/lower/isle.rs:221:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

cc @akirilov-arm

Wasmtime GitHub notifications bot (Jul 22 2022 at 19:52):

alexcrichton labeled issue #4513:

Given this input module:

(module
  (type (;0;) (func (param i64)))
  (func (;0;) (type 0) (param i64)
    (local f64 f64 i32)
    i32.const 0
    if  ;; label = @1
      unreachable
    end
    i32.const 0
    i32.const -1
    i32.rem_u
    f64.convert_i32_s
    f64.neg
    f64.neg
    drop
  )
)

this currently panics in codegen with:

$ cargo run -q compile foo.wat
thread '<unnamed>' panicked at 'assertion failed: first_mov_emitted', cranelift/codegen/src/isa/aarch64/lower/isle.rs:221:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

cc @akirilov-arm

Wasmtime GitHub notifications bot (Jul 22 2022 at 19:55):

alexcrichton commented on issue #4513:

Shrunken slightly:

(module
  (func (param i32) (result i32)
    local.get 0
    i32.const -1
    i32.rem_u
  )
)

Wasmtime GitHub notifications bot (Jul 22 2022 at 22:18):

cfallin commented on issue #4513:

This patch works locally for me with the above (and passes tests; to pass some tests the "match u32::MAX or u64::MAX for bits == 32" logic was necessary):

diff --git a/cranelift/codegen/src/isa/aarch64/lower/isle.rs b/cranelift/codegen/src/isa/aarch64/lower/isle.rs
index fa4556f03..bd1c50fd9 100644
--- a/cranelift/codegen/src/isa/aarch64/lower/isle.rs
+++ b/cranelift/codegen/src/isa/aarch64/lower/isle.rs
@@ -157,7 +157,9 @@ where
                 size: OperandSize::Size64,
             });
             return rd.to_reg();

-        } else if value == u64::MAX {
+        } else if (bits == 64 && value == u64::MAX)
+            || (bits == 32 && (value & u32::MAX as u64) == u32::MAX as u64)
+        {
             self.emit(&MInst::MovWide {
                 op: MoveWideOp::MovN,
                 rd,

@akirilov-arm I'm not sure if we fully grok the invariants here though (especially re: extensions), so could you check the above? The code was introduced as part of the recent CVE fix I believe.

Wasmtime GitHub notifications bot (Jul 25 2022 at 12:03):

akirilov-arm commented on issue #4513:

@cfallin I believe that you have pinpointed the problematic spot precisely, but I think that there is a further improvement to be made (actually a simplification to make it easier to reason about the code), so I am working on a fix.

Wasmtime GitHub notifications bot (Jul 25 2022 at 19:14):

akirilov-arm edited a comment on issue #4513:

@cfallin I believe that you have pinpointed the problematic spot precisely, but I think that there is a further improvement to be made (actually a simplification to make it easier to reason about the code), so I am working on a fix.

P.S. srem and urem tests are also severely lacking.

Wasmtime GitHub notifications bot (Jul 25 2022 at 19:19):

akirilov-arm labeled issue #4513:

Given this input module:

(module
  (type (;0;) (func (param i64)))
  (func (;0;) (type 0) (param i64)
    (local f64 f64 i32)
    i32.const 0
    if  ;; label = @1
      unreachable
    end
    i32.const 0
    i32.const -1
    i32.rem_u
    f64.convert_i32_s
    f64.neg
    f64.neg
    drop
  )
)

this currently panics in codegen with:

$ cargo run -q compile foo.wat
thread '<unnamed>' panicked at 'assertion failed: first_mov_emitted', cranelift/codegen/src/isa/aarch64/lower/isle.rs:221:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

cc @akirilov-arm

Wasmtime GitHub notifications bot (Jul 25 2022 at 19:19):

akirilov-arm labeled issue #4513:

Given this input module:

(module
  (type (;0;) (func (param i64)))
  (func (;0;) (type 0) (param i64)
    (local f64 f64 i32)
    i32.const 0
    if  ;; label = @1
      unreachable
    end
    i32.const 0
    i32.const -1
    i32.rem_u
    f64.convert_i32_s
    f64.neg
    f64.neg
    drop
  )
)

this currently panics in codegen with:

$ cargo run -q compile foo.wat
thread '<unnamed>' panicked at 'assertion failed: first_mov_emitted', cranelift/codegen/src/isa/aarch64/lower/isle.rs:221:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

cc @akirilov-arm

Wasmtime GitHub notifications bot (Jul 26 2022 at 16:35):

cfallin closed issue #4513:

Given this input module:

(module
  (type (;0;) (func (param i64)))
  (func (;0;) (type 0) (param i64)
    (local f64 f64 i32)
    i32.const 0
    if  ;; label = @1
      unreachable
    end
    i32.const 0
    i32.const -1
    i32.rem_u
    f64.convert_i32_s
    f64.neg
    f64.neg
    drop
  )
)

this currently panics in codegen with:

$ cargo run -q compile foo.wat
thread '<unnamed>' panicked at 'assertion failed: first_mov_emitted', cranelift/codegen/src/isa/aarch64/lower/isle.rs:221:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

cc @akirilov-arm

Last updated: Nov 22 2024 at 16:03 UTC