Fix cv-qualification of '*this' captures (and nasty bug PR27507 introduced by commit 263921 "Implement Lambda Capture of *this by Value as [=,*this]")

[faisalv]

Summary

The bug report by Gonzalo (https://llvm.org/bugs/show_bug.cgi?id=27507 -- which results in clang crashing when generic lambdas that capture 'this' are instantiated in contexts where the functionscopeinfo stack is not in a reliable state - yet getCurrentThisType expects it to be) - unearthed some additional bugs in regards to maintaining proper cv qualification through 'this' when performing by value captures of '*this'.

This patch attempts to correct those bugs and makes the following changes:

• when capturing 'this', we do not need to remember the type of 'this' within the LambdaScopeInfo's Capture - it is never really used for a this capture - so remove it.
• teach getCurrentThisType to walk the stack of lambdas (even in scenarios where we run out of LambdaScopeInfo's such as when instantiating call operators) looking for by copy captures of '*this' and resetting the type of 'this' based on the constness of that capturing lambda's call operator.

As an example, consider the member function 'foo' below and follow along with the static_asserts:

void foo() const {

  
  auto L = [*this] () mutable { 
    static_assert(is_same<decltype(this), X*>);
    ++d;
    auto M = [this] { 
      static_assert(is_same<decltype(this), X*>);  
      ++d;
      auto N = [] {
        static_assert(is_same<decltype(this), X*>); 
      };
    };
  };
  
  auto L1 = [*this] { 
    static_assert(is_same<decltype(this), const X*>);
    auto M = [this] () mutable { 
      static_assert(is_same<decltype(this), const X*>);  
      auto N = [] {
        static_assert(is_same<decltype(this), const X*>); 
      };
    };
  };
  auto L2 = [this] () mutable {
    static_assert(is_same<decltype(this), const X*>);  
  };
  auto GL = [*this] (auto a) mutable {
    static_assert(is_same<decltype(this), X*>);
    ++d;
    auto M = [this] (auto b) { 
      static_assert(is_same<decltype(this), X*>);  
      ++d;
      auto N = [] (auto c) {
        static_assert(is_same<decltype(this), X*>); 
      };
      N(3.14);
    };
    M("abc");
  };
  GL(3.14);
}

Please see the test file for additional tests.

Thanks!

[faisalv]

Is it too soon for a *ping* ;)

p.s. just added some comments.

lib/Sema/SemaExprCXX.cpp

904

That dyn_cast should be an 'isa'

1056

Hmm - I wonder if instead of using AdjustedThisTy, I should always use 'ThisTy' since it is being used in the initializer expression, and should probably inherit its cv qualifiers from an enclosing lambda? correct?

[rsmith]

lib/Sema/SemaExprCXX.cpp

898

No camel case for ClosureClass.

910

Please add a comment explaining this, I have no idea what special case you're checking for here.

952

Should you really stop here if this is not captured? There could still be a surrounding lambda with a mutable *this capture.

1056

Yes, don't use AdjustedThisTy here. You can test for this bug by giving the class both a (const T&) and a (T&)=delete copy ctor.

[rsmith]

I'd also like to know whether there are cases that this patch addresses but
Taewook Oh's patch does not, as the other patch involves a lot less
complexity.

[twoh]

My patch passes check-clang and the test cases in this patch as well.

BTW, newly added test cases in the patch seem to be passed even without the patch. Isn't the bug appears when template instantiation generates nested lambdas (because template instantiation updates the 'context')?

[faisalv]

OK - thanks - will take a closer look at this hopefuly this evening
or tomorrow - and respond to Richard's other comments too.
Faisal Vali

[faisalv]

OK - agree (and addressed in a forthcoming patch) all your comments - except for the one I could use some clarity on - please see below

lib/Sema/SemaExprCXX.cpp

910

That special case should have been removed before submission - it crept in as I was making incremental changes to the initial patch, and getCurrentThisType was still being called from RebuildLambdaScopeInfo. (Alternatively, I might have left it in to give Richard a sense of purpose ;)

952

Do you have the following example in mind? Or is it something different?

void foo() const {

auto L1 = [*this] { 
    static_assert(is_same<decltype(this), const X*>);

    auto M2 = [*this] () mutable { 
      static_assert(is_same<decltype(this), X*>);  
      auto N = [] {
        static_assert(is_same<decltype(this), X*>); 
      };
    };
  };

}

[faisalv]

This patch addresses all of Richard's comments - except one (on which I'm awaiting some additional clarity on, before I make any changes).

[faisalv]

lib/Parse/ParseDeclCXX.cpp

3651

This whitespace change shouldn't have been included - as an aside - we still strive unix style line endings right?

[twoh]

@faisalv, thank you for the update. I wonder if getCurrentThisType() is the best place to call adjustCVQualifiersForCXXThisWithinLambda(). It seems that getCurrentThisType() does more than its name suggests. Is there any other place that the adjustment function can be called?

[faisalv]

I'm also a huge advocate of simple composable interfaces that do the
'one' task that they are expected (i.e. named) to do - and do it well.
I thought about this some, but perhaps not exhaustively - and settled
on being ok with getCurThisType, always returning the correct type for
'this' as typed/referred to in code (with the right 'cv' quals) -
regardless of where it is called from. Nevertheless, I'm open to
suggestions if you have a better alternative candidate for where the
call to adjustCVQuals should go?

Thanks for thinking about this.

Faisal Vali

[twoh]

Hmm, yes it seems not so simple to move the call to another place. If I come up with a better idea I'll submit it as a separate patch, as I don't want to block this one. Thank you for your work, @faisalv!

[faisalv]

*ping*

[faisalv]

Committed as r272480
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160606/161728.html

include/clang/Sema/ScopeInfo.h

     /// \brief Retrieve the capture type for this capture, which is effectively
     /// the type of the non-static data member in the lambda/block structure
     /// that would store this capture.
-    QualType getCaptureType() const { return CaptureType; }
-    
+    QualType getCaptureType() const {
+      assert(!isThisCapture());
+      return CaptureType;
+    }
+
     Expr *getInitExpr() const {
       assert(!isVLATypeCapture() && "no init expression for type capture");
       return static_cast<Expr *>(InitExprAndCaptureKind.getPointer());

                                /*Cpy*/ nullptr));
   }
 
-  void addThisCapture(bool isNested, SourceLocation Loc, QualType CaptureType,
+  // Note, we do not need to add the type of 'this' since that is always
+  // retrievable from Sema::getCurrentThisType - and is also encoded within the
+  // type of the corresponding FieldDecl.
+  void addThisCapture(bool isNested, SourceLocation Loc,
                       Expr *Cpy, bool ByCopy);
 
   /// \brief Determine whether the C++ 'this' is captured.

 
 inline void
 CapturingScopeInfo::addThisCapture(bool isNested, SourceLocation Loc,
-                                   QualType CaptureType, Expr *Cpy,
+                                   Expr *Cpy,
                                    const bool ByCopy) {
-  Captures.push_back(Capture(Capture::ThisCapture, isNested, Loc, CaptureType,
+  Captures.push_back(Capture(Capture::ThisCapture, isNested, Loc, QualType(),
                              Cpy, ByCopy));
   CXXThisCaptureIndex = Captures.size();
 }

lib/Parse/ParseDeclCXX.cpp

     return true;
   case AttributeList::AT_WarnUnusedResult:
     return !ScopeName && AttrName->getName().equals("nodiscard");
-  case AttributeList::AT_Unused:
-    return !ScopeName && AttrName->getName().equals("maybe_unused");
+  case AttributeList::AT_Unused:
+    return !ScopeName && AttrName->getName().equals("maybe_unused");
   default:
     return false;
   }

lib/Sema/SemaDecl.cpp

 
     } else if (C.capturesThis()) {
       LSI->addThisCapture(/*Nested*/ false, C.getLocation(),
-                              S.getCurrentThisType(), /*Expr*/ nullptr,
+                              /*Expr*/ nullptr,
                               C.getCaptureKind() == LCK_StarThis);
     } else {
       LSI->addVLATypeCapture(C.getLocation(), I->getType());

lib/Sema/SemaExprCXX.cpp

   return false;
 }
 
+static QualType adjustCVQualifiersForCXXThisWithinLambda(
+    ArrayRef<FunctionScopeInfo *> FunctionScopes, QualType ThisTy,
+    DeclContext *CurSemaContext, ASTContext &ASTCtx) {
+
+  QualType ClassType = ThisTy->getPointeeType();
+  LambdaScopeInfo *CurLSI = nullptr;
+  DeclContext *CurDC = CurSemaContext;
+
+  // Iterate through the stack of lambdas starting from the innermost lambda to
+  // the outermost lambda, checking if '*this' is ever captured by copy - since
+  // that could change the cv-qualifiers of the '*this' object.
+  // The object referred to by '*this' starts out with the cv-qualifiers of its
+  // member function.  We then start with the innermost lambda and iterate
+  // outward checking to see if any lambda performs a by-copy capture of '*this'
+  // - and if so, any nested lambda must respect the 'constness' of that
+  // capturing lamdbda's call operator.
+  //
+
+  // The issue is that we cannot rely entirely on the FunctionScopeInfo stack
+  // since ScopeInfos are pushed on during parsing and treetransforming. But
+  // since a generic lambda's call operator can be instantiated anywhere (even
+  // end of the TU) we need to be able to examine its enclosing lambdas and so
+  // we use the DeclContext to get a hold of the closure-class and query it for
+  // capture information.  The reason we don't just resort to always using the
+  // DeclContext chain is that it is only mature for lambda expressions
+  // enclosing generic lambda's call operators that are being instantiated.
+
+  for (int I = FunctionScopes.size();
+       I-- && isa<LambdaScopeInfo>(FunctionScopes[I]);
+       CurDC = getLambdaAwareParentOfDeclContext(CurDC)) {
+    CurLSI = cast<LambdaScopeInfo>(FunctionScopes[I]);
+    
+    if (!CurLSI->isCXXThisCaptured()) 
+        continue;
+      
+    auto C = CurLSI->getCXXThisCapture();
+
+    if (C.isCopyCapture()) {
+      ClassType.removeLocalCVRQualifiers(Qualifiers::CVRMask);
+      if (CurLSI->CallOperator->isConst())
+        ClassType.addConst();
+      return ASTCtx.getPointerType(ClassType);
+    }
+  }
+  // We've run out of ScopeInfos but check if CurDC is a lambda (which can
+  // happen during instantiation of generic lambdas)
+  if (isLambdaCallOperator(CurDC)) {
+    assert(CurLSI);
+    assert(isGenericLambdaCallOperatorSpecialization(CurLSI->CallOperator));
+    assert(CurDC == getLambdaAwareParentOfDeclContext(CurLSI->CallOperator));
+    
+    auto IsThisCaptured =
+        [](CXXRecordDecl *Closure, bool &IsByCopy, bool &IsConst) {
+      IsConst = false;
+      IsByCopy = false;
+      for (auto &&C : Closure->captures()) {
+        if (C.capturesThis()) {
+          if (C.getCaptureKind() == LCK_StarThis)
+            IsByCopy = true;
+          if (Closure->getLambdaCallOperator()->isConst())
+            IsConst = true;
+          return true;
+        }
+      }
+      return false;
+    };
+
+    bool IsByCopyCapture = false;
+    bool IsConstCapture = false;
+    CXXRecordDecl *Closure = cast<CXXRecordDecl>(CurDC->getParent());
+    while (Closure &&
+           IsThisCaptured(Closure, IsByCopyCapture, IsConstCapture)) {
+      if (IsByCopyCapture) {
+        ClassType.removeLocalCVRQualifiers(Qualifiers::CVRMask);
+        if (IsConstCapture)
+          ClassType.addConst();
+        return ASTCtx.getPointerType(ClassType);
+      }
+      Closure = isLambdaCallOperator(Closure->getParent())
+                    ? cast<CXXRecordDecl>(Closure->getParent()->getParent())
+                    : nullptr;
+    }
+  }
+  return ASTCtx.getPointerType(ClassType);
+}
+
 QualType Sema::getCurrentThisType() {
   DeclContext *DC = getFunctionLevelDeclContext();
   QualType ThisTy = CXXThisTypeOverride;

       ThisTy = Context.getPointerType(ClassTy);
     }
   }
-   // Add const for '* this' capture if not mutable.
-  if (isLambdaCallOperator(CurContext)) {
-    LambdaScopeInfo *LSI = getCurLambda();
-    assert(LSI);
-    if (LSI->isCXXThisCaptured()) {
-      auto C = LSI->getCXXThisCapture();
-      QualType BaseType = ThisTy->getPointeeType();
-      if ((C.isThisCapture() && C.isCopyCapture()) &&
-          LSI->CallOperator->isConst() && !BaseType.isConstQualified()) {
-        BaseType.addConst();
-        ThisTy = Context.getPointerType(BaseType);
-      }
-    }
-  }
+
+  // If we are within a lambda's call operator, the cv-qualifiers of 'this'
+  // might need to be adjusted if the lambda or any of its enclosing lambda's
+  // captures '*this' by copy.
+  if (!ThisTy.isNull() && isLambdaCallOperator(CurContext))
+    return adjustCVQualifiersForCXXThisWithinLambda(FunctionScopes, ThisTy,
+                                                    CurContext, Context);
   return ThisTy;
 }
 

 static Expr *captureThis(Sema &S, ASTContext &Context, RecordDecl *RD,
                          QualType ThisTy, SourceLocation Loc,
                          const bool ByCopy) {
-  QualType CaptureThisTy = ByCopy ? ThisTy->getPointeeType() : ThisTy;
  
-  FieldDecl *Field
-       = FieldDecl::Create(Context, RD, Loc, Loc, nullptr, CaptureThisTy,
-                       Context.getTrivialTypeSourceInfo(CaptureThisTy, Loc),
-                        nullptr, false, ICIS_NoInit);
+  QualType AdjustedThisTy = ThisTy;
+  // The type of the corresponding data member (not a 'this' pointer if 'by
+  // copy').
+  QualType CaptureThisFieldTy = ThisTy;
+  if (ByCopy) {
+    // If we are capturing the object referred to by '*this' by copy, ignore any
+    // cv qualifiers inherited from the type of the member function for the type
+    // of the closure-type's corresponding data member and any use of 'this'.
+    CaptureThisFieldTy = ThisTy->getPointeeType();
+    CaptureThisFieldTy.removeLocalCVRQualifiers(Qualifiers::CVRMask);
+    AdjustedThisTy = Context.getPointerType(CaptureThisFieldTy);
+  }
+  
+  FieldDecl *Field = FieldDecl::Create(
+      Context, RD, Loc, Loc, nullptr, CaptureThisFieldTy,
+      Context.getTrivialTypeSourceInfo(CaptureThisFieldTy, Loc), nullptr, false,
+      ICIS_NoInit);
+
   Field->setImplicit(true);
   Field->setAccess(AS_private);
   RD->addDecl(Field);
-  Expr *This = new (Context) CXXThisExpr(Loc, ThisTy, /*isImplicit*/true);
+  Expr *This =
+      new (Context) CXXThisExpr(Loc, ThisTy, /*isImplicit*/ true);
   if (ByCopy) {
     Expr *StarThis =  S.CreateBuiltinUnaryOp(Loc,
                                       UO_Deref,
                                       This).get();
     InitializedEntity Entity = InitializedEntity::InitializeLambdaCapture(
-      nullptr, CaptureThisTy, Loc);
+      nullptr, CaptureThisFieldTy, Loc);
     InitializationKind InitKind = InitializationKind::CreateDirect(Loc, Loc, Loc);
     InitializationSequence Init(S, Entity, InitKind, StarThis);
     ExprResult ER = Init.Perform(S, Entity, InitKind, StarThis);

          "*this) by copy");
   // FIXME: We need to delay this marking in PotentiallyPotentiallyEvaluated
   // contexts.
-
+  QualType ThisTy = getCurrentThisType();
   for (unsigned idx = MaxFunctionScopesIndex; NumCapturingClosures; 
       --idx, --NumCapturingClosures) {
     CapturingScopeInfo *CSI = cast<CapturingScopeInfo>(FunctionScopes[idx]);
     Expr *ThisExpr = nullptr;
-    QualType ThisTy = getCurrentThisType();
+    
     if (LambdaScopeInfo *LSI = dyn_cast<LambdaScopeInfo>(CSI)) {
       // For lambda expressions, build a field and an initializing expression,
       // and capture the *enclosing object* by copy only if this is the first

                       false/*ByCopy*/);
 
     bool isNested = NumCapturingClosures > 1;
-    CSI->addThisCapture(isNested, Loc, ThisTy, ThisExpr, ByCopy);
+    CSI->addThisCapture(isNested, Loc, ThisExpr, ByCopy);
   }
   return false;
 }

test/SemaCXX/cxx1z-lambda-star-this.cpp

 // RUN: %clang_cc1 -std=c++1z -verify -fsyntax-only -fblocks -fms-extensions %s -DMS_EXTENSIONS
 // RUN: %clang_cc1 -std=c++1z -verify -fsyntax-only -fblocks -fdelayed-template-parsing -fms-extensions %s -DMS_EXTENSIONS -DDELAYED_TEMPLATE_PARSING
 
+template<class, class> constexpr bool is_same = false;
+template<class T> constexpr bool is_same<T, T> = true;
 
 namespace test_star_this {
 namespace ns1 {

   b.foo(); //expected-note{{in instantiation}}
 } // end main  
 } // end ns4
+namespace ns5 {
+
+struct X {
+  double d = 3.14;
+  X(const volatile X&);
+  void foo() {
+      
+  }
+  
+  void foo() const { //expected-note{{const}}
+    
+    auto L = [*this] () mutable { 
+      static_assert(is_same<decltype(this), X*>);
+      ++d;
+      auto M = [this] { 
+        static_assert(is_same<decltype(this), X*>);  
+        ++d;
+        auto N = [] {
+          static_assert(is_same<decltype(this), X*>); 
+        };
+      };
+    };
+    
+    auto L1 = [*this] { 
+      static_assert(is_same<decltype(this), const X*>);
+      auto M = [this] () mutable { 
+        static_assert(is_same<decltype(this), const X*>);  
+        auto N = [] {
+          static_assert(is_same<decltype(this), const X*>); 
+        };
+      };
+      auto M2 = [*this] () mutable { 
+        static_assert(is_same<decltype(this), X*>);  
+        auto N = [] {
+          static_assert(is_same<decltype(this), X*>); 
+        };
+      };
+    };
+    auto L2 = [this] () mutable {
+      static_assert(is_same<decltype(this), const X*>);  
+      ++d; //expected-error{{cannot assign}}
+    };
+    auto GL = [*this] (auto a) mutable {
+      static_assert(is_same<decltype(this), X*>);
+      ++d;
+      auto M = [this] (auto b) { 
+        static_assert(is_same<decltype(this), X*>);  
+        ++d;
+        auto N = [] (auto c) {
+          static_assert(is_same<decltype(this), X*>); 
+        };
+        N(3.14);
+      };
+      M("abc");
+    };
+    GL(3.14);
+ 
+  }
+  void foo() volatile const {
+    auto L = [this] () {
+      static_assert(is_same<decltype(this), const volatile X*>);
+      auto M = [*this] () mutable { 
+        static_assert(is_same<decltype(this), X*>);
+        auto N = [this] {
+          static_assert(is_same<decltype(this), X*>);
+          auto M = [] {
+            static_assert(is_same<decltype(this), X*>);
+          };
+        };
+        auto N2 = [*this] {
+          static_assert(is_same<decltype(this), const X*>);
+        };
+      };
+      auto M2 = [*this] () {
+        static_assert(is_same<decltype(this), const X*>); 
+        auto N = [this] {
+          static_assert(is_same<decltype(this), const X*>);
+        };
+      };
+    };
+  }
+  
+};
+
+} //end ns5
+namespace ns6 {
+struct X {
+  double d;
+  auto foo() const {
+    auto L = [*this] () mutable {
+      auto M = [=] (auto a) {
+        auto N = [this] {
+          ++d;
+          static_assert(is_same<decltype(this), X*>);
+          auto O = [*this] {
+            static_assert(is_same<decltype(this), const X*>);
+          };
+        };
+        N();
+        static_assert(is_same<decltype(this), X*>);
+      };
+      return M;
+    };
+    return L;
+  }
+}; 
+
+int main() {
+  auto L = X{}.foo();
+  auto M = L();
+  M(3.14);
+}
+} // end ns6
+namespace ns7 {
+
+struct X {
+  double d;
+  X();
+  X(const X&); 
+  X(X&) = delete;
+  auto foo() const {
+    //OK - the object used to initialize our capture is a const object and so prefers the non-deleted ctor.
+    const auto &&L = [*this] { };
+  }
+  
+}; 
+int main() {
+  X x;
+  x.foo();
+}
+} // end ns7
+
 } //end ns test_star_this
+