rsmith created this revision.
rsmith added a reviewer: rjmccall.
rsmith requested review of this revision.
Herald added a project: clang.
The condition variable is in scope in the loop increment, so we need to
emit the jump destination from wthin the scope of the condition
variable.
For GCC compatibility (and compatibility with real-world 'FOR_EACH'
macros), 'continue' is permitted in a statement expression within the
condition of a for loop, though, so there are two cases here:
- If the for loop has no condition variable, we can emit the jump destination
before emitting the condition.
- If the for loop has a condition variable, we must defer emitting the jump
destination until after emitting the variable. We diagnose a 'continue'
appearing in the initializer of the condition variable, because it would jump
past the initializer into the scope of that variable.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D98816
Files:
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/include/clang/Parse/Parser.h
clang/include/clang/Sema/Scope.h
clang/lib/CodeGen/CGStmt.cpp
clang/lib/Parse/ParseExprCXX.cpp
clang/lib/Parse/ParseStmt.cpp
clang/lib/Sema/SemaStmt.cpp
clang/test/CodeGenCXX/for-cond-var.cpp
clang/test/Parser/cxx2a-init-statement.cpp
clang/test/SemaCXX/scope-check.cpp
Index: clang/test/SemaCXX/scope-check.cpp
===================================================================
--- clang/test/SemaCXX/scope-check.cpp
+++ clang/test/SemaCXX/scope-check.cpp
@@ -629,3 +629,19 @@
}
} // namespace seh
+
+void continue_scope_check() {
+ // These are OK.
+ for (; ({break; true;});) {}
+ for (; ({continue; true;});) {}
+ for (; int n = ({break; 0;});) {}
+ for (; int n = 0; ({break;})) {}
+ for (; int n = 0; ({continue;})) {}
+
+ // This would jump past the initialization of 'n' to the increment (where 'n'
+ // is in scope).
+ for (; int n = ({continue; 0;});) {} // expected-error {{cannot jump from this continue statement to the loop increment; jump bypasses initialization of loop condition variable}}
+
+ // An intervening loop makes it OK again.
+ for (; int n = ({while (true) continue; 0;});) {}
+}
Index: clang/test/Parser/cxx2a-init-statement.cpp
===================================================================
--- clang/test/Parser/cxx2a-init-statement.cpp
+++ clang/test/Parser/cxx2a-init-statement.cpp
@@ -31,4 +31,12 @@
for (int n = 0; static int m = 0; ++n) {} // expected-error {{type name does not allow storage class}}
for (int n = 0; static int m : arr1) {} // expected-error {{loop variable 'm' may not be declared 'static'}}
+
+ // The init-statement and range are not break / continue scopes. (But the body is.)
+ for (int n = ({ break; 0; }); int m : arr1) {} // expected-error {{not in loop}}
+ for (int n = ({ continue; 0; }); int m : arr1) {} // expected-error {{not in loop}}
+ for (int arr[3]; int n : *({ break; &arr; })) {} // expected-error {{not in loop}}
+ for (int arr[3]; int n : *({ continue; &arr; })) {} // expected-error {{not in loop}}
+ for (int n = 0; int m : arr1) { break; }
+ for (int n = 0; int m : arr1) { continue; }
}
Index: clang/test/CodeGenCXX/for-cond-var.cpp
===================================================================
--- /dev/null
+++ clang/test/CodeGenCXX/for-cond-var.cpp
@@ -0,0 +1,125 @@
+// RUN: %clang_cc1 -emit-llvm -o - %s -triple x86_64-linux | FileCheck %s
+
+struct A {
+ A();
+ A(const A &);
+ ~A();
+ operator bool();
+ void *data;
+};
+
+A make();
+bool cond();
+void f(int);
+
+// PR49585: Ensure that 'continue' performs the proper cleanups in the presence
+// of a for loop condition variable.
+//
+// CHECK: define {{.*}} void @_Z7PR49585v(
+void PR49585() {
+ for (
+ // CHECK: call void @_Z1fi(i32 1)
+ // CHECK: br label %[[for_cond:.*]]
+ f(1);
+
+ // CHECK: [[for_cond]]:
+ // CHECK: call {{.*}} @_Z4makev(
+ // CHECK: call {{.*}} @_ZN1AcvbEv(
+ // CHECK: br i1 {{.*}}, label %[[for_body:.*]], label %[[for_cond_cleanup:.*]]
+ A a = make();
+
+ // CHECK: [[for_cond_cleanup]]:
+ // CHECK: store
+ // CHECK: br label %[[cleanup:.*]]
+
+ f(2)) {
+ // CHECK: [[for_body]]:
+ // CHECK: call {{.*}} @_Z4condv(
+ // CHECK: br i1 {{.*}}, label %[[if_then:.*]], label %[[if_end:.*]]
+ if (cond()) {
+ // CHECK: [[if_then]]:
+ // CHECK: call {{.*}} @_Z1fi(i32 3)
+ // CHECK: br label %[[for_inc:.*]]
+ f(3);
+ continue;
+ }
+
+ // CHECK: [[if_end]]:
+ // CHECK: call {{.*}} @_Z1fi(i32 4)
+ // CHECK: br label %[[for_inc]]
+ f(4);
+ }
+
+ // CHECK: [[for_inc]]:
+ // CHECK: call void @_Z1fi(i32 2)
+ // CHECK: store
+ // CHECK: br label %[[cleanup]]
+
+ // CHECK: [[cleanup]]:
+ // CHECK: call void @_ZN1AD1Ev(
+ // CHECK: load
+ // CHECK: switch {{.*}} label
+ // CHECK-NEXT: label %[[cleanup_cont:.*]]
+ // CHECK-NEXT: label %[[for_end:.*]]
+
+ // CHECK: [[cleanup_cont]]:
+ // CHECK: br label %[[for_cond]]
+
+ // CHECK [[for_end]]:
+ // CHECK: ret void
+}
+
+// CHECK: define {{.*}} void @_Z13PR49585_breakv(
+void PR49585_break() {
+ for (
+ // CHECK: call void @_Z1fi(i32 1)
+ // CHECK: br label %[[for_cond:.*]]
+ f(1);
+
+ // CHECK: [[for_cond]]:
+ // CHECK: call {{.*}} @_Z4makev(
+ // CHECK: call {{.*}} @_ZN1AcvbEv(
+ // CHECK: br i1 {{.*}}, label %[[for_body:.*]], label %[[for_cond_cleanup:.*]]
+ A a = make();
+
+ // CHECK: [[for_cond_cleanup]]:
+ // CHECK: store
+ // CHECK: br label %[[cleanup:.*]]
+
+ f(2)) {
+ // CHECK: [[for_body]]:
+ // CHECK: call {{.*}} @_Z4condv(
+ // CHECK: br i1 {{.*}}, label %[[if_then:.*]], label %[[if_end:.*]]
+ if (cond()) {
+ // CHECK: [[if_then]]:
+ // CHECK: call {{.*}} @_Z1fi(i32 3)
+ // CHECK: store
+ // CHECK: br label %[[cleanup:.*]]
+ f(3);
+ break;
+ }
+
+ // CHECK: [[if_end]]:
+ // CHECK: call {{.*}} @_Z1fi(i32 4)
+ // CHECK: br label %[[for_inc]]
+ f(4);
+ }
+
+ // CHECK: [[for_inc]]:
+ // CHECK: call void @_Z1fi(i32 2)
+ // CHECK: store
+ // CHECK: br label %[[cleanup]]
+
+ // CHECK: [[cleanup]]:
+ // CHECK: call void @_ZN1AD1Ev(
+ // CHECK: load
+ // CHECK: switch {{.*}} label
+ // CHECK-NEXT: label %[[cleanup_cont:.*]]
+ // CHECK-NEXT: label %[[for_end:.*]]
+
+ // CHECK: [[cleanup_cont]]:
+ // CHECK: br label %[[for_cond]]
+
+ // CHECK [[for_end]]:
+ // CHECK: ret void
+}
Index: clang/lib/Sema/SemaStmt.cpp
===================================================================
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -3000,6 +3000,12 @@
// C99 6.8.6.2p1: A break shall appear only in or as a loop body.
return StmtError(Diag(ContinueLoc, diag::err_continue_not_in_loop));
}
+ if (S->getFlags() & Scope::ConditionVarScope) {
+ // We cannot 'continue;' from within a statement expression in the
+ // initializer of a condition variable because we would jump past the
+ // initialization of that variable.
+ return StmtError(Diag(ContinueLoc, diag::err_continue_from_cond_var_init));
+ }
CheckJumpOutOfSEHFinally(*this, ContinueLoc, *S);
return new (Context) ContinueStmt(ContinueLoc);
Index: clang/lib/Parse/ParseStmt.cpp
===================================================================
--- clang/lib/Parse/ParseStmt.cpp
+++ clang/lib/Parse/ParseStmt.cpp
@@ -1959,7 +1959,6 @@
}
// Parse the second part of the for specifier.
- getCurScope()->AddFlags(Scope::BreakScope | Scope::ContinueScope);
if (!ForEach && !ForRangeInfo.ParsedForRangeDecl() &&
!SecondPart.isInvalid()) {
// Parse the second part of the for specifier.
@@ -1975,7 +1974,8 @@
ColonProtectionRAIIObject ColonProtection(*this, MightBeForRangeStmt);
SecondPart =
ParseCXXCondition(nullptr, ForLoc, Sema::ConditionKind::Boolean,
- MightBeForRangeStmt ? &ForRangeInfo : nullptr);
+ MightBeForRangeStmt ? &ForRangeInfo : nullptr,
+ /*EnterForConditionScope*/ true);
if (ForRangeInfo.ParsedForRangeDecl()) {
Diag(FirstPart.get() ? FirstPart.get()->getBeginLoc()
@@ -1992,6 +1992,9 @@
}
}
} else {
+ // We permit 'continue' and 'break' in the condition of a for loop.
+ getCurScope()->AddFlags(Scope::BreakScope | Scope::ContinueScope);
+
ExprResult SecondExpr = ParseExpression();
if (SecondExpr.isInvalid())
SecondPart = Sema::ConditionError();
@@ -2003,6 +2006,11 @@
}
}
+ // Enter a break / continue scope, if we didn't already enter one while
+ // parsing the second part.
+ if (!(getCurScope()->getFlags() & Scope::ContinueScope))
+ getCurScope()->AddFlags(Scope::BreakScope | Scope::ContinueScope);
+
// Parse the third part of the for statement.
if (!ForEach && !ForRangeInfo.ParsedForRangeDecl()) {
if (Tok.isNot(tok::semi)) {
Index: clang/lib/Parse/ParseExprCXX.cpp
===================================================================
--- clang/lib/Parse/ParseExprCXX.cpp
+++ clang/lib/Parse/ParseExprCXX.cpp
@@ -1987,11 +1987,30 @@
/// \param FRI If non-null, a for range declaration is permitted, and if
/// present will be parsed and stored here, and a null result will be returned.
///
+/// \param EnterForConditionScope If true, enter a continue/break scope at the
+/// appropriate moment for a 'for' loop.
+///
/// \returns The parsed condition.
Sema::ConditionResult Parser::ParseCXXCondition(StmtResult *InitStmt,
SourceLocation Loc,
Sema::ConditionKind CK,
- ForRangeInfo *FRI) {
+ ForRangeInfo *FRI,
+ bool EnterForConditionScope) {
+ // Helper to ensure we always enter a continue/break scope if requested.
+ struct ForConditionScopeRAII {
+ Scope *S;
+ void enter(bool IsConditionVariable) {
+ if (S) {
+ S->AddFlags(Scope::BreakScope | Scope::ContinueScope);
+ S->setIsConditionVarScope(IsConditionVariable);
+ }
+ }
+ ~ForConditionScopeRAII() {
+ if (S)
+ S->setIsConditionVarScope(false);
+ }
+ } ForConditionScope{EnterForConditionScope ? getCurScope() : nullptr};
+
ParenBraceBracketBalancer BalancerRAIIObj(*this);
PreferredType.enterCondition(Actions, Tok.getLocation());
@@ -2014,6 +2033,9 @@
// Determine what kind of thing we have.
switch (isCXXConditionDeclarationOrInitStatement(InitStmt, FRI)) {
case ConditionOrInitStatement::Expression: {
+ // If this is a for loop, we're entering its condition.
+ ForConditionScope.enter(/*IsConditionVariable=*/false);
+
ProhibitAttributes(attrs);
// We can have an empty expression here.
@@ -2056,6 +2078,9 @@
}
case ConditionOrInitStatement::ForRangeDecl: {
+ // This is 'for (init-stmt; for-range-decl : range-expr)'.
+ // We're not actually in a for loop yet, so 'break' and 'continue' aren't
+ // permitted here.
assert(FRI && "should not parse a for range declaration here");
SourceLocation DeclStart = Tok.getLocation(), DeclEnd;
DeclGroupPtrTy DG = ParseSimpleDeclaration(DeclaratorContext::ForInit,
@@ -2069,6 +2094,9 @@
break;
}
+ // If this is a for loop, we're entering its condition.
+ ForConditionScope.enter(/*IsConditionVariable=*/true);
+
// type-specifier-seq
DeclSpec DS(AttrFactory);
DS.takeAttributesFrom(attrs);
Index: clang/lib/CodeGen/CGStmt.cpp
===================================================================
--- clang/lib/CodeGen/CGStmt.cpp
+++ clang/lib/CodeGen/CGStmt.cpp
@@ -945,8 +945,8 @@
// Start the loop with a block that tests the condition.
// If there's an increment, the continue scope will be overwritten
// later.
- JumpDest Continue = getJumpDestInCurrentScope("for.cond");
- llvm::BasicBlock *CondBlock = Continue.getBlock();
+ JumpDest CondDest = getJumpDestInCurrentScope("for.cond");
+ llvm::BasicBlock *CondBlock = CondDest.getBlock();
EmitBlock(CondBlock);
bool LoopMustProgress = false;
@@ -964,24 +964,33 @@
SourceLocToDebugLoc(R.getBegin()),
SourceLocToDebugLoc(R.getEnd()), LoopMustProgress);
- // If the for loop doesn't have an increment we can just use the
- // condition as the continue block. Otherwise we'll need to create
- // a block for it (in the current scope, i.e. in the scope of the
- // condition), and that we will become our continue block.
- if (S.getInc())
- Continue = getJumpDestInCurrentScope("for.inc");
-
- // Store the blocks to use for break and continue.
- BreakContinueStack.push_back(BreakContinue(LoopExit, Continue));
-
// Create a cleanup scope for the condition variable cleanups.
LexicalScope ConditionScope(*this, S.getSourceRange());
+ // If the for loop doesn't have an increment we can just use the condition as
+ // the continue block. Otherwise, if there is no condition variable, we can
+ // form the continue block now. If there is a condition variable, we can't
+ // form the continue block until after we've emitted the condition, because
+ // the condition is in scope in the increment, but Sema's jump diagnostics
+ // ensure that there are no continues from the condition variable that jump
+ // to the loop increment.
+ JumpDest Continue;
+ if (!S.getInc())
+ Continue = CondDest;
+ else if (!S.getConditionVariable())
+ Continue = getJumpDestInCurrentScope("for.inc");
+ BreakContinueStack.push_back(BreakContinue(LoopExit, Continue));
+
if (S.getCond()) {
// If the for statement has a condition scope, emit the local variable
// declaration.
if (S.getConditionVariable()) {
EmitDecl(*S.getConditionVariable());
+
+ // We have entered the condition variable's scope, so we're now able to
+ // jump to the continue block.
+ Continue = getJumpDestInCurrentScope("for.inc");
+ BreakContinueStack.back().ContinueBlock = Continue;
}
llvm::BasicBlock *ExitBlock = LoopExit.getBlock();
Index: clang/include/clang/Sema/Scope.h
===================================================================
--- clang/include/clang/Sema/Scope.h
+++ clang/include/clang/Sema/Scope.h
@@ -129,11 +129,17 @@
/// This is a compound statement scope.
CompoundStmtScope = 0x400000,
- /// We are between inheritance colon and the real class/struct definition scope.
+ /// We are between inheritance colon and the real class/struct definition
+ /// scope.
ClassInheritanceScope = 0x800000,
/// This is the scope of a C++ catch statement.
CatchScope = 0x1000000,
+
+ /// This is a scope in which a condition variable is currently being
+ /// parsed. If such a scope is a ContinueScope, it's invalid to jump to the
+ /// continue block from here.
+ ConditionVarScope = 0x2000000,
};
private:
@@ -247,6 +253,17 @@
return const_cast<Scope*>(this)->getContinueParent();
}
+ // Set whether we're in the scope of a condition variable, where 'continue'
+ // is disallowed despite being a continue scope.
+ void setIsConditionVarScope(bool InConditionVarScope) {
+ Flags = (Flags & ~ConditionVarScope) |
+ (InConditionVarScope ? ConditionVarScope : 0);
+ }
+
+ bool isConditionVarScope() const {
+ return Flags & ConditionVarScope;
+ }
+
/// getBreakParent - Return the closest scope that a break statement
/// would be affected by.
Scope *getBreakParent() {
Index: clang/include/clang/Parse/Parser.h
===================================================================
--- clang/include/clang/Parse/Parser.h
+++ clang/include/clang/Parse/Parser.h
@@ -1991,7 +1991,8 @@
Sema::ConditionResult ParseCXXCondition(StmtResult *InitStmt,
SourceLocation Loc,
Sema::ConditionKind CK,
- ForRangeInfo *FRI = nullptr);
+ ForRangeInfo *FRI = nullptr,
+ bool EnterForConditionScope = false);
//===--------------------------------------------------------------------===//
// C++ Coroutines
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -5776,6 +5776,9 @@
def warn_unused_label : Warning<"unused label %0">,
InGroup<UnusedLabel>, DefaultIgnore;
+def err_continue_from_cond_var_init : Error<
+ "cannot jump from this continue statement to the loop increment; "
+ "jump bypasses initialization of loop condition variable">;
def err_goto_into_protected_scope : Error<
"cannot jump from this goto statement to its label">;
def ext_goto_into_protected_scope : ExtWarn<
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
