Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cranelift: incorrect narrow sdiv lowering on AArch64 #9537

Closed
mmcloughlin opened this issue Oct 31, 2024 · 0 comments · Fixed by #9541
Closed

Cranelift: incorrect narrow sdiv lowering on AArch64 #9537

mmcloughlin opened this issue Oct 31, 2024 · 0 comments · Fixed by #9541
Labels
bug Incorrect behavior in the current implementation that needs fixing cranelift Issues related to the Cranelift code generator

Comments

@mmcloughlin
Copy link
Contributor

mmcloughlin commented Oct 31, 2024

There may be a bug in the lowering of narrow sdiv instructions on AArch64.

.clif Test Case

function %div8(i8, i8) -> i8 {
block0(v0: i8, v1: i8):
  v2 = sdiv v0, v1
  return v2
}

; run: %div8(-128, -1) == -128

Note: I believe this would affect 16-bit sdiv as well, though I have not tested this case as thoroughly.

Steps to Reproduce

Run test case in clif-util via the interpreter and JIT on an AArch64 platform.

$ clif-util interpret sdiv.clif
$ clif-util run sdiv.clif

Expected Results

The CLIF execution should trap, via interpreter and JIT.

Actual Results

In the interpreter, we get Unexpected returned control flow which I confirmed with a small edit is actually a trap:

$ clif-util interpret --verbose sdiv.clif
thread 'main' panicked at cranelift/src/interpret.rs:129:34:
Unexpected returned control flow--this is likely a bug.
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

JIT passes the test case:

$ clif-util run --verbose sdiv.clif
sdiv.clif
1 file

Versions and Environment

Cranelift version or commit: recent main branch commit a82bdd833d1787953b866b2c375832dd9b911f1b

Operating system: macOS 15.0.1

Architecture: AArch64 (Apple M1)

Extra Info

Diagnosis

I believe the problem lies in the trap_if_div_overflow rule:

;; Check LHS is min_value, by subtracting 1 and branching if
;; there is overflow.
(_ Unit (emit (MInst.CCmpImm (size_from_ty ty)
x
(u8_into_uimm5 1)
(nzcv $false $false $false $false)
(Cond.Eq))))
(_ Unit (emit (MInst.TrapIf (cond_br_cond (Cond.Vs))
(trap_code_integer_overflow))))

This code is checking for the minimum 32-bit signed value, but this is not correct for the 8/16-bit cases. Subtracting 1 from -128 as a 32-bit value does not cause overflow, therefore the trap on the Vs condition does not fire.

Security

Discussed with @cfallin on Zulip who confirmed this is not a security-critical miscompile.

@mmcloughlin mmcloughlin added bug Incorrect behavior in the current implementation that needs fixing cranelift Issues related to the Cranelift code generator labels Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Incorrect behavior in the current implementation that needs fixing cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant