[clang][Sema] Fix the continue and break scope for while loops

clang/docs/ReleaseNotes.rst

@@ -161,6 +161,8 @@ Bug Fixes in This Version
   targets that treat ``_Float16``/``__fp16`` as native scalar types. Previously
   the warning was silently lost because the operands differed only by an implicit
   cast chain. (#GH149967).
+- Correct the continue and break scope for while statements to be after the
+  condition is evaluated.
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

clang/lib/CodeGen/CGStmt.cpp

@@ -1087,9 +1087,6 @@ void CodeGenFunction::EmitWhileStmt(const WhileStmt &S,
   // also become the break target.
   JumpDest LoopExit = getJumpDestInCurrentScope("while.end");
 
-  // Store the blocks to use for break and continue.
-  BreakContinueStack.push_back(BreakContinue(LoopExit, LoopHeader));
-
   // C++ [stmt.while]p2:
   //   When the condition of a while statement is a declaration, the
   //   scope of the variable that is declared extends from its point
@@ -1158,6 +1155,9 @@ void CodeGenFunction::EmitWhileStmt(const WhileStmt &S,
         << SourceRange(S.getWhileLoc(), S.getRParenLoc());
   }
 
+  // Store the blocks to use for break and continue.
+  BreakContinueStack.push_back(BreakContinue(LoopExit, LoopHeader));
+
   // Emit the loop body.  We have to emit this in a cleanup scope
   // because it might be a singleton DeclStmt.
   {
@@ -1206,8 +1206,6 @@ void CodeGenFunction::EmitDoStmt(const DoStmt &S,
 
   uint64_t ParentCount = getCurrentProfileCount();
 
-  // Store the blocks to use for break and continue.
-  BreakContinueStack.push_back(BreakContinue(LoopExit, LoopCond));
 
   // Emit the body of the loop.
   llvm::BasicBlock *LoopBody = createBasicBlock("do.body");
@@ -1220,10 +1218,13 @@ void CodeGenFunction::EmitDoStmt(const DoStmt &S,
   if (CGM.shouldEmitConvergenceTokens())
     ConvergenceTokenStack.push_back(emitConvergenceLoopToken(LoopBody));
 
+  // Store the blocks to use for break and continue.
+  BreakContinueStack.push_back(BreakContinue(LoopExit, LoopCond));
   {
     RunCleanupsScope BodyScope(*this);
     EmitStmt(S.getBody());
   }
+  BreakContinueStack.pop_back();
 
   EmitBlock(LoopCond.getBlock());
   // When single byte coverage mode is enabled, add a counter to loop condition.
@@ -1238,8 +1239,6 @@ void CodeGenFunction::EmitDoStmt(const DoStmt &S,
   // compares unequal to 0.  The condition must be a scalar type.
   llvm::Value *BoolCondVal = EvaluateExprAsBool(S.getCond());
 
-  BreakContinueStack.pop_back();
-
   // "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);
@@ -1328,7 +1327,6 @@ void CodeGenFunction::EmitForStmt(const ForStmt &S,
     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
@@ -1339,7 +1337,6 @@ void CodeGenFunction::EmitForStmt(const ForStmt &S,
       // We have entered the condition variable's scope, so we're now able to
       // jump to the continue block.
       Continue = S.getInc() ? getJumpDestInCurrentScope("for.inc") : CondDest;
-      BreakContinueStack.back().ContinueBlock = Continue;
     }
 
     // When single byte coverage mode is enabled, add a counter to loop
@@ -1381,7 +1378,6 @@ void CodeGenFunction::EmitForStmt(const ForStmt &S,
       EmitBlock(ExitBlock);
       EmitBranchThroughCleanup(LoopExit);
     }
-
     EmitBlock(ForBody);
   } else {
     // Treat it as a non-zero constant.  Don't even create a new block for the
@@ -1393,12 +1389,15 @@ void CodeGenFunction::EmitForStmt(const ForStmt &S,
     incrementProfileCounter(S.getBody());
   else
     incrementProfileCounter(&S);
+
+  BreakContinueStack.push_back(BreakContinue(LoopExit, Continue));
   {
     // Create a separate cleanup scope for the body, in case it is not
     // a compound statement.
     RunCleanupsScope BodyScope(*this);
     EmitStmt(S.getBody());
   }
+  BreakContinueStack.pop_back();
 
   // The last block in the loop's body (which unconditionally branches to the
   // `inc` block if there is one).
@@ -1412,8 +1411,6 @@ void CodeGenFunction::EmitForStmt(const ForStmt &S,
       incrementProfileCounter(S.getInc());
   }
 
-  BreakContinueStack.pop_back();
-
   ConditionScope.ForceCleanup();
 
   EmitStopPoint(&S);
@@ -1518,6 +1515,8 @@ CodeGenFunction::EmitCXXForRangeStmt(const CXXForRangeStmt &S,
     EmitStmt(S.getLoopVarStmt());
     EmitStmt(S.getBody());
   }
+  BreakContinueStack.pop_back();
+
   // The last block in the loop's body (which unconditionally branches to the
   // `inc` block if there is one).
   auto *FinalBodyBB = Builder.GetInsertBlock();
@@ -1527,8 +1526,6 @@ CodeGenFunction::EmitCXXForRangeStmt(const CXXForRangeStmt &S,
   EmitBlock(Continue.getBlock());
   EmitStmt(S.getInc());
 
-  BreakContinueStack.pop_back();
-
   EmitBranch(CondBlock);
 
   ForScope.ForceCleanup();

clang/lib/Parse/ParseExprCXX.cpp

@@ -1877,10 +1877,8 @@ Parser::ParseCXXCondition(StmtResult *InitStmt, SourceLocation Loc,
   struct ForConditionScopeRAII {
     Scope *S;
     void enter(bool IsConditionVariable) {
-      if (S) {
-        S->AddFlags(Scope::BreakScope | Scope::ContinueScope);
+      if (S)
         S->setIsConditionVarScope(IsConditionVariable);
-      }
     }
     ~ForConditionScopeRAII() {
       if (S)

clang/lib/Parse/ParseStmt.cpp

@@ -1728,12 +1728,10 @@ StmtResult Parser::ParseWhileStatement(SourceLocation *TrailingElseLoc) {
   // while, for, and switch statements are local to the if, while, for, or
   // switch statement (including the controlled statement).
   //
-  unsigned ScopeFlags;
+  unsigned ScopeFlags = 0;
   if (C99orCXX)
-    ScopeFlags = Scope::BreakScope | Scope::ContinueScope |
-                 Scope::DeclScope  | Scope::ControlScope;
-  else
-    ScopeFlags = Scope::BreakScope | Scope::ContinueScope;
+    ScopeFlags = Scope::DeclScope  | Scope::ControlScope;
+
   ParseScope WhileScope(this, ScopeFlags);
 
   // Parse the condition.
@@ -1744,6 +1742,8 @@ StmtResult Parser::ParseWhileStatement(SourceLocation *TrailingElseLoc) {
                                 Sema::ConditionKind::Boolean, LParen, RParen))
     return StmtError();
 
+  getCurScope()->AddFlags(Scope::BreakScope | Scope::ContinueScope);
+
   // OpenACC Restricts a while-loop inside of certain construct/clause
   // combinations, so diagnose that here in OpenACC mode.
   SemaOpenACC::LoopInConstructRAII LCR{getActions().OpenACC()};
@@ -1838,6 +1838,8 @@ StmtResult Parser::ParseDoStatement() {
   // A do-while expression is not a condition, so can't have attributes.
   DiagnoseAndSkipCXX11Attributes();
 
+  DoScope.Exit();
+
   SourceLocation Start = Tok.getLocation();
   ExprResult Cond = ParseExpression();
   if (!Cond.isUsable()) {
@@ -1848,7 +1850,6 @@ StmtResult Parser::ParseDoStatement() {
         Actions.getASTContext().BoolTy);
   }
   T.consumeClose();
-  DoScope.Exit();
 
   if (Cond.isInvalid() || Body.isInvalid())
     return StmtError();
@@ -2123,9 +2124,6 @@ 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();
@@ -2137,11 +2135,6 @@ StmtResult Parser::ParseForStatement(SourceLocation *TrailingElseLoc) {
     }
   }
 
-  // Enter a break / continue scope, if we didn't already enter one while
-  // parsing the second part.
-  if (!getCurScope()->isContinueScope())
-    getCurScope()->AddFlags(Scope::BreakScope | Scope::ContinueScope);
-
   // Parse the third part of the for statement.
   if (!ForEach && !ForRangeInfo.ParsedForRangeDecl()) {
     if (Tok.isNot(tok::semi)) {
@@ -2164,6 +2157,8 @@ StmtResult Parser::ParseForStatement(SourceLocation *TrailingElseLoc) {
   // Match the ')'.
   T.consumeClose();
 
+  getCurScope()->AddFlags(Scope::BreakScope | Scope::ContinueScope);
+
   // C++ Coroutines [stmt.iter]:
   //   'co_await' can only be used for a range-based for statement.
   if (CoawaitLoc.isValid() && !ForRangeInfo.ParsedForRangeDecl()) {

clang/test/CoverageMapping/break.c

@@ -33,12 +33,25 @@ int main(void) {     // CHECK: File 0, [[@LINE]]:16 -> {{[0-9]+}}:2 = #0
 }
 
 // CHECK-LABEL: break_continue_in_increment:
-// CHECK:  [[@LINE+6]]:11 -> [[@LINE+6]]:45 = #1
-// CHECK:  [[@LINE+5]]:18 -> [[@LINE+5]]:19 = #1
-// CHECK:  [[@LINE+4]]:21 -> [[@LINE+4]]:26 = #2
-// CHECK:  [[@LINE+3]]:33 -> [[@LINE+3]]:41 = (#1 - #2)
-// CHECK:  [[@LINE+3]]:5 -> [[@LINE+3]]:6 = #1
+/*
+  52:41 -> 57:2 = #0
+  53:10 -> 53:11 = ((#0 + #1) + #2)
+  Branch,File 0, 53:10 -> 53:11 = #1, 0
+
+  53:13 -> 56:4 = #1
+
+
+
+
+*/
+
+  // CHECK: [[@LINE+6]]:20 -> [[@LINE+6]]:21 = #2
+  // CHECK: [[@LINE+5]]:23 -> [[@LINE+5]]:28 = #3
+  // CHECK: [[@LINE+4]]:35 -> [[@LINE+4]]:43 = (#2 - #3)
+  // CHECK: [[@LINE+4]]:7 -> [[@LINE+4]]:8 = #2
 void break_continue_in_increment(int x) {
-  for (;; ({ if (x) break; else continue; }))
-    ;
+  while (1) {
+    for (;; ({ if (x) break; else continue; }))
+      ;
+  }
 }

clang/test/Sema/loop-condition-continue-scopes.c

@@ -0,0 +1,28 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s 
+
+void f() {
+  while (({ continue; 1; })) {}
+  // expected-error@-1 {{'continue' statement not in loop statement}}
+  while (({ break; 1; })) {}
+  // expected-error@-1 {{'break' statement not in loop or switch statement}}
+  do {} while (({ break; 1; }));
+  // expected-error@-1 {{'break' statement not in loop or switch statement}}
+  do {} while (({ continue; 1;}));
+  // expected-error@-1 {{'continue' statement not in loop statement}}
+  for (({ continue; });;) {}
+  // expected-error@-1 {{'continue' statement not in loop statement}}
+  for (;({ continue; 1;});) {}
+  // expected-error@-1 {{'continue' statement not in loop statement}}
+  for (;;({ continue;})) {}
+  // expected-error@-1 {{'continue' statement not in loop statement}}
+  for (({ break;});;) {}
+  // expected-error@-1 {{'break' statement not in loop or switch statement}}
+  for (;({ break; 1;});) {}
+  // expected-error@-1 {{'break' statement not in loop or switch statement}}
+  for (;;({ break;})) {}
+  // expected-error@-1 {{'break' statement not in loop or switch statement}}
+  switch(({break;1;})){
+  // expected-error@-1 {{'break' statement not in loop or switch statement}}
+    case 1: break;
+  }
+}