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

Bad codegen (most probably) on ARMv7 #42893

Closed
ghost opened this issue Jun 25, 2017 · 7 comments
Closed

Bad codegen (most probably) on ARMv7 #42893

ghost opened this issue Jun 25, 2017 · 7 comments
Labels
O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@ghost
Copy link

ghost commented Jun 25, 2017

The build for PR #42882 failed on ARM machines only. The failure is baffling and makes no sense at all. I managed to reproduce it on my Raspberry Pi and then tried minimizing the code bit by bit. This is the smallest example I have that demonstrates the problem:

#![feature(rand)]

use std::__rand::thread_rng;
use std::sync::atomic::{ATOMIC_USIZE_INIT, AtomicUsize, Ordering};

static GLOBAL: AtomicUsize = ATOMIC_USIZE_INIT;

struct Foo(usize);

impl Drop for Foo {
    fn drop(&mut self) {
        assert!(self.0 == 0);
        GLOBAL.fetch_add(0, Ordering::Relaxed);
        GLOBAL.fetch_add(0, Ordering::Relaxed);
    }
}

fn main() {
    let len = 1;

    for &modulus in &[100, 100] {
        println!("len = {}, modulus = {}", len, modulus);

        let _rng = thread_rng();

        (0..len).map(|_| Foo(0 % modulus)).collect::<Vec<_>>();
    }
}

Running in debug mode is all right:

     Running `target/debug/vector-sort-panic-safe`
len = 1, modulus = 100
len = 1, modulus = 100

But running in release mode prints out weird stuff:

     Running `target/release/vector-sort-panic-safe`
len = 1, modulus = 100
len = 1, modulus = 100
len = 1, modulus = 3785379840
len = 1, modulus = 3800891488
len = 1, modulus = 3959422609
len = 1, modulus = 3800891468
len = 1, modulus = 3959422587
len = 1, modulus = 3785359365
len = 1, modulus = 3959422388
len = 1, modulus = 3785379840
len = 1, modulus = 3942645753
len = 1, modulus = 100
len = 1, modulus = 100

Environment:

$ rustc --version
rustc 1.20.0-nightly (c9bb93576 2017-06-24)
$ cargo --version
cargo 0.21.0-nightly (d26fd6f08 2017-06-20)
$ uname -a
Linux raspberrypi 4.4.11-v7+ #888 SMP Mon May 23 20:10:33 BST 2016 armv7l GNU/Linux

cc @alexcrichton

@Mark-Simulacrum Mark-Simulacrum added the O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state label Jun 25, 2017
@Mark-Simulacrum
Copy link
Member

cc @arielb1 -- seems there's perhaps more to fix, even after #42740.

@arielb1
Copy link
Contributor

arielb1 commented Jun 25, 2017

Let me take a look

@arielb1 arielb1 added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 25, 2017
@arielb1
Copy link
Contributor

arielb1 commented Jun 25, 2017

This is supposed to occur on armv7-unknown-linux-gnueabihf or on armv6?

@ghost
Copy link
Author

ghost commented Jun 25, 2017

It's on armv7-unknown-linux-gnueabihf

@arielb1
Copy link
Contributor

arielb1 commented Jun 26, 2017

Bug confirmed.

@arielb1
Copy link
Contributor

arielb1 commented Jun 26, 2017

This looks fixed on LLVM 5.0, looks like we need another bisect run.

@arielb1
Copy link
Contributor

arielb1 commented Jun 26, 2017

Looks like 4868a15090d0dbaa91b772e498eadc92cfdebd7a - [ARM] Temporarily disable globals promotion to constant pools to prevent miscompilation. That is going into LLVM 4.0.1, so we might just get that.

LLVM 4.0.1 is actually just 21 commits:

[mips][mc] Fix a crash when disassembling odd sized sections
Use correct registers for "A" inline asm constraint
[X86] Remove special handling for 16 bit for A asm constraints.
Fix invalid addrspacecast due to combining alloca with global var
Adding const overloads of operator* and operator-> for DenseSet iterators
[Hexagon] Fix lowering of formal arguments of type i1
[InstCombine] Fix bug in pointer replacement
[CMake] Fix pthread handling for out-of-tree builds
CMake: Don't install llvm-tblgen twice
[MemCpyOpt] Only replace memcpy with bitcast if address spaces match
[ArgPromotion] Fix a truncated variable
[safestack] Disable stack coloring by default
merge-request.sh: Use https url for bugzilla
[ARM] Clear the constant pool cache on explicit .ltorg directives
[ARM] Temporarily disable globals promotion to constant pools to prevent miscompilation
[TLI] Add prototype checking for all remaining LibFuncs
[PPC] Properly update register save area offset
[PPC] When restoring R30 (PIC base pointer), mark it as <def>
Make library calls sensitive to regparm module flag (Fixes PR3997).
[GlobalMerge] Don't merge globals that may be preempted
[Support] Fix ErrorOr assertion when /proc/cpuinfo doesn't exist.

arielb1 added a commit to arielb1/rust that referenced this issue Jun 27, 2017
arielb1 added a commit to arielb1/rust that referenced this issue Jun 28, 2017
This backports fixes several codegen bugs, including rust-lang#42893.
bors added a commit that referenced this issue Jun 30, 2017
Rebase LLVM on top of LLVM 4.0.1

Fixes #42893.

Please don't backport this to beta as-is - I'm not sure I want rust-lang/llvm#84 to sneak to beta before it gets sufficient testing.

r? @alexcrichton
bors added a commit that referenced this issue Jun 30, 2017
[beta] rebase LLVM on top of 4.0.1

This backports fixes several codegen bugs, including #42893.

This also un-commits the introduction of #42750 (the StackColoring improvement) to beta. That seemed to have occurred by accident in #42927, but I'm not comfortable with sneaking a subtle codegen change like this to beta.

r? @alexcrichton
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state 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

2 participants