Author: faisalv Date: Sat Jun 11 11:41:54 2016 New Revision: 272480 URL: http://llvm.org/viewvc/llvm-project?rev=272480&view=rev Log: Fix cv-qualification of '*this' captures and nasty bug PR27507
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: o) 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. o) 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. This patch has been baking in review-hell for > 6 weeks - all the comments so far have been addressed and the bug (that it addresses in passing, and I regret not submitting as a separate patch initially) has been reported twice independently, so is frequent and important for us not to just sit on. I merged the cv qualification-fix and the PR-fix initially in one patch, since they resulted from my initial implementation of star-this and so were related. If someone really feels strongly, I can put in the time to revert this - separate the two out - and recommit. I won't claim it's immunized against all bugs, but I feel confident enough about the fix to land it for now. Modified: cfe/trunk/include/clang/Sema/ScopeInfo.h cfe/trunk/lib/Sema/SemaDecl.cpp cfe/trunk/lib/Sema/SemaExprCXX.cpp cfe/trunk/test/SemaCXX/cxx1z-lambda-star-this.cpp Modified: cfe/trunk/include/clang/Sema/ScopeInfo.h URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/ScopeInfo.h?rev=272480&r1=272479&r2=272480&view=diff ============================================================================== --- cfe/trunk/include/clang/Sema/ScopeInfo.h (original) +++ cfe/trunk/include/clang/Sema/ScopeInfo.h Sat Jun 11 11:41:54 2016 @@ -500,8 +500,11 @@ public: /// \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()); @@ -546,7 +549,10 @@ public: /*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. @@ -868,9 +874,9 @@ void FunctionScopeInfo::recordUseOfWeak( 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(); } Modified: cfe/trunk/lib/Sema/SemaDecl.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=272480&r1=272479&r2=272480&view=diff ============================================================================== --- cfe/trunk/lib/Sema/SemaDecl.cpp (original) +++ cfe/trunk/lib/Sema/SemaDecl.cpp Sat Jun 11 11:41:54 2016 @@ -11177,7 +11177,7 @@ static void RebuildLambdaScopeInfo(CXXMe } 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()); Modified: cfe/trunk/lib/Sema/SemaExprCXX.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExprCXX.cpp?rev=272480&r1=272479&r2=272480&view=diff ============================================================================== --- cfe/trunk/lib/Sema/SemaExprCXX.cpp (original) +++ cfe/trunk/lib/Sema/SemaExprCXX.cpp Sat Jun 11 11:41:54 2016 @@ -872,6 +872,92 @@ bool Sema::CheckCXXThrowOperand(SourceLo 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; @@ -902,20 +988,13 @@ QualType Sema::getCurrentThisType() { 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; } @@ -953,22 +1032,36 @@ Sema::CXXThisScopeRAII::~CXXThisScopeRAI 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); @@ -1067,12 +1160,12 @@ bool Sema::CheckCXXThisCapture(SourceLoc "*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 @@ -1087,7 +1180,7 @@ bool Sema::CheckCXXThisCapture(SourceLoc false/*ByCopy*/); bool isNested = NumCapturingClosures > 1; - CSI->addThisCapture(isNested, Loc, ThisTy, ThisExpr, ByCopy); + CSI->addThisCapture(isNested, Loc, ThisExpr, ByCopy); } return false; } Modified: cfe/trunk/test/SemaCXX/cxx1z-lambda-star-this.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/cxx1z-lambda-star-this.cpp?rev=272480&r1=272479&r2=272480&view=diff ============================================================================== --- cfe/trunk/test/SemaCXX/cxx1z-lambda-star-this.cpp (original) +++ cfe/trunk/test/SemaCXX/cxx1z-lambda-star-this.cpp Sat Jun 11 11:41:54 2016 @@ -3,6 +3,8 @@ // 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 { @@ -69,4 +71,161 @@ void main() { 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 GL1 = [*this] (auto a) { + static_assert(is_same<decltype(this), const X*>); + auto M = [this] (auto b) mutable { + static_assert(is_same<decltype(this), const X*>); + auto N = [] (auto c) { + static_assert(is_same<decltype(this), const X*>); + }; + return N; + }; + + auto M2 = [*this] (auto a) mutable { + static_assert(is_same<decltype(this), X*>); + auto N = [] (auto b) { + static_assert(is_same<decltype(this), X*>); + }; + return N; + }; + return [=](auto a) mutable { M(a)(a); M2(a)(a); }; + }; + + GL1("abc")("abc"); + + + 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 + _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
