Fix to PR8880 (clang dies processing a for loop).

[sepavloff]

Summary

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.

[sepavloff]

Updated patch according to notes of Justin Bogner.

[sepavloff]

Updated test for code generation.

[sepavloff]

Changed implementation according to reviewer's notes.

Code generation is changed so that condition and increment expressions
in loop header are considered as if they are inside break/continue
scope of the loop. Such behavior is more close to GCC in C++ mode.
In C mode such usage causes GCC compatibility warning.

[rsmith]

Thanks, this looks good.

include/clang/Basic/DiagnosticSemaKinds.td

6336 ↗

(On Diff #6518)

... "to the enclosing loop" might be clearer.

test/CodeGen/PR8880.c

9

This test will not work with a Clang built without asserts, because label names are not emitted into the text form of the IR. If you need to match label names, run the IR through opt -instnamer -S before piping it into FileCheck (see test/CodeGenCXX/for-range.cpp for an example).

test/Sema/loop-control.cpp

1 ↗

(On Diff #6518)

Can you fold this and loop-control.c into the same source file? (Maybe use -verify for the C test and -Werror for this one.)

[sepavloff]

Closed by commit rL199897 (authored by @sepavloff).

include/clang/Sema/Scope.h

public:
 
   const_udir_iterator using_directives_end() const {
     return UsingDirectives.end();
   }
 
   /// 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
 
 #endif

lib/Parse/ParseStmt.cpp

StmtResult Parser::ParseSwitchStatement(SourceLocation *TrailingElseLoc) {
   // A name introduced by a declaration in a condition is in scope from its
   // point of declaration until the end of the substatements controlled by the
   // condition.
   // C++ 3.3.2p4:
   // Names declared in the for-init-statement, and in the condition of if,
   // 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);
 
   // Parse the condition.
   ExprResult Cond;
   Decl *CondVar = 0;
   if (ParseParenExprOrCondition(Cond, CondVar, SwitchLoc, false))
StmtResult Parser::ParseSwitchStatement(SourceLocation *TrailingElseLoc) {
   //
   // C++ 6.4p1:
   // The substatement in a selection-statement (each substatement, in the else
   // form of the if statement) implicitly defines a local scope.
   //
   // 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));
 
   // Read the body statement.
   StmtResult Body(ParseStatement(TrailingElseLoc));
 
   // Pop the scopes.
   InnerScope.Exit();
StmtResult Parser::ParseWhileStatement(SourceLocation *TrailingElseLoc) {
   // A name introduced by a declaration in a condition is in scope from its
   // point of declaration until the end of the substatements controlled by the
   // condition.
   // C++ 3.3.2p4:
   // Names declared in the for-init-statement, and in the condition of if,
   // 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.
   ExprResult Cond;
   Decl *CondVar = 0;
   if (ParseParenExprOrCondition(Cond, CondVar, WhileLoc, true))
     return StmtError();
 
   FullExprArg FullCond(Actions.MakeFullExpr(Cond.get(), WhileLoc));
 
   // C99 6.8.5p5 - In C99, the body of the while 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.
   //
   // C++ 6.5p2:
   // The substatement in an iteration-statement implicitly defines a local scope
   // which is entered and exited each time through the loop.
   //
   // 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));
 
   // Read the body statement.
   StmtResult Body(ParseStatement(TrailingElseLoc));
 
   // Pop the body scope if needed.
   InnerScope.Exit();
ParseScope InnerScope(this, Scope::DeclScope,
                         (getLangOpts().C99 || getLangOpts().CPlusPlus) &&
                         Tok.isNot(tok::l_brace));
 
   // Read the body statement.
   StmtResult Body(ParseStatement());
 
   // Pop the body scope if needed.
   InnerScope.Exit();
+  getCurScope()->RemoveFlags(Scope::BreakScope | Scope::ContinueScope);
 
   if (Tok.isNot(tok::kw_while)) {
     if (!Body.isInvalid()) {
       Diag(Tok, diag::err_expected_while);
       Diag(DoLoc, diag::note_matching) << "'do'";
       SkipUntil(tok::semi, StopBeforeMatch);
     }
     return StmtError();
StmtResult Parser::ParseForStatement(SourceLocation *TrailingElseLoc) {
   // C++ 3.3.2p4:
   // Names declared in the for-init-statement, and in the condition of if,
   // while, for, and switch statements are local to the if, while, for, or
   // switch statement (including the controlled statement).
   // C++ 6.5.3p1:
   // 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);
 
   BalancedDelimiterTracker T(*this, tok::l_paren);
   T.consumeOpen();
 
   ExprResult Value;
 
if (Tok.isNot(tok::semi)) {
         SkipUntil(tok::r_paren, StopAtSemi | StopBeforeMatch);
     }
 
     if (Tok.is(tok::semi)) {
       ConsumeToken();
     }
 
     // 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();
 
   // We need to perform most of the semantic analysis for a C++0x for-range
   // statememt before parsing the body, in order to be able to deduce the type
   // of an auto-typed loop variable.
   StmtResult ForRangeStmt;

lib/Sema/Scope.cpp

bool Scope::containedInPrototypeScope() const {
   const Scope *S = this;
   while (S) {
     if (S->isFunctionPrototypeScope())
       return true;
     S = S->getParent();
   }
   return false;
 }
+
+void Scope::AddFlags(unsigned FlagsToSet) {
+  assert((FlagsToSet & ~(BreakScope | ContinueScope)) == 0 &&
+         "Unsupported scope flags");
+  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 & BreakScope) != 0 && "Already cleared");
+    BreakParent = AnyParent->BreakParent;
+ }
+  if (FlagsToClear & ContinueScope) {
+    assert((Flags & ContinueScope) != 0 && "Already cleared");
+    ContinueParent = AnyParent->ContinueParent;
+  }
+  Flags &= ~FlagsToClear;
+}

test/CodeGen/PR8880.c

+// RUN: %clang_cc1 -emit-llvm -o - %s | FileCheck %s
+
+void pr8880_cg_1(int *iptr) {
+// CHECK-LABEL: define void @pr8880_cg_1(
+  int i, j;
+// CHECK: br label %[[OUTER_COND:[0-9A-Za-z$._]+]]
+  for (i = 2; i != 10 ; i++ )
+// CHECK: [[OUTER_COND]]
+// CHECK: label %[[OUTER_BODY:[0-9A-Za-z$._]+]]
+// CHECK: [[OUTER_BODY]]
+// CHECK: label %[[INNER_COND:[0-9A-Za-z$._]+]]
+    for (j = 3 ; j < 22; (void)({ ++j; break; j;})) {
+// CHECK: [[INNER_COND]]
+// CHECK: label %[[INNER_BODY:[0-9A-Za-z$._]+]]
+// CHECK: [[INNER_BODY]]
+      *iptr = 7;
+// CHECK: label %[[INNER_INC:[0-9A-Za-z$._]+]]
+// CHECK: [[INNER_INC]]
+
+// break in 3rd expression of inner loop causes branch to end of inner loop
+
+// CHECK: br label %[[INNER_END:[0-9A-Za-z$._]+]]
+// CHECK: [[INNER_END]]
+    }
+// CHECK: br label %[[OUTER_INC:[0-9A-Za-z$._]+]]
+// CHECK: [[OUTER_INC]]
+// CHECK: br label %[[OUTER_COND]]
+// CHECK: ret
+}
+
+void pr8880_cg_2(int *iptr) {
+// CHECK-LABEL: define void @pr8880_cg_2(
+  int i, j;
+// CHECK: br label %[[OUTER_COND:[0-9A-Za-z$._]+]]
+  for (i = 2; i != 10 ; i++ )
+// CHECK: [[OUTER_COND]]
+// CHECK: label %[[OUTER_BODY:[0-9A-Za-z$._]+]]
+// CHECK: [[OUTER_BODY]]
+// CHECK: label %[[INNER_COND:[0-9A-Za-z$._]+]]
+    for (j = 3 ; j < 22; (void)({ ++j; continue; j;})) {
+// CHECK: [[INNER_COND]]
+// CHECK: label %[[INNER_BODY:[0-9A-Za-z$._]+]], label %[[INNER_END:[0-9A-Za-z$._]+]]
+// CHECK: [[INNER_BODY]]
+      *iptr = 7;
+// CHECK: label %[[INNER_INC:[0-9A-Za-z$._]+]]
+// CHECK: [[INNER_INC]]
+
+// continue in 3rd expression of inner loop causes branch to inc of inner loop
+
+// CHECK: br label %[[INNER_INC]]
+// CHECK: [[INNER_END]]
+    }
+// CHECK: br label %[[OUTER_INC:[0-9A-Za-z$._]+]]
+// CHECK: [[OUTER_INC]]
+// CHECK: br label %[[OUTER_COND]]
+// CHECK: ret
+}
+
+void pr8880_cg_3(int *iptr) {
+// CHECK-LABEL: define void @pr8880_cg_3(
+  int i, j;
+// CHECK: br label %[[OUTER_COND:[0-9A-Za-z$._]+]]
+  for (i = 2 ; i != 10 ; i++ )
+// CHECK: [[OUTER_COND]]
+// CHECK: label %[[OUTER_BODY:[0-9A-Za-z$._]+]]
+// CHECK: [[OUTER_BODY]]
+// CHECK: br label %[[INNER_COND:[0-9A-Za-z$._]+]]
+    for (j = 3 ; ({break; j;}); j++) {
+
+// break in 2nd expression of inner loop causes branch to end of outer loop
+
+// CHECK: [[INNER_COND]]
+// CHECK: br label %[[OUTER_END:[0-9A-Za-z$._]+]]
+// CHECK: label %[[INNER_BODY:[0-9A-Za-z$._]+]], label %[[INNER_END:[0-9A-Za-z$._]+]]
+// CHECK: [[INNER_BODY]]
+// CHECK: label %[[INNER_INC:[0-9A-Za-z$._]+]]
+// CHECK: [[INNER_INC]]
+      *iptr = 7;
+// CHECK: br label %[[INNER_COND]]
+    }
+// CHECK: [[INNER_END]]
+// CHECK: br label %[[OUTER_INC:[0-9A-Za-z$._]+]]
+// CHECK: [[OUTER_INC]]
+// CHECK: br label %[[OUTER_COND]]
+// CHECK: [[OUTER_END]]
+// CHECK: ret
+}
+
+void pr8880_cg_4(int *iptr) {
+// CHECK-LABEL: define void @pr8880_cg_4(
+  int i, j;
+// CHECK: br label %[[OUTER_COND:[0-9A-Za-z$._]+]]
+  for (i = 2 ; i != 10 ; i++ )
+// CHECK: [[OUTER_COND]]
+// CHECK: label %[[OUTER_BODY:[0-9A-Za-z$._]+]]
+// CHECK: [[OUTER_BODY]]
+// CHECK: br label %[[INNER_COND:[0-9A-Za-z$._]+]]
+    for (j = 3 ; ({continue; j;}); j++) {
+
+// continue in 2nd expression of inner loop causes branch to inc of outer loop
+
+// CHECK: [[INNER_COND]]
+// CHECK: br label %[[OUTER_INC:[0-9A-Za-z$._]+]]
+// CHECK: label %[[INNER_BODY:[0-9A-Za-z$._]+]], label %[[INNER_END:[0-9A-Za-z$._]+]]
+// CHECK: [[INNER_BODY]]
+// CHECK: label %[[INNER_INC:[0-9A-Za-z$._]+]]
+// CHECK: [[INNER_INC]]
+      *iptr = 7;
+// CHECK: br label %[[INNER_COND]]
+    }
+// CHECK: [[INNER_END]]
+// CHECK: br label %[[OUTER_INC:[0-9A-Za-z$._]+]]
+// CHECK: [[OUTER_INC]]
+// CHECK: br label %[[OUTER_COND]]
+// CHECK: [[OUTER_END]]
+// CHECK: ret
+}

test/Parser/bad-control.c

 /* RUN: %clang_cc1 -fsyntax-only -verify %s
 */
 void foo() { 
   break; /* expected-error {{'break' statement not in loop or switch statement}} */
 }
 
 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}} */
+}

test/Sema/statements.c

void foo(enum x X) {
 
   switch (X) { // expected-warning {{5 enumeration values not handled in switch: 'c', 'd', 'e'...}}
   case a:
   case b:
     break;
   }
 }
 
-// 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;
 }