-
Notifications
You must be signed in to change notification settings - Fork 13k
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 loop optimized out by inlining? #42009
Comments
This smells like LLVM deducing that |
Well, but this function can be called, so emitting a trap would be wrong. EDIT: Oh, I guess "no code" means it doesn't return to the caller, but instead "falls through" to whatever is next in the text? That's indeed bad. LLVM is probably responsible for removing the loop, but that doesn't explain why there is no return. |
@RalfJung From a low-level perspective, yes, it is possible to call the function, but any program that does call it is incorrect. This gets into the argument about the scope of "undefined behavior"... And yes, a function containing no machine instructions will indeed fall through to whatever is next. LLVM is trying to have cake and eat it too -- it has removed the return instruction because it was unreachable, but it was only unreachable because of the infinite loop, which has also been removed. Note that If you actually write |
Are we sure LLVM is doing both of these transformations? I think that would be incorrect even in C, and I cannot get clang to do the same: https://godbolt.org/g/tyCz9C Couldn't it be be that rustc is already removing/not emitting the return (because it thinks the function will never terminate), and then LLVM removes the loop (because C says you can do that)? |
@RalfJung The direct Rust equivalent of your C code doesn't have the loop eliminated either. In both languages, there is no ; Function Attrs: norecurse noreturn nounwind readnone uwtable
define i32 @square()() local_unnamed_addr #0 {
br label %1
; <label>:1: ; preds = %0, %1
br label %1
} |
Ah, so it takes that indirection through another function to trigger the bug. Interesting. |
Indeed LLVM's current behavior also violates the C standard, because it doesn't account for C's provisions for loop with cetain conditions being allowed to loop forever.
Other parts of LLVM will gladly assume that code after an infinite loop is dead (e.g., simplifycfg will kill blocks not reachable from the entry block), so even if we added redundant instructions after diverging expressions this wouldn't be a robust workaround. In fact I'm wouldn't be surprised if simplifycfg ran at least once before the passes which remove infinite loops, so it may never help. |
Regardless, it seems pretty clear now this is a duplicate, so let's close this and continue discussion in the older issue. |
I disagree. The other issue is not about a bug in LLVM or LLVM violating the C spec. It is about a mismatch between the Rust semantics and the LLVM IR semantics, where the latter permits removing infinite loops under some circumstances and the former does not. OTOH, this bug here seems to actually be about a bug in LLVM, where LLVM performs optimizations that are not even valid under the LLVM IR semantics. |
We don't track LLVM bugs on this issue tracker, though. We track bugs in Rust (some of which may be ultimately rooted in LLVM bugs). But aside from that, I would strongly argue that both are the exaxct same LLVM issue — both issues are a matter of no-progress loops getting optimized out, whether you want to describe this as "invalid optimization" or "lacking ability to express no-progress loops in IR". In fact, there's a single bug on the LLVM bugzille for this: https://bugs.llvm.org//show_bug.cgi?id=965 (note the many duplicates). |
Sure.
I see. My latest information on #28728 was that currently, there's not enough interest from the LLVM side to support languages that do not permit removing infinite side-effect-free loops. Seems I was wrong, and the issue is more severe than that -- they don't even support C properly... What I am saying is LLVM could fix https://bugs.llvm.org//show_bug.cgi?id=965 by compiling https://godbolt.org/g/1ApTJq to a function that returns immediately. That would be a correct fix for the bug as far as C is concerned, but #28728 would remain unfixed. In contrast, any fix to https://bugs.llvm.org//show_bug.cgi?id=965 will fix this bug at hand. |
C does not permit the loop to be removed in |
Oh, I see. I was not aware C and C++ differ here. But, yeah, considering how LLVM treats this bug and comments like #28728 (comment), I have to agree this is a duplicate. |
Ah, nice hack. :) If LLVM implemented the C spec properly, that should indeed be a sound compilation scheme. Funny enough, this shouldn't even make any difference (in the CFG, after some normalization) compared to lowering the loop the "obvious" way. Which I guess is the reason why LLVM is broken. |
I'm going to close this in favor of #28728 |
Enable TrapUnreachable in LLVM. This patch enables LLVM's TrapUnreachable flag, which tells it to translate `unreachable` instructions into hardware trap instructions, rather than allowing control flow to "fall through" into whatever code happens to follow it in memory. This follows up on #28728 (comment). For example, for @zackw's testcase [here](#42009 (comment)), the output function contains a `ud2` instead of no code, so it won't "fall through" into whatever happens to be next in memory. (I'm also working on the problem of LLVM optimizing away infinite loops, but the patch here is useful independently.) I tested this patch on a few different codebases, and the code size increase ranged from 0.0% to 0.1%.
This crate
when compiled without optimization (
rustc --crate-type rlib --emit asm
), producesbut when compiled with optimization on (
rustc --crate-type rlib --emit asm -O
), no assembly instructions at all are emitted fortest_noret
:I expected to get something like
Identical behavior observed with
rustc 1.17.0 (56124baa9 2017-04-24)
andrustc 1.19.0-nightly (386b0b9d3 2017-05-14)
.The text was updated successfully, but these errors were encountered: