r193073 - Fix to PR8880 (clang dies processing a for loop).
Serge Pavlov
sepavloff at gmail.com
Tue Oct 22 10:33:14 PDT 2013
This problem has been fixed in r193170.
I regret about troubles. GCC rejects very similar syntax and without a
proper test it was very simple to break software that uses this strange
feature.
I believe the fix is useful and change in r193170 will be enough to pacify
QT world.
Sorry.
--Serge
2013/10/22 Benjamin Kramer <benny.kra at gmail.com>
>
> On 21.10.2013, at 11:34, Serge Pavlov <sepavloff at gmail.com> wrote:
>
> > Author: sepavloff
> > Date: Mon Oct 21 04:34:44 2013
> > New Revision: 193073
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=193073&view=rev
> > Log:
> > 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
>
> Who reviewed this?
>
> This breaks Qt, as seen in the test case you removed. While there are
> differences in behavior between Clang and GCC here that should be fixed,
> GCC does allow those statements in some places in loops. Bug reports are
> already pouring in[1], please revert and fix this patch.
>
> [1] http://llvm.org/bugs/show_bug.cgi?id=17649
>
> - Ben
>
> >
> > Modified:
> > cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
> > cfe/trunk/include/clang/Parse/Parser.h
> > cfe/trunk/include/clang/Sema/Scope.h
> > cfe/trunk/lib/Parse/ParseStmt.cpp
> > cfe/trunk/lib/Parse/Parser.cpp
> > cfe/trunk/lib/Sema/Scope.cpp
> > cfe/trunk/test/Analysis/dead-stores.c
> > cfe/trunk/test/Parser/bad-control.c
> > cfe/trunk/test/Sema/statements.c
> >
> > Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=193073&r1=193072&r2=193073&view=diff
> >
> ==============================================================================
> > --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
> > +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Mon Oct 21
> 04:34:44 2013
> > @@ -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">;
> >
> > Modified: cfe/trunk/include/clang/Parse/Parser.h
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Parse/Parser.h?rev=193073&r1=193072&r2=193073&view=diff
> >
> ==============================================================================
> > --- cfe/trunk/include/clang/Parse/Parser.h (original)
> > +++ cfe/trunk/include/clang/Parse/Parser.h Mon Oct 21 04:34:44 2013
> > @@ -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 {
> >
> > Modified: cfe/trunk/include/clang/Sema/Scope.h
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Scope.h?rev=193073&r1=193072&r2=193073&view=diff
> >
> ==============================================================================
> > --- cfe/trunk/include/clang/Sema/Scope.h (original)
> > +++ cfe/trunk/include/clang/Sema/Scope.h Mon Oct 21 04:34:44 2013
> > @@ -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
> >
> > Modified: cfe/trunk/lib/Parse/ParseStmt.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseStmt.cpp?rev=193073&r1=193072&r2=193073&view=diff
> >
> ==============================================================================
> > --- cfe/trunk/lib/Parse/ParseStmt.cpp (original)
> > +++ cfe/trunk/lib/Parse/ParseStmt.cpp Mon Oct 21 04:34:44 2013
> > @@ -1135,6 +1135,9 @@ StmtResult Parser::ParseSwitchStatement(
> > 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(
> > 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(S
> > 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(S
> >
> > 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(Sou
> > 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(Sou
> >
> 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.
> >
> > Modified: cfe/trunk/lib/Parse/Parser.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/Parser.cpp?rev=193073&r1=193072&r2=193073&view=diff
> >
> ==============================================================================
> > --- cfe/trunk/lib/Parse/Parser.cpp (original)
> > +++ cfe/trunk/lib/Parse/Parser.cpp Mon Oct 21 04:34:44 2013
> > @@ -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,
> >
> > Modified: cfe/trunk/lib/Sema/Scope.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/Scope.cpp?rev=193073&r1=193072&r2=193073&view=diff
> >
> ==============================================================================
> > --- cfe/trunk/lib/Sema/Scope.cpp (original)
> > +++ cfe/trunk/lib/Sema/Scope.cpp Mon Oct 21 04:34:44 2013
> > @@ -69,3 +69,40 @@ bool Scope::containedInPrototypeScope()
> > }
> > 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;
> > +}
> > +
> >
> > Modified: cfe/trunk/test/Analysis/dead-stores.c
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/dead-stores.c?rev=193073&r1=193072&r2=193073&view=diff
> >
> ==============================================================================
> > --- cfe/trunk/test/Analysis/dead-stores.c (original)
> > +++ cfe/trunk/test/Analysis/dead-stores.c Mon Oct 21 04:34:44 2013
> > @@ -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
> >
> > Modified: cfe/trunk/test/Parser/bad-control.c
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Parser/bad-control.c?rev=193073&r1=193072&r2=193073&view=diff
> >
> ==============================================================================
> > --- cfe/trunk/test/Parser/bad-control.c (original)
> > +++ cfe/trunk/test/Parser/bad-control.c Mon Oct 21 04:34:44 2013
> > @@ -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;
> > + }
> > +}
> >
> > Modified: cfe/trunk/test/Sema/statements.c
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/statements.c?rev=193073&r1=193072&r2=193073&view=diff
> >
> ==============================================================================
> > --- cfe/trunk/test/Sema/statements.c (original)
> > +++ cfe/trunk/test/Sema/statements.c Mon Oct 21 04:34:44 2013
> > @@ -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;
> > }
> >
> >
> > _______________________________________________
> > cfe-commits mailing list
> > cfe-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
--
Thanks,
--Serge
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131023/c3a43115/attachment.html>
More information about the cfe-commits
mailing list