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

thumbv6m-none-eabi compiler builtin ABI bug #39056

Closed
dnseitz opened this issue Jan 14, 2017 · 6 comments
Closed

thumbv6m-none-eabi compiler builtin ABI bug #39056

dnseitz opened this issue Jan 14, 2017 · 6 comments
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state P-low Low priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@dnseitz
Copy link

dnseitz commented Jan 14, 2017

I'm helping to build a small operating system to run on Cortex-M0 processors. I'm compiling for the thumbv6m-none-eabi target. When using the compiler builtin function __aeabi_lmul it seems that the compiler is passing in the arguments to the wrong registers. For example, this piece of code:

fn do_mul(a: isize, b: isize) -> isize {
  a * b
}

Gets compiled down to:

   0:   b580            push    {r7, lr}
   2:   af00            add     r7, sp, #0
   4:   b088            sub     sp, #32
   6:   460a            mov     r2, r1
   8:   4603            mov     r3, r0
   a:   9004            str     r0, [sp, #16]
   c:   9105            str     r1, [sp, #20]
   e:   9203            str     r2, [sp, #12]
  10:   9302            str     r3, [sp, #8]
  12:   e7ff            b.n     14 <lmul_test::do_a_mul::hb8c09fc6501fea17+0x14>
  14:   9804            ldr     r0, [sp, #16]
  16:   9006            str     r0, [sp, #24]
  18:   9805            ldr     r0, [sp, #20]
  1a:   9007            str     r0, [sp, #28]
  1c:   9906            ldr     r1, [sp, #24]
  1e:   17c2            asrs    r2, r0, #31
  20:   17cb            asrs    r3, r1, #31
  22:   9001            str     r0, [sp, #4]
  24:   4608            mov     r0, r1
  26:   4611            mov     r1, r2
  28:   9a01            ldr     r2, [sp, #4]
  2a:   f7ff fffe       bl      0 <__aeabi_lmul>
  ...

According the the ARM ABI Documentation (Section 5.1.1.1), passing a 64bit value as an argument should store them in registers r0 and r1 or r2 and r3 as a pair. With the low bits being stored in r0/2 and the high bits stored in r1/3.

Looking at the dissasembled code, the first argument, which was initially stored in r0 gets its low and high bits split into r0 and r3 respectively, not the expected r0 and r1. The second argument also gets split into r2 and r1 for its low and high bits. So it appears that the high bits for the two values have been swapped.

Because the values are sign extended from 32 bits, this is only an issue if the signs of the two arguments differ. If they're the same the high bit registers will happen to be the same. But when the signs differ the function returns a wrong result in the r1 register. This causes a panic if overflow checking is enabled.

I've not noticed this issue anywhere else, but it could possibly be hidden around in some of the other compiler builtins.

I've created a sample repo here that shows off the bug. You'll need an ARM toolchain to actually compile it down to a binary, but you should still be able to cross compile it into a static library. (the command I was using was xargo build --target=thumbv6m-none-eabi)

Version info:

lmul-test|master ⇒ rustc --version --verbose 
rustc 1.16.0-nightly (47c8d9fdc 2017-01-08)
binary: rustc
commit-hash: 47c8d9fdcf2e6502cf4ca7d7f059fdc1a2810afa
commit-date: 2017-01-08
host: x86_64-apple-darwin
release: 1.16.0-nightly
LLVM version: 3.9
@kaini
Copy link

kaini commented Mar 22, 2017

I was recently hit by this bug as well and after further investigation I think that this might be a LLVM bug. I was able to reduce your test case to the following standalone LLVM-IR:

define i32 @invalid_call(i32, i32) {
  %3 = call { i32, i1 } @llvm.smul.with.overflow.i32(i32 %0, i32 %1)
  %4 = extractvalue { i32, i1 } %3, 0
  %5 = extractvalue { i32, i1 } %3, 1
  br i1 %5, label %overflow, label %no_overflow

no_overflow:
  ret i32 %4

overflow:
  ret i32 0
}

declare { i32, i1 } @llvm.smul.with.overflow.i32(i32, i32)

This compiles (llc -mtriple thumbv6m-none-eabi -mcpu=cortex-m0 -O3 test.ll; LLVM version 4.0.0) to:

push	{r7, lr}
mov	r2, r1
asrs	r1, r2, #31
asrs	r3, r0, #31
bl	__aeabi_lmul
...

As you have correctly noticed this extends r0 into r0:r3 (instead of r0:r1) and r1 into r1:r2 (instead of r2:r3).

@arielb1 arielb1 added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 4, 2017
@pnkfelix
Copy link
Member

pnkfelix commented Apr 6, 2017

triage: P-low

@rust-highfive rust-highfive added P-low Low priority and removed I-nominated labels Apr 6, 2017
@pnkfelix
Copy link
Member

pnkfelix commented Apr 6, 2017

Suspected to be LLVM bug, so filing a bug there might be prudent.

@kaini
Copy link

kaini commented Apr 7, 2017

I just wanted to do so and it seems that someone noticed the bug.
https://reviews.llvm.org/D31807 seems to be a fix.

I guess this bug can be closed once Rust upgrades LLVM.

@japaric
Copy link
Member

japaric commented Apr 13, 2017

@kaini If you feel this is urgent to fix, you can backport that patch to the rust-lang/llvm fork and then update the llvm submodule in this repo.

@sanxiyn sanxiyn added the O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state label Jun 8, 2017
@parched
Copy link
Contributor

parched commented Jun 17, 2017

@dnseitz This should be fixed in the latest nightly now.

@arielb1 arielb1 closed this as completed Jun 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state P-low Low priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants