[PATCH] Fix to PR8880 (clang dies processing a for loop).
Serge Pavlov
sepavloff at gmail.com
Wed Jan 8 09:12:14 PST 2014
Thank you for review!
2014/1/8 Justin Bogner <mail at justinbogner.com>
> Serge Pavlov <sepavloff at gmail.com> writes:
> > 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. With this
> > change clang interprets such constructs as GCC in C++ mode: 'break'
> > and 'continue' in the 3rd expression refer to the loop itself, in
> > the 1st and 2nd expressions - to outer loop.
> >
> > This revision is reincarnation of
> http://llvm-reviews.chandlerc.com/D2018, which was accidentally closed.
> >
> > http://llvm-reviews.chandlerc.com/D2518
> >
> > Files:
> > include/clang/Sema/Scope.h
> > lib/Parse/ParseStmt.cpp
> > lib/Sema/Scope.cpp
> > test/CodeGen/PR8880.c
> > test/Parser/bad-control.c
> > test/Sema/statements.c
> >
> > Index: include/clang/Sema/Scope.h
> > ===================================================================
> > --- include/clang/Sema/Scope.h
> > +++ include/clang/Sema/Scope.h
> > @@ -354,6 +354,16 @@
> > /// 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 AddFlags(unsigned Flags);
> > +
> > + /// \brief Clears the specified scope flags and adjusts the scope
> state
> > + /// variables accordingly.
> > + ///
> > + void RemoveFlags(unsigned Flags);
> > };
> >
> > } // end namespace clang
> > Index: lib/Parse/ParseStmt.cpp
> > ===================================================================
> > --- lib/Parse/ParseStmt.cpp
> > +++ lib/Parse/ParseStmt.cpp
> > @@ -1170,7 +1170,7 @@
> > // while, for, and switch statements are local to the if, while, for,
> or
> > // switch statement (including the controlled statement).
> > //
> > - unsigned ScopeFlags = Scope::BreakScope | Scope::SwitchScope;
> > + unsigned ScopeFlags = Scope::SwitchScope;
> > if (C99orCXX)
> > ScopeFlags |= Scope::DeclScope | Scope::ControlScope;
> > ParseScope SwitchScope(this, ScopeFlags);
> > @@ -1208,6 +1208,7 @@
> > // See comments in ParseIfStatement for why we create a scope for the
> > // condition and a new scope for substatement in C++.
> > //
> > + getCurScope()->AddFlags(Scope::BreakScope);
> > ParseScope InnerScope(this, Scope::DeclScope,
> > C99orCXX && Tok.isNot(tok::l_brace));
> >
> > @@ -1259,12 +1260,9 @@
> > // 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.
> > @@ -1286,6 +1284,7 @@
> > // See comments in ParseIfStatement for why we create a scope for the
> > // condition and a new scope for substatement in C++.
> > //
> > + getCurScope()->AddFlags(Scope::BreakScope | Scope::ContinueScope);
> > ParseScope InnerScope(this, Scope::DeclScope,
> > C99orCXX && Tok.isNot(tok::l_brace));
> >
> > @@ -1337,6 +1336,7 @@
> >
> > // Pop the body scope if needed.
> > InnerScope.Exit();
> > + getCurScope()->RemoveFlags(Scope::BreakScope | Scope::ContinueScope);
> >
> > if (Tok.isNot(tok::kw_while)) {
> > if (!Body.isInvalid()) {
> > @@ -1419,12 +1419,9 @@
> > // Names declared in the for-init-statement are in the same
> declarative-region
> > // as those declared in the condition.
> > //
> > - unsigned ScopeFlags;
> > + unsigned ScopeFlags = 0;
> > if (C99orCXXorObjC)
> > - ScopeFlags = Scope::BreakScope | Scope::ContinueScope |
> > - Scope::DeclScope | Scope::ControlScope;
> > - else
> > - ScopeFlags = Scope::BreakScope | Scope::ContinueScope;
> > + ScopeFlags = Scope::DeclScope | Scope::ControlScope;
> >
> > ParseScope ForScope(this, ScopeFlags);
> >
> > @@ -1573,12 +1570,15 @@
> > }
> >
> > // Parse the third part of the for specifier.
> > + getCurScope()->AddFlags(Scope::BreakScope | Scope::ContinueScope);
> > if (Tok.isNot(tok::r_paren)) { // for (...;...;)
> > ExprResult Third = ParseExpression();
> > // FIXME: The C++11 standard doesn't actually say that this is a
> > // discarded-value expression, but it clearly should be.
> > ThirdPart = Actions.MakeFullDiscardedValueExpr(Third.take());
> > }
> > + } else {
> > + getCurScope()->AddFlags(Scope::BreakScope | Scope::ContinueScope);
> > }
> > // Match the ')'.
> > T.consumeClose();
> > @@ -1617,6 +1617,7 @@
> > // See comments in ParseIfStatement for why we create a scope for
> > // for-init-statement/condition and a new scope for substatement in
> C++.
> > //
> > + getCurScope()->AddFlags(Scope::BreakScope | Scope::ContinueScope);
>
> Aren't these flags already set, since you set them in both pahts of the
> if/else above?
>
Yes, you are right. Removed.
> > ParseScope InnerScope(this, Scope::DeclScope,
> > C99orCXXorObjC && Tok.isNot(tok::l_brace));
> >
> > Index: lib/Sema/Scope.cpp
> > ===================================================================
> > --- lib/Sema/Scope.cpp
> > +++ lib/Sema/Scope.cpp
> > @@ -69,3 +69,34 @@
> > }
> > return false;
> > }
> > +
> > +void Scope::AddFlags(unsigned FlagsToSet) {
> > + assert((FlagsToSet & ~(BreakScope | ContinueScope)) == 0 ||
> > + "Unsupported scope flags");
> > + assert((Flags & ControlScope) != 0 || "Must be control scope");
>
> I think you mean && here. None of these asserts will ever fire.
>
Ops, you are absolutely right. And check for ControlScope is wrong, do
loops haven't it.
> > + 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::RemoveFlags(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");
> > + BreakParent = AnyParent->BreakParent;
> > + }
> > + if (FlagsToClear & ContinueScope) {
> > + assert((Flags & ControlScope) != 0 || "Must be control scope");
> > + assert((Flags & ContinueScope) != 0 || "Already cleared");
> > + ContinueParent = AnyParent->ContinueParent;
> > + }
> > + Flags &= ~FlagsToClear;
> > +}
> > Index: test/CodeGen/PR8880.c
> > ===================================================================
> > --- /dev/null
> > +++ test/CodeGen/PR8880.c
> > @@ -0,0 +1,93 @@
> > +// RUN: %clang_cc1 -emit-llvm -o - %s | FileCheck %s
> > +
> > +void pr8880_cg_1(int *iptr) {
> > +// CHECK-LABEL: define void @pr8880_cg_1(
> > +// CHECK-NEXT: entry:
> > + int i, j;
> > + for (i = 2; i != 10 ; i++ )
> > +// CHECK: for.cond{{[0-9]*}}:
> > +// CHECK: for.body{{[0-9]*}}:
> > + for (j = 3 ; j < 22; (void)({ ++j; break; j;})) {
> > +// CHECK: for.cond{{[0-9]*}}:
> > +// CHECK: for.body{{[0-9]*}}:
> > + *iptr = 7;
> > +// CHECK: for.inc{{[0-9]*}}:
> > +
> > +// break in 3rd expression of inner loop causes branch to end of inner
> loop
> > +
> > +// CHECK: br label %for.end[[INNER_END:[0-9]*]]
> > +// CHECK: for.end[[INNER_END]]:
> > + }
> > +// CHECK: for.inc{{[0-9]*}}:
> > +// CHECK: for.end{{[0-9]*}}:
> > +// CHECK-NEXT: ret void
> > +}
>
> The labels you're keying on here won't have names in non-asserts builds,
> so these tests will fail.
>
Important note, special thanks! I used different labels in the updated
patch.
--
Thanks,
--Serge
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140109/65819c4b/attachment.html>
More information about the cfe-commits
mailing list