[Clang] Implement P2809: Trivial infinite loops are not Undefined Behavior #90066

cor3ntin · 2024-04-25T14:42:44Z

This is applied as a DR to C++11 (C++98 did not guarantee forward progress and is left untouched)

As an extension (and to preserve existing behavior in C), we consider all controlling expression that can be constant folded
in the front end, not just standard constant expressions.

…avior https://wg21.link/P2809R3

llvmbot · 2024-04-25T14:43:16Z

@llvm/pr-subscribers-clang-modules
@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: cor3ntin (cor3ntin)

Changes

https://wg21.link/P2809R3

This is applied as a DR to C++11 (C++98 did not guarantee forward progress and is left untouched)

As an extension (and to preserve existing behavior in C), we consider all controlling expression that can be constant folded
in the front end, not just standard constant expressions.

Full diff: https://github.com/llvm/llvm-project/pull/90066.diff

6 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+3-1)
(modified) clang/lib/CodeGen/CGStmt.cpp (+71-10)
(modified) clang/lib/CodeGen/CodeGenFunction.cpp (+1)
(modified) clang/lib/CodeGen/CodeGenFunction.h (+1-22)
(modified) clang/test/CodeGenCXX/attr-mustprogress.cpp (+30-40)
(modified) clang/www/cxx_status.html (+1-1)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 64526ed6d06f55..f5906c2dd4eb52 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -122,7 +122,7 @@ C++23 Feature Support
   materialize temporary object which is a prvalue in discarded-value expression.
 - Implemented `P1774R8: Portable assumptions <https://wg21.link/P1774R8>`_.
 
-- Implemented `P2448R2: Relaxing some constexpr restrictions <https://wg21.link/P2448R2>`_.
+- Implemented `P2809R3: Trivial infinite loops are not Undefined Behavior <https://wg21.link/P2809R3>`_.
 
 C++2c Feature Support
 ^^^^^^^^^^^^^^^^^^^^^
@@ -131,6 +131,8 @@ C++2c Feature Support
 
 - Implemented `P2573R2: = delete("should have a reason"); <https://wg21.link/P2573R2>`_
 
+- Implemented `P2573R2: = delete("should have a reason"); <https://wg21.link/P2573R2>`_
+
 
 Resolutions to C++ Defect Reports
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/CodeGen/CGStmt.cpp b/clang/lib/CodeGen/CGStmt.cpp
index 576fe2f7a2d46f..7a0ad8a73b9fce 100644
--- a/clang/lib/CodeGen/CGStmt.cpp
+++ b/clang/lib/CodeGen/CGStmt.cpp
@@ -908,6 +908,73 @@ void CodeGenFunction::EmitIfStmt(const IfStmt &S) {
     incrementProfileCounter(&S);
 }
 
+bool CodeGenFunction::checkIfLoopMustProgress(const Expr *ControllingExpression,
+                                              bool IsTrivialCXXLoop) {
+  if (CGM.getCodeGenOpts().getFiniteLoops() ==
+      CodeGenOptions::FiniteLoopsKind::Always)
+    return true;
+  if (CGM.getCodeGenOpts().getFiniteLoops() ==
+      CodeGenOptions::FiniteLoopsKind::Never)
+    return false;
+
+  // 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.
+  // As an extension, we consisider loops whose constant expression
+  // can be constant-folded.
+  Expr::EvalResult Result;
+  bool CondIsConstInt =
+      !ControllingExpression ||
+      (ControllingExpression->EvaluateAsInt(Result, getContext()) &&
+       Result.Val.isInt());
+  bool IsTrue = CondIsConstInt &&
+                (!ControllingExpression || Result.Val.getInt().getBoolValue());
+
+  if (getLangOpts().C99 && CondIsConstInt)
+    return false;
+
+  // Loops with non-constant conditions must make progress in C11 and later.
+  if (getLangOpts().C11)
+    return true;
+
+  // [C++26][intro.progress] (DR)
+  // The implementation may assume that any thread will eventually do one of the
+  // following:
+  // [...]
+  // - continue execution of a trivial infinite loop ([stmt.iter.general]).
+  if (getLangOpts().CPlusPlus11) {
+    if (IsTrivialCXXLoop && IsTrue) {
+      CurFn->removeFnAttr(llvm::Attribute::MustProgress);
+      return false;
+    }
+    return true;
+  }
+
+  return false;
+}
+
+// [C++26][stmt.iter.general] (DR)
+// A trivially empty iteration statement is an iteration statement matching one
+// of the following forms:
+//  - while ( expression ) ;
+//  - while ( expression ) { }
+//  - do ; while ( expression ) ;
+//  - do { } while ( expression ) ;
+//  - for ( init-statement expression(opt); ) ;
+//  - for ( init-statement expression(opt); ) { }
+template <typename LoopStmt> static bool hasEmptyLoopBody(const LoopStmt &S) {
+  if constexpr (std::is_same_v<LoopStmt, ForStmt>) {
+    if (S.getInc())
+      return false;
+  }
+  const Stmt *Body = S.getBody();
+  if (!Body || isa<NullStmt>(Body))
+    return true;
+  if (const CompoundStmt *Compound = dyn_cast<CompoundStmt>(Body))
+    return Compound->body_empty();
+  return false;
+}
+
 void CodeGenFunction::EmitWhileStmt(const WhileStmt &S,
                                     ArrayRef<const Attr *> WhileAttrs) {
   // Emit the header for the loop, which will also become
@@ -942,13 +1009,12 @@ void CodeGenFunction::EmitWhileStmt(const WhileStmt &S,
   // while(1) is common, avoid extra exit blocks.  Be sure
   // to correctly handle break/continue though.
   llvm::ConstantInt *C = dyn_cast<llvm::ConstantInt>(BoolCondVal);
-  bool CondIsConstInt = C != nullptr;
-  bool EmitBoolCondBranch = !CondIsConstInt || !C->isOne();
+  bool EmitBoolCondBranch = !C || !C->isOne();
   const SourceRange &R = S.getSourceRange();
   LoopStack.push(LoopHeader.getBlock(), CGM.getContext(), CGM.getCodeGenOpts(),
                  WhileAttrs, SourceLocToDebugLoc(R.getBegin()),
                  SourceLocToDebugLoc(R.getEnd()),
-                 checkIfLoopMustProgress(CondIsConstInt));
+                 checkIfLoopMustProgress(S.getCond(), hasEmptyLoopBody(S)));
 
   // When single byte coverage mode is enabled, add a counter to loop condition.
   if (llvm::EnableSingleByteCoverage)
@@ -1059,14 +1125,13 @@ void CodeGenFunction::EmitDoStmt(const DoStmt &S,
   // "do {} while (0)" is common in macros, avoid extra blocks.  Be sure
   // to correctly handle break/continue though.
   llvm::ConstantInt *C = dyn_cast<llvm::ConstantInt>(BoolCondVal);
-  bool CondIsConstInt = C;
   bool EmitBoolCondBranch = !C || !C->isZero();
 
   const SourceRange &R = S.getSourceRange();
   LoopStack.push(LoopBody, CGM.getContext(), CGM.getCodeGenOpts(), DoAttrs,
                  SourceLocToDebugLoc(R.getBegin()),
                  SourceLocToDebugLoc(R.getEnd()),
-                 checkIfLoopMustProgress(CondIsConstInt));
+                 checkIfLoopMustProgress(S.getCond(), hasEmptyLoopBody(S)));
 
   // As long as the condition is true, iterate the loop.
   if (EmitBoolCondBranch) {
@@ -1109,15 +1174,11 @@ void CodeGenFunction::EmitForStmt(const ForStmt &S,
   llvm::BasicBlock *CondBlock = CondDest.getBlock();
   EmitBlock(CondBlock);
 
-  Expr::EvalResult Result;
-  bool CondIsConstInt =
-      !S.getCond() || S.getCond()->EvaluateAsInt(Result, getContext());
-
   const SourceRange &R = S.getSourceRange();
   LoopStack.push(CondBlock, CGM.getContext(), CGM.getCodeGenOpts(), ForAttrs,
                  SourceLocToDebugLoc(R.getBegin()),
                  SourceLocToDebugLoc(R.getEnd()),
-                 checkIfLoopMustProgress(CondIsConstInt));
+                 checkIfLoopMustProgress(S.getCond(), hasEmptyLoopBody(S)));
 
   // Create a cleanup scope for the condition variable cleanups.
   LexicalScope ConditionScope(*this, S.getSourceRange());
diff --git a/clang/lib/CodeGen/CodeGenFunction.cpp b/clang/lib/CodeGen/CodeGenFunction.cpp
index 86a6ddd80cc114..95c0784ad9760c 100644
--- a/clang/lib/CodeGen/CodeGenFunction.cpp
+++ b/clang/lib/CodeGen/CodeGenFunction.cpp
@@ -1465,6 +1465,7 @@ void CodeGenFunction::GenerateCode(GlobalDecl GD, llvm::Function *Fn,
 
   // Ensure that the function adheres to the forward progress guarantee, which
   // is required by certain optimizations.
+  // The attribute will be removed if the body contains a trivial empty loop.
   if (checkIfFunctionMustProgress())
     CurFn->addFnAttr(llvm::Attribute::MustProgress);
 
diff --git a/clang/lib/CodeGen/CodeGenFunction.h b/clang/lib/CodeGen/CodeGenFunction.h
index ff1873325d409f..f0e8e47bc0e420 100644
--- a/clang/lib/CodeGen/CodeGenFunction.h
+++ b/clang/lib/CodeGen/CodeGenFunction.h
@@ -635,28 +635,7 @@ class CodeGenFunction : public CodeGenTypeCache {
   /// Returns true if a loop must make progress, which means the mustprogress
   /// attribute can be added. \p HasConstantCond indicates whether the branch
   /// condition is a known constant.
-  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;
-  }
+  bool checkIfLoopMustProgress(const Expr *, bool IsTrivialCXXLoop);
 
   const CodeGen::CGBlockInfo *BlockInfo = nullptr;
   llvm::Value *BlockPointer = nullptr;
diff --git a/clang/test/CodeGenCXX/attr-mustprogress.cpp b/clang/test/CodeGenCXX/attr-mustprogress.cpp
index 843f5460426ccf..5a24e9426d1308 100644
--- a/clang/test/CodeGenCXX/attr-mustprogress.cpp
+++ b/clang/test/CodeGenCXX/attr-mustprogress.cpp
@@ -24,21 +24,21 @@ int b = 0;
 // CHECK: datalayout
 
 // CXX98-NOT:  mustprogress
-// CXX11:      mustprogress
+// CXX11-NOT:  mustprogress
 // FINITE-NOT: mustprogress
 // CHECK-LABEL: @_Z2f0v(
 // CHECK-NEXT:  entry:
 // CHECK-NEXT:    br label %for.cond
 // CHECK:       for.cond:
 // CXX98-NOT:    br {{.*}} llvm.loop
-// CXX11-NEXT:   br label %for.cond, !llvm.loop [[LOOP1:!.*]]
+// CXX11-NOT:    br {{.*}} llvm.loop
 // FINITE-NEXT:  br label %for.cond, !llvm.loop [[LOOP1:!.*]]
 void f0() {
   for (; ;) ;
 }
 
 // CXX98-NOT:  mustprogress
-// CXX11:      mustprogress
+// CXX11-NOT:  mustprogress
 // FINITE-NOT: mustprogress
 // CHECK-LABEL: @_Z2f1v(
 // CHECK-NEXT:  entry:
@@ -46,8 +46,8 @@ void f0() {
 // CHECK:       for.cond:
 // CHECK-NEXT:    br i1 true, label %for.body, label %for.end
 // CHECK:       for.body:
-// CXX98-NOT:     br {{.*}}, !llvm.loop
-// CXX11-NEXT:    br label %for.cond, !llvm.loop [[LOOP2:!.*]]
+// CXX98-NOT:    br {{.*}}, !llvm.loop
+// CXX11-NOT:    br {{.*}} llvm.loop
 // FINITE-NEXT:  br label %for.cond, !llvm.loop [[LOOP2:!.*]]
 // CHECK:       for.end:
 // CHECK-NEXT:    ret void
@@ -81,7 +81,7 @@ void f2() {
 }
 
 // CXX98-NOT:  mustprogress
-// CXX11:      mustprogress
+// CXX11-NOT:  mustprogress
 // FINITE-NOT: mustprogress
 // CHECK-LABEL: @_Z1Fv(
 // CHECK-NEXT:  entry:
@@ -90,7 +90,7 @@ void f2() {
 // CHECK-NEXT:    br i1 true, label %for.body, label %for.end
 // CHECK:       for.body:
 // CXX98-NOT:     br {{.*}}, !llvm.loop
-// CXX11-NEXT:    br label %for.cond, !llvm.loop [[LOOP4:!.*]]
+// CXX11-NOT:     br {{.*}}, !llvm.loop
 // FINITE-NEXT:   br label %for.cond, !llvm.loop [[LOOP4:!.*]]
 // CHECK:       for.end:
 // CHECK-NEXT:    br label %for.cond1
@@ -114,7 +114,7 @@ void F() {
 }
 
 // CXX98-NOT:  mustprogress
-// CXX11:      mustprogress
+// CXX11-NOT:  mustprogress
 // FINITE-NOT: mustprogress
 // CHECK-LABEL: @_Z2F2v(
 // CHECK-NEXT:  entry:
@@ -134,7 +134,7 @@ void F() {
 // CHECK-NEXT:    br i1 true, label %for.body2, label %for.end3
 // CHECK:       for.body2:
 // CXX98-NOT:     br {{.*}}, !llvm.loop
-// CXX11-NEXT:    br label %for.cond1, !llvm.loop [[LOOP7:!.*]]
+// CXX11-NOT:     br {{.*}}, !llvm.loop
 // FINITE-NEXT:   br label %for.cond1, !llvm.loop [[LOOP7:!.*]]
 // CHECK:       for.end3:
 // CHECK-NEXT:    ret void
@@ -147,14 +147,14 @@ void F2() {
 }
 
 // CXX98-NOT:  mustprogress
-// CXX11:      mustprogress
+// CXX11-NOT:  mustprogress
 // FINITE-NOT: mustprogress
 // CHECK-LABEL: @_Z2w1v(
 // CHECK-NEXT:  entry:
 // CHECK-NEXT:    br label %while.body
 // CHECK:       while.body:
 // CXX98-NOT:     br {{.*}}, !llvm.loop
-// CXX11-NEXT:    br label %while.body, !llvm.loop [[LOOP8:!.*]]
+// CXX11-NOT:     br {{.*}}, !llvm.loop
 // FINITE-NEXT:   br label %while.body, !llvm.loop [[LOOP8:!.*]]
 //
 void w1() {
@@ -186,7 +186,7 @@ void w2() {
 }
 
 // CXX98-NOT:  mustprogress
-// CXX11:      mustprogress
+// CXX11-NOT:  mustprogress
 // FINITE-NOT: mustprogress
 // CHECK-LABEL: @_Z1Wv(
 // CHECK-NEXT:  entry:
@@ -204,7 +204,7 @@ void w2() {
 // CHECK-NEXT:    br label %while.body2
 // CHECK:       while.body2:
 // CXX98-NOT:    br {{.*}}, !llvm.loop
-// CXX11-NEXT:   br label %while.body2, !llvm.loop [[LOOP11:!.*]]
+// CXX11-NOT:    br {{.*}}, !llvm.loop
 // FINITE-NEXT:  br label %while.body2, !llvm.loop [[LOOP11:!.*]]
 //
 void W() {
@@ -215,14 +215,14 @@ void W() {
 }
 
 // CXX98-NOT:  mustprogress
-// CXX11:      mustprogress
+// CXX11-NOT:  mustprogress
 // FINITE-NOT: mustprogress
 // CHECK-LABEL: @_Z2W2v(
 // CHECK-NEXT:  entry:
 // CHECK-NEXT:    br label %while.body
 // CHECK:       while.body:
 // CXX98-NOT:     br {{.*}}, !llvm.loop
-// CXX11-NEXT:    br label %while.body, !llvm.loop [[LOOP12:!.*]]
+// CXX11-NOT:     br label %while.body, !llvm.loop [[LOOP12:!.*]]
 // FINITE-NEXT:   br label %while.body, !llvm.loop [[LOOP12:!.*]]
 //
 void W2() {
@@ -233,7 +233,7 @@ void W2() {
 }
 
 // CXX98-NOT:  mustprogress
-// CXX11:      mustprogress
+// CXX11-NOT:  mustprogress
 // FINITE-NOT: mustprogress
 // CHECK-LABEL: @_Z2d1v(
 // CHECK-NEXT:  entry:
@@ -242,7 +242,7 @@ void W2() {
 // CHECK-NEXT:    br label %do.cond
 // CHECK:       do.cond:
 // CXX98-NOT:     br {{.*}}, !llvm.loop
-// CXX11-NEXT:    br i1 true, label %do.body, label %do.end, !llvm.loop [[LOOP13:!.*]]
+// CXX11-NOT:     br {{.*}}, !llvm.loop
 // FINITE-NEXT:   br i1 true, label %do.body, label %do.end, !llvm.loop [[LOOP13:!.*]]
 // CHECK:       do.end:
 // CHECK-NEXT:    ret void
@@ -278,7 +278,7 @@ void d2() {
 }
 
 // CXX98-NOT:  mustprogress
-// CXX11:      mustprogress
+// CXX11-NOT:  mustprogress
 // FINITE-NOT: mustprogress
 // CHECK-LABEL: @_Z1Dv(
 // CHECK-NEXT:  entry:
@@ -287,7 +287,7 @@ void d2() {
 // CHECK-NEXT:    br label %do.cond
 // CHECK:       do.cond:
 // CXX98-NOT:     br {{.*}}, !llvm.loop
-// CXX11-NEXT:    br i1 true, label %do.body, label %do.end, !llvm.loop [[LOOP15:!.*]]
+// CXX11-NOT:     br {{.*}}, !llvm.loop
 // FINITE-NEXT:   br i1 true, label %do.body, label %do.end, !llvm.loop [[LOOP15:!.*]]
 // CHECK:       do.end:
 // CHECK-NEXT:    br label %do.body1
@@ -312,8 +312,8 @@ void D() {
   while (a == b);
 }
 
-// CXX98-NOT : mustprogress
-// CXX11:      mustprogress
+// CXX98-NOT:  mustprogress
+// CXX11-NOT:  mustprogress
 // FINITE-NOT: mustprogress
 // CHECK-LABEL: @_Z2D2v(
 // CHECK-NEXT:  entry:
@@ -333,7 +333,7 @@ void D() {
 // CHECK-NEXT:    br label %do.cond2
 // CHECK:       do.cond2:
 // CXX98-NOT:     br {{.*}}, !llvm.loop
-// CXX11-NEXT:    br i1 true, label %do.body1, label %do.end3, !llvm.loop [[LOOP18:!.*]]
+// CXX11-NOT:     br {{.*}}, !llvm.loop
 // FINITE-NEXT:   br i1 true, label %do.body1, label %do.end3, !llvm.loop [[LOOP18:!.*]]
 // CHECK:       do.end3:
 // CHECK-NEXT:    ret void
@@ -347,22 +347,12 @@ void D2() {
   while (1);
 }
 
-// CXX11: [[LOOP1]] = distinct !{[[LOOP1]], [[MP:!.*]]}
+// CXX11: [[LOOP1:.*]] = distinct !{[[LOOP1]], [[MP:.*]]}
 // CXX11: [[MP]] = !{!"llvm.loop.mustprogress"}
-// CXX11: [[LOOP2]] = distinct !{[[LOOP2]], [[MP]]}
-// CXX11: [[LOOP3]] = distinct !{[[LOOP3]], [[MP]]}
-// CXX11: [[LOOP4]] = distinct !{[[LOOP4]], [[MP]]}
-// CXX11: [[LOOP5]] = distinct !{[[LOOP5]], [[MP]]}
-// CXX11: [[LOOP6]] = distinct !{[[LOOP6]], [[MP]]}
-// CXX11: [[LOOP7]] = distinct !{[[LOOP7]], [[MP]]}
-// CXX11: [[LOOP8]] = distinct !{[[LOOP8]], [[MP]]}
-// CXX11: [[LOOP9]] = distinct !{[[LOOP9]], [[MP]]}
-// CXX11: [[LOOP10]] = distinct !{[[LOOP10]], [[MP]]}
-// CXX11: [[LOOP11]] = distinct !{[[LOOP11]], [[MP]]}
-// CXX11: [[LOOP12]] = distinct !{[[LOOP12]], [[MP]]}
-// CXX11: [[LOOP13]] = distinct !{[[LOOP13]], [[MP]]}
-// CXX11: [[LOOP14]] = distinct !{[[LOOP14]], [[MP]]}
-// CXX11: [[LOOP15]] = distinct !{[[LOOP15]], [[MP]]}
-// CXX11: [[LOOP16]] = distinct !{[[LOOP16]], [[MP]]}
-// CXX11: [[LOOP17]] = distinct !{[[LOOP17]], [[MP]]}
-// CXX11: [[LOOP18]] = distinct !{[[LOOP18]], [[MP]]}
+// CXX11: [[LOOP2:.*]] = distinct !{[[LOOP2]], [[MP]]}
+// CXX11: [[LOOP3:.*]] = distinct !{[[LOOP3]], [[MP]]}
+// CXX11: [[LOOP4:.*]] = distinct !{[[LOOP4]], [[MP]]}
+// CXX11: [[LOOP5:.*]] = distinct !{[[LOOP5]], [[MP]]}
+// CXX11: [[LOOP6:.*]] = distinct !{[[LOOP6]], [[MP]]}
+// CXX11: [[LOOP7:.*]] = distinct !{[[LOOP7]], [[MP]]}
+// CXX11: [[LOOP8:.*]] = distinct !{[[LOOP8]], [[MP]]}
diff --git a/clang/www/cxx_status.html b/clang/www/cxx_status.html
index c233171e63c811..0d796597d05c0e 100755
--- a/clang/www/cxx_status.html
+++ b/clang/www/cxx_status.html
@@ -187,7 +187,7 @@ <h2 id="cxx26">C++2c implementation status</h2>
  <tr>
   <td>Trivial infinite loops are not Undefined Behavior</td>
   <td><a href="https://wg21.link/P2809R3">P2809R3</a> (<a href="#dr">DR</a>)</td>
-  <td class="none" align="center">No</td>
+  <td class="unreleased" align="center">Clang 19</td>
  </tr>
  <tr>
   <td>Erroneous behaviour for uninitialized reads</td>

erichkeane · 2024-04-25T14:44:05Z

clang/docs/ReleaseNotes.rst

@@ -131,6 +131,8 @@ C++2c Feature Support

 - Implemented `P2573R2: = delete("should have a reason"); <https://wg21.link/P2573R2>`_

+- Implemented `P2573R2: = delete("should have a reason"); <https://wg21.link/P2573R2>`_


What is going on here?

Good question!

erichkeane · 2024-04-25T14:45:24Z

clang/lib/CodeGen/CGStmt.cpp

@@ -908,6 +908,73 @@ void CodeGenFunction::EmitIfStmt(const IfStmt &S) {
    incrementProfileCounter(&S);
 }

+bool CodeGenFunction::checkIfLoopMustProgress(const Expr *ControllingExpression,
+                                              bool IsTrivialCXXLoop) {
+  if (CGM.getCodeGenOpts().getFiniteLoops() ==


Is this not reversed? If a loop is finite, I thought it is NOT MustProgress?

Hrm... I see this is exsting code, but this confuses me/is reverse from what I thought.

erichkeane · 2024-04-25T14:48:08Z

clang/lib/CodeGen/CGStmt.cpp

+      !ControllingExpression ||
+      (ControllingExpression->EvaluateAsInt(Result, getContext()) &&
+       Result.Val.isInt());
+  bool IsTrue = CondIsConstInt &&


Suggested change

bool IsTrue = CondIsConstInt &&

bool CondIsTrue = CondIsConstInt &&

clang/lib/CodeGen/CGStmt.cpp

Whether a function make progress is determined exclusively by loops metadata.

clang/docs/ReleaseNotes.rst

This reverts commit 5671ea9.

shafik · 2024-04-29T18:28:32Z

clang/lib/CodeGen/CodeGenFunction.cpp

@@ -1465,6 +1465,7 @@ void CodeGenFunction::GenerateCode(GlobalDecl GD, llvm::Function *Fn,

  // Ensure that the function adheres to the forward progress guarantee, which
  // is required by certain optimizations.
+  // The attribute will be removed if the body contains a trivial empty loop.


Suggested change

// The attribute will be removed if the body contains a trivial empty loop.

// In C++11 and forward, the attribute will be removed if the body contains a trivial empty loop.

shafik · 2024-04-29T18:35:04Z

clang/lib/CodeGen/CGStmt.cpp

+  const Stmt *Body = S.getBody();
+  if (!Body || isa<NullStmt>(Body))
+    return true;
+  if (const CompoundStmt *Compound = dyn_cast<CompoundStmt>(Body))


It does not look like the test cover the CompoundStmt case.

I added some tests (and test for false conditions, which are not trivial in c++)

yronglin · 2024-04-30T00:41:26Z

I'd like to add the test case from the paper:

#include <iostream>

int main() {
  while (true)
    ; 
}

void unreachable() {
  std::cout << "Hello world!" << std::endl;
}

I tried this PR locally, the empty loop will not been deleted now(with -O3). the generated IR looks good.
Before:

; Function Attrs: mustprogress nofree norecurse noreturn nosync nounwind willreturn memory(none) uwtable
define dso_local noundef i32 @main() local_unnamed_addr #0 {
  unreachable
}

Since this PR:

; Function Attrs: nofree norecurse noreturn nosync nounwind memory(none) uwtable
define dso_local noundef i32 @main() local_unnamed_addr #0 {
  br label %1

1:                                                ; preds = %0, %1
  br label %1
}

cor3ntin · 2024-04-30T06:07:29Z

@yronglin I'm reluctant to do that, that would be testing the optimizer in the front end.
If the patch did not fix this bug, there would be a back end bug that should be fixed there

yronglin · 2024-04-30T09:21:10Z

@yronglin I'm reluctant to do that, that would be testing the optimizer in the front end. If the patch did not fix this bug, there would be a back end bug that should be fixed there

Agree!

…n/infiniteloop

erichkeane

This LGTM, but please give @efriedma-quic a day or two to confirm this looks right for him too.

github-actions · 2024-04-30T13:26:38Z

✅ With the latest revision this PR passed the C/C++ code formatter.

efriedma-quic · 2024-04-30T17:15:10Z

clang/lib/CodeGen/CGStmt.cpp

+
+  if (CGM.getCodeGenOpts().getFiniteLoops() ==
+          CodeGenOptions::FiniteLoopsKind::Always &&
+      !getLangOpts().CPlusPlus11)


If we're going to define -ffinite-loops to allow trivial infinite loops, might as well do it in all modes, not just C++11 mode?

Yes, I wasn't sure what to do here. Does it make sense to override the standard? but given the loop is for sure infinite, it could be spelled --ub.
On the other hand, if we override all modes, is the flag useful at all?

There are basically four modes we have logic for here:

No mustprogress optimizations

C11 mustprogress

C++23 mustprogress

always mustprogress

I don't think there's really much reason to keep "always mustprogress" mode; like you note, it's not really useful. Let's just make -ffinite-loops mean the new C++ rules.

Of course, -fno-finite-loops means no mustprogress.

This does mean we don't provide a way to explicitly access C11 mustprogress mode, I guess; it's not equivalent to either of -ffinite-loops or -fno-finite-loops. But not sure how much we care. We could add a flag -fc11-finite-loops if someone does care.

…n/infiniteloop

even in the presence of -ffinite-loops

efriedma-quic · 2024-05-02T18:52:34Z

clang/lib/CodeGen/CGStmt.cpp

+  bool CondIsTrue = CondIsConstInt && (!ControllingExpression ||
+                                       Result.Val.getInt().getBoolValue());
+
+  if (getLangOpts().C99 && CondIsConstInt)


Where did getLangOpts().C99 come from? I think the old code treated c89/c99/c++98 as equivalent (never must-progress).

Where did getLangOpts().C99 come from? I think the old code treated c89/c99/c++98 as equivalent (never must-progress).

Good catch. that is supposed to be C89. I guess C && !c11

This turns out to just not be useful at all (but harmless) because we end up landing at the end of the function (and return false there)

efriedma-quic

LGTM

cor3ntin · 2024-05-02T20:22:08Z

@erichkeane @efriedma-quic Thanks for the review, I'll probably land that tomorrow.

hubert-reinterpretcast

@cor3ntin, can you take a look at the case I added? Thanks.

hubert-reinterpretcast · 2024-05-04T00:24:28Z

clang/lib/CodeGen/CGStmt.cpp

+  // As an extension, we consisider loops whose constant expression
+  // can be constant-folded.
+  Expr::EvalResult Result;
+  bool CondIsConstInt =


The following does not have UB according to the standard, but Clang treats it as UB:

int main(void) { int x = 1; while (__builtin_is_constant_evaluated() || x); return 0; }

is_constant_evaluated is equivalent to if consteval, i.e. it's true in a context which is manifestly constant-evaluated. So... is the condition manifestly constant-evaluated in this case? If it is, that's fine, I guess, but the standard definition of "manifestly constant-evaluated" should probably be amended to reflect that. Otherwise, I'm not sure how to interpret the standard here; whether an expression is manifestly constant-evaluated changes the way it's represented in the AST.

See also #89565.

One case to consider:

int main() { while (!__builtin_is_constant_evaluated()) {} }

If you treat the condition of the loop as manifestly constant-evaluated, the loop is finite: the condition evaluates to false. If we say it's not manifestly constant-evaluated, but do some sort of constant-evaluation anyway, it's a well-defined infinite loop. If we somehow treat it as manifestly constant-evaluated for the constant evaluation, but don't use the result of the constant evaluation at runtime, it's UB.

I guess we could also say that if the condition would evaluate to false if we treat it as manifestly constant-evaluated, it's not manifestly constant-evaluated. So we'd amend [expr.const]p20 to say something like: "An expression or conversion is manifestly constant-evaluated if it is [...] the condition of trivially empty iteration statement, is a constant expression when interpreted as a constant-expression, and the constant expression would evaluate to true".

In any case, addressing this probably doesn't require modifying any of the code in this patch; the change would be to make Sema introduce a ConstantExpr when appropriate.

I guess we could also say that if the condition would evaluate to false if we treat it as manifestly constant-evaluated, it's not manifestly constant-evaluated. So we'd amend [expr.const]p20 to say something like: "An expression or conversion is manifestly constant-evaluated if it is [...] the condition of trivially empty iteration statement, is a constant expression when interpreted as a constant-expression, and the constant expression would evaluate to true".

Do we have any motivation for doing so?
In addition of being extremely mind-bendy, there is no practical application.

As a standards-wording issue, the standard has to do something here: it doesn't define what it means to constant-evaluate something in a context that isn't manifestly constant-evaluated.

As a practical matter, it's very unlikely the precise wording the standard uses actually matters, sure.

As a standards-wording issue, the standard has to do something here

This was extensively discussed in CWG. The wording makes the evaluation for the purposes of determining whether the loop is trivially infinite a manifestly constant-evaluated context. It leaves the normal evaluation as not a manifestly constant-evaluated context.

The operative words are "interpreted as a constant-expression" in https://eel.is/c++draft/stmt.iter.general#3.

So... we treat it as a manifestly constant-evaluated for the purpose of checking whether the loop is trivial, but then flips to not manifestly constant-evaluated for the actual evaluation at runtime? The wording could use some clarification...

Due to the way Sema::CheckForImmediateInvocation works, once we decide an expression is not manifestly constant-evaluated, we can't actually constant-evaluate it, I think; the AST is modified. Maybe we can run constant-evaluation before Sema::CheckForImmediateInvocation runs, though.

So... we treat it as a manifestly constant-evaluated for the purpose of checking whether the loop is trivial, but then flips to not manifestly constant-evaluated for the actual evaluation at runtime?

Yes.

The wording could use some clarification...

The absence of wording to change the condition itself (that is, what is used at runtime) to be manifestly constant-evaluated is intentional. This wording is not unique. It is also used for the determination of constant initialization, which #89565 (that you referred to) points out is also broken in Clang (however, in contrast to the condition case, once determined to be constant initialization, the initializer itself is considered manifestly constant-evaluated).

Maybe we can run constant-evaluation before Sema::CheckForImmediateInvocation runs, though.

I think that makes sense. The constant-expression that we are evaluating introduces an immediate function context that suppresses immediate invocations (https://eel.is/c++draft/expr.const#16).

Example:

struct A { constexpr A(const int &x) : p(&x) {} const int *p; }; consteval A a() { return {42}; } enum { X = (a(), 0) }; // OK, no immediate invocations; Clang and GCC both accept

[Clang] Implement P2809: Trivial infinite loops are not Undefined Beh…

a89ed30

…avior https://wg21.link/P2809R3

cor3ntin requested review from AaronBallman and erichkeane April 25, 2024 14:42

llvmbot added clang Clang issues not falling into any other category clang:codegen IR generation bugs: mangling, exceptions, etc. labels Apr 25, 2024

erichkeane reviewed Apr 25, 2024

View reviewed changes

efriedma-quic reviewed Apr 25, 2024

View reviewed changes

clang/lib/CodeGen/CGStmt.cpp Show resolved Hide resolved

Remove the mustprogress attributes on functions.

5671ea9

Whether a function make progress is determined exclusively by loops metadata.

llvmbot added clang:modules C++20 modules and Clang Header Modules clang:openmp OpenMP related changes to Clang labels Apr 29, 2024

cor3ntin added 2 commits April 29, 2024 14:57

Fix release notes

c7293a0

Address Erich's feedback

2d9290b

yronglin reviewed Apr 29, 2024

View reviewed changes

clang/docs/ReleaseNotes.rst Outdated Show resolved Hide resolved

cor3ntin added 2 commits April 29, 2024 15:19

Fix release notes (take 2)

0323287

Revert "Remove the mustprogress attributes on functions."

7d8aa18

This reverts commit 5671ea9.

shafik reviewed Apr 29, 2024

View reviewed changes

Fix tests and Address Shafik's comments

9e656a8

vogelsgesang mentioned this pull request Apr 29, 2024

[Clang] Address post commit feedbacks in #89906 #90495

Merged

Merge remote-tracking branch 'llvm_be_very_careful/main' into corenti…

043132f

…n/infiniteloop

erichkeane approved these changes Apr 30, 2024

View reviewed changes

cor3ntin added 2 commits April 30, 2024 15:36

Format

b99e8b2

Format again

6b43d8c

efriedma-quic reviewed Apr 30, 2024

View reviewed changes

cor3ntin added 2 commits May 1, 2024 10:53

Merge remote-tracking branch 'llvm_be_very_careful/main' into corenti…

d97c458

…n/infiniteloop

Trivial infinite loops are considered infinite,

2acda98

even in the presence of -ffinite-loops

efriedma-quic reviewed May 2, 2024

View reviewed changes

cor3ntin force-pushed the corentin/infiniteloop branch from f0a707e to c5991ed Compare May 2, 2024 20:11

efriedma-quic approved these changes May 2, 2024

View reviewed changes

Remove useless C99 check

aa5f785

cor3ntin force-pushed the corentin/infiniteloop branch from c5991ed to aa5f785 Compare May 2, 2024 20:20

cor3ntin merged commit 6421171 into llvm:main May 3, 2024
5 checks passed

cor3ntin deleted the corentin/infiniteloop branch May 3, 2024 12:10

hubert-reinterpretcast reviewed May 4, 2024

View reviewed changes

MitalAshok mentioned this pull request Jun 30, 2024

clang >=13.0.0 seems provide invalid optimize for C++, empty while(true) loop and -O1 #97211

Open

		@@ -131,6 +131,8 @@ C++2c Feature Support

		- Implemented `P2573R2: = delete("should have a reason"); <https://wg21.link/P2573R2>`_

		- Implemented `P2573R2: = delete("should have a reason"); <https://wg21.link/P2573R2>`_

	bool IsTrue = CondIsConstInt &&
	bool CondIsTrue = CondIsConstInt &&

	// The attribute will be removed if the body contains a trivial empty loop.
	// In C++11 and forward, the attribute will be removed if the body contains a trivial empty loop.

[Clang] Implement P2809: Trivial infinite loops are not Undefined Behavior #90066

[Clang] Implement P2809: Trivial infinite loops are not Undefined Behavior #90066

Uh oh!

Conversation

cor3ntin commented Apr 25, 2024

Uh oh!

llvmbot commented Apr 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yronglin commented Apr 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cor3ntin commented Apr 30, 2024

Uh oh!

yronglin commented Apr 30, 2024

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efriedma-quic left a comment

Choose a reason for hiding this comment

Uh oh!

cor3ntin commented May 2, 2024

Uh oh!

Uh oh!

hubert-reinterpretcast left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efriedma-quic May 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

llvmbot commented Apr 25, 2024 •

edited

Loading

yronglin commented Apr 30, 2024 •

edited

Loading

github-actions bot commented Apr 30, 2024 •

edited

Loading

efriedma-quic May 4, 2024 •

edited

Loading