This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGa875721d8a2d: PR49585: Emit the jump destination for a for
loop 'continue' from within the… (authored by rsmith).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D98816/new/
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
@@ -948,8 +948,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;
@@ -967,24 +967,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
