Author: Richard Smith Date: 2021-03-17T16:24:04-07:00 New Revision: a875721d8a2dacb894106a2cefa18828bf08f25d
URL: https://github.com/llvm/llvm-project/commit/a875721d8a2dacb894106a2cefa18828bf08f25d DIFF: https://github.com/llvm/llvm-project/commit/a875721d8a2dacb894106a2cefa18828bf08f25d.diff LOG: PR49585: Emit the jump destination for a for loop 'continue' from within the scope of the condition variable. 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. Reviewed By: rjmccall Differential Revision: https://reviews.llvm.org/D98816 Added: clang/test/CodeGenCXX/for-cond-var.cpp Modified: 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/Parser/cxx2a-init-statement.cpp clang/test/SemaCXX/scope-check.cpp Removed: ################################################################################ diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 7b07a570b104..492ff63fe5ad 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -5776,6 +5776,9 @@ def note_goto_ms_asm_label : Note< 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< diff --git a/clang/include/clang/Parse/Parser.h b/clang/include/clang/Parse/Parser.h index a9f063d410a0..290b451771a6 100644 --- a/clang/include/clang/Parse/Parser.h +++ b/clang/include/clang/Parse/Parser.h @@ -1991,7 +1991,8 @@ class Parser : public CodeCompletionHandler { 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 index b7260f15fe1b..b499ba1e7c2a 100644 --- a/clang/include/clang/Sema/Scope.h +++ b/clang/include/clang/Sema/Scope.h @@ -129,11 +129,17 @@ class Scope { /// 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 @@ class Scope { 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() { diff --git a/clang/lib/CodeGen/CGStmt.cpp b/clang/lib/CodeGen/CGStmt.cpp index 85306f6882ef..6461e2011216 100644 --- a/clang/lib/CodeGen/CGStmt.cpp +++ b/clang/lib/CodeGen/CGStmt.cpp @@ -948,8 +948,8 @@ void CodeGenFunction::EmitForStmt(const ForStmt &S, // 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 @@ void CodeGenFunction::EmitForStmt(const ForStmt &S, 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 index 8052795c0c1e..befa7709f2d9 100644 --- a/clang/lib/Parse/ParseExprCXX.cpp +++ b/clang/lib/Parse/ParseExprCXX.cpp @@ -1987,11 +1987,30 @@ Parser::ParseCXXTypeConstructExpression(const DeclSpec &DS) { /// \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 @@ Sema::ConditionResult Parser::ParseCXXCondition(StmtResult *InitStmt, // 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 @@ Sema::ConditionResult Parser::ParseCXXCondition(StmtResult *InitStmt, } 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 @@ Sema::ConditionResult Parser::ParseCXXCondition(StmtResult *InitStmt, 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 index 54655863e3ab..798b8d0d7eb1 100644 --- a/clang/lib/Parse/ParseStmt.cpp +++ b/clang/lib/Parse/ParseStmt.cpp @@ -1959,7 +1959,6 @@ StmtResult Parser::ParseForStatement(SourceLocation *TrailingElseLoc) { } // 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 @@ StmtResult Parser::ParseForStatement(SourceLocation *TrailingElseLoc) { 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 @@ StmtResult Parser::ParseForStatement(SourceLocation *TrailingElseLoc) { } } } 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 @@ StmtResult Parser::ParseForStatement(SourceLocation *TrailingElseLoc) { } } + // 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 index e25d69931538..4a275e6bd2a1 100644 --- a/clang/lib/Sema/SemaStmt.cpp +++ b/clang/lib/Sema/SemaStmt.cpp @@ -3000,6 +3000,12 @@ Sema::ActOnContinueStmt(SourceLocation ContinueLoc, Scope *CurScope) { // 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 index 000000000000..45b4a82cb905 --- /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 index 3b1862f1d3c5..727ee63d1b92 100644 --- a/clang/test/Parser/cxx2a-init-statement.cpp +++ b/clang/test/Parser/cxx2a-init-statement.cpp @@ -31,4 +31,12 @@ void f() { 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 index 518a1e9f2606..7eec58ca057a 100644 --- a/clang/test/SemaCXX/scope-check.cpp +++ b/clang/test/SemaCXX/scope-check.cpp @@ -629,3 +629,19 @@ void indirect_jumps(void **ip) { } } // 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;});) {} +} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
