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 loop optimization can make safe programs crash #28728

Closed
RalfJung opened this issue Sep 29, 2015 · 111 comments
Closed

LLVM loop optimization can make safe programs crash #28728

RalfJung opened this issue Sep 29, 2015 · 111 comments
Assignees
Labels
A-LLVM C-bug E-medium I-needs-decision I-unsound P-medium T-compiler WG-embedded

Comments

@RalfJung
Copy link
Member

@RalfJung RalfJung commented Sep 29, 2015

The following snippet crashes when compiled in release mode on current stable, beta and nightly:

enum Null {}

fn foo() -> Null { loop { } }

fn create_null() -> Null {
    let n = foo();

    let mut i = 0;
    while i < 100 { i += 1; }
    return n;
}

fn use_null(n: Null) -> ! {
    match n { }
}


fn main() {
    use_null(create_null());
}

https://play.rust-lang.org/?gist=1f99432e4f2dccdf7d7e&version=stable

This is based on the following example of LLVM removing a loop that I was made aware of: https://github.com/simnalamburt/snippets/blob/12e73f45f3/rust/infinite.rs.
What seems to happen is that since C allows LLVM to remove endless loops that have no side-effect, we end up executing a match that has no arms.

@steveklabnik steveklabnik added the A-LLVM label Sep 29, 2015
@ranma42
Copy link
Contributor

@ranma42 ranma42 commented Sep 29, 2015

The LLVM IR of the optimised code is

; Function Attrs: noreturn nounwind readnone uwtable
define internal void @_ZN4main20h5ec738167109b800UaaE() unnamed_addr #0 {
entry-block:
  unreachable
}

This kind of optimisation breaks the main assumption that should normally hold on uninhabited types: it should be impossible to have a value of that type.
rust-lang/rfcs#1216 proposes to explicitly handle such types in Rust. It might be effective in ensuring that LLVM never has to handle them and in injecting the appropriate code to ensure divergence when needed (IIUIC this could be achieved with appropriate attributes or intrinsic calls).
This topic has also been recently discussed in the LLVM mailing list: http://lists.llvm.org/pipermail/llvm-dev/2015-July/088095.html

@alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Sep 29, 2015

triage: I-nominated

Seems bad! If LLVM doesn't have a way to say "yes, this loop really is infinite" though then we may just have to sit-and-wait for the upstream discussion to settle.

@alexcrichton alexcrichton added the T-compiler label Sep 29, 2015
@ranma42
Copy link
Contributor

@ranma42 ranma42 commented Sep 29, 2015

A way to prevent infinite loops from being optimised away is to add unsafe {asm!("" :::: "volatile")} inside of them. This is similar to the llvm.noop.sideeffect intrinsic that has been proposed in the LLVM mailing list, but it might prevent some optimisations.
In order to avoid the performance loss and to still guarantee that diverging functions/loops are not optimised away, I believe that it should be sufficient to insert an empty non-optimisable loop (i.e. loop { unsafe { asm!("" :::: "volatile") } }) if uninhabited values are in scope.
If LLVM optimises the code which should diverge to the point that it does not diverge anymore, such loops will ensure that the control flow is still unable to proceed.
In "lucky" case in which LLVM is unable to optimise the diverging code, such loop will be removed by DCE.

@geofft
Copy link
Contributor

@geofft geofft commented Sep 29, 2015

Is this related to #18785? That one's about infinite recursion to be UB, but it sounds like the fundamental cause might be similar: LLVM doesn't consider not halting to be a side effect, so if a function has no side effects other than not halting, it's happy to optimize it away.

@arielb1
Copy link
Contributor

@arielb1 arielb1 commented Sep 29, 2015

@geofft

It's the same issue.

@RalfJung
Copy link
Member Author

@RalfJung RalfJung commented Sep 29, 2015

Yes, looks like it's the same. Further down that issue, they show how to get undef, from which I assume it's not hard to make a (seemingly safe) program crash.

@simnalamburt
Copy link
Contributor

@simnalamburt simnalamburt commented Sep 29, 2015

👍

@ranma42
Copy link
Contributor

@ranma42 ranma42 commented Sep 29, 2015

@bluss bluss added the I-wrong label Sep 29, 2015
@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Oct 1, 2015

So I've been wondering how long until somebody reports this. :) In my opinion, the best solution would of course be if we could tell LLVM not to be so aggressive about potentially infinite loops. Otherwise, the only thing I think we can do is to do a conservative analysis in Rust itself that determines whether:

  1. the loop will terminate OR
  2. the loop will have side-effects (I/O operations etc, I forget precisely how this is defined in C)

Either of this should be enough to avoid undefined behavior.

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Oct 1, 2015

triage: P-medium

We'd like to see what LLVM will do before we invest a lot of effort on our side, and this seems relatively unlikely to cause problems in practice (though I have personally hit this while developing the compiler as well). There are no backwards incomatibility issues to be concerned about.

@rust-highfive rust-highfive added P-medium and removed I-nominated labels Oct 1, 2015
@dotdash
Copy link
Contributor

@dotdash dotdash commented Oct 1, 2015

Quoting from the LLVM mailing list discussion:

 The implementation may assume that any thread will eventually do one of the following:
   - terminate
   - make a call to a library I/O function
   - access or modify a volatile object, or
   - perform a synchronization operation or an atomic operation

 [Note: This is intended to allow compiler transformations such as removal of empty loops, even
  when termination cannot be proven. — end note ]

@ranma42
Copy link
Contributor

@ranma42 ranma42 commented Oct 2, 2015

@dotdash The excerpt you are quoting comes from the C++ specification; it is basically the answer to "how it [having side effects] is defined in C" (also confirmed by the standard committee: http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1528.htm ).

Regarding what is the expected behaviour of the LLVM IR there is some confusion. https://llvm.org/bugs/show_bug.cgi?id=24078 shows that there seems to be no accurate & explicit specification of the semantics of infinite loops in LLVM IR. It aligns with the semantics of C++, most likely for historical reasons and for convenience (I only managed to track down https://groups.google.com/forum/#!topic/llvm-dev/j2vlIECKkdE which apparently refers to a time when infinite loops were not optimised away, some time before the C/C++ specs were updated to allow it).

From the thread it is clear that there is the desire to optimise C++ code as effectively as possible (i.e. also taking into account the opportunity to remove infinite loops), but in the same thread several developers (including some that actively contribute to LLVM) have shown interest in the ability to preserve infinite loops, as they are needed for other languages.

@dotdash
Copy link
Contributor

@dotdash dotdash commented Oct 2, 2015

@ranma42 I'm aware of that, I just quoted that for reference, because one possibility to work-around this would be to detect such loops in rust and add one of the above to it to stop LLVM from performing this optimization.

@bstrie
Copy link
Contributor

@bstrie bstrie commented Nov 30, 2015

Is this a soundness issue? If so, we should tag it as such.

@bluss
Copy link
Member

@bluss bluss commented Nov 30, 2015

Yes, following @ranma42's example, this way shows how it readily defeats array bounds checks. playground link

@bluss bluss added I-unsound and removed I-wrong labels Nov 30, 2015
@arielb1 arielb1 added I-wrong and removed I-unsound labels Dec 2, 2015
@arielb1
Copy link
Contributor

@arielb1 arielb1 commented Dec 2, 2015

@bluss

The policy is that wrong-code issues that are also soundness issues (i.e. most of them) should be tagged I-wrong.

@brson brson added I-unsound E-hard and removed E-hard labels Aug 4, 2016
parasyte added a commit to rust-console/cargo-n64 that referenced this issue Dec 28, 2020
- Allows the rrt0 start function to panic when main returns
- Fixes undefined behavior
- See: rust-lang/rust#28728
parasyte added a commit to rust-console/cargo-n64 that referenced this issue Dec 28, 2020
* Update rrt0 and remove the empty loop

- Allows the rrt0 start function to panic when main returns
- Fixes undefined behavior
- See: rust-lang/rust#28728

* Fix link in doc comment
@brson
Copy link
Contributor

@brson brson commented Jan 10, 2021

I don't see it mentioned here: LLVM now has a "mustprogress" attribute:

It appears to implement the proposal from way back in 2015, wherein C/C++ must annotate their functions "mustprogress" to enable these types of optimizations. The way it reads to me is that Rust should stop miscompiling these situations in the future just by not using this attribute.

Edit: oh I see @RalfJung linked this previously in some collapsed comments.

@shepmaster
Copy link
Member

@shepmaster shepmaster commented Jan 18, 2021

After #77972, this is the smallest reproduction of the original case I could make. Fails in Rust 1.49.0 and 1.51.0-nightly (2021-01-17 4253153):

pub fn oops() {
    (|| loop {
        drop(42)
    })()
}

@nikic nikic self-assigned this Jan 23, 2021
@nikic
Copy link
Contributor

@nikic nikic commented Jan 23, 2021

This should be mostly fixed in LLVM 12 by https://reviews.llvm.org/D94106. There's still a minor issue remaining (side-effects can be hoisted across infinite loops).

@oxalica
Copy link
Contributor

@oxalica oxalica commented Jan 24, 2021

The upstream issue is marked as fixed now. https://bugs.llvm.org/show_bug.cgi?id=965

@bstrie
Copy link
Contributor

@bstrie bstrie commented Jan 24, 2021

There's still a minor issue remaining (side-effects can be hoisted across infinite loops).

Is there an LLVM bug to track this issue? (I presume it's not something rustc can address on its own?)

Also, if this issue were fully addressed, would we want to revert #77972? If so, that would be worth tracking somewhere. Even though it appears to cause no noticeable regressions, not doing unnecessary work seems valuable.

@nikic
Copy link
Contributor

@nikic nikic commented Jan 24, 2021

There's still a minor issue remaining (side-effects can be hoisted across infinite loops).

Is there an LLVM bug to track this issue? (I presume it's not something rustc can address on its own?)

This issue is fixed by https://reviews.llvm.org/D95288. There might be more places with problematic assumptions, but at least I'm not aware of any. (In default-enabled passes that is. The attributor has known issues.)

Also, if this issue were fully addressed, would we want to revert #77972? If so, that would be worth tracking somewhere. Even though it appears to cause no noticeable regressions, not doing unnecessary work seems valuable.

We shouldn't revert that yet as Rust also supports older LLVM versions, but we can limit sideeffect insertion to older versions at least.

@oberien
Copy link
Contributor

@oberien oberien commented Feb 1, 2021

I just found the following miscompilation resulting from this bug, which simply compiles to ud2 on stable, beta and nightly:

pub fn oops() -> u32 {
    (0..).sum() // or .last().unwrap() instead of .sum()
}

It's a minified version of the following snippet where I mistyped take_while as filter:

pub fn foo(end: u32) -> u32 {
    (0..)
        .map(|i| i * i)
        .filter(|&i| i < end)
        .sum()
}

@Necior
Copy link

@Necior Necior commented Feb 9, 2021

I just want to notice that it's a bug on which beginner Rustaceans may stumble upon when working on the final exercise from The Book (Building a Multithreaded Web Server).

When I was playing with the web server's code, for a moment I had an empty loop body:

impl Worker {
    fn new(id: usize, receiver: Arc<Mutex<mpsc::Receiver<Job>>>) -> Worker {
        let thread = thread::spawn(|| loop {});
        Worker { id, thread }
    }
}

instead of (Listing 20-20):

impl Worker {
    fn new(id: usize, receiver: Arc<Mutex<mpsc::Receiver<Job>>>) -> Worker {
        let thread = thread::spawn(move || loop {
            let job = receiver.lock().unwrap().recv().unwrap();
            println!("Worker {} got a job; executing.", id);
            job();
        });
        Worker { id, thread }
    }
}

Running the first version ends with Illegal instruction (core dumped). This example boils down to #46076 which was closed as a duplicate of #28728 (this issue). It's also similar to #28728 (comment).

I keep my fingers crossed for solving this issue 🤞

@nagisa
Copy link
Member

@nagisa nagisa commented Mar 5, 2021

Closed by #81451

@daira
Copy link
Contributor

@daira daira commented May 17, 2021

Thankyou to everyone who helped to get this fixed.

However, I would like to note that it took over five years from the original report, and four years after LLVM devs said they would fix it on their side (#28728 (comment)), for this memory-unsafe soundness bug to actually be fixed in Rust.

Is this the expected length of time for fixes to memory-unsafe soundness bugs? :-/

It could, of course, have been fixed much earlier by just adding a side-effect into infinite loops [edit: I know that this is a strong statement but I have seen nothing that contradicts it], and then removing that workaround as a performance optimization after LLVM fixed their bug.

It doesn't appear to be the only example: #52652 (which also could be triggered from safe code as noted in #52652 (comment)) took over three years to be fixed, measuring from the original attempted fix in #46833.

Based on the evidence of this bug and #52652, there appears to be a de facto policy to leave tricky unsoundness bugs unfixed for long periods, whenever fixing them in the most obvious way could cause any potential performance regression or break any (no matter how obscure) code. I would like to register my objection to that de facto policy, which I do not think well serves the majority of Rust users.

@SimonSapin
Copy link
Contributor

@SimonSapin SimonSapin commented May 17, 2021

@daira While I think you make good points, closed issues are generally considered resolved and not in need of attention, so new comments there are easy to miss. Consider starting a new discussion elsewhere (though to be honest I’m not sure which venue would be best in this case.)

@RalfJung
Copy link
Member Author

@RalfJung RalfJung commented May 17, 2021

https://internals.rust-lang.org/ might be a reasonable place.

(FWIW, I disagree with your assessment @daira, in particular about "just" adding a side-effect into infinite loops -- it's not that simple; as usual, one has to be careful with "why don't you just"-style questions.)

@daira
Copy link
Contributor

@daira daira commented May 17, 2021

I used "just" advisedly. It really wouldn't have been hard, as compiler bugs go, to use that approach. Specifically, my understanding is that adding @llvm.sideeffect as originally proposed in #18785 would have been sufficient. No compiler bug of this kind is trivial, but they should not take 5 years to fix. (Correction: over 6 years, counting from #18785; that bug wasn't specific to loops but it was essentially the same problem.)

@Aaron1011
Copy link
Member

@Aaron1011 Aaron1011 commented May 17, 2021

It doesn't appear to be the only example: #52652

That bug was 'fixed' very quickly, but the fix had to be reverted because it broke the rlua crate. The full fix required an RFC to add new -unwind ABIs to the language. While it's definitely a good idea to prioritize fixing soundness issues, it's not always a matter of making an invisible change to the compiler backend.

@rust-lang rust-lang locked and limited conversation to collaborators May 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-LLVM C-bug E-medium I-needs-decision I-unsound P-medium T-compiler WG-embedded
Projects
None yet
Development

No branches or pull requests