[Sema] Fix an assert when a block captures a constexpr local

[erik.pilkington]

Summary

MarkVarDeclODRUsed indirectly calls captureInBlock, which creates a copy expression. The copy expression is insulated in it's own ExpressionEvaluationContext, so it saves, mutates, and restores MaybeODRUseExprs as CleanupVarDeclMarking is iterating through it, leading to a crash. Fix this by iterating through a local copy of MaybeODRUseExprs.

rdar://47493525

[rjmccall]

clang/lib/Sema/SemaExpr.cpp

15692

It looks like SmallPtrSet's move constructor does actually guarantee to leave the source empty; you could probably just assert that. But I don't think there's an algorithmic cost, so this is fine, too.

Please leave an assertion at the bottom of this function that the set is empty.

[erik.pilkington]

Add an assert.

clang/lib/Sema/SemaExpr.cpp

15692

If you have to actually check the constructor to verify the client code is doing the right thing then it makes more sense to just be explicit, IMO. New patch adds the assert though.

[rjmccall]

clang/lib/Sema/SemaExpr.cpp

15692

If it were perf-significant I might disagree, but it shouldn't be, so this is fine. LGTM.

[erik.pilkington]

Landed in r357040. (I forgot to write Differential revision:!)

clang/include/clang/Sema/Sema.h

   /// Used to control the generation of ExprWithCleanups.
   CleanupInfo Cleanup;
 
   /// ExprCleanupObjects - This is the stack of objects requiring
   /// cleanup that are created by the current full expression.  The
   /// element type here is ExprWithCleanups::Object.
   SmallVector<BlockDecl*, 8> ExprCleanupObjects;
 
-  /// Store a list of either DeclRefExprs or MemberExprs
-  ///  that contain a reference to a variable (constant) that may or may not
-  ///  be odr-used in this Expr, and we won't know until all lvalue-to-rvalue
-  ///  and discarded value conversions have been applied to all subexpressions
-  ///  of the enclosing full expression.  This is cleared at the end of each
-  ///  full expression.
-  llvm::SmallPtrSet<Expr*, 2> MaybeODRUseExprs;
+  /// Store a set of either DeclRefExprs or MemberExprs that contain a reference
+  /// to a variable (constant) that may or may not be odr-used in this Expr, and
+  /// we won't know until all lvalue-to-rvalue and discarded value conversions
+  /// have been applied to all subexpressions of the enclosing full expression.
+  /// This is cleared at the end of each full expression.
+  using MaybeODRUseExprSet = llvm::SmallPtrSet<Expr *, 2>;
+  MaybeODRUseExprSet MaybeODRUseExprs;
 
   std::unique_ptr<sema::FunctionScopeInfo> PreallocatedFunctionScope;
 
   /// Stack containing information about each of the nested
   /// function, block, and method scopes that are currently active.
   SmallVector<sema::FunctionScopeInfo *, 4> FunctionScopes;
 
   typedef LazyVector<TypedefNameDecl *, ExternalSemaSource,

     /// The number of active cleanup objects when we entered
     /// this expression evaluation context.
     unsigned NumCleanupObjects;
 
     /// The number of typos encountered during this expression evaluation
     /// context (i.e. the number of TypoExprs created).
     unsigned NumTypos;
 
-    llvm::SmallPtrSet<Expr*, 2> SavedMaybeODRUseExprs;
+    MaybeODRUseExprSet SavedMaybeODRUseExprs;
 
     /// The lambdas that are present within this context, if it
     /// is indeed an unevaluated context.
     SmallVector<LambdaExpr *, 2> Lambdas;
 
     /// The declaration that provides context for lambda expressions
     /// and block literals if the normal declaration context does not
     /// suffice, e.g., in a default function argument.

clang/lib/Sema/SemaExpr.cpp

   // If a constant-expression is a reference to a variable where we delay
   // deciding whether it is an odr-use, just assume we will apply the
   // lvalue-to-rvalue conversion.  In the one case where this doesn't happen
   // (a non-type template argument), we have special handling anyway.
   UpdateMarkingForLValueToRValue(Res.get());
   return Res;
 }
 
 void Sema::CleanupVarDeclMarking() {
-  for (Expr *E : MaybeODRUseExprs) {
+  // Iterate through a local copy in case MarkVarDeclODRUsed makes a recursive
+  // call.
+  MaybeODRUseExprSet LocalMaybeODRUseExprs;
+  std::swap(LocalMaybeODRUseExprs, MaybeODRUseExprs);
+
+  for (Expr *E : LocalMaybeODRUseExprs) {
     VarDecl *Var;
     SourceLocation Loc;
     if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(E)) {
       Var = cast<VarDecl>(DRE->getDecl());
       Loc = DRE->getLocation();
     } else if (MemberExpr *ME = dyn_cast<MemberExpr>(E)) {
       Var = cast<VarDecl>(ME->getMemberDecl());
       Loc = ME->getMemberLoc();
     } else {
       llvm_unreachable("Unexpected expression");
     }
 
     MarkVarDeclODRUsed(Var, Loc, *this,
                        /*MaxFunctionScopeIndex Pointer*/ nullptr);
   }
 
-  MaybeODRUseExprs.clear();
+  assert(MaybeODRUseExprs.empty() &&
+         "MarkVarDeclODRUsed failed to cleanup MaybeODRUseExprs?");
 }
 
-
 static void DoMarkVarDeclReferenced(Sema &SemaRef, SourceLocation Loc,
                                     VarDecl *Var, Expr *E) {
   assert((!E || isa<DeclRefExpr>(E) || isa<MemberExpr>(E)) &&
          "Invalid Expr argument to DoMarkVarDeclReferenced");
   Var->setReferenced();
 
   TemplateSpecializationKind TSK = Var->getTemplateSpecializationKind();
 

clang/test/SemaCXX/blocks.cpp

namespace test6c {
   void run(void (^)(), void (^)());
   void test() {
     int aCapturedVar = 0;
     run(^{ (void) aCapturedVar; },
         ^{ struct A { static void foo() {} };
             A::foo(); });
   }
 }
+
+namespace test7 {
+struct S {};
+void f() {
+  constexpr S s;
+  auto some_block = ^{ (void)s; };
+}
+}