Open leonardopsantos opened 5 months ago
Are expecting that another thread will modify g_lock
to exit this loop?
while (g_lock != DO_ACTION) {
}
If so you need to at least use volatile
so the compiler doesn't hoist the load out of the loop.
The volatile inline asm makes the compiler think that g_load
might be changed by the inline assembly itself so it prevents the load from being moved out of the loop.
This is not my example, I'm just stating that the example shown in https://github.com/llvm/llvm-project/issues/60419 leads to this problem.
Moreover, infinite loops are allowed in C code. My production code is supposed to busy wait (simple snippet, the actual code is more complicated than that):
__attribute__((__noreturn__)) void error_wait_forever()
{
while(1);
}
I'm working on a base-metal RISC-V target, I do not have OS traps or signals. GCC behaves as expected, clang
doesn't. The code generated by clang
just falls thought and now I can't put my system in a known state when something unexpected happens.
From here:
t looks insane to me to fall through to the next function.
Yes, for me too. It took me two days to understand why my core's program counter was everywhere but where it was supposed to be.
This comment by @lhmouse says everything:
So, suppose you are building a compiler. You want people to appreciate your compiler, so you want to make a good compiler that actually compilers their code as they may expect. Then you should ask yourself why someone would write while(true) ;. They do that for a reason, which you should take into account. And they do not expect your compiler to produce undefined behavior for them. You will be blamed for that.
Please fix this behaviour with at least a compiler warning. Or better yet, just don't do this at all.
Note GCC just does the keep infinite loop around by accident with C++; that is it didn't remove the empty infinite loop early enough before making it the end of the function so it can't remove after that. With C, on the other hand the way infinite loops should be handled differently. I am not 100% sure on the rules there though.
@leonardopsantos the example in #60419 doesn't have a constant condition so I believe it hit the end C11 check at the bottom of this code. If use the #60419 test case with -std=c99, the loop stays.
bool checkIfLoopMustProgress(bool HasConstantCond) {
if (CGM.getCodeGenOpts().getFiniteLoops() ==
CodeGenOptions::FiniteLoopsKind::Always)
return true;
if (CGM.getCodeGenOpts().getFiniteLoops() ==
CodeGenOptions::FiniteLoopsKind::Never)
return false;
// If the containing function must make progress, loops also must make
// progress (as in C++11 and later).
if (checkIfFunctionMustProgress())
return true;
// Now apply rules for plain C (see 6.8.5.6 in C11).
// Loops with constant conditions do not have to make progress in any C
// version.
if (HasConstantCond)
return false;
// Loops with non-constant conditions must make progress in C11 and later.
return getLangOpts().C11;
}
Is your code C or C++?
Pure C: https://godbolt.org/z/da7obrEoP
Indeed, adding -std=c99
preserves the loop: https://godbolt.org/z/TjG5EnbG8
Pure C: https://godbolt.org/z/da7obrEoP
Indeed, adding
-std=c99
preserves the loop: https://godbolt.org/z/TjG5EnbG8
Isn't that the code from https://github.com/llvm/llvm-project/issues/60419? I was asking about your production code. As far as I can tell in pure C, a while (1)
loop should be preserved regardless of the standard version.
Yes, that's the example com #60419 .
What's odd is that using -std=c99
preserves the loop, and using -std=c11
does not. Like you said, the loop should be preserved regardess of the C standard used.
With -std=c11
: https://godbolt.org/z/6MKnE1TPE
I've solved my production code with:
while(1) { asm volatile(""); }
Yes, that's the example com #60419 .
What's odd is that using
-std=c99
preserves the loop, and using-std=c11
does not. Like you said, the loop should be preserved regardess of the C standard used.
I only said that it should preserved if the condition is constant in the source code like while (1)
. while (g_lock != DO_ACTION)
does NOT have a constant condition as far as the frontend is concerned.
@leonardopsantos : "infinite loops are allowed in C code": it's not that simple. C11 6.8.5p6:
An iteration statement whose controlling expression is not a constant expression,156) that performs no input/output operations, does not access volatile objects, and performs no synchronization or atomic operations in its body, controlling expression, or (in the case of a for statement) its expression-3, may be assumed by the implementation to terminate.
So your
g_lock = DO_WAIT;
while (g_lock != DO_ACTION) {
}
violates this rule. Its behavior is undefined, and clang is within its rights to delete it and all following code.
If the idea is that some other thread will set g_lock
to DO_ACTION
and thus terminate the loop, that would be UB for another reason, as it is a data race to concurrently read and write a non-atomic variable without additional synchronization.
You simply can't write code this way in modern C. I don't think that this is a bug. If you think it's unreasonable (and it's true that you wouldn't be the only one), your beef is with the ISO C committee, not with clang.
Not exactly a duplicate of https://github.com/llvm/llvm-project/issues/60419 because that issue specifically mentions x86.
I just hit this issue in RISC-V bare-metal target with
clang 17.0.0
, so most definitively not restricted to x86. My code is fairly similar to the test case above:The disassembly for that is:
There's no control instruction at the end of
worker
, so it just falls through todontcallme
.I'm baffled as to how can anyone think that's reasonable.
GCC for RISC-V does the right thing and adds a
j
: https://godbolt.org/z/h55bYsdo9I've tried the
-fsanitize=undefined
flag as suggested here, but that doesn't work: https://godbolt.org/z/aYrqfsca7The workaround is very counter-intuitive:
Yields:
Also here: https://godbolt.org/z/f6Y6qdrMT
I also disagree that https://github.com/llvm/llvm-project/issues/60622, https://github.com/llvm/llvm-project/issues/60419, and https://github.com/llvm/llvm-project/issues/60588 should be closed. Especially because there are no proposed workarounds.
https://github.com/llvm/llvm-project/issues/60622 has been over for over an year, and it's currently frozen for comments.