diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -5776,6 +5776,9 @@ def warn_unused_label : Warning<"unused label %0">, InGroup, 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< diff --git a/clang/include/clang/Parse/Parser.h b/clang/include/clang/Parse/Parser.h --- a/clang/include/clang/Parse/Parser.h +++ b/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 diff --git a/clang/include/clang/Sema/Scope.h b/clang/include/clang/Sema/Scope.h --- a/clang/include/clang/Sema/Scope.h +++ b/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(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() { diff --git a/clang/lib/CodeGen/CGStmt.cpp b/clang/lib/CodeGen/CGStmt.cpp --- a/clang/lib/CodeGen/CGStmt.cpp +++ b/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(); diff --git a/clang/lib/Parse/ParseExprCXX.cpp b/clang/lib/Parse/ParseExprCXX.cpp --- a/clang/lib/Parse/ParseExprCXX.cpp +++ b/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); diff --git a/clang/lib/Parse/ParseStmt.cpp b/clang/lib/Parse/ParseStmt.cpp --- a/clang/lib/Parse/ParseStmt.cpp +++ b/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)) { diff --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp --- a/clang/lib/Sema/SemaStmt.cpp +++ b/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); diff --git a/clang/test/CodeGenCXX/for-cond-var.cpp b/clang/test/CodeGenCXX/for-cond-var.cpp new file mode 100644 --- /dev/null +++ b/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 +} diff --git a/clang/test/Parser/cxx2a-init-statement.cpp b/clang/test/Parser/cxx2a-init-statement.cpp --- a/clang/test/Parser/cxx2a-init-statement.cpp +++ b/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; } } diff --git a/clang/test/SemaCXX/scope-check.cpp b/clang/test/SemaCXX/scope-check.cpp --- a/clang/test/SemaCXX/scope-check.cpp +++ b/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;});) {} +}