PR40642: Fix determination of whether the final statement of a statementexpression is a discarded-value expression.

[rsmith]

Summary

We used to get this wrong in three ways:

1. During parsing, an expression-statement followed by the }) ending a statement expression was always treated as producing the value of the statement expression. That's wrong for ({ if (1) expr; })
2. During template instantiation, various kinds of statement (most statements not appearing directly in a compound-statement) were not treated as discarded-value expressions, resulting in missing volatile loads (etc).
3. In all contexts, an expression-statement with attributes was not treated as producing the value of the statement expression, eg ({ [[attr]] expr; }).

[aaron.ballman]

Thank you for working on this! It looks like we did have to add the new AST node after all, but the changes seem pretty good. Should anything be updated in the AST dumper for this (such as printing whether a Stmt is a value-producing statement or not)?

include/clang/AST/Stmt.h

1598–1602

We typically implement this in reverse, where the non-const version holds the actual implementation and the const version performs a const_cast.

include/clang/Parse/Parser.h

374

Rather than passing around IsInStmtExpr to a bunch of parser APIs, would it make more sense to add an RAII object that sets some state on Parser to test it? The proliferation of arguments seems unfortunate given how much of a corner case statement expressions are.

lib/Sema/SemaExpr.cpp

13319–13322

const auto *

13355

auto *

[rsmith]

Address a couple of review comments.

[rsmith]

@ABataev Is it intentional that we do not propagate Allowed through labels? For example:

void f() {
  #pragma omp barrier // ok

label:
  #pragma omp barrier // error, "cannot be an immediate substatement"

label:
  ;
  #pragma omp barrier // ok
}

?

include/clang/AST/Stmt.h

1598–1602

We do. Do you think that's preferable? I like that this way around proves that the const version is const-correct, but it's a tiny benefit and I'm fine with just being consistent.

include/clang/Parse/Parser.h

374

Yeah, I tried that approach first, but the parser state turns out to be much worse, because it puts a burden on every form of statement that constructs a nested parsing context to reset that state. We can put the resetting on ParseScope, but it still needs to have an effect in the case where the scope is disabled, which is surprising, and there's no guarantee such statement constructs that can end in an expression will have a nested scope (consider, for instance, constructs like return x; or goto *label;). This is really local state that should only be propagated through a very small number of syntactic constructs rather than global state.

Maybe we could combine the new flag with the AllowOpenMPStandalone / AllowedConstructsKind flag into a more general kind of "statement context". The propagation rules aren't quite the same (AllowOpenMPStandalone doesn't get propagated through labels whereas IsInStmtExpr does), which is a little awkward, but maybe that's not so bad -- and maybe that's actually a bug in the OpenMP implementation?

[ABataev]

In D57984#1394050, @rsmith wrote:

@ABataev Is it intentional that we do not propagate Allowed through labels? For example:

void f() {
  #pragma omp barrier // ok

label:
  #pragma omp barrier // error, "cannot be an immediate substatement"

label:
  ;
  #pragma omp barrier // ok
}

?

No, it is a bug.

[rsmith]

In D57984#1394067, @ABataev wrote:

In D57984#1394050, @rsmith wrote:

@ABataev Is it intentional that we do not propagate Allowed through labels? For example:

void f() {
  #pragma omp barrier // ok

label:
  #pragma omp barrier // error, "cannot be an immediate substatement"

label:
  ;
  #pragma omp barrier // ok
}

?

No, it is a bug.

Great, then I'll unify this new flag with the Allowed mechanism and fix the bug as part of this change. Thanks!

[ABataev]

In D57984#1394167, @rsmith wrote:

In D57984#1394067, @ABataev wrote:

In D57984#1394050, @rsmith wrote:

@ABataev Is it intentional that we do not propagate Allowed through labels? For example:

void f() {
  #pragma omp barrier // ok

label:
  #pragma omp barrier // error, "cannot be an immediate substatement"

label:
  ;
  #pragma omp barrier // ok
}

?

No, it is a bug.

Great, then I'll unify this new flag with the Allowed mechanism and fix the bug as part of this change. Thanks!

Sure, thanks!

[aaron.ballman]

Considering that this has been fertile ground for buggy edge cases, should we revert my original changes from the 8.0 branch to give this more time to bake on trunk before releasing? Or do you think this is fine to move onto the release branch once finalized?

include/clang/AST/Stmt.h

1598–1602

Personally, I prefer the way you have it here (though I wish we had a global helper function to hide the dispatch dance).

include/clang/Parse/Parser.h

374

Maybe we could combine the new flag with the AllowOpenMPStandalone / AllowedConstructsKind flag into a more general kind of "statement context".

That seems like a sensible approach to me.

[rsmith]

Combine WithinStmtExpr flag with AllowedStmtKinds into a more general statement
context. In doing so, fix some bugs where the OpenMP context was not being
propagated properly through labels and expression-statements starting with @.

[aaron.ballman]

include/clang/Parse/Parser.h

374

It's a bit strange that the previous two enumerators are "Allow" and this is "In". Maybe it will be less of a concern when I see the uses though...

398

Spurious newline.

lib/Parse/ParseStmt.cpp

1033

ParsedStmtContext SubStmtCtx =
      ParsedStmtContext::Compound |
      (isStmtExpr ? ParsedStmtContext::InStmtExpr : 0);

?

1048

You can drop the nullptr here.

1085–1086

Should this be done as part of handleExprStmt()?

[ABataev]

include/clang/Parse/Parser.h

382

We have llvm/ADT/BitmaskEnum.h, which defines LLVM_MARK_AS_BITMASK_ENUM for the bitwise ops on the enums. Maybe it is better to use it rather than manually define the required operations?

lib/CodeGen/CGStmt.cpp

401

You don't need else if here, just ]ша] is enough since there is break; in the previous substatement.

[rsmith]

Address review comments.

include/clang/Parse/Parser.h

374

It's just a coincidence that the first two are both Allow* flags.

382

That makes usage of the context a bit more verbose (StmtCtx & X -> (StmtCtx & X) != ParsedStmtContext()), but I think that's acceptable.

lib/Parse/ParseStmt.cpp

1033

There's no implicit conversion from 0 to ParsedStmtContext, but yeah, something like that :)

1085–1086

In principle that might make sense, but it'd require passing the attributes through a few extra layers, and also making sure the attributes aren't handled twice along the more-common statement parsing codepaths. I'm inclined to defer that until we have a need to have the attributes in hand when processing a full-expression (and I suspect that time may never come).

[aaron.ballman]

LGTM!

lib/Parse/ParseStmt.cpp

1085–1086

Yeah, I think it's worth delaying that change then. Thank you for the explanation!

include/clang/AST/Expr.h

SubobjectAdjustment(const MemberPointerType *MPT, Expr *RHS)
     this->Ptr.MPT = MPT;
     this->Ptr.RHS = RHS;
   }
 };
 
 /// This represents one expression.  Note that Expr's are subclasses of Stmt.
 /// This allows an expression to be transparently used any place a Stmt is
 /// required.
-class Expr : public Stmt {
+class Expr : public ValueStmt {
   QualType TR;
 
 protected:
   Expr(StmtClass SC, QualType T, ExprValueKind VK, ExprObjectKind OK,
        bool TD, bool VD, bool ID, bool ContainsUnexpandedParameterPack)
-    : Stmt(SC)
+    : ValueStmt(SC)
   {
     ExprBits.TypeDependent = TD;
     ExprBits.ValueDependent = VD;
     ExprBits.InstantiationDependent = ID;
     ExprBits.ValueKind = VK;
     ExprBits.ObjectKind = OK;
     assert(ExprBits.ObjectKind == OK && "truncated kind");
     ExprBits.ContainsUnexpandedParameterPack = ContainsUnexpandedParameterPack;
     setType(T);
   }
 
   /// Construct an empty expression.
-  explicit Expr(StmtClass SC, EmptyShell) : Stmt(SC) { }
+  explicit Expr(StmtClass SC, EmptyShell) : ValueStmt(SC) { }
 
 public:
   QualType getType() const { return TR; }
   void setType(QualType t) {
     // In C++, the type of an expression is always adjusted so that it
     // will not have reference type (C++ [expr]p6). Use
     // QualType::getNonReferenceType() to retrieve the non-reference
     // type. Additionally, inspect Expr::isLvalue to determine whether

include/clang/AST/Stmt.h

 Stmt *SwitchCase::getSubStmt() {
   if (auto *CS = dyn_cast<CaseStmt>(this))
     return CS->getSubStmt();
   else if (auto *DS = dyn_cast<DefaultStmt>(this))
     return DS->getSubStmt();
   llvm_unreachable("SwitchCase is neither a CaseStmt nor a DefaultStmt!");
 }
 
+/// Represents a statement that could possibly have a value and type. This
+/// covers expression-statements, as well as labels and attributed statements.
+///
+/// Value statements have a special meaning when they are the last non-null
+/// statement in a GNU statement expression, where they determine the value
+/// of the statement expression.
+class ValueStmt : public Stmt {
+protected:
+  using Stmt::Stmt;
+
+public:
+  const Expr *getExprStmt() const;
+  Expr *getExprStmt() {
+    const ValueStmt *ConstThis = this;
+    return const_cast<Expr*>(ConstThis->getExprStmt());
+  }
+
+  static bool classof(const Stmt *T) {
+    return T->getStmtClass() >= firstValueStmtConstant &&
+           T->getStmtClass() <= lastValueStmtConstant;
+  }
+};
+
 /// LabelStmt - Represents a label, which has a substatement.  For example:
 ///    foo: return;
-class LabelStmt : public Stmt {
+class LabelStmt : public ValueStmt {
   LabelDecl *TheDecl;
   Stmt *SubStmt;
 
 public:
   /// Build a label statement.
   LabelStmt(SourceLocation IL, LabelDecl *D, Stmt *substmt)
-      : Stmt(LabelStmtClass), TheDecl(D), SubStmt(substmt) {
+      : ValueStmt(LabelStmtClass), TheDecl(D), SubStmt(substmt) {
     setIdentLoc(IL);
   }
 
   /// Build an empty label statement.
-  explicit LabelStmt(EmptyShell Empty) : Stmt(LabelStmtClass, Empty) {}
+  explicit LabelStmt(EmptyShell Empty) : ValueStmt(LabelStmtClass, Empty) {}
 
   SourceLocation getIdentLoc() const { return LabelStmtBits.IdentLoc; }
   void setIdentLoc(SourceLocation L) { LabelStmtBits.IdentLoc = L; }
 
   LabelDecl *getDecl() const { return TheDecl; }
   void setDecl(LabelDecl *D) { TheDecl = D; }
 
   const char *getName() const;
public:
   }
 };
 
 /// Represents an attribute applied to a statement.
 ///
 /// Represents an attribute applied to a statement. For example:
 ///   [[omp::for(...)]] for (...) { ... }
 class AttributedStmt final
-    : public Stmt,
+    : public ValueStmt,
       private llvm::TrailingObjects<AttributedStmt, const Attr *> {
   friend class ASTStmtReader;
   friend TrailingObjects;
 
   Stmt *SubStmt;
 
   AttributedStmt(SourceLocation Loc, ArrayRef<const Attr *> Attrs,
                  Stmt *SubStmt)
-      : Stmt(AttributedStmtClass), SubStmt(SubStmt) {
+      : ValueStmt(AttributedStmtClass), SubStmt(SubStmt) {
     AttributedStmtBits.NumAttrs = Attrs.size();
     AttributedStmtBits.AttrLoc = Loc;
     std::copy(Attrs.begin(), Attrs.end(), getAttrArrayPtr());
   }
 
   explicit AttributedStmt(EmptyShell Empty, unsigned NumAttrs)
-      : Stmt(AttributedStmtClass, Empty) {
+      : ValueStmt(AttributedStmtClass, Empty) {
     AttributedStmtBits.NumAttrs = NumAttrs;
     AttributedStmtBits.AttrLoc = SourceLocation{};
     std::fill_n(getAttrArrayPtr(), NumAttrs, nullptr);
   }
 
   const Attr *const *getAttrArrayPtr() const {
     return getTrailingObjects<const Attr *>();
   }

include/clang/Basic/StmtNodes.td

 class AttrSubject;
 
 class Stmt<bit abstract = 0> : AttrSubject {
   bit Abstract = abstract;
 }
 
 class DStmt<Stmt base, bit abstract = 0> : Stmt<abstract> {
   Stmt Base = base;
 }
 
 // Statements
 def NullStmt : Stmt;
 def CompoundStmt : Stmt;
-def LabelStmt : Stmt;
-def AttributedStmt : Stmt;
 def IfStmt : Stmt;
 def SwitchStmt : Stmt;
 def WhileStmt : Stmt;
 def DoStmt : Stmt;
 def ForStmt : Stmt;
 def GotoStmt : Stmt;
 def IndirectGotoStmt : Stmt;
 def ContinueStmt : Stmt;
 def BreakStmt : Stmt;
 def ReturnStmt : Stmt;
 def DeclStmt  : Stmt;
 def SwitchCase : Stmt<1>;
 def CaseStmt : DStmt<SwitchCase>;
 def DefaultStmt : DStmt<SwitchCase>;
 def CapturedStmt : Stmt;
 
+// Statements that might produce a value (for example, as the last non-null
+// statement in a GNU statement-expression).
+def ValueStmt : Stmt<1>;
+def LabelStmt : DStmt<ValueStmt>;
+def AttributedStmt : DStmt<ValueStmt>;
+
 // Asm statements
 def AsmStmt : Stmt<1>;
 def GCCAsmStmt : DStmt<AsmStmt>;
 def MSAsmStmt : DStmt<AsmStmt>;
 
 // Obj-C statements
 def ObjCAtTryStmt : Stmt;
 def ObjCAtCatchStmt : Stmt;
 def ObjCAtFinallyStmt : Stmt;
 def ObjCAtThrowStmt : Stmt;
 def ObjCAtSynchronizedStmt : Stmt;
 def ObjCForCollectionStmt : Stmt;
 def ObjCAutoreleasePoolStmt : Stmt;
 
 // C++ statements
 def CXXCatchStmt : Stmt;
 def CXXTryStmt : Stmt;
 def CXXForRangeStmt : Stmt;
 
 // C++ Coroutines TS statements
 def CoroutineBodyStmt : Stmt;
 def CoreturnStmt : Stmt;
 
 // Expressions
-def Expr : Stmt<1>;
+def Expr : DStmt<ValueStmt, 1>;
 def PredefinedExpr : DStmt<Expr>;
 def DeclRefExpr : DStmt<Expr>;
 def IntegerLiteral : DStmt<Expr>;
 def FixedPointLiteral : DStmt<Expr>;
 def FloatingLiteral : DStmt<Expr>;
 def ImaginaryLiteral : DStmt<Expr>;
 def StringLiteral : DStmt<Expr>;
 def CharacterLiteral : DStmt<Expr>;

include/clang/Parse/Parser.h

class Parser : public CodeCompletionHandler {
   /// declarations/definitions when indexing.
   bool SkipFunctionBodies;
 
   /// The location of the expression statement that is being parsed right now.
   /// Used to determine if an expression that is being parsed is a statement or
   /// just a regular sub-expression.
   SourceLocation ExprStatementTokLoc;
 
-  /// Tests whether an expression value is discarded based on token lookahead.
-  /// It will return true if the lexer is currently processing the })
-  /// terminating a GNU statement expression and false otherwise.
-  bool isExprValueDiscarded();
+  /// Whether we are parsing a "top-level" statement in a statement expression.
+  /// The statement within a labeled expression in a statement expression is
+  /// considered to be "top-level".
+  enum class WithinStmtExpr { NotInStmtExpr, InStmtExpr };
+
+  /// Act on an expression statement that might be the last statement in a
+  /// GNU statement expression. Checks whether we are actually at the end of
+  /// a statement expression and builds a suitable expression statement.
+  StmtResult handleExprStmt(ExprResult E, WithinStmtExpr IsInStmtExpr);
 
 public:
   Parser(Preprocessor &PP, Sema &Actions, bool SkipFunctionBodies);
   ~Parser() override;
 
   const LangOptions &getLangOpts() const { return PP.getLangOpts(); }
   const TargetInfo &getTargetInfo() const { return PP.getTargetInfo(); }
   Preprocessor &getPreprocessor() const { return PP; }
   Sema &getActions() const { return Actions; }
   AttributeFactory &getAttrFactory() { return AttrFactory; }
 
   const Token &getCurToken() const { return Tok; }
   Scope *getCurScope() const { return Actions.getCurScope(); }
   void incrementMSManglingNumber() const {
     return Actions.incrementMSManglingNumber();
   }
 
   Decl  *getObjCDeclContext() const { return Actions.getObjCDeclContext(); }
 
   // Type forwarding.  All of these are statically 'void*', but they may all be
   // different actual classes based on the actions in place.
   typedef OpaquePtr<DeclGroupRef> DeclGroupPtrTy;
   typedef OpaquePtr<TemplateName> TemplateTy;
 
   typedef SmallVector<TemplateParameterList *, 4> TemplateParameterLists;
 
   typedef Sema::FullExprArg FullExprArg;
 
   // Parsing methods.
 
   /// Initialize - Warm up the parser.
   ///
private:
   /// A SmallVector of statements, with stack size 32 (as that is the only one
   /// used.)
   typedef SmallVector<Stmt*, 32> StmtVector;
   /// A SmallVector of expressions, with stack size 12 (the maximum used.)
   typedef SmallVector<Expr*, 12> ExprVector;
   /// A SmallVector of types.
   typedef SmallVector<ParsedType, 12> TypeVector;
 
-  StmtResult ParseStatement(SourceLocation *TrailingElseLoc = nullptr,
-                            bool AllowOpenMPStandalone = false);
+  StmtResult
+  ParseStatement(SourceLocation *TrailingElseLoc = nullptr,
+                 bool AllowOpenMPStandalone = false,
+                 WithinStmtExpr IsInStmtExpr = WithinStmtExpr::NotInStmtExpr);
   enum AllowedConstructsKind {
     /// Allow any declarations, statements, OpenMP directives.
     ACK_Any,
     /// Allow only statements and non-standalone OpenMP directives.
     ACK_StatementsOpenMPNonStandalone,
     /// Allow statements and all executable OpenMP directives
     ACK_StatementsOpenMPAnyExecutable
   };
-  StmtResult
-  ParseStatementOrDeclaration(StmtVector &Stmts, AllowedConstructsKind Allowed,
-                              SourceLocation *TrailingElseLoc = nullptr);
+  StmtResult ParseStatementOrDeclaration(
+      StmtVector &Stmts, AllowedConstructsKind Allowed,
+      SourceLocation *TrailingElseLoc = nullptr,
+      WithinStmtExpr IsInStmtExpr = WithinStmtExpr::NotInStmtExpr);
   StmtResult ParseStatementOrDeclarationAfterAttributes(
                                          StmtVector &Stmts,
                                          AllowedConstructsKind Allowed,
                                          SourceLocation *TrailingElseLoc,
+                                         WithinStmtExpr IsInStmtExpr,
                                          ParsedAttributesWithRange &Attrs);
-  StmtResult ParseExprStatement();
-  StmtResult ParseLabeledStatement(ParsedAttributesWithRange &attrs);
+  StmtResult ParseExprStatement(WithinStmtExpr IsInStmtExpr);
+  StmtResult ParseLabeledStatement(ParsedAttributesWithRange &attrs,
+                                   WithinStmtExpr IsInStmtExpr);
   StmtResult ParseCaseStatement(bool MissingCase = false,
                                 ExprResult Expr = ExprResult());
   StmtResult ParseDefaultStatement();
   StmtResult ParseCompoundStatement(bool isStmtExpr = false);
   StmtResult ParseCompoundStatement(bool isStmtExpr,
                                     unsigned ScopeFlags);
   void ParseCompoundStatementLeadingPragmas();
   bool ConsumeNullStmt(StmtVector &Stmts);
private:
   StmtResult ParseContinueStatement();
   StmtResult ParseBreakStatement();
   StmtResult ParseReturnStatement();
   StmtResult ParseAsmStatement(bool &msAsm);
   StmtResult ParseMicrosoftAsmStatement(SourceLocation AsmLoc);
   StmtResult ParsePragmaLoopHint(StmtVector &Stmts,
                                  AllowedConstructsKind Allowed,
                                  SourceLocation *TrailingElseLoc,
+                                 WithinStmtExpr IsInStmtExpr,
                                  ParsedAttributesWithRange &Attrs);
 
   /// Describes the behavior that should be taken for an __if_exists
   /// block.
   enum IfExistsBehavior {
     /// Parse the block; this code is always used.
     IEB_Parse,
     /// Skip the block entirely; this code is never used.
private:
   StmtResult ParseSEHTryBlock();
   StmtResult ParseSEHExceptBlock(SourceLocation Loc);
   StmtResult ParseSEHFinallyBlock(SourceLocation Loc);
   StmtResult ParseSEHLeaveStatement();
 
   //===--------------------------------------------------------------------===//
   // Objective-C Statements
 
-  StmtResult ParseObjCAtStatement(SourceLocation atLoc);
+  StmtResult ParseObjCAtStatement(SourceLocation atLoc,
+                                  WithinStmtExpr IsInStmtExpr);
   StmtResult ParseObjCTryStmt(SourceLocation atLoc);
   StmtResult ParseObjCThrowStmt(SourceLocation atLoc);
   StmtResult ParseObjCSynchronizedStmt(SourceLocation atLoc);
   StmtResult ParseObjCAutoreleasePoolStmt(SourceLocation atLoc);
 
 
   //===--------------------------------------------------------------------===//
   // C99 6.7: Declarations.

include/clang/Sema/Sema.h

public:
   void setFunctionHasBranchIntoScope();
   void setFunctionHasBranchProtectedScope();
   void setFunctionHasIndirectGoto();
 
   void PushCompoundScope(bool IsStmtExpr);
   void PopCompoundScope();
 
   sema::CompoundScopeInfo &getCurCompoundScope() const;
-  bool isCurCompoundStmtAStmtExpr() const;
 
   bool hasAnyUnrecoverableErrorsInThisFunction() const;
 
   /// Retrieve the current block, if any.
   sema::BlockScopeInfo *getCurBlock();
 
   /// Retrieve the current lambda scope info, if any.
   /// \param IgnoreNonLambdaCapturingScope true if should find the top-most
public:
 
   /// ActOnAddrLabel - Parse the GNU address of label extension: "&&foo".
   ExprResult ActOnAddrLabel(SourceLocation OpLoc, SourceLocation LabLoc,
                             LabelDecl *TheDecl);
 
   void ActOnStartStmtExpr();
   ExprResult ActOnStmtExpr(SourceLocation LPLoc, Stmt *SubStmt,
                            SourceLocation RPLoc); // "({..})"
+  // Handle the final expression in a statement expression.
+  ExprResult ActOnStmtExprResult(ExprResult E);
   void ActOnStmtExprError();
 
   // __builtin_offsetof(type, identifier(.identifier|[expr])*)
   struct OffsetOfComponent {
     SourceLocation LocStart, LocEnd;
     bool isBrackets;  // true if [expr], false if .ident
     union {
       IdentifierInfo *IdentInfo;

lib/AST/Stmt.cpp

CompoundStmt *CompoundStmt::CreateEmpty(const ASTContext &C,
                                         unsigned NumStmts) {
   void *Mem =
       C.Allocate(totalSizeToAlloc<Stmt *>(NumStmts), alignof(CompoundStmt));
   CompoundStmt *New = new (Mem) CompoundStmt(EmptyShell());
   New->CompoundStmtBits.NumStmts = NumStmts;
   return New;
 }
 
+const Expr *ValueStmt::getExprStmt() const {
+  const Stmt *S = this;
+  do {
+    if (const auto *E = dyn_cast<Expr>(S))
+      return E;
+
+    if (const auto *LS = dyn_cast<LabelStmt>(S))
+      S = LS->getSubStmt();
+    else if (const auto *AS = dyn_cast<AttributedStmt>(S))
+      S = AS->getSubStmt();
+    else
+      llvm_unreachable("unknown kind of ValueStmt");
+  } while (isa<ValueStmt>(S));
+
+  return nullptr;
+}
+
 const char *LabelStmt::getName() const {
   return getDecl()->getIdentifier()->getNameStart();
 }
 
 AttributedStmt *AttributedStmt::Create(const ASTContext &C, SourceLocation Loc,
                                        ArrayRef<const Attr*> Attrs,
                                        Stmt *SubStmt) {
   assert(!Attrs.empty() && "Attrs should not be empty");

lib/CodeGen/CGStmt.cpp

for (CompoundStmt::const_body_iterator I = S.body_begin(),
     EmitStmt(*I);
 
   Address RetAlloca = Address::invalid();
   if (GetLast) {
     // 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 (const LabelStmt *LS = dyn_cast<LabelStmt>(LastStmt)) {
+    const Expr *E = nullptr;
+    while (true) {
+      if (isa<Expr>(LastStmt)) {
+        E = cast<Expr>(LastStmt);
+        break;
+      } else if (const LabelStmt *LS = dyn_cast<LabelStmt>(LastStmt)) {
         EmitLabel(LS->getDecl());
         LastStmt = LS->getSubStmt();
+      } else if (const AttributedStmt *AS =
+                     dyn_cast<AttributedStmt>(LastStmt)) {
+        // FIXME: Update this if we ever have attributes that affect the
+        // semantics of an expression.
+        LastStmt = AS->getSubStmt();
+      } else {
+        llvm_unreachable("unknown value statement");
+      }
     }
 
     EnsureInsertPoint();
 
-    QualType ExprTy = cast<Expr>(LastStmt)->getType();
+    QualType ExprTy = E->getType();
     if (hasAggregateEvaluationKind(ExprTy)) {
-      EmitAggExpr(cast<Expr>(LastStmt), AggSlot);
+      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(cast<Expr>(LastStmt), RetAlloca, Qualifiers(),
-                       /*IsInit*/false);
+      EmitAnyExprToMem(E, RetAlloca, Qualifiers(),
+                       /*IsInit*/ false);
     }
 
   }
 
   return RetAlloca;
 }
 
 void CodeGenFunction::SimplifyForwardingBlocks(llvm::BasicBlock *BB) {

lib/Parse/ParseObjc.cpp

Decl *Parser::ParseObjCMethodDefinition() {
   Actions.AddAnyMethodToGlobalPool(MDecl);
   assert (CurParsedObjCImpl
           && "ParseObjCMethodDefinition - Method out of @implementation");
   // Consume the tokens and store them for later parsing.
   StashAwayMethodOrFunctionBodyTokens(MDecl);
   return MDecl;
 }
 
-StmtResult Parser::ParseObjCAtStatement(SourceLocation AtLoc) {
+StmtResult Parser::ParseObjCAtStatement(SourceLocation AtLoc,
+                                        WithinStmtExpr IsInStmtExpr) {
   if (Tok.is(tok::code_completion)) {
     Actions.CodeCompleteObjCAtStatement(getCurScope());
     cutOffParsing();
     return StmtError();
   }
 
   if (Tok.isObjCAtKeyword(tok::objc_try))
     return ParseObjCTryStmt(AtLoc);
if (Res.isInvalid()) {
     // doing this opens us up to the possibility of infinite loops if
     // ParseExpression does not consume any tokens.
     SkipUntil(tok::semi);
     return StmtError();
   }
 
   // Otherwise, eat the semicolon.
   ExpectAndConsumeSemi(diag::err_expected_semi_after_expr);
-  return Actions.ActOnExprStmt(Res, isExprValueDiscarded());
+  return handleExprStmt(Res, IsInStmtExpr);
 }
 
 ExprResult Parser::ParseObjCAtExpression(SourceLocation AtLoc) {
   switch (Tok.getKind()) {
   case tok::code_completion:
     Actions.CodeCompleteObjCAtExpression(getCurScope());
     cutOffParsing();
     return ExprError();

lib/Parse/ParseStmt.cpp

 
 //===----------------------------------------------------------------------===//
 // C99 6.8: Statements and Blocks.
 //===----------------------------------------------------------------------===//
 
 /// Parse a standalone statement (for instance, as the body of an 'if',
 /// 'while', or 'for').
 StmtResult Parser::ParseStatement(SourceLocation *TrailingElseLoc,
-                                  bool AllowOpenMPStandalone) {
+                                  bool AllowOpenMPStandalone,
+                                  WithinStmtExpr IsInStmtExpr) {
   StmtResult Res;
 
   // We may get back a null statement if we found a #pragma. Keep going until
   // we get an actual statement.
   do {
     StmtVector Stmts;
     Res = ParseStatementOrDeclaration(
         Stmts, AllowOpenMPStandalone ? ACK_StatementsOpenMPAnyExecutable
                                      : ACK_StatementsOpenMPNonStandalone,
-        TrailingElseLoc);
+        TrailingElseLoc, IsInStmtExpr);
   } while (!Res.isInvalid() && !Res.get());
 
   return Res;
 }
 
 /// ParseStatementOrDeclaration - Read 'statement' or 'declaration'.
 ///       StatementOrDeclaration:
 ///         statement

 ///
 /// [OBC] objc-throw-statement:
 /// [OBC]   '@' 'throw' expression ';'
 /// [OBC]   '@' 'throw' ';'
 ///
 StmtResult
 Parser::ParseStatementOrDeclaration(StmtVector &Stmts,
                                     AllowedConstructsKind Allowed,
-                                    SourceLocation *TrailingElseLoc) {
+                                    SourceLocation *TrailingElseLoc,
+                                    WithinStmtExpr IsInStmtExpr) {
 
   ParenBraceBracketBalancer BalancerRAIIObj(*this);
 
   ParsedAttributesWithRange Attrs(AttrFactory);
   MaybeParseCXX11Attributes(Attrs, nullptr, /*MightBeObjCMessageSend*/ true);
   if (!MaybeParseOpenCLUnrollHintAttribute(Attrs))
     return StmtError();
 
   StmtResult Res = ParseStatementOrDeclarationAfterAttributes(
-      Stmts, Allowed, TrailingElseLoc, Attrs);
+      Stmts, Allowed, TrailingElseLoc, IsInStmtExpr, Attrs);
 
   assert((Attrs.empty() || Res.isInvalid() || Res.isUsable()) &&
          "attributes on empty statement");
 
   if (Attrs.empty() || Res.isInvalid())
     return Res;
 
   return Actions.ProcessStmtAttributes(Res.get(), Attrs, Attrs.Range);
bool ValidateCandidate(const TypoCorrection &candidate) override {
     return CorrectionCandidateCallback::ValidateCandidate(candidate);
   }
 
 private:
   Token NextToken;
 };
 }
 
-StmtResult
-Parser::ParseStatementOrDeclarationAfterAttributes(StmtVector &Stmts,
-          AllowedConstructsKind Allowed, SourceLocation *TrailingElseLoc,
-          ParsedAttributesWithRange &Attrs) {
+StmtResult Parser::ParseStatementOrDeclarationAfterAttributes(
+    StmtVector &Stmts, AllowedConstructsKind Allowed,
+    SourceLocation *TrailingElseLoc, WithinStmtExpr IsInStmtExpr,
   const char *SemiError = nullptr;
   StmtResult Res;
 
   // Cases in this switch statement should fall through if the parser expects
   // the token to end in a semicolon (in which case SemiError should be set),
   // or they directly 'return;' if not.
 Retry:
   tok::TokenKind Kind  = Tok.getKind();
   SourceLocation AtLoc;
   switch (Kind) {
   case tok::at: // May be a @try or @throw statement
     {
       ProhibitAttributes(Attrs); // TODO: is it correct?
       AtLoc = ConsumeToken();  // consume @
-      return ParseObjCAtStatement(AtLoc);
+      return ParseObjCAtStatement(AtLoc, IsInStmtExpr);
     }
 
   case tok::code_completion:
     Actions.CodeCompleteOrdinaryName(getCurScope(), Sema::PCC_Statement);
     cutOffParsing();
     return StmtError();
 
   case tok::identifier: {
     Token Next = NextToken();
     if (Next.is(tok::colon)) { // C99 6.8.1: labeled-statement
       // identifier ':' statement
-      return ParseLabeledStatement(Attrs);
+      return ParseLabeledStatement(Attrs, IsInStmtExpr);
     }
 
     // Look up the identifier, and typo-correct it to a keyword if it's not
     // found.
     if (Next.isNot(tok::coloncolon)) {
       // Try to limit which sets of keywords should be included in typo
       // correction based on what the next token is.
       if (TryAnnotateName(/*IsAddressOfOperand*/ false,
if ((getLangOpts().CPlusPlus || getLangOpts().MicrosoftExt ||
       return Actions.ActOnDeclStmt(Decl, DeclStart, DeclEnd);
     }
 
     if (Tok.is(tok::r_brace)) {
       Diag(Tok, diag::err_expected_statement);
       return StmtError();
     }
 
-    return ParseExprStatement();
+    return ParseExprStatement(IsInStmtExpr);
   }
 
   case tok::kw_case:                // C99 6.8.1: labeled-statement
     return ParseCaseStatement();
   case tok::kw_default:             // C99 6.8.1: labeled-statement
     return ParseDefaultStatement();
 
   case tok::l_brace:                // C99 6.8.2: compound-statement
Retry:
 
   case tok::annot_pragma_ms_vtordisp:
     ProhibitAttributes(Attrs);
     HandlePragmaMSVtorDisp();
     return StmtEmpty();
 
   case tok::annot_pragma_loop_hint:
     ProhibitAttributes(Attrs);
-    return ParsePragmaLoopHint(Stmts, Allowed, TrailingElseLoc, Attrs);
+    return ParsePragmaLoopHint(Stmts, Allowed, TrailingElseLoc, IsInStmtExpr,
+                               Attrs);
 
   case tok::annot_pragma_dump:
     HandlePragmaDump();
     return StmtEmpty();
 
   case tok::annot_pragma_attribute:
     HandlePragmaAttribute();
     return StmtEmpty();
   }
 
   // If we reached this code, the statement must end in a semicolon.
   if (!TryConsumeToken(tok::semi) && !Res.isInvalid()) {
     // If the result was valid, then we do want to diagnose this.  Use
     // ExpectAndConsume to emit the diagnostic, even though we know it won't
     // succeed.
     ExpectAndConsume(tok::semi, diag::err_expected_semi_after_stmt, SemiError);
     // Skip until we see a } or ;, but don't eat it.
     SkipUntil(tok::r_brace, StopAtSemi | StopBeforeMatch);
   }
 
   return Res;
 }
 
 /// Parse an expression statement.
-StmtResult Parser::ParseExprStatement() {
+StmtResult Parser::ParseExprStatement(WithinStmtExpr IsInStmtExpr) {
   // If a case keyword is missing, this is where it should be inserted.
   Token OldToken = Tok;
 
   ExprStatementTokLoc = Tok.getLocation();
 
   // expression[opt] ';'
   ExprResult Expr(ParseExpression());
   if (Expr.isInvalid()) {
Diag(OldToken, diag::err_expected_case_before_expression)
       << FixItHint::CreateInsertion(OldToken.getLocation(), "case ");
 
     // Recover parsing as a case statement.
     return ParseCaseStatement(/*MissingCase=*/true, Expr);
   }
 
   // Otherwise, eat the semicolon.
   ExpectAndConsumeSemi(diag::err_expected_semi_after_expr);
-  return Actions.ActOnExprStmt(Expr, isExprValueDiscarded());
+  return handleExprStmt(Expr, IsInStmtExpr);
 }
 
 /// ParseSEHTryBlockCommon
 ///
 /// seh-try-block:
 ///   '__try' compound-statement seh-handler
 ///
 /// seh-handler:

 }
 
 /// ParseLabeledStatement - We have an identifier and a ':' after it.
 ///
 ///       labeled-statement:
 ///         identifier ':' statement
 /// [GNU]   identifier ':' attributes[opt] statement
 ///
-StmtResult Parser::ParseLabeledStatement(ParsedAttributesWithRange &attrs) {
+StmtResult Parser::ParseLabeledStatement(ParsedAttributesWithRange &attrs,
+                                         WithinStmtExpr IsInStmtExpr) {
   assert(Tok.is(tok::identifier) && Tok.getIdentifierInfo() &&
          "Not an identifier!");
 
   Token IdentTok = Tok;  // Save the whole token.
   ConsumeToken();  // eat the identifier.
 
   assert(Tok.is(tok::colon) && "Not a label!");
 
if (Tok.is(tok::kw___attribute)) {
     else if (isDeclarationStatement()) {
       StmtVector Stmts;
       // FIXME: We should do this whether or not we have a declaration
       // statement, but that doesn't work correctly (because ProhibitAttributes
       // can't handle GNU attributes), so only call it in the one case where
       // GNU attributes are allowed.
       SubStmt = ParseStatementOrDeclarationAfterAttributes(
           Stmts, /*Allowed=*/ACK_StatementsOpenMPNonStandalone, nullptr,
-          TempAttrs);
+          IsInStmtExpr, TempAttrs);
       if (!TempAttrs.empty() && !SubStmt.isInvalid())
         SubStmt = Actions.ProcessStmtAttributes(SubStmt.get(), TempAttrs,
                                                 TempAttrs.Range);
     } else {
       Diag(Tok, diag::err_expected_after) << "__attribute__" << tok::semi;
     }
   }
 
   // If we've not parsed a statement yet, parse one now.
   if (!SubStmt.isInvalid() && !SubStmt.isUsable())
-    SubStmt = ParseStatement();
+    SubStmt = ParseStatement(nullptr, false, IsInStmtExpr);
 
   // Broken substmt shouldn't prevent the label from being added to the AST.
   if (SubStmt.isInvalid())
     SubStmt = Actions.ActOnNullStmt(ColonLoc);
 
   LabelDecl *LD = Actions.LookupOrCreateLabel(IdentTok.getIdentifierInfo(),
                                               IdentTok.getLocation());
   Actions.ProcessDeclAttributeList(Actions.CurScope, LD, attrs);
bool Parser::ConsumeNullStmt(StmtVector &Stmts) {
   if (EndLoc.isInvalid())
     return false;
 
   Diag(StartLoc, diag::warn_null_statement)
       << FixItHint::CreateRemoval(SourceRange(StartLoc, EndLoc));
   return true;
 }
 
-bool Parser::isExprValueDiscarded() {
-  if (Actions.isCurCompoundStmtAStmtExpr()) {
-    // Look to see if the next two tokens close the statement expression;
+StmtResult Parser::handleExprStmt(ExprResult E, WithinStmtExpr IsInStmtExpr) {
+  bool IsStmtExprResult = false;
+  if (IsInStmtExpr == WithinStmtExpr::InStmtExpr) {
+    // 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.
-    return Tok.isNot(tok::r_brace) || NextToken().isNot(tok::r_paren);
+    IsStmtExprResult = Tok.is(tok::r_brace) && NextToken().is(tok::r_paren);
   }
-  return true;
+
+  if (IsStmtExprResult)
+    E = Actions.ActOnStmtExprResult(E);
+  return Actions.ActOnExprStmt(E, /*DiscardedValue=*/!IsStmtExprResult);
 }
 
 /// ParseCompoundStatementBody - Parse a sequence of statements and invoke the
 /// ActOnCompoundStmt action.  This expects the '{' to be the current token, and
 /// consume the '}' at the end of the block.  It does not manipulate the scope
 /// stack.
 StmtResult Parser::ParseCompoundStatementBody(bool isStmtExpr) {
   PrettyStackTraceLoc CrashInfo(PP.getSourceManager(),
DeclGroupPtrTy Res =
         Actions.FinalizeDeclaratorGroup(getCurScope(), DS, DeclsInGroup);
     StmtResult R = Actions.ActOnDeclStmt(Res, LabelLoc, Tok.getLocation());
 
     ExpectAndConsumeSemi(diag::err_expected_semi_declaration);
     if (R.isUsable())
       Stmts.push_back(R.get());
   }
 
+  WithinStmtExpr IsInStmtExpr =
+      isStmtExpr ? WithinStmtExpr::InStmtExpr : WithinStmtExpr::NotInStmtExpr;
+
   while (!tryParseMisplacedModuleImport() && Tok.isNot(tok::r_brace) &&
          Tok.isNot(tok::eof)) {
     if (Tok.is(tok::annot_pragma_unused)) {
       HandlePragmaUnused();
       continue;
     }
 
     if (ConsumeNullStmt(Stmts))
       continue;
 
     StmtResult R;
     if (Tok.isNot(tok::kw___extension__)) {
-      R = ParseStatementOrDeclaration(Stmts, ACK_Any);
+      R = ParseStatementOrDeclaration(Stmts, ACK_Any, nullptr, IsInStmtExpr);
     } else {
       // __extension__ can start declarations and it can also be a unary
       // operator for expressions.  Consume multiple __extension__ markers here
       // until we can determine which is which.
       // FIXME: This loses extension expressions in the AST!
       SourceLocation ExtLoc = ConsumeToken();
       while (Tok.is(tok::kw___extension__))
         ConsumeToken();
if (Tok.isNot(tok::kw___extension__)) {
         // Otherwise this was a unary __extension__ marker.
         ExprResult Res(ParseExpressionWithLeadingExtension(ExtLoc));
 
         if (Res.isInvalid()) {
           SkipUntil(tok::semi);
           continue;
         }
 
-        // FIXME: Use attributes?
         // Eat the semicolon at the end of stmt and convert the expr into a
         // statement.
         ExpectAndConsumeSemi(diag::err_expected_semi_after_expr);
-        R = Actions.ActOnExprStmt(Res, isExprValueDiscarded());
+        R = handleExprStmt(Res, IsInStmtExpr);
+        if (R.isUsable())
+          R = Actions.ProcessStmtAttributes(R.get(), attrs, attrs.Range);
       }
     }
 
     if (R.isUsable())
       Stmts.push_back(R.get());
   }
 
   SourceLocation CloseLoc = Tok.getLocation();
StmtResult Parser::ParseReturnStatement() {
   if (IsCoreturn)
     return Actions.ActOnCoreturnStmt(getCurScope(), ReturnLoc, R.get());
   return Actions.ActOnReturnStmt(ReturnLoc, R.get(), getCurScope());
 }
 
 StmtResult Parser::ParsePragmaLoopHint(StmtVector &Stmts,
                                        AllowedConstructsKind Allowed,
                                        SourceLocation *TrailingElseLoc,
+                                       WithinStmtExpr IsInStmtExpr,
                                        ParsedAttributesWithRange &Attrs) {
   // Create temporary attribute list.
   ParsedAttributesWithRange TempAttrs(AttrFactory);
 
   // Get loop hints and consume annotated token.
   while (Tok.is(tok::annot_pragma_loop_hint)) {
     LoopHint Hint;
     if (!HandlePragmaLoopHint(Hint))
       continue;
 
     ArgsUnion ArgHints[] = {Hint.PragmaNameLoc, Hint.OptionLoc, Hint.StateLoc,
                             ArgsUnion(Hint.ValueExpr)};
     TempAttrs.addNew(Hint.PragmaNameLoc->Ident, Hint.Range, nullptr,
                      Hint.PragmaNameLoc->Loc, ArgHints, 4,
                      ParsedAttr::AS_Pragma);
   }
 
   // Get the next statement.
   MaybeParseCXX11Attributes(Attrs);
 
   StmtResult S = ParseStatementOrDeclarationAfterAttributes(
-      Stmts, Allowed, TrailingElseLoc, Attrs);
+      Stmts, Allowed, TrailingElseLoc, IsInStmtExpr, Attrs);
 
   Attrs.takeAllFrom(TempAttrs);
   return S;
 }
 
 Decl *Parser::ParseFunctionStatementBody(Decl *Decl, ParseScope &BodyScope) {
   assert(Tok.is(tok::l_brace));
   SourceLocation LBraceLoc = Tok.getLocation();

lib/Sema/SemaExpr.cpp

 ExprResult Sema::ActOnAddrLabel(SourceLocation OpLoc, SourceLocation LabLoc,
                                 LabelDecl *TheDecl) {
   TheDecl->markUsed(Context);
   // Create the AST node.  The address of a label always has type 'void*'.
   return new (Context) AddrLabelExpr(OpLoc, LabLoc, TheDecl,
                                      Context.getPointerType(Context.VoidTy));
 }
 
-/// Given the last statement in a statement-expression, check whether
-/// the result is a producing expression (like a call to an
-/// ns_returns_retained function) and, if so, rebuild it to hoist the
-/// release out of the full-expression.  Otherwise, return null.
-/// Cannot fail.
-static Expr *maybeRebuildARCConsumingStmt(Stmt *Statement) {
-  // Should always be wrapped with one of these.
-  ExprWithCleanups *cleanups = dyn_cast<ExprWithCleanups>(Statement);
-  if (!cleanups) return nullptr;
-
-  ImplicitCastExpr *cast = dyn_cast<ImplicitCastExpr>(cleanups->getSubExpr());
-  if (!cast || cast->getCastKind() != CK_ARCConsumeObject)
-    return nullptr;
-
-  // Splice out the cast.  This shouldn't modify any interesting
-  // features of the statement.
-  Expr *producer = cast->getSubExpr();
-  assert(producer->getType() == cast->getType());
-  assert(producer->getValueKind() == cast->getValueKind());
-  cleanups->setSubExpr(producer);
-  return cleanups;
-}
-
 void Sema::ActOnStartStmtExpr() {
   PushExpressionEvaluationContext(ExprEvalContexts.back().Context);
 }
 
 void Sema::ActOnStmtExprError() {
   // Note that function is also called by TreeTransform when leaving a
   // StmtExpr scope without rebuilding anything.
 
Sema::ActOnStmtExpr(SourceLocation LPLoc, Stmt *SubStmt,
   // 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()) {
-    Stmt *LastStmt = Compound->body_back();
-    LabelStmt *LastLabelStmt = nullptr;
-    // If LastStmt is a label, skip down through into the body.
-    while (LabelStmt *Label = dyn_cast<LabelStmt>(LastStmt)) {
-      LastLabelStmt = Label;
-      LastStmt = Label->getSubStmt();
-    }
+    if (const auto *LastStmt = dyn_cast<ValueStmt>(Compound->body_back())) {
+      if (const Expr *Value = LastStmt->getExprStmt()) {
+        StmtExprMayBindToTemp = true;
+        Ty = Value->getType();
+      }
+    }
+  }
+
+  // FIXME: Check that expression type is complete/non-abstract; statement
+  // expressions are not lvalues.
+  Expr *ResStmtExpr = new (Context) StmtExpr(Compound, Ty, LPLoc, RPLoc);
+  if (StmtExprMayBindToTemp)
+    return MaybeBindToTemporary(ResStmtExpr);
+  return ResStmtExpr;
 
-    if (Expr *LastE = dyn_cast<Expr>(LastStmt)) {
-      // Do function/array conversion on the last expression, but not
-      // lvalue-to-rvalue.  However, initialize an unqualified type.
-      ExprResult LastExpr = DefaultFunctionArrayConversion(LastE);
-      if (LastExpr.isInvalid())
-        return ExprError();
-      Ty = LastExpr.get()->getType().getUnqualifiedType();
+ExprResult Sema::ActOnStmtExprResult(ExprResult ER) {
+  if (ER.isInvalid())
+    return ExprError();
+
+  ER = DefaultFunctionArrayConversion(ER.get());
+  if (ER.isInvalid())
+  Expr *E = ER.get();
+
+  if (E->isTypeDependent())
+    return E;
 
-      if (!Ty->isDependentType() && !LastExpr.get()->isTypeDependent()) {
-        // In ARC, if the final expression ends in a consume, splice
-        // the consume out and bind it later.  In the alternate case
-        // (when dealing with a retainable type), the result
-        // initialization will create a produce.  In both cases the
-        // result will be +1, and we'll need to balance that out with
-        // a bind.
-        if (Expr *rebuiltLastStmt
-              = maybeRebuildARCConsumingStmt(LastExpr.get())) {
-          LastExpr = rebuiltLastStmt;
-        } else {
-          LastExpr = PerformCopyInitialization(
-              InitializedEntity::InitializeStmtExprResult(LPLoc, Ty),
-              SourceLocation(), LastExpr);
-        }
-
-        if (LastExpr.isInvalid())
-          return ExprError();
-        if (LastExpr.get() != nullptr) {
-          if (!LastLabelStmt)
-            Compound->setLastStmt(LastExpr.get());
-          else
-            LastLabelStmt->setSubStmt(LastExpr.get());
-          StmtExprMayBindToTemp = true;
-        }
-      }
-    }
-  }
-
-  // FIXME: Check that expression type is complete/non-abstract; statement
-  // expressions are not lvalues.
-  Expr *ResStmtExpr = new (Context) StmtExpr(Compound, Ty, LPLoc, RPLoc);
-  if (StmtExprMayBindToTemp)
-    return MaybeBindToTemporary(ResStmtExpr);
-  return ResStmtExpr;
+  auto *Cast = dyn_cast<ImplicitCastExpr>(E);
+  if (Cast && Cast->getCastKind() == CK_ARCConsumeObject)
+    return Cast->getSubExpr();
+
+  // FIXME: Provide a better location for the initialization.
+  return PerformCopyInitialization(
+      InitializedEntity::InitializeStmtExprResult(
+          E->getBeginLoc(), E->getType().getUnqualifiedType()),
+      SourceLocation(), E);
 }
 
 ExprResult Sema::BuildBuiltinOffsetOf(SourceLocation BuiltinLoc,
                                       TypeSourceInfo *TInfo,
                                       ArrayRef<OffsetOfComponent> Components,
                                       SourceLocation RParenLoc) {
   QualType ArgTy = TInfo->getType();
   bool Dependent = ArgTy->isDependentType();
class TransformToPE : public TreeTransform<TransformToPE> {
     typedef TreeTransform<TransformToPE> BaseTransform;
 
   public:
     TransformToPE(Sema &SemaRef) : BaseTransform(SemaRef) { }
 
     // Make sure we redo semantic analysis
     bool AlwaysRebuild() { return true; }
 
-    // Make sure we handle LabelStmts correctly.
-    // FIXME: This does the right thing, but maybe we need a more general
-    // fix to TreeTransform?
-    StmtResult TransformLabelStmt(LabelStmt *S) {
-      S->getDecl()->setStmt(nullptr);
-      return BaseTransform::TransformLabelStmt(S);
-    }
-
     // We need to special-case DeclRefExprs referring to FieldDecls which
     // are not part of a member pointer formation; normal TreeTransforming
     // doesn't catch this case because of the way we represent them in the AST.
     // FIXME: This is a bit ugly; is it really the best way to handle this
     // case?
     //
     // Error on DeclRefExprs referring to FieldDecls.
     ExprResult TransformDeclRefExpr(DeclRefExpr *E) {

lib/Sema/SemaStmt.cpp

 void Sema::ActOnFinishOfCompoundStmt() {
   PopCompoundScope();
 }
 
 sema::CompoundScopeInfo &Sema::getCurCompoundScope() const {
   return getCurFunction()->CompoundScopes.back();
 }
 
-bool Sema::isCurCompoundStmtAStmtExpr() const {
-  return getCurCompoundScope().IsStmtExpr;
-}
-
 StmtResult Sema::ActOnCompoundStmt(SourceLocation L, SourceLocation R,
                                    ArrayRef<Stmt *> Elts, bool isStmtExpr) {
   const unsigned NumElts = Elts.size();
 
   // If we're in C89 mode, check that we don't have any decls after stmts.  If
   // so, emit an extension diagnostic.
   if (!getLangOpts().C99 && !getLangOpts().CPlusPlus) {
     // Note that __extension__ can be around a decl.

lib/Sema/TreeTransform.h

public:
   /// This is used in the (relatively rare) contexts where it is acceptable
   /// for transformation to produce a class template type with deduced
   /// template arguments.
   /// @{
   QualType TransformTypeWithDeducedTST(QualType T);
   TypeSourceInfo *TransformTypeWithDeducedTST(TypeSourceInfo *DI);
   /// @}
 
+  /// The reason why the value of a statement is not discarded, if any.
+  enum StmtDiscardKind {
+    SDK_Discarded,
+    SDK_NotDiscarded,
+    SDK_StmtExprResult,
+  };
+
   /// Transform the given statement.
   ///
   /// By default, this routine transforms a statement by delegating to the
   /// appropriate TransformXXXStmt function to transform a specific kind of
   /// statement or the TransformExpr() function to transform an expression.
   /// Subclasses may override this function to transform statements using some
   /// other mechanism.
   ///
   /// \returns the transformed statement.
-  StmtResult TransformStmt(Stmt *S, bool DiscardedValue = false);
+  StmtResult TransformStmt(Stmt *S, StmtDiscardKind SDK = SDK_Discarded);
 
   /// Transform the given statement.
   ///
   /// By default, this routine transforms a statement by delegating to the
   /// appropriate TransformOMPXXXClause function to transform a specific kind
   /// of clause. Subclasses may override this function to transform statements
   /// using some other mechanism.
   ///
#include "clang/AST/TypeLocNodes.def"
 
   StmtResult TransformOMPExecutableDirective(OMPExecutableDirective *S);
 
 // FIXME: We use LLVM_ATTRIBUTE_NOINLINE because inlining causes a ridiculous
 // amount of stack usage with clang.
 #define STMT(Node, Parent)                        \
   LLVM_ATTRIBUTE_NOINLINE \
   StmtResult Transform##Node(Node *S);
+#define VALUESTMT(Node, Parent)                   \
+  LLVM_ATTRIBUTE_NOINLINE \
+  StmtResult Transform##Node(Node *S, StmtDiscardKind SDK);
 #define EXPR(Node, Parent)                        \
   LLVM_ATTRIBUTE_NOINLINE \
   ExprResult Transform##Node(Node *E);
 #define ABSTRACT_STMT(Stmt)
 #include "clang/AST/StmtNodes.inc"
 
 #define OPENMP_CLAUSE(Name, Class)                        \
   LLVM_ATTRIBUTE_NOINLINE \
TypeSourceInfo *TransformTSIInObjectScope(TypeLoc TL, QualType ObjectType,
                                             CXXScopeSpec &SS);
 
   QualType TransformDependentNameType(TypeLocBuilder &TLB,
                                       DependentNameTypeLoc TL,
                                       bool DeducibleTSTContext);
 };
 
 template <typename Derived>
-StmtResult TreeTransform<Derived>::TransformStmt(Stmt *S, bool DiscardedValue) {
+StmtResult TreeTransform<Derived>::TransformStmt(Stmt *S, StmtDiscardKind SDK) {
   if (!S)
     return S;
 
   switch (S->getStmtClass()) {
   case Stmt::NoStmtClass: break;
 
   // Transform individual statement nodes
+  // Pass SDK into statements that can produce a value
 #define STMT(Node, Parent)                                              \
   case Stmt::Node##Class: return getDerived().Transform##Node(cast<Node>(S));
+#define VALUESTMT(Node, Parent)                                         \
+  case Stmt::Node##Class:                                               \
+    return getDerived().Transform##Node(cast<Node>(S), SDK);
 #define ABSTRACT_STMT(Node)
 #define EXPR(Node, Parent)
 #include "clang/AST/StmtNodes.inc"
 
   // Transform expressions by calling TransformExpr.
 #define STMT(Node, Parent)
 #define ABSTRACT_STMT(Stmt)
 #define EXPR(Node, Parent) case Stmt::Node##Class:
 #include "clang/AST/StmtNodes.inc"
     {
       ExprResult E = getDerived().TransformExpr(cast<Expr>(S));
-      if (E.isInvalid())
-        return StmtError();
 
-      return getSema().ActOnExprStmt(E, DiscardedValue);
+      if (SDK == SDK_StmtExprResult)
+        E = getSema().ActOnStmtExprResult(E);
+      return getSema().ActOnExprStmt(E, SDK == SDK_Discarded);
     }
   }
 
   return S;
 }
 
 template<typename Derived>
 OMPClause *TreeTransform<Derived>::TransformOMPClause(OMPClause *S) {

 TreeTransform<Derived>::TransformCompoundStmt(CompoundStmt *S,
                                               bool IsStmtExpr) {
   Sema::CompoundScopeRAII CompoundScope(getSema());
 
   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());
+    StmtResult Result = getDerived().TransformStmt(
+        B,
+        IsStmtExpr && B == S->body_back() ? 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.
StmtResult Case = getDerived().RebuildCaseStmt(S->getCaseLoc(),
                                                        LHS.get(),
                                                        S->getEllipsisLoc(),
                                                        RHS.get(),
                                                        S->getColonLoc());
   if (Case.isInvalid())
     return StmtError();
 
   // Transform the statement following the case
-  StmtResult SubStmt = getDerived().TransformStmt(S->getSubStmt());
+  StmtResult SubStmt =
+      getDerived().TransformStmt(S->getSubStmt());
   if (SubStmt.isInvalid())
     return StmtError();
 
   // Attach the body to the case statement
   return getDerived().RebuildCaseStmtBody(Case.get(), SubStmt.get());
 }
 
-template<typename Derived>
-StmtResult
-TreeTransform<Derived>::TransformDefaultStmt(DefaultStmt *S) {
+template <typename Derived>
+StmtResult TreeTransform<Derived>::TransformDefaultStmt(DefaultStmt *S) {
   // Transform the statement following the default case
-  StmtResult SubStmt = getDerived().TransformStmt(S->getSubStmt());
+  StmtResult SubStmt =
+      getDerived().TransformStmt(S->getSubStmt());
   if (SubStmt.isInvalid())
     return StmtError();
 
   // Default statements are always rebuilt
   return getDerived().RebuildDefaultStmt(S->getDefaultLoc(), S->getColonLoc(),
                                          SubStmt.get());
 }
 
 template<typename Derived>
 StmtResult
-TreeTransform<Derived>::TransformLabelStmt(LabelStmt *S) {
-  StmtResult SubStmt = getDerived().TransformStmt(S->getSubStmt());
+TreeTransform<Derived>::TransformLabelStmt(LabelStmt *S, StmtDiscardKind SDK) {
+  StmtResult SubStmt = getDerived().TransformStmt(S->getSubStmt(), SDK);
   if (SubStmt.isInvalid())
     return StmtError();
 
   Decl *LD = getDerived().TransformDecl(S->getDecl()->getLocation(),
                                         S->getDecl());
   if (!LD)
     return StmtError();
 
+  // If we're transforming "in-place" (we're not creating new local
+  // declarations), assume we're replacing the old label statement
+  // and clear out the reference to it.
+  if (LD == S->getDecl())
+    S->getDecl()->setStmt(nullptr);
 
   // FIXME: Pass the real colon location in.
   return getDerived().RebuildLabelStmt(S->getIdentLoc(),
                                        cast<LabelDecl>(LD), SourceLocation(),
                                        SubStmt.get());
 }
 
 template <typename Derived>
case attr::X:                                                                \
     return getDerived().Transform##X##Attr(cast<X##Attr>(R));
 #include "clang/Basic/AttrList.inc"
   default:
     return R;
   }
 }
 
 template <typename Derived>
-StmtResult TreeTransform<Derived>::TransformAttributedStmt(AttributedStmt *S) {
+StmtResult
+TreeTransform<Derived>::TransformAttributedStmt(AttributedStmt *S,
+                                                StmtDiscardKind SDK) {
   bool AttrsChanged = false;
   SmallVector<const Attr *, 1> Attrs;
 
   // Visit attributes and keep track if any are transformed.
   for (const auto *I : S->getAttrs()) {
     const Attr *R = getDerived().TransformAttr(I);
     AttrsChanged |= (I != R);
     Attrs.push_back(R);
   }
 
-  StmtResult SubStmt = getDerived().TransformStmt(S->getSubStmt());
+  StmtResult SubStmt = getDerived().TransformStmt(S->getSubStmt(), SDK);
   if (SubStmt.isInvalid())
     return StmtError();
 
   if (SubStmt.get() == S->getSubStmt() && !AttrsChanged)
     return S;
 
   return getDerived().RebuildAttributedStmt(S->getAttrLoc(), Attrs,
                                             SubStmt.get());
return getDerived().RebuildObjCAutoreleasePoolStmt(
                         S->getAtLoc(), Body.get());
 }
 
 template<typename Derived>
 StmtResult
 TreeTransform<Derived>::TransformObjCForCollectionStmt(
                                                   ObjCForCollectionStmt *S) {
   // Transform the element statement.
-  StmtResult Element = getDerived().TransformStmt(S->getElement());
+  StmtResult Element =
+      getDerived().TransformStmt(S->getElement(), SDK_NotDiscarded);
   if (Element.isInvalid())
     return StmtError();
 
   // Transform the collection expression.
   ExprResult Collection = getDerived().TransformExpr(S->getCollection());
   if (Collection.isInvalid())
     return StmtError();
 

test/CodeGenCXX/stmtexpr.cpp

 //    Do cleanup.
 // CHECK: call {{.*}} @_ZN1AD1Ev
 // CHECK: switch
 //    Reload before v assignment.
 // CHECK: %[[v1:[^ ]*]] = load float, float* %[[tmp1]]
 // CHECK: %[[v2:[^ ]*]] = load float, float* %[[tmp2]]
 // CHECK: store float %[[v1]], float* %v.realp
 // CHECK: store float %[[v2]], float* %v.imagp
+
+extern "C" void then(int);
+
+// CHECK-LABEL: @{{.*}}volatile_load
+void volatile_load() {
+  volatile int n;
+
+  // CHECK-NOT: load volatile
+  // CHECK: load volatile
+  // CHECK-NOT: load volatile
+  ({n;});
+
+  // CHECK-LABEL: @then(i32 1)
+  then(1);
+
+  // CHECK-NOT: load volatile
+  // CHECK: load volatile
+  // CHECK-NOT: load volatile
+  ({goto lab; lab: n;});
+
+  // CHECK-LABEL: @then(i32 2)
+  then(2);
+
+  // CHECK-NOT: load volatile
+  // CHECK: load volatile
+  // CHECK-NOT: load volatile
+  ({[[gsl::suppress("foo")]] n;});
+
+  // CHECK-LABEL: @then(i32 3)
+  then(3);
+
+  // CHECK-NOT: load volatile
+  // CHECK: load volatile
+  // CHECK-NOT: load volatile
+  ({if (true) n;});
+
+  // CHECK: }
+}
+
+// CHECK-LABEL: @{{.*}}volatile_load_template
+template<typename T>
+void volatile_load_template() {
+  volatile T n;
+
+  // CHECK-NOT: load volatile
+  // CHECK: load volatile
+  // CHECK-NOT: load volatile
+  ({n;});
+
+  // CHECK-LABEL: @then(i32 1)
+  then(1);
+
+  // CHECK-NOT: load volatile
+  // CHECK: load volatile
+  // CHECK-NOT: load volatile
+  ({goto lab; lab: n;});
+
+  // CHECK-LABEL: @then(i32 2)
+  then(2);
+
+  // CHECK-NOT: load volatile
+  // CHECK: load volatile
+  // CHECK-NOT: load volatile
+  ({[[gsl::suppress("foo")]] n;});
+
+  // CHECK-LABEL: @then(i32 3)
+  then(3);
+
+  // CHECK-NOT: load volatile
+  // CHECK: load volatile
+  // CHECK-NOT: load volatile
+  ({if (true) n;});
+
+  // CHECK: }
+}
+template void volatile_load_template<int>();

test/CodeGenCXX/volatile.cpp

-// RUN: %clang_cc1 %s -triple=x86_64-apple-darwin10 -emit-llvm -std=c++98 -o - | FileCheck %s
+// RUN: %clang_cc1 %s -triple=x86_64-apple-darwin10 -emit-llvm -std=c++98 -o - | FileCheck -check-prefix=CHECK -check-prefix=CHECK98 %s
 // RUN: %clang_cc1 %s -triple=x86_64-apple-darwin10 -emit-llvm -std=c++11 -o - | FileCheck -check-prefix=CHECK -check-prefix=CHECK11 %s
 
 // Check that IR gen doesn't try to do an lvalue-to-rvalue conversion
 // on a volatile reference result.  rdar://problem/8338198
 namespace test0 {
   struct A {
     A(const A& t);
     A& operator=(const A& t);
namespace test1 {
   // CHECK-LABEL: define void @_ZN5test14testEv()
   void test() {
     // CHECK:      [[TMP:%.*]] = load i32*, i32** @_ZN5test11xE, align 8
     // CHECK11-NEXT: {{%.*}} = load volatile i32, i32* [[TMP]], align 4
     // CHECK-NEXT: ret void
     *x;
   }
 }
+
+namespace PR40642 {
+  template <class T> struct S {
+    // CHECK-LABEL: define {{.*}} @_ZN7PR406421SIiE3fooEv(
+    void foo() {
+      // CHECK98-NOT: load volatile
+      // CHECK11: load volatile
+      if (true)
+        reinterpret_cast<const volatile unsigned char *>(m_ptr)[0];
+      // CHECK: }
+    }
+    int *m_ptr;
+  };
+
+  void f(S<int> *x) { x->foo(); }
+}