sepavloff added you to the CC list for the revision "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,
similar to this:
for ( ; ({ if (first) { first = 0; continue; } 0; }); )
Such usage must be diagnosed as an error, GCC rejects it.
To recognize such patterns the flags BreakScope and ContinueScope are
temporarily turned off while parsing condition expression.
Please review the fix.
Thank you.
http://llvm-reviews.chandlerc.com/D1762
Files:
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Parse/Parser.h
include/clang/Sema/Scope.h
lib/Parse/ParseStmt.cpp
lib/Parse/Parser.cpp
lib/Sema/Scope.cpp
test/Analysis/dead-stores.c
test/Parser/bad-control.c
test/Sema/statements.c
Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -6201,9 +6201,9 @@
// 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">;
Index: include/clang/Parse/Parser.h
===================================================================
--- include/clang/Parse/Parser.h
+++ include/clang/Parse/Parser.h
@@ -695,6 +695,22 @@
}
}
+ /// \brief Sets the specified flags in the properties of 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 the properties of 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();
}
@@ -706,6 +722,12 @@
/// ExitScope - Pop a scope off the scope stack.
void ExitScope();
+ /// \brief Set the specified flags in the current scope.
+ void SetScopeFlags(unsigned Flags);
+
+ /// \brief Clear 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 {
Index: include/clang/Sema/Scope.h
===================================================================
--- include/clang/Sema/Scope.h
+++ include/clang/Sema/Scope.h
@@ -343,6 +343,16 @@
/// Init - This is used by the parser to implement scope caching.
///
void Init(Scope *parent, unsigned flags);
+
+ /// \brief Set up the specified scope flags and adjusts scope state
+ /// variables accordingly.
+ ///
+ void SetFlags(unsigned Flags);
+
+ /// \brief Clear the specified scope flags and adjusts scope state
+ /// variables accordingly.
+ ///
+ void ClearFlags(unsigned Flags);
};
} // end namespace clang
Index: lib/Parse/ParseStmt.cpp
===================================================================
--- lib/Parse/ParseStmt.cpp
+++ lib/Parse/ParseStmt.cpp
@@ -1116,6 +1116,9 @@
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;
@@ -1138,6 +1141,9 @@
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.
@@ -1208,14 +1214,20 @@
ScopeFlags = Scope::BreakScope | Scope::ContinueScope;
ParseScope WhileScope(this, ScopeFlags);
+ // Disable 'break' and 'condition' while parsing condition.
+ WhileScope.ClearFlags(Scope::BreakScope | Scope::ContinueScope);
+
// Parse the condition.
ExprResult Cond;
Decl *CondVar = 0;
if (ParseParenExprOrCondition(Cond, CondVar, WhileLoc, true))
return StmtError();
FullExprArg FullCond(Actions.MakeFullExpr(Cond.get(), WhileLoc));
+ // Allow 'break' and 'continue' in the body of 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.
@@ -1295,6 +1307,9 @@
return StmtError();
}
+ // Do not allow 'break' and 'continue' in 'while' condition expression.
+ DoScope.ClearFlags(Scope::BreakScope | Scope::ContinueScope);
+
// Parse the parenthesized condition.
BalancedDelimiterTracker T(*this, tok::l_paren);
T.consumeOpen();
@@ -1374,6 +1389,9 @@
BalancedDelimiterTracker T(*this, tok::l_paren);
T.consumeOpen();
+ // Until body starts 'break' and 'continue' cannot be used.
+ ForScope.ClearFlags(Scope::BreakScope | Scope::ContinueScope);
+
ExprResult Value;
bool ForEach = false, ForRange = false;
@@ -1549,6 +1567,9 @@
T.getCloseLocation());
}
+ // When parsing for body 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.
Index: lib/Parse/Parser.cpp
===================================================================
--- lib/Parse/Parser.cpp
+++ lib/Parse/Parser.cpp
@@ -390,6 +390,15 @@
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,
Index: lib/Sema/Scope.cpp
===================================================================
--- lib/Sema/Scope.cpp
+++ lib/Sema/Scope.cpp
@@ -69,3 +69,40 @@
}
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 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;
+}
+
Index: test/Analysis/dead-stores.c
===================================================================
--- test/Analysis/dead-stores.c
+++ test/Analysis/dead-stores.c
@@ -476,18 +476,6 @@
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
Index: test/Parser/bad-control.c
===================================================================
--- test/Parser/bad-control.c
+++ test/Parser/bad-control.c
@@ -7,3 +7,87 @@
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;
+ }
+}
Index: test/Sema/statements.c
===================================================================
--- test/Sema/statements.c
+++ test/Sema/statements.c
@@ -95,7 +95,7 @@
// 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;
}
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
