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

Rustc 1.12.0 Windows build of ethcore crate fails with LLVM error #36924

Closed
rphmeier opened this issue Oct 3, 2016 · 23 comments
Closed

Rustc 1.12.0 Windows build of ethcore crate fails with LLVM error #36924

rphmeier opened this issue Oct 3, 2016 · 23 comments
Assignees
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@rphmeier
Copy link
Contributor

rphmeier commented Oct 3, 2016

Error produced:

EH pad must be jumped to via an unwind edge
  %cleanuppad13 = cleanuppad within none []
  br i1 %1927, label %bb99, label %bb102_cleanup_trampoline_bb99, !dbg !719592

As seen here: https://ci.appveyor.com/project/NikolayVolf/parity-g802m/build/1.3.0+1997#L640
Triggered by this update from 1.10.0 to 1.12.0: https://github.com/ethcore/parity/pull/2423

Either LLVM is broken or the generated IR is.
Feel free to change the title to something more descriptive. Unfortunately I haven't got a windows machine for testing, just the one broken appveyor build.

@rphmeier rphmeier changed the title Rustc 1.12.0 Windows build of ethcore crate Rustc 1.12.0 Windows build of ethcore crate fails with LLVM error Oct 3, 2016
@alexcrichton alexcrichton added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 3, 2016
@alexcrichton
Copy link
Member

Historically most of these have been LLVM bugs, unfortunately, which are notoriously hard to track down. @rphmeier do you know if it'd be possible to minimize the test case at all?

@rphmeier
Copy link
Contributor Author

rphmeier commented Oct 3, 2016

@alexcrichton yikes, I have no idea to be honest. Like I said, I haven't got a windows machine to test with and the test case as it stands is about 10-15k LoC on its own...

@arielb1 arielb1 self-assigned this Oct 3, 2016
@nikomatsakis
Copy link
Contributor

This may well be related to MIR, as well.

cc @rust-lang/compiler

@rphmeier
Copy link
Contributor Author

rphmeier commented Oct 4, 2016

Confirmed a MIR issue: build succeeds with RUSTFLAGS= -Zorbit=off
https://ci.appveyor.com/project/NikolayVolf/parity-g802m/build/1.3.0+2055

@alexcrichton alexcrichton added the A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html label Oct 4, 2016
@nikomatsakis
Copy link
Contributor

@rphmeier I am trying to get a windows setup up and going to test this and other reported problems (my previous Windows VM recently mysteriously lost its Network Driver and I've never been able to figure out how to get it back...). Any tips you can give me for reproducing it? I guess just a vanilla x86_64 MSVC configuration?

@rphmeier
Copy link
Contributor Author

rphmeier commented Oct 4, 2016

@nikomatsakis Thanks for looking into this. A vanilla x86_64 MSVC setup sounds right. The appveyor.yml contains a few additional setup steps which you might find useful: https://github.com/ethcore/parity/blob/master/appveyor.yml

@nikomatsakis
Copy link
Contributor

So I am able to reproduce this. Interestingly, debug builds don't seem to encounter the issue.

@nikomatsakis
Copy link
Contributor

Huh, somewhat frustratingly, this...stopped reproducing for now. Not sure what is different now!

@nikomatsakis
Copy link
Contributor

Ah, I see, cargo build must be run from the parity directory, not parity/parity. This probably affects some relevant Cargo.toml settings...

@arielb1
Copy link
Contributor

arielb1 commented Oct 5, 2016

Compiling parity takes a very long amount of time and does not seem to reproduce the bug - are there any good STR?

@nikomatsakis
Copy link
Contributor

@arielb1 and I discussed on IRC -- I am definitely able to reproduce, but I think @arielb1 was testing with 1.14, not 1.12. So this is quite possibly a bug that has been fixed in the meantime.

@arielb1
Copy link
Contributor

arielb1 commented Oct 6, 2016

The problematic function seems to be _ZN93_$LT$ethcore..evm..interpreter..Interpreter$LT$Cost$GT$$u20$as$u20$ethcore..evm..evm..Evm$GT$4exec17h1655a9f21f79ac34E"(%"2.std::result::Result<evm::evm::GasLeft, evm::evm::Error>"* noalias nocapture sret dereferenceable(64), %"evm::interpreter::Interpreter<usize>, and LLVM seems to be chocking on it. I'm running a dump of simplifycfg to see whether something pops up (should have done that earlier).

I think dropping zeroing might have fixedhidden it - it makes some of the control flow simpler (after inlining).

@arielb1
Copy link
Contributor

arielb1 commented Oct 6, 2016

The code that is optimized wrong looks like this, basically:


bb111:                                            ; preds = %bb112
  store i8 0, i8* %tmp191
  cleanupret from %cleanuppad7 unwind label %bb110

bb112:                                            ; preds = %bb113, %bb113, %bb113
  %497 = load i8, i8* %tmp191, !range !3
  %498 = trunc i8 %497 to i1
  br i1 %498, label %bb111, label %bb112_cleanup_trampoline_bb110

bb113:                                            ; preds = %bb114
  %499 = getelementptr inbounds %"evm::interpreter::InstructionResult<usize>", %"evm::interpreter::InstructionResult<usize>"* %result, i32 0, i32 0
  %500 = load i64, i64* %499, !range !12
  switch i64 %500, label %bb113_cleanup_trampoline_bb110 [
    i64 0, label %bb112
    i64 1, label %bb112
    i64 6, label %bb112
  ]

Somehow, it is optimized into this (that is not only simplifycfg, but also a fair bunch of other passes):

bb113:                                            ; preds = %bb114
  %1427 = icmp ult i64 %result.sroa.0.1, 7
  br i1 %1427, label %bb110, label %bb113_cleanup_trampoline_bb110

Where bb110 is a cleanup pad:

bb110:                                            ; preds = %bb112_cleanup_trampoline_bb110, %bb113_cleanup_trampoline_bb110, %bb114_cleanup_trampoline_bb110, %panic, %bb111, %bb97, %bb59, %bb56, %bb54, %bb51, %bb50, %bb48, %bb47, %bb44, %bb43, %bb41, %bb37, %bb36, %bb34, %bb32, %bb31, %bb29, %bb28, %bb27, %bb25, %bb24, %bb23, %bb22, %bb21, %bb20
  %cleanuppad8 = cleanuppad within none []
  ...

cc @majnemer.

See that %bb110 has no

@arielb1
Copy link
Contributor

arielb1 commented Oct 6, 2016

Crashing IR available at https://dl.dropboxusercontent.com/u/35340625/ethcore-rust-1.12.ll.xz (huge, un-minified). opt output available at https://dl.dropboxusercontent.com/u/35340625/verify-report.xz.

@brson brson added the P-high High priority label Oct 6, 2016
@majnemer
Copy link

majnemer commented Oct 6, 2016

Reduced IR:

target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-pc-windows-msvc"

declare void @throw()

define void @bad_fn(i64 %val) personality i32 (...)* @__CxxFrameHandler3 {
entry:
  invoke void @throw()
          to label %unreachable unwind label %cleanup1

unreachable:
  unreachable

cleanup1:
  %cleanuppad1 = cleanuppad within none []
  switch i64 %val, label %cleanupdone2 [
    i64 0, label %cleanupdone1
    i64 1, label %cleanupdone1
    i64 6, label %cleanupdone1
  ]

cleanupdone1:
  cleanupret from %cleanuppad1 unwind label %cleanup2

cleanupdone2:
  cleanupret from %cleanuppad1 unwind label %cleanup2

cleanup2:
  %phi = phi i1 [ true, %cleanupdone1 ], [ true, %cleanupdone2 ]
  %cleanuppad2 = cleanuppad within none []
  call void @throw() [ "funclet"(token %cleanuppad2) ]
  unreachable
}

declare i32 @__CxxFrameHandler3(...)

The bug can be reproduced using opt -simplifycfg

@majnemer
Copy link

majnemer commented Oct 6, 2016

The following patch fixes the reduced testcase, could you please test it?

diff --git a/lib/Transforms/Utils/SimplifyCFG.cpp b/lib/Transforms/Utils/SimplifyCFG.cpp
index 90ce672..2702b76 100644
--- a/lib/Transforms/Utils/SimplifyCFG.cpp
+++ b/lib/Transforms/Utils/SimplifyCFG.cpp
@@ -4503,7 +4503,8 @@ GetCaseResults(SwitchInst *SI, ConstantInt *CaseVal, BasicBlock *CaseDest,
        ++I) {
     if (TerminatorInst *T = dyn_cast<TerminatorInst>(I)) {
       // If the terminator is a simple branch, continue to the next block.
-      if (T->getNumSuccessors() != 1)
+      const auto *BI = dyn_cast<BranchInst>(T);
+      if (!BI || !BI->isUnconditional())
         return false;
       Pred = CaseDest;
       CaseDest = T->getSuccessor(0);

@arielb1
Copy link
Contributor

arielb1 commented Oct 6, 2016

@majnemer

Is there an LLVM issue/PR for it?

@arielb1
Copy link
Contributor

arielb1 commented Oct 6, 2016

Patch appears to fix problem (on Linux at least).

@majnemer
Copy link

majnemer commented Oct 7, 2016

Fixed in LLVM r283517.

@nikomatsakis
Copy link
Contributor

This should be fixed on nightly builds now, right? I think we're just waiting on a backport?

@arielb1
Copy link
Contributor

arielb1 commented Oct 13, 2016

Sure.

@brson
Copy link
Contributor

brson commented Oct 14, 2016

I believe #37030 is the PR that fixes this. Not backported yet.

@arielb1
Copy link
Contributor

arielb1 commented Oct 14, 2016

This is fixed by the SimplifyCfg PR, which was backported (that got merged into rustc PR #37030).

@brson brson closed this as completed Oct 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. 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

6 participants