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

Infinite recursion optimized away with opt-level=z #97428

Closed
ldm0 opened this issue May 26, 2022 · 10 comments · Fixed by #97690
Closed

Infinite recursion optimized away with opt-level=z #97428

ldm0 opened this issue May 26, 2022 · 10 comments · Fixed by #97690
Assignees
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness 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

@ldm0
Copy link
Contributor

ldm0 commented May 26, 2022

I tried this code:

#[derive(Default)]
pub struct Run {
    pub a: bool,
    pub b: bool,
    pub c: Option<String>,
}

impl Run {
    pub fn run(&mut self) {
        if !self.a && !self.b {
            self.run();
        }
    }
}

pub fn foo() -> Run {
    let mut run = Run::default();
    run.a = false;
    run.b = false;
    run.run();
    run
}

I expected to see this happen: foo runs infinitely

Instead, this happened: foo return directly

The opt-level is set to z: https://rust.godbolt.org/z/n1v471j6E

@ldm0 ldm0 added the C-bug Category: This is a bug. label May 26, 2022
@ldm0 ldm0 changed the title Infinite recursion optimized away Infinite recursion optimized away with opt-level=z May 26, 2022
@tmiasko tmiasko added the A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. label May 26, 2022
@ldm0
Copy link
Contributor Author

ldm0 commented May 26, 2022

According to #28728 (comment) and https://www.llvm.org/docs/LangRef.html#llvm-loop-mustprogress-metadata, rustc doesn't emit mustprogress, which means the loop generated by the tail call shouldn't be removed(I guess).

@tmiasko tmiasko added the I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness label May 26, 2022
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label May 26, 2022
@hkratz
Copy link
Contributor

hkratz commented May 26, 2022

This was apparently working in 1.58 and regressed in 1.59.

@rustbot label regression-from-stable-to-stable

@rustbot rustbot added the regression-from-stable-to-stable Performance or correctness regression from one stable version to another. label May 26, 2022
@ldm0
Copy link
Contributor Author

ldm0 commented May 26, 2022

Small reproducer: https://github.com/ldm0/rust_test1

$ cargo run --release 
    Finished release [optimized] target(s) in 0.04s
     Running `target/release/rust_test1`
start
done: Run { a: false, b: false, c: None }

@nikic nikic self-assigned this May 26, 2022
@nikic
Copy link
Contributor

nikic commented May 26, 2022

From a cursory look, probably due to: https://github.com/llvm/llvm-project/blob/cc871cf6b50f6a76f2083d192e2254a16832224b/llvm/lib/Transforms/Utils/Local.cpp#L2369

@ldm0
Copy link
Contributor Author

ldm0 commented May 26, 2022

From cargo bisect:

Regression in nightly-2021-09-25

Probably related to #88243

@inquisitivecrystal inquisitivecrystal added P-critical Critical priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels May 27, 2022
@inquisitivecrystal
Copy link
Contributor

inquisitivecrystal commented May 27, 2022

I'm tagging this as P-critical, as discussed in the prioritization working group.

The following reproduction is concerning:

#[derive(Default)]
pub struct Run {
    pub a: Option<String>,
}

impl Run {
    #[inline(never)]
    pub fn run(&mut self) -> ! {
        self.run()
    }
}

pub fn foo() -> Run {
    let mut run = Run::default();
    run.run();
    run
}

fn main() {
    foo();
}

This exits with an illegal hardware instruction, which is not good. Presumably, run() is returning even though it returns !. One of many ways in which this can likely be unsound.

@inquisitivecrystal inquisitivecrystal added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 27, 2022
@nikic
Copy link
Contributor

nikic commented May 30, 2022

Upstream fix: llvm/llvm-project@2e101cc

Upstream backport issue: llvm/llvm-project#55777

@inquisitivecrystal
Copy link
Contributor

inquisitivecrystal commented Jun 5, 2022

Reopening to beta-nominate. This regression is a nasty miscompilation that can silently induce unsoundness in valid code, given a certain opt-level setting, and is not platform-specific. The patch to LLVM to fix this is quite small: llvm/llvm-project@2e101cc

@inquisitivecrystal inquisitivecrystal added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jun 5, 2022
@nikic nikic removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jun 5, 2022
@apiraino
Copy link
Contributor

apiraino commented Jun 9, 2022

As per T-compiler meeting (Zulip notes), lowering priority of this issue since has been there for a long time and the "illegal hardware instruction" mentioned is the ud2 we generate intentionally here.

@rustbot label +P-high -P-critical

@rustbot rustbot added P-high High priority and removed P-critical Critical priority labels Jun 9, 2022
ehuss pushed a commit to ehuss/rust that referenced this issue Jun 24, 2022
Update LLVM submodule

Merge upstream release/14.x branch.

Fixes rust-lang#97428.
@nikic
Copy link
Contributor

nikic commented Jul 6, 2022

Closing this as the beta backport has happened.

@nikic nikic closed this as completed Jul 6, 2022
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. C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness 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

Successfully merging a pull request may close this issue.

7 participants