[PATCH] Prevent gnu-expression-statements from clobbering potential lambda captures

[Faisal Vali]

Hi rsmith, doug.gregor,

As pointed out by Richard:
  void test() {
    const int x = 10;
    [=](auto a) {
      return [=](auto b) {
        return +x + ({0;b;}); <-- this would prevent x from being captured - though we need it to be per the standard
      };
    }; 
 }

This is fixed by saving adn restoring the state associated with potential-captures (similar to how MaybeODRUsedExprs are handled) when a new expression eval context is pushed on the stack.

Interestingly - the instantiation dependence of the full gnu-expression statement (or the individual expression-statements) does not percolate through to the containing full expression.  
The temporary fix i introduced only works if the final expression-statement is type dependent - but won't work for {(0; b; 0});
Any thoughts on how to fix this too?

http://llvm-reviews.chandlerc.com/D2170

Files:
  include/clang/Sema/ScopeInfo.h
  include/clang/Sema/Sema.h
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaExprCXX.cpp

Index: include/clang/Sema/ScopeInfo.h
===================================================================
--- include/clang/Sema/ScopeInfo.h
+++ include/clang/Sema/ScopeInfo.h
@@ -751,8 +751,19 @@
   }
   void clearPotentialCaptures() {
     PotentiallyCapturingExprs.clear();
+    NonODRUsedCapturingExprs.clear();
     PotentialThisCaptureLocation = SourceLocation();
   }
+  void swapPotentialCaptures(llvm::SmallVectorImpl<Expr *> &PotCapExprs,
+      llvm::SmallSet<Expr *, 8> &NonODRUsedCapExprs, 
+      SourceLocation &PotThisCapLoc) {
+    PotentiallyCapturingExprs.swap(PotCapExprs);
+    NonODRUsedCapturingExprs.swap(NonODRUsedCapExprs);
+    SourceLocation Tmp = PotThisCapLoc;
+    PotThisCapLoc = PotentialThisCaptureLocation;
+    PotentialThisCaptureLocation = Tmp;
+  }
+       
   unsigned getNumPotentialVariableCaptures() const { 
     return PotentiallyCapturingExprs.size(); 
   }
Index: include/clang/Sema/Sema.h
===================================================================
--- include/clang/Sema/Sema.h
+++ include/clang/Sema/Sema.h
@@ -662,7 +662,11 @@
     unsigned NumCleanupObjects;
 
     llvm::SmallPtrSet<Expr*, 2> SavedMaybeODRUseExprs;
+    llvm::SmallSet<Expr*, 8> SavedLambdaNonODRUsedCapturingExprs;
+    llvm::SmallVector<Expr*, 4> SavedLambdaPotentiallyCapturingExprs;
+    SourceLocation SavedLambdaPotentialThisCaptureLocation;
 
+
     /// \brief The lambdas that are present within this context, if it
     /// is indeed an unevaluated context.
     SmallVector<LambdaExpr *, 2> Lambdas;
Index: lib/Sema/SemaExpr.cpp
===================================================================
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -11015,9 +11015,15 @@
                                                ExprNeedsCleanups,
                                                LambdaContextDecl,
                                                IsDecltype));
+  ExpressionEvaluationContextRecord &ER = ExprEvalContexts.back();
   ExprNeedsCleanups = false;
   if (!MaybeODRUseExprs.empty())
-    std::swap(MaybeODRUseExprs, ExprEvalContexts.back().SavedMaybeODRUseExprs);
+    std::swap(MaybeODRUseExprs, ER.SavedMaybeODRUseExprs);
+  if (sema::LambdaScopeInfo *LSI = getCurLambda()) {
+    LSI->swapPotentialCaptures(ER.SavedLambdaPotentiallyCapturingExprs, 
+        ER.SavedLambdaNonODRUsedCapturingExprs, 
+        ER.SavedLambdaPotentialThisCaptureLocation);
+  }
 }
 
 void
@@ -11062,7 +11068,7 @@
       }
     }
   }
-
+  sema::LambdaScopeInfo *const LSI = getCurLambda();
   // When are coming out of an unevaluated context, clear out any
   // temporaries that we may have created as part of the evaluation of
   // the expression in that context: they aren't relevant because they
@@ -11073,11 +11079,27 @@
     ExprNeedsCleanups = Rec.ParentNeedsCleanups;
     CleanupVarDeclMarking();
     std::swap(MaybeODRUseExprs, Rec.SavedMaybeODRUseExprs);
+    if (LSI)
+      LSI->swapPotentialCaptures(Rec.SavedLambdaPotentiallyCapturingExprs, 
+          Rec.SavedLambdaNonODRUsedCapturingExprs, 
+          Rec.SavedLambdaPotentialThisCaptureLocation);
   // Otherwise, merge the contexts together.
   } else {
     ExprNeedsCleanups |= Rec.ParentNeedsCleanups;
     MaybeODRUseExprs.insert(Rec.SavedMaybeODRUseExprs.begin(),
                             Rec.SavedMaybeODRUseExprs.end());
+    if (LSI) {
+      LSI->PotentiallyCapturingExprs.append(
+          Rec.SavedLambdaPotentiallyCapturingExprs.begin(), 
+          Rec.SavedLambdaPotentiallyCapturingExprs.end());
+      LSI->NonODRUsedCapturingExprs.insert(
+          Rec.SavedLambdaNonODRUsedCapturingExprs.begin(),
+          Rec.SavedLambdaNonODRUsedCapturingExprs.end());
+      if (!LSI->PotentialThisCaptureLocation.isValid())  
+        LSI->PotentialThisCaptureLocation = 
+            Rec.SavedLambdaPotentialThisCaptureLocation;
+
+    }
   }
 
   // Pop the current expression evaluation context off the stack.
Index: lib/Sema/SemaExprCXX.cpp
===================================================================
--- lib/Sema/SemaExprCXX.cpp
+++ lib/Sema/SemaExprCXX.cpp
@@ -5863,8 +5863,19 @@
     
   assert(!S.isUnevaluatedContext());  
   assert(S.CurContext->isDependentContext()); 
+  // We need the type-dependence check too because the instantiation dependence
+  // doesn't percolate through gnu-statement expressions.
+  // For e.g.,:
+  //  const int x = 10;
+  //  [=](auto a) {
+  //    return [=](auto b) {
+  //      return +x + ({0;b;}); <-- this is not instantiation dependent?!
+  //    };
+  //  }; 
+  // FIXME: If an expression is type-dependent, it should end up being 
+  // instantiation dependent too.
   const bool IsFullExprInstantiationDependent = 
-      FE->isInstantiationDependent();
+      FE->isInstantiationDependent() || FE->isTypeDependent();
   // All the potentially captureable variables in the current nested 
   // lambda (within a generic outer lambda), must be captured by an
   // outer lambda that is enclosed within a non-dependent context.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D2170.1.patch
Type: text/x-patch
Size: 5014 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131113/4f3fc1b5/attachment.bin>