From b429f77ee9e8b602950546f37ce07e0e42a08ae9 Mon Sep 17 00:00:00 2001 From: Chris Fallin Date: Wed, 24 Mar 2021 22:15:36 -0700 Subject: [PATCH] Handle `srem` properly when `avoid_div_traps` is false. The codegen for div/rem ops has two modes, depending on the `avoid_div_traps` flag: it can either do all checks for trapping conditions explicitly, and use explicit trap instructions, then use a hardware divide instruction that will not trap (`avoid_div_traps == true`); or it can run in a mode where a hardware FP fault on the divide instruction implies a Wasm trap (`avoid_div_traps == false`). Wasmtime uses the former while Lucet (for example) uses the latter. It turns out that because we run all our spec tests run under Wasmtime, we missed a spec corner case that fails in the latter: INT_MIN % -1 == 0 per the spec, but causes a trap with the x86 signed divide/remainder instruction. Hence, in Lucet, this specific remainder computation would incorrectly result in a Wasm trap. This PR fixes the issue by just forcing use of the explicit-checks implementation for `srem` even when `avoid_div_traps` is false. --- cranelift/codegen/src/isa/x64/inst/emit.rs | 1 - cranelift/codegen/src/isa/x64/lower.rs | 3 ++- .../filetests/isa/x64/div-checks-run.clif | 12 ++++++++++++ .../filetests/isa/x64/div-checks.clif | 19 +++++++++++++++++++ 4 files changed, 33 insertions(+), 2 deletions(-) create mode 100644 cranelift/filetests/filetests/isa/x64/div-checks-run.clif create mode 100644 cranelift/filetests/filetests/isa/x64/div-checks.clif diff --git a/cranelift/codegen/src/isa/x64/inst/emit.rs b/cranelift/codegen/src/isa/x64/inst/emit.rs index 2e8e7d9d1528..1186b4f4a735 100644 --- a/cranelift/codegen/src/isa/x64/inst/emit.rs +++ b/cranelift/codegen/src/isa/x64/inst/emit.rs @@ -887,7 +887,6 @@ pub(crate) fn emit( // idiv %divisor // // $done: - debug_assert!(info.flags().avoid_div_traps()); // Check if the divisor is zero, first. let inst = Inst::cmp_rmi_r(*size, RegMemImm::imm(0), divisor.to_reg()); diff --git a/cranelift/codegen/src/isa/x64/lower.rs b/cranelift/codegen/src/isa/x64/lower.rs index 190462caaf7e..1ac9806055af 100644 --- a/cranelift/codegen/src/isa/x64/lower.rs +++ b/cranelift/codegen/src/isa/x64/lower.rs @@ -5181,7 +5181,8 @@ fn lower_insn_to_regs>( input_ty, )); - if flags.avoid_div_traps() { + // Always do explicit checks for `srem`: otherwise, INT_MIN % -1 is not handled properly. + if flags.avoid_div_traps() || op == Opcode::Srem { // A vcode meta-instruction is used to lower the inline checks, since they embed // pc-relative offsets that must not change, thus requiring regalloc to not // interfere by introducing spills and reloads. diff --git a/cranelift/filetests/filetests/isa/x64/div-checks-run.clif b/cranelift/filetests/filetests/isa/x64/div-checks-run.clif new file mode 100644 index 000000000000..615e214bcc0e --- /dev/null +++ b/cranelift/filetests/filetests/isa/x64/div-checks-run.clif @@ -0,0 +1,12 @@ +test run +set avoid_div_traps=false +target x86_64 +feature "experimental_x64" + +function %f0(i32, i32) -> i32 { +block0(v0: i32, v1: i32): + v2 = srem.i32 v0, v1 + return v2 +} + +; run: %f0(0x80000000, 0xffffffff) == 0 diff --git a/cranelift/filetests/filetests/isa/x64/div-checks.clif b/cranelift/filetests/filetests/isa/x64/div-checks.clif new file mode 100644 index 000000000000..0faa59bc9fbc --- /dev/null +++ b/cranelift/filetests/filetests/isa/x64/div-checks.clif @@ -0,0 +1,19 @@ +test compile +set avoid_div_traps=false +target x86_64 +feature "experimental_x64" + +;; We should get the checked-div/rem sequence (`srem` pseudoinst below) even +;; when `avoid_div_traps` above is false (i.e. even when the host is normally +;; willing to accept SIGFPEs as Wasm traps). The machine will SIGFPE in some +;; cases when `srem` is valid (specifically -INT_MIN % -1). +function %f0(i64, i64) -> i64 { +block0(v0: i64, v1: i64): + v2 = srem.i64 v0, v1 +; check: movq %rdi, %rax +; nextln: movl $$0, %edx +; nextln: srem $$rax:$$rdx, %rsi +; nextln: movq %rdx, %rax + + return v2 +}