[Sema] Add variable captured by a block to the enclosing lambda's potential capture list

[ahatanak]

Summary

When compiling the following code, DoMarkVarDeclReferenced fails to capture variable "outerp":

auto lambda =[&](auto p) {
  return ^{
    return p + outerp;
  }();
};

This happens because Sema::getCurLambda() returns the lambda scope only when it's the last scope that has been pushed to Sema::FunctionScopes and therefore returns null if there is a block enclosed in the lambda. To fix this bug, this patch defines function Sema::getInnermostLambda() and uses it in DoMarkVarDeclReferenced to get the innermost lambda scope.

rdar://problem/28412462

[rjmccall]

Richard should probably weigh in about whether we should be recording potential captures at *all* capturing scopes. But at the very least, I think you have a bug here where the variable is declared outside of the block but within the lambda; in that case, it is definitely not a potential capture of the lambda.

[ahatanak]

In D25556#569809, @rjmccall wrote:

Richard should probably weigh in about whether we should be recording potential captures at *all* capturing scopes. But at the very least, I think you have a bug here where the variable is declared outside of the block but within the lambda; in that case, it is definitely not a potential capture of the lambda.

Ah, that's a good point.

When a variable is declared outside of the block but within the lambda, CheckIfAnyEnclosingLambdasMustCaptureAnyPotentialCaptures in SemaExprCXX.cpp will not capture the variable because getStackIndexOfNearestEnclosingCaptureCapableLambda doesn't return a proper index, so it looks like it's still safe to add the variable here even when it cannot be captured by the lambda. I think it's possible to check that the variable is not declared within the lambda before adding it to the list of potential captures in the code above, but I'm not sure whether we have to do that.

Also, could you elaborate on when you think we don't have to record a variable as a potential capture? My understanding is that you need to record the variable as a potential capture when the variable is part of a full expression that depends on a generic parameter because you don't know whether the variable is odr-used and needs to be captured by the lambda until you look at the full expression. Perhaps there are exceptions to this rule when the variable is used in a block?

[rjmccall]

In D25556#572054, @ahatanak wrote:

In D25556#569809, @rjmccall wrote:

Richard should probably weigh in about whether we should be recording potential captures at *all* capturing scopes. But at the very least, I think you have a bug here where the variable is declared outside of the block but within the lambda; in that case, it is definitely not a potential capture of the lambda.

Ah, that's a good point.

When a variable is declared outside of the block but within the lambda, CheckIfAnyEnclosingLambdasMustCaptureAnyPotentialCaptures in SemaExprCXX.cpp will not capture the variable because getStackIndexOfNearestEnclosingCaptureCapableLambda doesn't return a proper index, so it looks like it's still safe to add the variable here even when it cannot be captured by the lambda. I think it's possible to check that the variable is not declared within the lambda before adding it to the list of potential captures in the code above, but I'm not sure whether we have to do that.

Also, could you elaborate on when you think we don't have to record a variable as a potential capture? My understanding is that you need to record the variable as a potential capture when the variable is part of a full expression that depends on a generic parameter because you don't know whether the variable is odr-used and needs to be captured by the lambda until you look at the full expression. Perhaps there are exceptions to this rule when the variable is used in a block?

I mean that it might be important for us to be recording this at block scopes, not just lambda scopes. Maybe not; I guess we do eagerly instantiate blocks, which we can't necessarily do with lambdas. This is something that Richard needs to weigh in on.

[ahatanak]

Add the variable to the list of potential captures only if it's declared outside of the lambda expression. I don't think we need to check that the variable is declared outside of the lambda because getStackIndexOfNearestEnclosingCaptureCapableLambda will prevent it from being captured, but it's better not to add the variable to the list needlessly.

[ahatanak]

Rebase.

r286584 changed getCurLambda to optionally skip CapturedRegionScopeInfos. This patch changes it to skip BlockScopeInfos too.

[ahatanak]

ping

[ahatanak]

Add a few more people who have looked at this part of clang in the past to the reviewers list.

[malcolm.parsons]

I don't see anything wrong with this, but I don't think I'm qualified to approve it.

[ahatanak]

Rebase

[rjmccall]

Well, I was hoping that Richard would weigh in, but absent that, I don't have any objections. Since it fixes a known bug, let's just go ahead and do it. LGTM.

cfe/trunk/include/clang/Sema/Sema.h

public:
   sema::CompoundScopeInfo &getCurCompoundScope() const;
 
   bool hasAnyUnrecoverableErrorsInThisFunction() const;
 
   /// \brief Retrieve the current block, if any.
   sema::BlockScopeInfo *getCurBlock();
 
   /// Retrieve the current lambda scope info, if any.
-  /// \param IgnoreCapturedRegions true if should find the top-most lambda scope
-  /// info ignoring all inner captured regions scope infos.
-  sema::LambdaScopeInfo *getCurLambda(bool IgnoreCapturedRegions = false);
+  /// \param IgnoreNonLambdaCapturingScope true if should find the top-most
+  /// lambda scope info ignoring all inner capturing scopes that are not
+  /// lambda scopes.
+  sema::LambdaScopeInfo *
+  getCurLambda(bool IgnoreNonLambdaCapturingScope = false);
 
   /// \brief Retrieve the current generic lambda info, if any.
   sema::LambdaScopeInfo *getCurGenericLambda();
 
   /// \brief Retrieve the current captured region, if any.
   sema::CapturedRegionScopeInfo *getCurCapturedRegion();
 
   /// WeakTopLevelDeclDecls - access to \#pragma weak-generated Decls

cfe/trunk/lib/Sema/Sema.cpp

if (CurBSI && CurBSI->TheDecl &&
     // We have switched contexts due to template instantiation.
     assert(!CodeSynthesisContexts.empty());
     return nullptr;
   }
 
   return CurBSI;
 }
 
-LambdaScopeInfo *Sema::getCurLambda(bool IgnoreCapturedRegions) {
+LambdaScopeInfo *Sema::getCurLambda(bool IgnoreNonLambdaCapturingScope) {
   if (FunctionScopes.empty())
     return nullptr;
 
   auto I = FunctionScopes.rbegin();
-  if (IgnoreCapturedRegions) {
+  if (IgnoreNonLambdaCapturingScope) {
     auto E = FunctionScopes.rend();
-    while (I != E && isa<CapturedRegionScopeInfo>(*I))
+    while (I != E && isa<CapturingScopeInfo>(*I) && !isa<LambdaScopeInfo>(*I))
       ++I;
     if (I == E)
       return nullptr;
   }
   auto *CurLSI = dyn_cast<LambdaScopeInfo>(*I);
   if (CurLSI && CurLSI->Lambda &&
       !CurLSI->Lambda->Encloses(CurContext)) {
     // We have switched contexts due to template instantiation.

cfe/trunk/lib/Sema/SemaExpr.cpp

if (OdrUseContext && E &&
     // If this is a dependent context, we don't need to mark variables as
     // odr-used, but we may still need to track them for lambda capture.
     // FIXME: Do we also need to do this inside dependent typeid expressions
     // (which are modeled as unevaluated at this point)?
     const bool RefersToEnclosingScope =
         (SemaRef.CurContext != Var->getDeclContext() &&
          Var->getDeclContext()->isFunctionOrMethod() && Var->hasLocalStorage());
     if (RefersToEnclosingScope) {
-      if (LambdaScopeInfo *const LSI =
-              SemaRef.getCurLambda(/*IgnoreCapturedRegions=*/true)) {
+      LambdaScopeInfo *const LSI =
+          SemaRef.getCurLambda(/*IgnoreNonLambdaCapturingScope=*/true);
+      if (LSI && !LSI->CallOperator->Encloses(Var->getDeclContext())) {
         // If a variable could potentially be odr-used, defer marking it so
         // until we finish analyzing the full expression for any
         // lvalue-to-rvalue
         // or discarded value conversions that would obviate odr-use.
         // Add it to the list of potential captures that will be analyzed
         // later (ActOnFinishFullExpr) for eventual capture and odr-use marking
         // unless the variable is a reference that was initialized by a constant
         // expression (this will never need to be captured or odr-used).

cfe/trunk/test/SemaObjCXX/blocks.mm

-// RUN: %clang_cc1 -fsyntax-only -verify -fblocks -Wno-objc-root-class -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -fblocks -Wno-objc-root-class -std=c++14 %s
 @protocol NSObject;
 
 void bar(id(^)(void));
 void foo(id <NSObject>(^objectCreationBlock)(void)) {
     return bar(objectCreationBlock); // OK
 }
 
 void bar2(id(*)(void));
namespace DependentReturn {
   struct X { };
   void operator+(X, X);
   bool operator==(X, X);
   bool operator!=(X, X);
 
   template void f<X>(X);
 }
 
+namespace GenericLambdaCapture {
+int test(int outerp) {
+  auto lambda =[&](auto p) {
+    return ^{
+      return p + outerp;
+    }();
+  };
+  return lambda(1);
+}
+}
+
 namespace MoveBlockVariable {
 struct B0 {
 };
 
 struct B1 { // expected-note 2 {{candidate constructor (the implicit}}
   B1(B0&&); // expected-note {{candidate constructor not viable}}
 };
 
 B1 test_move() {
   __block B0 b;
   return b; // expected-error {{no viable conversion from returned value of type 'MoveBlockVariable::B0' to function return type 'MoveBlockVariable::B1'}}
 }
 }