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

LLVM crash: Basic Block does not have terminator! #88043

Closed
dtolnay opened this issue Aug 15, 2021 · 15 comments · Fixed by #88056 or #88124
Closed

LLVM crash: Basic Block does not have terminator! #88043

dtolnay opened this issue Aug 15, 2021 · 15 comments · Fixed by #88056 or #88124
Assignees
Labels
C-bug Category: This is a bug. I-crash Issue: The compiler crashes (SIGSEGV, SIGABRT, etc). Use I-ICE instead when the compiler panics. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ P-critical Critical 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

@dtolnay
Copy link
Member

dtolnay commented Aug 15, 2021

The following is minimized from https://crates.io/crates/lalrpop, which is regressed as of nightly-2021-08-15.

fn bump() -> Option<usize> {
    unreachable!()
}

fn take_until(terminate: impl Fn() -> bool) {
    loop {
        if terminate() {
            return;
        } else {
            bump();
        }
    }
}

fn main() {
    take_until(|| true);
}
$ rustc +nightly-2021-08-14 main.rs
$ ./main
$ # :)

$ rustc +nightly-2021-08-15 main.rs
Basic Block in function '_ZN4main10take_until17h0067b8a660429bc9E' does not have terminator!
label %3
in function _ZN4main10take_until17h0067b8a660429bc9E
LLVM ERROR: Broken function found, compilation aborted!

$ rustc +nightly-2021-08-15 --version --verbose
rustc 1.56.0-nightly (8007b506a 2021-08-14)
binary: rustc
commit-hash: 8007b506ac5da629f223b755f5a5391edd5f6d01
commit-date: 2021-08-14
host: x86_64-unknown-linux-gnu
release: 1.56.0-nightly
LLVM version: 12.0.1
@dtolnay dtolnay added I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-bug Category: This is a bug. labels Aug 15, 2021
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Aug 15, 2021
@dtolnay
Copy link
Member Author

dtolnay commented Aug 15, 2021

Relevant commit range is 5a19ffe...8007b50. Of the 9 landed PRs, the ones that look potentially relevant to codegen are #85020 and #83417. @lrh2000 @tmandry @erikdesjardins @oli-obk

dtolnay added a commit to dtolnay/lalrproc that referenced this issue Aug 15, 2021
@dtolnay dtolnay added the E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc label Aug 15, 2021
@dtolnay
Copy link
Member Author

dtolnay commented Aug 15, 2021

I tried all the merge commits in order of merge:

so that narrows this down to #83417 (@erikdesjardins @oli-obk).

@dtolnay dtolnay removed the E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc label Aug 15, 2021
@Aaron1011
Copy link
Member

This passes under Miri, so I suspect that we've exposed an LLVM bug.

@dtolnay
Copy link
Member Author

dtolnay commented Aug 15, 2021

Not sure it's an LLVM bug because it seems the IR is broken even before any LLVM passes. I tried:

$ rustc +nightly-2021-08-14 -Cno-prepopulate-passes --emit=llvm-ir src/main.rs -o before.ll
$ opt before.ll  # ok

$ rustc +nightly-2021-08-15 -Cno-prepopulate-passes --emit=llvm-ir src/main.rs -o after.ll
$ opt after.ll
opt: after.ll:287:1: error: expected instruction opcode
bb1:                                              ; preds = %bb0
^

Before: https://paste.rs/o7U
After: https://paste.rs/ftk
Diff: https://paste.rs/gWL.diff

The main::take_until looks like legitimately broken input IR. Notice the extractvalue insts relocated from the front of bb4 to the end of bb0 (invalidly after the terminator).

What in LLVM is actually running during a -Cno-prepopulate-passes build?

@oli-obk
Copy link
Contributor

oli-obk commented Aug 15, 2021

I think we should do a quick revert and then figure this out without any pressure

@oli-obk
Copy link
Contributor

oli-obk commented Aug 15, 2021

the mir changes are

    bb2: {
-        _0 = const ();                   // scope 0 at src/main.rs:8:13: 8:19
-        drop(_1) -> bb5;                 // scope 0 at src/main.rs:13:1: 13:2
+        drop(_1) -> bb4;                 // scope 0 at src/main.rs:13:1: 13:2
    }

    bb3: {
-        _5 = bump() -> [return: bb4, unwind: bb6]; // scope 0 at src/main.rs:10:13: 10:19
+        _5 = bump() -> [return: bb0, unwind: bb5]; // scope 0 at src/main.rs:10:13: 10:19
                                         // mir::Constant
                                         // + span: src/main.rs:10:13: 10:17
                                         // + literal: Const { ty: fn() -> std::option::Option<usize> {bump}, val: Value(Scalar(<ZST>)) }
-    }
-
-    bb4: {
-        goto -> bb0;                     // scope 0 at src/main.rs:6:5: 12:6
    }

that seems totally reasonable, so we need to check what codegen_llvm makes of it

@oli-obk
Copy link
Contributor

oli-obk commented Aug 15, 2021

I wonder why the goto collapse only happens if the zst assignment is eliminated. it seems like we need to preserve bb4 in mir or inject it in codegen_llvm, but first we should find out why the goto collapse is happening at all

@tmiasko
Copy link
Contributor

tmiasko commented Aug 15, 2021

There are no control-flow simplification passes after removal of dead local variables, which is why it didn't happen so far. Still, it seems a bit surprising it wasn't reported earlier:

pub fn f(_a: Option<String>) -> Option<u32> {
    loop {
        f(None);
        ()
    }
}

fn main() {
    f(None);
}
$ rustc +stable a.rs
Basic Block in function '_ZN1a1f17h9901a33e93da62b7E' does not have terminator!
label %4
in function _ZN1a1f17h9901a33e93da62b7E
LLVM ERROR: Broken function found, compilation aborted!

Adding a new cleanup pass before codegen would be one way to address this.

@erikdesjardins
Copy link
Contributor

I swear half of my PRs break the compiler 😅

I'm excited for what bugs arise when we start using undef for uninitialized consts...

Revert PR: #88056

@apiraino
Copy link
Contributor

Assigning priority as discussed in the Zulip thread of the Prioritization Working Group.

@rustbot label -I-prioritize +P-critical

@rustbot rustbot added P-critical Critical priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Aug 16, 2021
@bors bors closed this as completed in 806b399 Aug 17, 2021
@oli-obk oli-obk reopened this Aug 17, 2021
@oli-obk
Copy link
Contributor

oli-obk commented Aug 17, 2021

The issue was not fixed, it is an LLVM crash on stable as #88043 (comment) shows

@oli-obk oli-obk added I-crash Issue: The compiler crashes (SIGSEGV, SIGABRT, etc). Use I-ICE instead when the compiler panics. and removed regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Aug 17, 2021
@dtolnay dtolnay added the regression-from-stable-to-stable Performance or correctness regression from one stable version to another. label Aug 17, 2021
@dtolnay
Copy link
Member Author

dtolnay commented Aug 17, 2021

Relabeling regression because #88043 (comment) worked in 1.51.0. It bisects to nightly-2021-03-02 (e37a13c...4f20caa).

@dtolnay
Copy link
Member Author

dtolnay commented Aug 17, 2021

#78360 is the only codegen change in that nightly. @tmiasko @wesleywiser

@wesleywiser
Copy link
Member

Reopening to track the stable-to-stable regression.

@wesleywiser wesleywiser self-assigned this Aug 26, 2021
@wesleywiser
Copy link
Member

Backport was merged.

bors added a commit to rust-lang-ci/rust that referenced this issue Sep 19, 2021
…r=davidtwco

Start block is not allowed to have basic block predecessors

* The MIR validator is extended to detect potential violations.
* The start block has no predecessors after building MIR, so no changes are required there.
* The SimplifyCfg could previously violate this requirement when collapsing goto chains, so transformation is disabled for the start block, which also substantially simplifies the implementation.
* The LLVM function entry block also must not have basic block predecessors. Previously, to ensure that code generation had to perform necessary adjustments. This now became unnecessary.

The motivation behind the change is to align with analogous requirement in LLVM, and to avoid potential latent bugs like the one reported in rust-lang#88043.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. I-crash Issue: The compiler crashes (SIGSEGV, SIGABRT, etc). Use I-ICE instead when the compiler panics. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ P-critical Critical 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
8 participants