Over-aggressively optimization on infinite loops affects RISC-V platforms as well

[leonardopsantos]

Not exactly a duplicate of #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:

#define DO_WAIT 0
#define DO_ACTION 1

int g_lock = DO_WAIT;

void __attribute__((noinline)) worker() {
  for (;;) {

    volatile int i = 0;
    i++;
    g_lock = DO_WAIT;

    while (g_lock != DO_ACTION) {
    }

    i = 0xCAFECAFE;
  }
  return;
}

void dontcallme() {
    volatile int i = 0;
    i++;
}

int main() {
  worker();
  return 0;
}

The disassembly for that is:

worker:                                 # @worker
# %bb.0:                                # %for.cond
	addi	sp, sp, -16
	sw	zero, 12(sp)
	lw	a0, 12(sp)
	addi	a0, a0, 1
	sw	a0, 12(sp)
.Lfunc_end0:
	.size	worker, .Lfunc_end0-worker
                                        # -- End function
	.globl	dontcallme                      # -- Begin function dontcallme
	.p2align	2
	.type	dontcallme,@function
dontcallme:                             # @dontcallme
# %bb.0:                                # %entry
	addi	sp, sp, -16
	sw	zero, 12(sp)
	lw	a0, 12(sp)
	addi	a0, a0, 1
	sw	a0, 12(sp)
	addi	sp, sp, 16
	ret
.Lfunc_end1:
	.size	dontcallme, .Lfunc_end1-dontcallme
                                        # -- End function
	.globl	main                            # -- Begin function main

There's no control instruction at the end of worker, so it just falls through to dontcallme.

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/h55bYsdo9

I've tried the -fsanitize=undefined flag as suggested here, but that doesn't work: https://godbolt.org/z/aYrqfsca7

The workaround is very counter-intuitive:

    while (g_lock != DO_ACTION) {
        asm volatile("");
    }

Yields:

.LBB0_2:                                # %while.body
                                        #   Parent Loop BB0_1 Depth=1
                                        # =>  This Inner Loop Header: Depth=2
	#APP
	#NO_APP
	lw	a3, %pcrel_lo(.Lpcrel_hi0)(a0)
	bne	a3, a1, .LBB0_2
# %bb.3:                                # %while.end
                                        #   in Loop: Header=BB0_1 Depth=1
	sw	a2, 12(sp)
	j	.LBB0_1
.Lfunc_end0:
	.size	worker, .Lfunc_end0-worker
                                        # -- End function

Also here: https://godbolt.org/z/f6Y6qdrMT

I also disagree that #60622, #60419, and #60588 should be closed. Especially because there are no proposed workarounds.

#60622 has been over for over an year, and it's currently frozen for comments.

[topperc]

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.

[leonardopsantos]

This is not my example, I'm just stating that the example shown in #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.

[pinskia]

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]

See https://discourse.llvm.org/t/can-we-keep-must-progress-optimizations-around-infinite-loop-in-c-while-avoiding-some-surprising-behavior/69205/5

Rust had the same problem: rust-lang/rust#28728

Similar to LLVM bug 965?

[topperc]

@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++?

[leonardopsantos]

Pure C: https://godbolt.org/z/da7obrEoP

Indeed, adding -std=c99 preserves the loop: https://godbolt.org/z/TjG5EnbG8

[topperc]

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 #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.

[leonardopsantos]

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(""); }

[topperc]

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.

[neldredge]

@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.