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 loops are optimized out in languages without forward progress guarantees (C, Rust) #1337
Comments
Please attach the .bc file you are inputting to opt here. I get: void %f() { bb: ; preds = %bb, %entry int %call_f() { bb: ; preds = %bb, %entry which is right. |
Ah, n/m I figured it out, you're using -O0. |
In this case, it don't really have anything to do with noreturn. Here globalmodrefaa determines that f() For example: int call_f() { void f() { Compiles to: int %call_f() { return: ; preds = %entry With: llvm-gcc dce.c -emit-llvm -S -o - | llvm-as | opt -globalsmodref-aa -adce | llvm-dis |
How is this an issue with alias analysis? The function doesn't read or write memory. The issue is that ADCE is making bad assumptions. The new version of ADCE has switched to using "mayWriteToMemory" instead of alias analysis... it still has the same issue, though, if you put the readnone attribute on f. I hesitate to suggest a new function attribute, but having an attribute which guarantees a function will eventually return (the opposite of noreturn, I guess), would make some optimizations more feasible, like a correct ADCE and LICM with potentially trapping instructions. A similar sort of testcase for LICM (where it assumes calls always return): |
Same issue in LoopStrengthReduce: declare i32 @a(i32) readonly define void @b() { loopstart: exit: Run opt -loop-reduce to reproduce. Maybe we should add a function to Instruction, something like hasNoSideEffects, which returns false for all call instructions. Then, we can make it a bit smarter if there's eventually a way to indicate that a function doesn't have any infinite loops. |
This is likely to trigger much more frequently with the new addreadattrs pass. |
Is this actually a bug? |
I'm pretty sure turning a well-defined infinite loop into a program with undefined behavior qualifies as a bug. |
Fair enough. There is a similar issue with Here's what I suggest |
This affects our embedded target's simulator where we stop simulation immediately when program enters _exit() from _start() with a crt0() like void _start(void) attribute((noinline)); void _start(void) /* override normal exit. simulator should stop when _exit() is seen / When compiled with llvm-gcc -O0 this results in: define void @_start() nounwind noinline { return: ; No predecessors! declare i32 @main(...) define void @_exit(i32 %retval) noreturn nounwind noinline { bb: ; preds = %bb, %entry return: ; No predecessors! When compiling with -O2 the call to _exit() is removed and our simulator define void @_start() nounwind noinline { define void @_exit(i32 %retval) noreturn nounwind readnone noinline { bb: ; preds = %bb, %entry We can workaround this issue by compiling crt0() with -O0 so it's not a serious problem for us. |
*** Bug llvm/llvm-bugzilla-archive#3593 has been marked as a duplicate of this bug. *** |
Most functions that are obviously readnone/readonly will also be obviously halting. I think adding that annotation and a utility method for 'hasSideEffect' is the right way to go. Just make sure that it's invalid to declare noreturn+halting at the same time. |
*** Bug llvm/llvm-bugzilla-archive#5387 has been marked as a duplicate of this bug. *** |
As a random note, we recently started looking for wrongful-termination and wrongful-nontermination code generation bugs in LLVM, and ran across this. I don't want to be a whiner but since we hit this bug a lot, we have to avoid doing any more of this kind of testing until this problem is fixed. |
*** Bug llvm/llvm-bugzilla-archive#5644 has been marked as a duplicate of this bug. *** |
rdar://8401758 |
A fix for this was posted to the mailing list: |
The correct link is http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20100705/103670.html |
*** Bug llvm/llvm-bugzilla-archive#10858 has been marked as a duplicate of this bug. *** |
*** Bug llvm/llvm-bugzilla-archive#12033 has been marked as a duplicate of this bug. *** |
*** Bug llvm/llvm-bugzilla-archive#14454 has been marked as a duplicate of this bug. *** |
*** Bug llvm/llvm-bugzilla-archive#14973 has been marked as a duplicate of this bug. *** |
*** Bug llvm/llvm-bugzilla-archive#16733 has been marked as a duplicate of this bug. *** |
*** Bug llvm/llvm-bugzilla-archive#31217 has been marked as a duplicate of this bug. *** |
I think the last time we discussed this seriously was in July 2015: I have a different proposal. The forward progress required by C++ is the odd duck here. We should ask frontends to put a function attribute on functions that they want to receive this kind of optimization. It should be safe to remove this attribute when doing cross-language inlining during LTO. Passes like ADCE or functionattrs can power down when a function lacks this attribute, so we don't delete infinite loops or infer that infinite loop functions are readnone in C. |
I posted a patch implementing @llvm.sideeffect(): https://reviews.llvm.org/D38336 |
Light testing suggests that considering Would a patch implementing this idea be accepted? |
Simon, for your proposal to be correct you would have to treat all non-returning functions as side-effecting, but functions which lack the 'noreturn' attribute might not return. As is, your proposal would decrease the chance that a user encounters a miscompile, but not fix it entirely. I don't know whether llvm-commits would accept such a patch. There's a more broad rule to follow when designing IR attributes. LLVM IR is designed to be cross-language at function call boundaries (and possibly within a function after inlining), so you shouldn't assume anything about a function's behaviour unless you can see the definition, or have an attribute on the declaration or call site. It follows that we have to make the most pessimistic assumptions in the absence of attributes. Function attributes should be designed to constrain the possible behaviour of the function, and we can only optimize in the presence of the attributes. Reid's proposal is correct. We can keep 'noreturn' since it constrains the function (it must not return) while also adding a 'forwards-progress' which constrains the function to either perform a side-effect or return. A single function could have both attributes, for example a function containing the select loop of a server. In C++ mode clang could add 'forwards-progress' on all function declarations except those marked extern "C". |
Are there any plans to move ahead with Reid Kleckner's proposal to add a "forwards-progress" attribute to a C / C++ function? I confess that I am one of the clang users/developers that would prefer that clang provide some way of leaving a noreturn function which contains only an infinite loop intact. Some applications that we've encountered use such functions as "parking" functions where an error condition can go to wait until serviced by an interrupt. We currently insert an asm("") statement into such functions to fake a side effect so that the optimizer will not remove calls in the same compilation unit to the "parking" function. If the "forwards-progress" attribute that Reid proposes were implemented, it seems that if we want a "parking" function to stay intact in C++ mode, we'd still need some way to turn off the "forwards-progress" attribute, maybe a "waiting-for-service" attribute? Maybe an C/C++-callabel intrinsic that maps to the @llvm.side-effect() intinsic? |
IICU there is another push towards resolving this, by adding a |
*** Bug llvm/llvm-bugzilla-archive#47189 has been marked as a duplicate of this bug. *** |
This issue is fixed in LLVM 12 by the introduction of the mustprogress attribute and loop metadata, inference of willreturn and finally limitation of call DCE to willreturn functions. There might still be some incorrect assumptions left over here and there, but the core issue is fixed now, and we should track remaining issues separately. |
Very nice, congratulations! |
* [flang][driver] Add support for `--target`/`--triple` This patch adds support for: * `--target` in the compiler driver (`flang-new`) * `--triple` in the frontend driver (`flang-new -fc1`) The semantics of these flags are inherited from `clangDriver`, i.e. consistent with `clang --target` and `clang -cc1 --triple`, respectively. A new structure is defined, `TargetOptions`, that will hold various Frontend options related to the target. Currently, this is mostly a placeholder that contains the target triple. In the future, it will be used for storing e.g. the CPU to tune for or the target features to enable. Additionally, the following target/triple related options are enabled [*]: `-print-effective-triple`, `-print-target-triple` and `-print-targets`. [*] These options were actually available before (like all other options defined in `clangDriver`), but not included in `flang-new --help`. `flang-new` would just use `native` for defining the target, so these options were of little value. With `--target`/`--triple` enabled, these options become very helpful in identifying the available targets. This patch makes sure that these are "advertised" by including them in the output of `--help`. * Add a bit of documentation * Update the test requirements
Extended Description
Hi,
I have been playing with functions marked attribute((noreturn));
Specifically consider a following code:
void f(void) attribute((noreturn));
int call_f()
{
f();
}
void f()
{
while(1)
;
}
With "opt -globalsmodref-aa -adce" this gets compiled to:
int %call_f() {
entry:
%retval = alloca int, align 4 ; <int*> [#uses=1]
unreachable ; **** WRONG!!
return: ; No predecessors!
%retval = load int* %retval ; [#uses=1]
ret int %retval
void %f() {
entry:
br label %bb
bb: ; preds = %bb, %entry
br label %bb
return: ; No predecessors!
ret void
}
The text was updated successfully, but these errors were encountered: