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

llvm-svn: 193073

diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 8797ef5ce1a01b6c5b25a4e87bd56040896a76af..fa5a3115b02a4463dd3551b47a88a6c5dac9960c 100644 (file)
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/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">;

diff --git a/clang/include/clang/Parse/Parser.h b/clang/include/clang/Parse/Parser.h
index 1fd1d866b239b7ecf60f1c931be473aebb52b654..ece532f0599688289176f3bf138bcfa9838c2a68 100644 (file)
--- a/clang/include/clang/Parse/Parser.h
+++ b/clang/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 {

diff --git a/clang/include/clang/Sema/Scope.h b/clang/include/clang/Sema/Scope.h
index 249a4c74311bbcb95a4a147f8a8fb6315bfefe52..4b0563ed549c495d3a1d3cf4b193415b495d1116 100644 (file)
--- a/clang/include/clang/Sema/Scope.h
+++ b/clang/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

diff --git a/clang/lib/Parse/ParseStmt.cpp b/clang/lib/Parse/ParseStmt.cpp
index f57ff97ceb514664c49996b690bdd87b2086be2b..395fb394dc49716d3e70a6cc3ec2b41a389ce79c 100644 (file)
--- a/clang/lib/Parse/ParseStmt.cpp
+++ b/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.

diff --git a/clang/lib/Parse/Parser.cpp b/clang/lib/Parse/Parser.cpp
index 9b6c97ac195fb06071b8a83d20dd757edadb67db..2e1ace53ea8476a1ac73c370021c35d65ea9534f 100644 (file)
--- a/clang/lib/Parse/Parser.cpp
+++ b/clang/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,

diff --git a/clang/lib/Sema/Scope.cpp b/clang/lib/Sema/Scope.cpp
index 10f12ce844f8de9c77e41de63876a5f23047bc4a..8c2a8b3773566e713d1cc464e1c19af170bd34b5 100644 (file)
--- a/clang/lib/Sema/Scope.cpp
+++ b/clang/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;
+}
+

diff --git a/clang/test/Analysis/dead-stores.c b/clang/test/Analysis/dead-stores.c
index 067a0504f13d8ae199caa32710cc61ab4b447742..43f305bec655f208bef24f40c06f671d8bc8a831 100644 (file)
--- a/clang/test/Analysis/dead-stores.c
+++ b/clang/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

diff --git a/clang/test/Parser/bad-control.c b/clang/test/Parser/bad-control.c
index 480d81be0d571dbc2a766f7691c3cf1b045ed55f..dc0ce11ce59c3a48fb3edad6b37c4237fc499df1 100644 (file)
--- a/clang/test/Parser/bad-control.c
+++ b/clang/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;
+  }
+}

diff --git a/clang/test/Sema/statements.c b/clang/test/Sema/statements.c
index f01ee40867150a52703c57768a12b1270a7b88a5..1b84f4bf7caf281dc2ef891f5f82a36e0fdc7cd9 100644 (file)
--- a/clang/test/Sema/statements.c
+++ b/clang/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;
 }