Ignore trailing NullStmts in StmtExprs for GCC compatibility

[domdom]

Summary

Ignore trailing NullStmts in compound expressions when determining the result type and value. This is to match the GCC behavior which ignores semicolons at the end of compound expressions.

[domdom]

Formatting

[aaron.ballman]

Can you also add a test case to TestMiscStmts() in clang\test\AST\ast-dump-stmt.c that demonstrates we calculate the correct resulting type and don't lose the extra null statements from the AST?

clang/lib/Parse/ParseStmt.cpp

975–984

gcc -> GCC

Also, add a full stop to the end of the comment.

976

This should be unsigned rather than int.

978

Prefer ++LookHead; since the result isn't needed.

984

Remove spurious newline.

clang/lib/Sema/SemaExpr.cpp

13372

Add full stop to the end of the sentence.

13374–13377

Add full stop to the end of the sentence.

13377

Add full stop to the end of the sentence.

[domdom]

Thanks for your comments, @aaron.ballman. I've addressed the comments and added a test case as suggested.

This revealed an issue with the code-gen side of things, so I fixed that and added another test case for that as well.

[aaron.ballman]

clang/include/clang/AST/Stmt.h

1303

How about getIndexOfLastNonNullStmt() since it doesn't return the Stmt* itself?

clang/lib/CodeGen/CGStmt.cpp

387

const Stmt *

412

What happens if ExprResult is nullptr?

clang/test/CodeGen/exprs.c

202

Does this test need the extra null statement between the 3; and 4;?

[domdom]

clang/include/clang/AST/Stmt.h

1303

Agreed.

1328

getLastNonNullStmt asserts anyway, should I remove this?

clang/lib/CodeGen/CGStmt.cpp

412

Then ExprResult == *I should not be evaluated. (Since GetLast would be false)

clang/test/CodeGen/exprs.c

202

Not strictly speaking, no. Just added it to ensure it has no effect.

[aaron.ballman]

clang/include/clang/AST/Stmt.h

1303

What do you think about having this function return an optional<unsigned> rather than asserting? This will keep the API safe when called on an empty CompoundStmt.

1328

If you did switch to using an optional, then you can assert the optional has a value here. The effect should be the same as your current approach, but with slightly more tolerance.

1337

And if getLastIndexOfNonNullStmt() returns an empty value here, you can return nullptr to signify that there is no statement expression result possible.

clang/lib/CodeGen/CGStmt.cpp

412

I think I was worried about a different situation that isn't present in this patch. However, if you switch to using optional, then getStmtExprResult() could potentially return null as well (when GetLast is true), so we'd have to handle that (probably with an assertion).

clang/test/CodeGen/exprs.c

202

That sounds good to me!

[domdom]

Switched getIndexOfLastNonNullStmt to return Optional<unsigned> instead of Stmt* as per comments.

[aaron.ballman]

clang/include/clang/AST/Stmt.h

1301–1302

Given the name of the function, why return the index of the last null statement if it only contains null statements?

1312

The only way you can get here is if all statements are null statements, so this should return None as well, shouldn't it?

1338–1339

return ExprResult ? body_begin()[*ExprResult] : nullptr;

[riccibruno]

clang/include/clang/AST/Stmt.h

1302

Additionally I am not sure that the comment is optimal. This is inside CompoundStmt and it is therefore strange to use "if this compound expression", since the compound expression is represented with the StmtExpr node.

1326

I think it needs at the very least to mention that this is about the GNU extension. Perhaps it would be useful to mention the relation between the CompoundStmt node and the StmtExpr node ? Also more generally is it not possible to avoid mutating the compound statement node after it has been created ?

1335

Same, I would find it clearer if you mentioned the extension.

[rsmith]

You'll also need to update TreeTransform::TransformCompoundStmt. (And please add some tests for template instantiation.)

[domdom]

Sorry I it's taken me a while to get back to this work. I've rebased the changes and taken advantage of the refactoring to stop modifying the CompoundStmt after creating it. This definitely simplified the changes required in Stmt.h, which is nice.

I've addressed the the need to update the TreeTransformer for the template case, and added a test case for that.

Something I should ask, it seems like GCC only ignores the NullStmts at the end if it's in C mode. Should clang match this behaviour exactly?

[aaron.ballman]

In D57086#1535873, @domdom wrote:

Sorry I it's taken me a while to get back to this work. I've rebased the changes and taken advantage of the refactoring to stop modifying the CompoundStmt after creating it. This definitely simplified the changes required in Stmt.h, which is nice.

I've addressed the the need to update the TreeTransformer for the template case, and added a test case for that.

Thanks! I think this is looking good to me, but you should wait for the other reviewers before committing in case they have further concerns.

Something I should ask, it seems like GCC only ignores the NullStmts at the end if it's in C mode. Should clang match this behaviour exactly?

I can't think of a reason that this should only happen in C mode, can you @rsmith?

clang/test/SemaCXX/statements.cpp

41–52

Be sure to run the patch through clang-format.

[rsmith]

In D57086#1546386, @aaron.ballman wrote:

In D57086#1535873, @domdom wrote:

Something I should ask, it seems like GCC only ignores the NullStmts at the end if it's in C mode. Should clang match this behaviour exactly?

I can't think of a reason that this should only happen in C mode, can you @rsmith?

No, I can't think of such a reason either. When we've seen such weirdnesses before (eg, break or continue in a statement expression in a for loop increment expression) we generally try to pick a behavior that's consistent across languages, and warn on the cases where we give a different behavior from GCC as a result.

[domdom]

clang-format the patch

[aaron.ballman]

In D57086#1549632, @domdom wrote:

clang-format the patch

Thanks! Do you need someone to commit on your behalf?

[domdom]

In D57086#1550514, @aaron.ballman wrote:

In D57086#1549632, @domdom wrote:

clang-format the patch

Thanks! Do you need someone to commit on your behalf?

You are very welcome; thank you both for your comments!

I do need someone to commit on my behalf :)

[aaron.ballman]

In D57086#1551354, @domdom wrote:

In D57086#1550514, @aaron.ballman wrote:

In D57086#1549632, @domdom wrote:

clang-format the patch

Thanks! Do you need someone to commit on your behalf?

You are very welcome; thank you both for your comments!

I do need someone to commit on my behalf :)

I'm sorry for the incredibly long delay in committing this for you -- I managed to lose track of this thread. I went to apply the changes today and get the following test failures when trying on Windows 10 x64:

FAIL: Clang :: AST/ast-dump-stmt.c (152 of 11055)
******************** TEST 'Clang :: AST/ast-dump-stmt.c' FAILED ********************
Script:
--
: 'RUN: at line 1';   c:\cmakebuilds\build\x64-debug\bin\clang.exe -cc1 -internal-isystem c:\cmakebuilds\build\x64-debug\lib\clang\9.0.0\include -nostdsysteminc -std=gnu11 -ast-dump C:\llvm\tools\clang\test\AST\ast-dump-stmt.c | c:\cmakebuilds\build\x64-debug\bin\filecheck.exe -strict-whitespace C:\llvm\tools\clang\test\AST\ast-dump-stmt.c
--
Exit Code: 1

Command Output (stdout):
--
$ ":" "RUN: at line 1"
$ "c:\cmakebuilds\build\x64-debug\bin\clang.exe" "-cc1" "-internal-isystem" "c:\cmakebuilds\build\x64-debug\lib\clang\9.0.0\include" "-nostdsysteminc" "-std=gnu11" "-ast-dump" "C:\llvm\tools\clang\test\AST\ast-dump-stmt.c"
# command stderr:
C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:48:3: warning: expression result unused
  -T1;
  ^~~
C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:49:3: warning: expression result unused
  -T2;
  ^~~
C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:58:3: warning: expression result unused
  ~T1;
  ^~~
C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:59:3: warning: expression result unused
  ~T2;
  ^~~
C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:70:21: warning: expression result unused
  _Generic(i, int : 12);
                    ^~
C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:79:21: warning: expression result unused
  _Generic(i, int : 12, default : 0);
                    ^~
C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:90:34: warning: expression result unused
  _Generic(i, default : 0, int : 12);
                                 ^~
C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:101:21: warning: expression result unused
  _Generic(i, int : 12, float : 10, default : 100);
                    ^~
C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:142:3: warning: expression result unused
  0;
  ^
C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:288:8: warning: expression result unused
  for (b; b; b)
       ^
C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:288:14: warning: expression result unused
  for (b; b; b)
             ^
C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:367:17: warning: expression result unused
  ({int a = 10; a;});
                ^
C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:375:3: warning: expression result unused
  ({int a = 10; a;;; });
  ^~~~~~~~~~~~~~~~~~~~~
13 warnings generated.

$ "c:\cmakebuilds\build\x64-debug\bin\filecheck.exe" "-strict-whitespace" "C:\llvm\tools\clang\test\AST\ast-dump-stmt.c"
# command stderr:
C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:376:18: error: CHECK-NEXT: expected string not found in input
  // CHECK-NEXT: StmtExpr 0x{{[^ ]*}} <line:[[@LINE-1]]:3, col:22> 'int'
                 ^
<stdin>:282:5: note: scanning from here
    `-StmtExpr 0x19f2d493d18 <line:375:3, col:23> 'int'
    ^
<stdin>:282:5: note: with "@LINE-1" equal to "375"
    `-StmtExpr 0x19f2d493d18 <line:375:3, col:23> 'int'
    ^
<stdin>:282:14: note: possible intended match here
    `-StmtExpr 0x19f2d493d18 <line:375:3, col:23> 'int'
             ^

error: command failed with exit status: 1

--

It looks like the column number is off by one, and I wasn't certain why. Can you look into that before I commit?

[domdom]

Thank you for catching that @aaron.ballman!

I forgot to update the test after the running clang-format. I thought I ran the tests again before submitting, but I guess I didn't :P

Thanks again!

[aaron.ballman]

I've commit on your behalf in r365498. Thank you for the patch!

clang/include/clang/AST/Stmt.h

class CompoundStmt final : public Stmt,
   explicit CompoundStmt(EmptyShell Empty) : Stmt(CompoundStmtClass, Empty) {}
 
   void setStmts(ArrayRef<Stmt *> Stmts);
 
 public:
   static CompoundStmt *Create(const ASTContext &C, ArrayRef<Stmt *> Stmts,
                               SourceLocation LB, SourceLocation RB);
 
   // Build an empty compound statement with a location.
   explicit CompoundStmt(SourceLocation Loc)
       : Stmt(CompoundStmtClass), RBraceLoc(Loc) {
     CompoundStmtBits.NumStmts = 0;
     CompoundStmtBits.LBraceLoc = Loc;
   }
 
   // Build an empty compound statement.
   static CompoundStmt *CreateEmpty(const ASTContext &C, unsigned NumStmts);
 
   bool body_empty() const { return CompoundStmtBits.NumStmts == 0; }
   unsigned size() const { return CompoundStmtBits.NumStmts; }
 
   using body_iterator = Stmt **;
   using body_range = llvm::iterator_range<body_iterator>;
 
   body_range body() { return body_range(body_begin(), body_end()); }
   body_iterator body_begin() { return getTrailingObjects<Stmt *>(); }
   body_iterator body_end() { return body_begin() + size(); }
   Stmt *body_front() { return !body_empty() ? body_begin()[0] : nullptr; }
 
   Stmt *body_back() {
     return !body_empty() ? body_begin()[size() - 1] : nullptr;
   }
 
-  void setLastStmt(Stmt *S) {
-    assert(!body_empty() && "setLastStmt");
-    body_begin()[size() - 1] = S;
-  }
-
   using const_body_iterator = Stmt *const *;
   using body_const_range = llvm::iterator_range<const_body_iterator>;
 
   body_const_range body() const {
     return body_const_range(body_begin(), body_end());
   }
 
   const_body_iterator body_begin() const {
     return getTrailingObjects<Stmt *>();
   }
 
   const_body_iterator body_end() const { return body_begin() + size(); }
 
   const Stmt *body_front() const {
     return !body_empty() ? body_begin()[0] : nullptr;
   }
 
   const Stmt *body_back() const {
     return !body_empty() ? body_begin()[size() - 1] : nullptr;
   }
 
   using reverse_body_iterator = std::reverse_iterator<body_iterator>;
public:
   const_reverse_body_iterator body_rbegin() const {
     return const_reverse_body_iterator(body_end());
   }
 
   const_reverse_body_iterator body_rend() const {
     return const_reverse_body_iterator(body_begin());
   }
 
+  // Get the Stmt that StmtExpr would consider to be the result of this
+  // compound statement. This is used by StmtExpr to properly emulate the GCC
+  // compound expression extension, which ignores trailing NullStmts when
+  // getting the result of the expression.
+  // i.e. ({ 5;;; })
+  //           ^^ ignored
+  // If we don't find something that isn't a NullStmt, just return the last
+  // Stmt.
+  Stmt *getStmtExprResult() {
+    for (auto *B : llvm::reverse(body())) {
+      if (!isa<NullStmt>(B))
+        return B;
+    }
+    return body_back();
+  }
+
+  const Stmt *getStmtExprResult() const {
+    return const_cast<CompoundStmt *>(this)->getStmtExprResult();
+  }
+
   SourceLocation getBeginLoc() const { return CompoundStmtBits.LBraceLoc; }
   SourceLocation getEndLoc() const { return RBraceLoc; }
 
   SourceLocation getLBracLoc() const { return CompoundStmtBits.LBraceLoc; }
   SourceLocation getRBracLoc() const { return RBraceLoc; }
 
   static bool classof(const Stmt *T) {
     return T->getStmtClass() == CompoundStmtClass;

clang/lib/CodeGen/CGStmt.cpp

Address CodeGenFunction::EmitCompoundStmt(const CompoundStmt &S, bool GetLast,
   return EmitCompoundStmtWithoutScope(S, GetLast, AggSlot);
 }
 
 Address
 CodeGenFunction::EmitCompoundStmtWithoutScope(const CompoundStmt &S,
                                               bool GetLast,
                                               AggValueSlot AggSlot) {
 
-  for (CompoundStmt::const_body_iterator I = S.body_begin(),
-       E = S.body_end()-GetLast; I != E; ++I)
-    EmitStmt(*I);
+  const Stmt *ExprResult = S.getStmtExprResult();
+  assert((!GetLast || (GetLast && ExprResult)) &&
+         "If GetLast is true then the CompoundStmt must have a StmtExprResult");
 
   Address RetAlloca = Address::invalid();
-  if (GetLast) {
+
+  for (auto *CurStmt : S.body()) {
+    if (GetLast && ExprResult == CurStmt) {
       // We have to special case labels here.  They are statements, but when put
       // at the end of a statement expression, they yield the value of their
       // subexpression.  Handle this by walking through all labels we encounter,
       // emitting them before we evaluate the subexpr.
       // Similar issues arise for attributed statements.
-    const Stmt *LastStmt = S.body_back();
-    while (!isa<Expr>(LastStmt)) {
-      if (const auto *LS = dyn_cast<LabelStmt>(LastStmt)) {
+      while (!isa<Expr>(ExprResult)) {
+        if (const auto *LS = dyn_cast<LabelStmt>(ExprResult)) {
           EmitLabel(LS->getDecl());
-        LastStmt = LS->getSubStmt();
-      } else if (const auto *AS = dyn_cast<AttributedStmt>(LastStmt)) {
+          ExprResult = LS->getSubStmt();
+        } else if (const auto *AS = dyn_cast<AttributedStmt>(ExprResult)) {
           // FIXME: Update this if we ever have attributes that affect the
           // semantics of an expression.
-        LastStmt = AS->getSubStmt();
+          ExprResult = AS->getSubStmt();
         } else {
           llvm_unreachable("unknown value statement");
         }
       }
 
       EnsureInsertPoint();
 
-    const Expr *E = cast<Expr>(LastStmt);
+      const Expr *E = cast<Expr>(ExprResult);
       QualType ExprTy = E->getType();
       if (hasAggregateEvaluationKind(ExprTy)) {
         EmitAggExpr(E, AggSlot);
       } else {
         // We can't return an RValue here because there might be cleanups at
         // the end of the StmtExpr.  Because of that, we have to emit the result
         // here into a temporary alloca.
         RetAlloca = CreateMemTemp(ExprTy);
         EmitAnyExprToMem(E, RetAlloca, Qualifiers(),
                          /*IsInit*/ false);
       }
+    } else {
+      EmitStmt(CurStmt);
+    }
   }
 
   return RetAlloca;
 }
 
 void CodeGenFunction::SimplifyForwardingBlocks(llvm::BasicBlock *BB) {
   llvm::BranchInst *BI = dyn_cast<llvm::BranchInst>(BB->getTerminator());
 

clang/lib/Parse/ParseStmt.cpp

bool Parser::ConsumeNullStmt(StmtVector &Stmts) {
   Diag(StartLoc, diag::warn_null_statement)
       << FixItHint::CreateRemoval(SourceRange(StartLoc, EndLoc));
   return true;
 }
 
 StmtResult Parser::handleExprStmt(ExprResult E, ParsedStmtContext StmtCtx) {
   bool IsStmtExprResult = false;
   if ((StmtCtx & ParsedStmtContext::InStmtExpr) != ParsedStmtContext()) {
-    // Look ahead to see if the next two tokens close the statement expression;
-    // if so, this expression statement is the last statement in a
-    // statment expression.
-    IsStmtExprResult = Tok.is(tok::r_brace) && NextToken().is(tok::r_paren);
+    // For GCC compatibility we skip past NullStmts.
+    unsigned LookAhead = 0;
+    while (GetLookAheadToken(LookAhead).is(tok::semi)) {
+      ++LookAhead;
+    }
+    // Then look to see if the next two tokens close the statement expression;
+    // if so, this expression statement is the last statement in a statment
+    // expression.
+    IsStmtExprResult = GetLookAheadToken(LookAhead).is(tok::r_brace) &&
+                       GetLookAheadToken(LookAhead + 1).is(tok::r_paren);
   }
 
   if (IsStmtExprResult)
     E = Actions.ActOnStmtExprResult(E);
   return Actions.ActOnExprStmt(E, /*DiscardedValue=*/!IsStmtExprResult);
 }
 
 /// ParseCompoundStatementBody - Parse a sequence of statements and invoke the

clang/lib/Sema/SemaExpr.cpp

Sema::ActOnStmtExpr(SourceLocation LPLoc, Stmt *SubStmt,
   PopExpressionEvaluationContext();
 
   // FIXME: there are a variety of strange constraints to enforce here, for
   // example, it is not possible to goto into a stmt expression apparently.
   // More semantic analysis is needed.
 
   // If there are sub-stmts in the compound stmt, take the type of the last one
   // as the type of the stmtexpr.
   QualType Ty = Context.VoidTy;
   bool StmtExprMayBindToTemp = false;
   if (!Compound->body_empty()) {
-    if (const auto *LastStmt = dyn_cast<ValueStmt>(Compound->body_back())) {
+    // For GCC compatibility we get the last Stmt excluding trailing NullStmts.
+    if (const auto *LastStmt =
+            dyn_cast<ValueStmt>(Compound->getStmtExprResult())) {
       if (const Expr *Value = LastStmt->getExprStmt()) {
         StmtExprMayBindToTemp = true;
         Ty = Value->getType();
       }
     }
   }
 
   // FIXME: Check that expression type is complete/non-abstract; statement

clang/lib/Sema/TreeTransform.h

 }
 
 template<typename Derived>
 StmtResult
 TreeTransform<Derived>::TransformCompoundStmt(CompoundStmt *S,
                                               bool IsStmtExpr) {
   Sema::CompoundScopeRAII CompoundScope(getSema());
 
+  const Stmt *ExprResult = S->getStmtExprResult();
   bool SubStmtInvalid = false;
   bool SubStmtChanged = false;
   SmallVector<Stmt*, 8> Statements;
   for (auto *B : S->body()) {
     StmtResult Result = getDerived().TransformStmt(
-        B,
-        IsStmtExpr && B == S->body_back() ? SDK_StmtExprResult : SDK_Discarded);
+        B, IsStmtExpr && B == ExprResult ? SDK_StmtExprResult : SDK_Discarded);
 
     if (Result.isInvalid()) {
       // Immediately fail if this was a DeclStmt, since it's very
       // likely that this will cause problems for future statements.
       if (isa<DeclStmt>(B))
         return StmtError();
 
       // Otherwise, just keep processing substatements and fail later.

clang/test/AST/ast-dump-stmt.c

void TestMiscStmts(void) {
   ({int a = 10; a;});
   // CHECK: StmtExpr 0x{{[^ ]*}} <line:[[@LINE-1]]:3, col:20> 'int'
   // CHECK-NEXT: CompoundStmt
   // CHECK-NEXT: DeclStmt
   // CHECK-NEXT: VarDecl 0x{{[^ ]*}} <col:5, col:13> col:9 used a 'int' cinit
   // CHECK-NEXT: IntegerLiteral 0x{{[^ ]*}} <col:13> 'int' 10
   // CHECK-NEXT: ImplicitCastExpr
   // CHECK-NEXT: DeclRefExpr 0x{{[^ ]*}} <col:17> 'int' lvalue Var 0x{{[^ ]*}} 'a' 'int'
+  ({int a = 10; a;;; });
+  // CHECK-NEXT: StmtExpr 0x{{[^ ]*}} <line:[[@LINE-1]]:3, col:23> 'int'
+  // CHECK-NEXT: CompoundStmt
+  // CHECK-NEXT: DeclStmt
+  // CHECK-NEXT: VarDecl 0x{{[^ ]*}} <col:5, col:13> col:9 used a 'int' cinit
+  // CHECK-NEXT: IntegerLiteral 0x{{[^ ]*}} <col:13> 'int' 10
+  // CHECK-NEXT: ImplicitCastExpr
+  // CHECK-NEXT: DeclRefExpr 0x{{[^ ]*}} <col:17> 'int' lvalue Var 0x{{[^ ]*}} 'a' 'int'
+  // CHECK-NEXT: NullStmt
+  // CHECK-NEXT: NullStmt
 }

clang/test/CodeGen/exprs.c

typedef union u {
     int i;
 } strct;
 int returns_int(void);
 void f18() {
   (strct)returns_int();
 }
 // CHECK-LABEL: define void @f18()
 // CHECK: call i32 @returns_int()
+
+// Ensure the right stmt is returned
+int f19() {
+  return ({ 3;;4;; });
+}
+// CHECK-LABEL: define i32 @f19()
+// CHECK: [[T:%.*]] = alloca i32
+// CHECK: store i32 4, i32* [[T]]
+// CHECK: [[L:%.*]] = load i32, i32* [[T]]
+// CHECK: ret i32 [[L]]

clang/test/Sema/statements.c

struct Bug {
     union Nested {
       typeof(({ unsigned long __ptr; (int *)(0); })) __val;
     } n;
   };
   enum E {
     SIZE = sizeof(({unsigned long __ptr; __ptr;}))
   };
 }
+
+// GCC ignores empty statements at the end of compound expressions where the
+// result type is concerned.
+void test13() {
+  int a;
+  a = ({ 1; });
+  a = ({1;; });
+  a = ({int x = 1; (void)x; }); // expected-error {{assigning to 'int' from incompatible type 'void'}}
+  a = ({int x = 1; (void)x;; }); // expected-error {{assigning to 'int' from incompatible type 'void'}}
+}
+
+void test14() { return ({}); }
+void test15() {
+  return ({;;;; });
+}
+void test16() {
+  return ({test:;; });
+}

clang/test/SemaCXX/statements.cpp

 }
 void test5() {
   char buf[1];
   __asm__ ("":"+r" (buf));
 }
 
 struct MMX_t {};
 void test6() { __asm__("" : "=m"(*(MMX_t *)0)); }
+
+template <typename T>
+T test7(T v) {
+  return ({ // expected-warning{{use of GNU statement expression extension}}
+    T a = v;
+    a;
+    ;
+    ;
+  });
+}
+
+void test8() {
+  int a = test7(1);
+  double b = test7(2.0);
+}