[PATCH] Fix to PR8880 (clang dies processing a for loop).
Justin Bogner
mail at justinbogner.com
Wed Jan 8 00:58:44 PST 2014
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?
> 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.
> + 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.
> +
> +void pr8880_cg_2(int *iptr) {
> +// CHECK-LABEL: define void @pr8880_cg_2(
> +// 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; continue; j;})) {
> +// CHECK: for.cond{{[0-9]*}}:
> +// CHECK: for.body{{[0-9]*}}:
> + *iptr = 7;
> +// CHECK: for.inc[[INNER_INC:[0-9]*]]:
> +
> +// continue in 3rd expression of inner loop causes branch to inc of inner loop
> +
> +// CHECK: br label %for.inc[[INNER_INC]]
> +// CHECK: for.end{{[0-9]*}}:
> + }
> +// CHECK: for.inc{{[0-9]*}}:
> +// CHECK: for.end{{[0-9]*}}:
> +// CHECK-NEXT: ret void
> +}
> +
> +void pr8880_cg_3(int *iptr) {
> +// CHECK-LABEL: define void @pr8880_cg_3(
> +// 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 ; ({break; j;}); j++) {
> +// CHECK: for.cond{{[0-9]*}}:
> +
> +// break in 2nd expression of inner loop causes branch to end of outer loop
> +
> +// CHECK: br label %for.end[[OUTER_END:[0-9]*]]
> +// CHECK: for.body{{[0-9]*}}:
> + *iptr = 7;
> +// CHECK: for.inc{{[0-9]*}}:
> +// CHECK: for.end{{[0-9]*}}:
> + }
> +// CHECK: for.inc{{[0-9]*}}:
> +// CHECK: for.end[[OUTER_END]]:
> +// CHECK-NEXT: ret void
> +}
> +
> +void pr8880_cg_4(int *iptr) {
> +// CHECK-LABEL: define void @pr8880_cg_4(
> +// 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 ; ({continue; j;}); j++) {
> +// CHECK: for.cond{{[0-9]*}}:
> +
> +// continue in 2nd expression of inner loop causes branch to inc of outer loop
> +
> +// CHECK: br label %for.inc[[OUTER_INC:[0-9]*]]
> +// CHECK: for.body{{[0-9]*}}:
> + *iptr = 7;
> +// CHECK: for.inc{{[0-9]*}}:
> +// CHECK: for.end{{[0-9]*}}:
> + }
> +// CHECK: for.inc[[OUTER_INC:[0-9]*]]:
> +// CHECK: for.end{{[0-9]*}}:
> +// CHECK-NEXT: ret void
> +}
> Index: test/Parser/bad-control.c
> ===================================================================
> --- test/Parser/bad-control.c
> +++ test/Parser/bad-control.c
> @@ -7,3 +7,135 @@
> void foo2() {
> continue; /* expected-error {{'continue' statement not in loop statement}} */
> }
> +
> +int pr8880_1() {
> + int first = 1;
> + for ( ; ({ if (first) { first = 0; continue; } 0; }); ) /* expected-error {{'continue' statement not in loop statement}} */
> + return 0;
> + return 1;
> +}
> +
> +void pr8880_2(int first) {
> + for ( ; ({ if (first) { first = 0; break; } 0; }); ) {} /* expected-error {{'break' statement not in loop or switch statement}} */
> +}
> +
> +// GCC rejects this code
> +void pr8880_3(int first) {
> + for ( ; ; (void)({ if (first) { first = 0; continue; } 0; })) {}
> +}
> +
> +// GCC rejects this code (see also tests/Analysis/dead-stores.c rdar8014335()
> +void pr8880_4(int first) {
> + for ( ; ; (void)({ if (first) { first = 0; break; } 0; })) {}
> +}
> +
> +void pr8880_5 (int first) {
> + while(({ if (first) { first = 0; continue; } 0; })) {} /* expected-error {{'continue' statement not in loop statement}} */
> +}
> +
> +void pr8880_6 (int first) {
> + while(({ if (first) { first = 0; break; } 0; })) {} /* expected-error {{'break' statement not in loop or switch statement}} */
> +}
> +
> +void pr8880_7 (int first) {
> + do {} while(({ if (first) { first = 0; continue; } 0; })); /* expected-error {{'continue' statement not in loop statement}} */
> +}
> +
> +void pr8880_8 (int first) {
> + do {} while(({ if (first) { first = 0; break; } 0; })); /* expected-error {{'break' statement not in loop or switch statement}} */
> +}
> +
> +int pr8880_9 (int first) {
> + switch(({ if (first) { first = 0; break; } 1; })) { /* expected-error {{'break' statement not in loop or switch statement}} */
> + case 2: return 2;
> + default: return 0;
> + }
> +}
> +
> +void pr8880_10(int i) {
> + for ( ; i != 10 ; i++ )
> + for ( ; ; (void)({ ++i; continue; i;})) {}
> +}
> +
> +void pr8880_11(int i) {
> + for ( ; i != 10 ; i++ )
> + for ( ; ; (void)({ ++i; break; i;})) {}
> +}
> +
> +void pr8880_12(int i, int j) {
> + for ( ; i != 10 ; i++ )
> + for ( ; ({if (i) continue; i;}); j++) {}
> +}
> +
> +void pr8880_13(int i, int j) {
> + for ( ; i != 10 ; i++ )
> + for ( ; ({if (i) break; i;}); j++) {}
> +}
> +
> +void pr8880_14(int i) {
> + for ( ; i != 10 ; i++ )
> + while(({if (i) break; i;})) {}
> +}
> +
> +void pr8880_15(int i) {
> + while (--i)
> + while(({if (i) continue; i;})) {}
> +}
> +
> +void pr8880_16(int i) {
> + for ( ; i != 10 ; i++ )
> + do {} while(({if (i) break; i;}));
> +}
> +
> +void pr8880_17(int i) {
> + for ( ; i != 10 ; i++ )
> + do {} while(({if (i) continue; i;}));
> +}
> +
> +void pr8880_18(int x, int y) {
> + while(x > 0)
> + switch(({if(y) break; y;})) {
> + case 2: x = 0;
> + }
> +}
> +
> +void pr8880_19(int x, int y) {
> + switch(x) {
> + case 1:
> + switch(({if(y) break; y;})) {
> + case 2: x = 0;
> + }
> + }
> +}
> +
> +void pr8880_20(int x, int y) {
> + switch(x) {
> + case 1:
> + while(({if (y) break; y;})) {}
> + }
> +}
> +
> +void pr8880_21(int x, int y) {
> + switch(x) {
> + case 1:
> + do {} while(({if (y) break; y;}));
> + }
> +}
> +
> +void pr8880_22(int x, int y) {
> + switch(x) {
> + case 1:
> + for ( ; ; (void)({ ++y; break; y;})) {}
> + }
> +}
> +
> +void pr8880_23(int x, int y) {
> + switch(x) {
> + case 1:
> + for ( ; ({ ++y; break; y;}); ++y) {}
> + }
> +}
> +
> +void pr8880_24() {
> + for (({break;});;); /* expected-error {{'break' statement not in loop or switch statement}} */
> +}
> Index: test/Sema/statements.c
> ===================================================================
> --- test/Sema/statements.c
> +++ test/Sema/statements.c
> @@ -90,12 +90,9 @@
> }
> }
>
> -// PR 8880
> -// FIXME: Clang should reject this, since GCC does. Previously this
> -// 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
More information about the cfe-commits
mailing list