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

Update LLVM to fix miscompiles with -Copt-level=z on Windows #45824

Merged
merged 1 commit into from
Nov 13, 2017

Conversation

dotdash
Copy link
Contributor

@dotdash dotdash commented Nov 7, 2017

Fixes #45034

@dotdash
Copy link
Contributor Author

dotdash commented Nov 7, 2017

Needs to wait for rust-lang/llvm#95

@kennytm kennytm added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Nov 7, 2017
@alexcrichton
Copy link
Member

r=me with the new merge commit, also would it be possible to add a test for this?

@dotdash
Copy link
Contributor Author

dotdash commented Nov 7, 2017

If someone could test that this crashes on i686 windows msvc with -Copt-level=z

#![feature(test)]
extern crate test;

fn foo(x: i32, y: i32) -> i64 {
    (x + y) as i64
}

#[inline(never)]
fn bar() {
    let f = Box::new(0);
    let y: fn(i32, i32) -> i64 = test::black_box(foo);
    test::black_box(y(1, 2));
}

fn main() {
    bar();
}

@nagisa
Copy link
Member

nagisa commented Nov 7, 2017

The test from the comment above does crash on 11-03 nightly. Compiling it complains about f being unused (consider using _f).

@nagisa
Copy link
Member

nagisa commented Nov 7, 2017

That being said, I feel that a codegen test would be way more appropriate if one is possible.

@dotdash
Copy link
Contributor Author

dotdash commented Nov 8, 2017

Are there any codegen tests that check for assembler output?

@dotdash
Copy link
Contributor Author

dotdash commented Nov 8, 2017

Doesn't seem like there is, and I currently don't have time to adjust the test runner to support it. :-/

So this is free for someone to pick up.

@dotdash dotdash closed this Nov 8, 2017
@alexcrichton
Copy link
Member

Er sorry @dotdash I didn't mean to imply a test was required, just if we already had one I figured it'd be good to include! I'm totally fine merging this regardless (as it's upstream in LLVM anyway). Do you want to check in the test you gisted above and r=me?

@dotdash
Copy link
Contributor Author

dotdash commented Nov 8, 2017

@bors r=alexcrichton

@bors
Copy link
Collaborator

bors commented Nov 8, 2017

📌 Commit 8b6caa2 has been approved by alexcrichton

@kennytm kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 10, 2017
@kennytm
Copy link
Member

kennytm commented Nov 10, 2017

@bors r-

CI failed, “error: -O and -C opt-level both provided” on the new test case.

[00:52:35] failures:
[00:52:35] 
[00:52:35] ---- [run-pass] run-pass/msvc-opt-minsize.rs stdout ----
[00:52:35] 	
[00:52:35] error: compilation failed!
[00:52:35] status: exit code: 101
[00:52:35] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/run-pass/msvc-opt-minsize.rs" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass" "--target=x86_64-unknown-linux-gnu" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/msvc-opt-minsize.stage2-x86_64-unknown-linux-gnu" "-Crpath" "-O" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-Copt-level=z" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/msvc-opt-minsize.stage2-x86_64-unknown-linux-gnu.aux"
[00:52:35] stdout:
[00:52:35] ------------------------------------------
[00:52:35] 
[00:52:35] ------------------------------------------
[00:52:35] stderr:
[00:52:35] ------------------------------------------
[00:52:35] error: -O and -C opt-level both provided
[00:52:35] 
[00:52:35] 
[00:52:35] ------------------------------------------
[00:52:35] 
[00:52:35] thread '[run-pass] run-pass/msvc-opt-minsize.rs' panicked at 'explicit panic', /checkout/src/tools/compiletest/src/runtest.rs:2498:8
[00:52:35] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[00:52:35] 
[00:52:35] 
[00:52:35] failures:
[00:52:35]     [run-pass] run-pass/msvc-opt-minsize.rs
[00:52:35] 
[00:52:35] test result: FAILED. 2808 passed; 1 failed; 14 ignored; 0 measured; 0 filtered out
[00:52:35] 
[00:52:35] thread 'main' panicked at 'Some tests failed', /checkout/src/tools/compiletest/src/main.rs:329:21
[00:52:35] 

@kennytm kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 10, 2017
@nagisa
Copy link
Member

nagisa commented Nov 10, 2017

Sadly, using -Copt-level requires promoting the test to a run-make.

@bors
Copy link
Collaborator

bors commented Nov 11, 2017

☔ The latest upstream changes (presumably #45806) made this pull request unmergeable. Please resolve the merge conflicts.

@dotdash
Copy link
Contributor Author

dotdash commented Nov 12, 2017

@bors r=alexcrichton

@bors
Copy link
Collaborator

bors commented Nov 12, 2017

📌 Commit 1a8c9f8 has been approved by alexcrichton

@kennytm kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 12, 2017
@alexcrichton alexcrichton self-assigned this Nov 12, 2017
@bors
Copy link
Collaborator

bors commented Nov 13, 2017

⌛ Testing commit 1a8c9f8 with merge aca22a8...

bors added a commit that referenced this pull request Nov 13, 2017
Update LLVM to fix miscompiles with -Copt-level=z on Windows

Fixes #45034
@bors
Copy link
Collaborator

bors commented Nov 13, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing aca22a8 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants