Fix to PR8880 (clang dies processing a for loop).

Due to statement expressions supported as GCC extension, it is possible
to put 'break' or 'continue' into a loop/switch statement but outside its
body, for example:

    for ( ; ({ if (first) { first = 0; continue; } 0; }); )

Such usage must be diagnosed as an error, GCC rejects it. To recognize
this and similar patterns the flags BreakScope and ContinueScope are
temporarily turned off while parsing condition expression.

Differential Revision: http://llvm-reviews.chandlerc.com/D1762


git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@193073 91177308-0d34-0410-b5e6-96231b3b80d8

include/clang/Basic/DiagnosticSemaKinds.td

@@ -6225,9 +6225,9 @@ def warn_cfstring_truncated : Warning<
 
 // Statements.
 def err_continue_not_in_loop : Error<
-  "'continue' statement not in loop statement">;
+  "'continue' statement not in loop statement body">;
 def err_break_not_in_loop_or_switch : Error<
-  "'break' statement not in loop or switch statement">;
+  "'break' statement not in loop or switch statement body">;
 def err_default_not_in_switch : Error<
   "'default' statement not in switch statement">;
 def err_case_not_in_switch : Error<"'case' statement not in switch statement">;

include/clang/Parse/Parser.h

@@ -696,6 +696,22 @@ public:
       }
     }
 
+    /// \brief Sets the specified flags in this scope.
+    /// \param Flags Set of or'd flags (specified in Scope::Scopeflags) that
+    ///              must be set.
+    void SetFlags(unsigned Flags) {
+      if (Self)
+        Self->SetScopeFlags(Flags);
+    }
+
+    /// \brief Clear the specified flags in this scope.
+    /// \param Flags Set of or'd flags (specified in Scope::Scopeflags) that
+    ///              must be cleared.
+    void ClearFlags(unsigned Flags) {
+      if (Self)
+        Self->ClearScopeFlags(Flags);
+    }
+
     ~ParseScope() {
       Exit();
     }
@@ -707,6 +723,12 @@ public:
   /// ExitScope - Pop a scope off the scope stack.
   void ExitScope();
 
+  /// \brief Sets the specified flags in the current scope.
+  void SetScopeFlags(unsigned Flags);
+
+  /// \brief Clears the specified flags in the current scope.
+  void ClearScopeFlags(unsigned Flags);
+
 private:
   /// \brief RAII object used to modify the scope flags for the current scope.
   class ParseScopeFlags {

include/clang/Sema/Scope.h

@@ -342,6 +342,16 @@ public:
   /// Init - This is used by the parser to implement scope caching.
   ///
   void Init(Scope *parent, unsigned flags);
+
+  /// \brief Sets up the specified scope flags and adjusts the scope state
+  /// variables accordingly.
+  ///
+  void SetFlags(unsigned Flags);
+
+  /// \brief Clears the specified scope flags and adjusts the scope state
+  /// variables accordingly.
+  ///
+  void ClearFlags(unsigned Flags);
 };
 
 }  // end namespace clang

lib/Parse/ParseStmt.cpp

@@ -1135,6 +1135,9 @@ StmtResult Parser::ParseSwitchStatement(SourceLocation *TrailingElseLoc) {
     ScopeFlags |= Scope::DeclScope | Scope::ControlScope;
   ParseScope SwitchScope(this, ScopeFlags);
 
+  // Temporarily disable 'break' while parsing condition.
+  SwitchScope.ClearFlags(Scope::BreakScope);
+
   // Parse the condition.
   ExprResult Cond;
   Decl *CondVar = 0;
@@ -1157,6 +1160,9 @@ StmtResult Parser::ParseSwitchStatement(SourceLocation *TrailingElseLoc) {
     return Switch;
   }
 
+  // Enable 'break' in the body of switch statement.
+  SwitchScope.SetFlags(Scope::BreakScope);
+
   // C99 6.8.4p3 - In C99, the body of the switch statement is a scope, even if
   // there is no compound stmt.  C90 does not have this clause.  We only do this
   // if the body isn't a compound statement to avoid push/pop in common cases.
@@ -1227,6 +1233,9 @@ StmtResult Parser::ParseWhileStatement(SourceLocation *TrailingElseLoc) {
     ScopeFlags = Scope::BreakScope | Scope::ContinueScope;
   ParseScope WhileScope(this, ScopeFlags);
 
+  // Disable 'break' and 'continue' while parsing condition.
+  WhileScope.ClearFlags(Scope::BreakScope | Scope::ContinueScope);
+
   // Parse the condition.
   ExprResult Cond;
   Decl *CondVar = 0;
@@ -1235,6 +1244,9 @@ StmtResult Parser::ParseWhileStatement(SourceLocation *TrailingElseLoc) {
 
   FullExprArg FullCond(Actions.MakeFullExpr(Cond.get(), WhileLoc));
 
+  // Allow 'break' and 'continue' in the body of the statement.
+  WhileScope.SetFlags(Scope::BreakScope | Scope::ContinueScope);
+
   // C99 6.8.5p5 - In C99, the body of the if statement is a scope, even if
   // there is no compound stmt.  C90 does not have this clause.  We only do this
   // if the body isn't a compound statement to avoid push/pop in common cases.
@@ -1314,6 +1326,9 @@ StmtResult Parser::ParseDoStatement() {
     return StmtError();
   }
 
+  // Do not allow 'break' and 'continue' in 'while' condition expression.
+  DoScope.ClearFlags(Scope::BreakScope | Scope::ContinueScope);
+
   // Parse the parenthesized expression.
   BalancedDelimiterTracker T(*this, tok::l_paren);
   T.consumeOpen();
@@ -1391,6 +1406,10 @@ StmtResult Parser::ParseForStatement(SourceLocation *TrailingElseLoc) {
   BalancedDelimiterTracker T(*this, tok::l_paren);
   T.consumeOpen();
 
+  // Until loop body starts, statements 'break' and 'continue' cannot
+  // be used.
+  ForScope.ClearFlags(Scope::BreakScope | Scope::ContinueScope);
+
   ExprResult Value;
 
   bool ForEach = false, ForRange = false;
@@ -1566,6 +1585,9 @@ StmtResult Parser::ParseForStatement(SourceLocation *TrailingElseLoc) {
                                                      T.getCloseLocation());
   }
 
+  // When parsing body of 'for' statement, 'break' and 'continue' may be used.
+  ForScope.SetFlags(Scope::BreakScope | Scope::ContinueScope);
+
   // C99 6.8.5p5 - In C99, the body of the if statement is a scope, even if
   // there is no compound stmt.  C90 does not have this clause.  We only do this
   // if the body isn't a compound statement to avoid push/pop in common cases.

lib/Parse/Parser.cpp

@@ -390,6 +390,15 @@ void Parser::ExitScope() {
     ScopeCache[NumCachedScopes++] = OldScope;
 }
 
+void Parser::SetScopeFlags(unsigned Flags) {
+  Actions.CurScope->SetFlags(Flags);
+}
+
+void Parser::ClearScopeFlags(unsigned Flags) {
+  Actions.CurScope->ClearFlags(Flags);
+}
+
+
 /// Set the flags for the current scope to ScopeFlags. If ManageFlags is false,
 /// this object does nothing.
 Parser::ParseScopeFlags::ParseScopeFlags(Parser *Self, unsigned ScopeFlags,

lib/Sema/Scope.cpp

@@ -69,3 +69,40 @@ bool Scope::containedInPrototypeScope() const {
   }
   return false;
 }
+
+void Scope::SetFlags(unsigned FlagsToSet) {
+  assert((FlagsToSet & ~(BreakScope | ContinueScope)) == 0 ||
+         "Unsupported scope flags");
+  assert ((Flags & ControlScope) != 0 || "Must be control scope");
+  if (FlagsToSet & BreakScope) {
+    assert((Flags & BreakScope) == 0 || "Already set");
+    BreakParent = this;
+  }
+  if (FlagsToSet & ContinueScope) {
+    assert((Flags & ContinueScope) == 0 || "Already set");
+    ContinueParent = this;
+  }
+  Flags |= FlagsToSet;
+}
+
+void Scope::ClearFlags(unsigned FlagsToClear) {
+  assert((FlagsToClear & ~(BreakScope | ContinueScope)) == 0 ||
+         "Unsupported scope flags");
+  if (FlagsToClear & BreakScope) {
+    assert((Flags & ControlScope) != 0 || "Must be control scope");
+    assert((Flags & BreakScope) != 0 || "Already cleared");
+    // This is a loop or switch scope.  Flag BreakScope is removed temporarily
+    // when parsing the loop or switch header, to prevent constructs like this:
+    // \code
+    // while (({ if(a>N) break; a}))
+    // \endcode
+    BreakParent = 0;
+ }
+  if (FlagsToClear & ContinueScope) {
+    assert ((Flags & ControlScope) != 0 || "Must be control scope");
+    assert((Flags & ContinueScope) != 0 || "Already cleared");
+    ContinueParent = 0;
+  }
+  Flags &= ~FlagsToClear;
+}
+

test/Analysis/dead-stores.c

@@ -476,18 +476,6 @@ int f26_nestedblocks() {
   return y;
 }
 
-// The FOREACH macro in QT uses 'break' statements within statement expressions
-// placed within the increment code of for loops.
-void rdar8014335() {
-  for (int i = 0 ; i != 10 ; ({ break; })) {
-    for ( ; ; ({ ++i; break; })) ;
-    // Note that the next value stored to 'i' is never executed
-    // because the next statement to be executed is the 'break'
-    // in the increment code of the first loop.
-    i = i * 3; // expected-warning{{Value stored to 'i' is never read}} expected-warning{{The left operand to '*' is always 1}}
-  }
-}
-
 // <rdar://problem/8320674> NullStmts followed by do...while() can lead to disconnected CFG
 //
 // This previously caused bogus dead-stores warnings because the body of the first do...while was

test/Parser/bad-control.c

@@ -7,3 +7,87 @@ void foo() {
 void foo2() { 
   continue; /* expected-error {{'continue' statement not in loop statement}} */
 }
+
+int pr8880() {
+  int first = 1;
+  for ( ; ({ if (first) { first = 0; continue; } 0; }); ) /* expected-error {{'continue' statement not in loop statement}} */
+    return 0;
+  return 1;
+}
+
+int pr8880_2 (int a) {
+  int first = a;
+  while(({ if (first) { first = 0; continue; } 0; })) /* expected-error {{'continue' statement not in loop statement}} */
+    return a;
+}
+
+int pr8880_3 (int a) {
+  int first = a;
+  while(({ if (first) { first = 0; break; } 0; })) /* expected-error {{'break' statement not in loop or switch statement}} */
+    return a;
+}
+
+int pr8880_4 (int a) {
+  int first = a;
+  do {
+    return a;
+  } while(({ if (first) { first = 0; continue; } 0; })); /* expected-error {{'continue' statement not in loop statement}} */
+}
+
+int pr8880_5 (int a) {
+  int first = a;
+  do {
+    return a;
+  } while(({ if (first) { first = 0; break; } 0; })); /* expected-error {{'break' statement not in loop or switch statement}} */
+}
+
+int pr8880_6 (int a) {
+  int first = a;
+  switch(({ if (first) { first = 0; break; } a; })) { /* expected-error {{'break' statement not in loop or switch statement}} */
+  case 2: return a;
+  default: return 0;
+  }
+  return 1;
+}
+
+void pr8880_7() {
+  for (int i = 0 ; i != 10 ; i++ ) {
+    for ( ; ; ({ ++i; break; })) { // expected-error {{'break' statement not in loop or switch statement}}
+    }
+  }
+}
+
+void pr8880_8() {
+  for (int i = 0 ; i != 10 ; i++ )
+    for ( ; ; ({ ++i; break; })) { // expected-error {{'break' statement not in loop or switch statement}}
+    }
+}
+
+void pr8880_9(int x, int y) {
+  switch(x) {
+  case 1:
+    while(({if (y) break; y;})) {} // expected-error {{'break' statement not in loop or switch statement}}
+  }
+}
+
+void pr8880_10(int x, int y) {
+  while(x > 0) {
+    switch(({if(y) break; y;})) { // expected-error {{'break' statement not in loop or switch statement}}
+    case 2: x=0;
+    }
+  }
+}
+
+void pr8880_11() {
+  for (int i = 0 ; i != 10 ; i++ ) {
+    while(({if (i) break; i;})) {} // expected-error {{'break' statement not in loop or switch statement}}
+  }
+}
+
+// Moved from Analysis/dead-stores.c
+void rdar8014335() {
+  for (int i = 0 ; i != 10 ; ({ break; })) { // expected-error {{'break' statement not in loop or switch statement}}
+    for ( ; ; ({ ++i; break; })) ; // expected-error {{'break' statement not in loop or switch statement}}
+    i = i * 3;
+  }
+}

test/Sema/statements.c

@@ -95,7 +95,7 @@ void foo(enum x X) {
 // was causing a crash in the CFG builder.
 int test_pr8880() {
   int first = 1;
-  for ( ; ({ if (first) { first = 0; continue; } 0; }); )
+  for ( ; ({ if (first) { first = 0; continue; } 0; }); ) // expected-error {{'continue' statement not in loop statement}}
     return 0;
   return 1;
 }