Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 13 commits into from
May 3, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
a89ed30
[Clang] Implement P2809: Trivial infinite loops are not Undefined Beh…
cor3ntin Apr 25, 2024
5671ea9
Remove the mustprogress attributes on functions.
cor3ntin Apr 29, 2024
c7293a0
Fix release notes
cor3ntin Apr 29, 2024
2d9290b
Address Erich's feedback
cor3ntin Apr 29, 2024
0323287
Fix release notes (take 2)
cor3ntin Apr 29, 2024
7d8aa18
Revert "Remove the mustprogress attributes on functions."
cor3ntin Apr 29, 2024
9e656a8
Fix tests and Address Shafik's comments
cor3ntin Apr 29, 2024
043132f
Merge remote-tracking branch 'llvm_be_very_careful/main' into corenti…
cor3ntin Apr 30, 2024
b99e8b2
Format
cor3ntin Apr 30, 2024
6b43d8c
Format again
cor3ntin Apr 30, 2024
d97c458
Merge remote-tracking branch 'llvm_be_very_careful/main' into corenti…
cor3ntin May 1, 2024
2acda98
Trivial infinite loops are considered infinite,
cor3ntin May 1, 2024
aa5f785
Remove useless C99 check
cor3ntin May 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions clang/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,9 @@ C++2c Feature Support

- Implemented `P2748R5 Disallow Binding a Returned Glvalue to a Temporary <https://wg21.link/P2748R5>`_.

- Implemented `P2809R3: Trivial infinite loops are not Undefined Behavior <https://wg21.link/P2809R3>`_.


Resolutions to C++ Defect Reports
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
- Substitute template parameter pack, when it is not explicitly specified
Expand Down Expand Up @@ -294,6 +297,10 @@ Modified Compiler Flags
- Carved out ``-Wformat`` warning about scoped enums into a subwarning and
make it controlled by ``-Wformat-pedantic``. Fixes #GH88595.

- Trivial infinite loops (i.e loops with a constant controlling expresion
evaluating to ``true`` and an empty body such as ``while(1);``)
are considered infinite, even when the ``-ffinite-loop`` flag is set.

Removed Compiler Flags
-------------------------

Expand Down
2 changes: 1 addition & 1 deletion clang/include/clang/Driver/Options.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3971,7 +3971,7 @@ def funroll_loops : Flag<["-"], "funroll-loops">, Group<f_Group>,
def fno_unroll_loops : Flag<["-"], "fno-unroll-loops">, Group<f_Group>,
HelpText<"Turn off loop unroller">, Visibility<[ClangOption, CC1Option]>;
def ffinite_loops: Flag<["-"], "ffinite-loops">, Group<f_Group>,
HelpText<"Assume all loops are finite.">, Visibility<[ClangOption, CC1Option]>;
HelpText<"Assume all non-trivial loops are finite.">, Visibility<[ClangOption, CC1Option]>;
def fno_finite_loops: Flag<["-"], "fno-finite-loops">, Group<f_Group>,
HelpText<"Do not assume that any loop is finite.">,
Visibility<[ClangOption, CC1Option]>;
Expand Down
77 changes: 67 additions & 10 deletions clang/lib/CodeGen/CGStmt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -908,6 +908,69 @@ void CodeGenFunction::EmitIfStmt(const IfStmt &S) {
incrementProfileCounter(&S);
}

bool CodeGenFunction::checkIfLoopMustProgress(const Expr *ControllingExpression,
bool HasEmptyBody) {
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 =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@efriedma-quic efriedma-quic May 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

!ControllingExpression ||
(ControllingExpression->EvaluateAsInt(Result, getContext()) &&
Result.Val.isInt());

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

// Loops with non-constant conditions must make progress in C11 and later.
if (getLangOpts().C11 && !CondIsConstInt)
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 (CGM.getCodeGenOpts().getFiniteLoops() ==
CodeGenOptions::FiniteLoopsKind::Always ||
getLangOpts().CPlusPlus11) {
if (HasEmptyBody && CondIsTrue) {
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))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
Expand Down Expand Up @@ -942,13 +1005,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)
Expand Down Expand Up @@ -1059,14 +1121,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) {
Expand Down Expand Up @@ -1109,15 +1170,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());
Expand Down
2 changes: 2 additions & 0 deletions clang/lib/CodeGen/CodeGenFunction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1471,6 +1471,8 @@ void CodeGenFunction::GenerateCode(GlobalDecl GD, llvm::Function *Fn,

// Ensure that the function adheres to the forward progress guarantee, which
// is required by certain optimizations.
// In C++11 and up, the attribute will be removed if the body contains a
// trivial empty loop.
if (checkIfFunctionMustProgress())
CurFn->addFnAttr(llvm::Attribute::MustProgress);

Expand Down
23 changes: 1 addition & 22 deletions clang/lib/CodeGen/CodeGenFunction.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -636,28 +636,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 HasEmptyBody);

const CodeGen::CGBlockInfo *BlockInfo = nullptr;
llvm::Value *BlockPointer = nullptr;
Expand Down
12 changes: 6 additions & 6 deletions clang/test/CodeGen/attr-mustprogress.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ int b = 0;
// CHECK: for.cond:
// C99-NOT: br {{.*}}!llvm.loop
// C11-NOT: br {{.*}}!llvm.loop
// FINITE-NEXT: br {{.*}}!llvm.loop
// FINITE-NOR: br {{.*}}!llvm.loop
//
void f0(void) {
for (; ;) ;
Expand All @@ -45,7 +45,7 @@ void f0(void) {
// CHECK: for.body:
// C99-NOT: br {{.*}}, !llvm.loop
// C11-NOT: br {{.*}}, !llvm.loop
// FINITE-NEXT: br {{.*}}, !llvm.loop
// FINITE-NOT: br {{.*}}, !llvm.loop
// CHECK: for.end:
// CHECK-NEXT: ret void
//
Expand Down Expand Up @@ -84,7 +84,7 @@ void f2(void) {
// CHECK: for.body:
// C99-NOT: br {{.*}}, !llvm.loop
// C11-NOT: br {{.*}}, !llvm.loop
// FINITE-NEXT: br {{.*}}, !llvm.loop
// FINITE-NOT: br {{.*}}, !llvm.loop
// CHECK: for.end:
// CHECK-NEXT: br label %for.cond1
// CHECK: for.cond1:
Expand Down Expand Up @@ -113,7 +113,7 @@ void F(void) {
// CHECK: while.body:
// C99-NOT: br {{.*}}, !llvm.loop
// C11-NOT: br {{.*}}, !llvm.loop
// FINITE-NEXT: br {{.*}}, !llvm.loop
// FINITE-NOT: br {{.*}}, !llvm.loop
//
void w1(void) {
while (1) {
Expand Down Expand Up @@ -159,7 +159,7 @@ void w2(void) {
// CHECK: while.body2:
// C99-NOT: br {{.*}} !llvm.loop
// C11-NOT: br {{.*}} !llvm.loop
// FINITE-NEXT: br {{.*}} !llvm.loop
// FINITE-NOT: br {{.*}} !llvm.loop
//
void W(void) {
while (a == b) {
Expand All @@ -177,7 +177,7 @@ void W(void) {
// CHECK: do.cond:
// C99-NOT: br {{.*}}, !llvm.loop
// C11-NOT: br {{.*}}, !llvm.loop
// FINITE-NEXT: br {{.*}}, !llvm.loop
// FINITE-NOT: br {{.*}}, !llvm.loop
// CHECK: do.end:
// CHECK-NEXT: ret void
//
Expand Down
Loading
Loading