faisalv created this revision.
faisalv added reviewers: rsmith, hubert.reinterpretcast.
faisalv added subscribers: cfe-commits, gnzlbg.
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!
http://reviews.llvm.org/D19783
Files:
include/clang/Sema/ScopeInfo.h
lib/Sema/SemaDecl.cpp
lib/Sema/SemaExprCXX.cpp
test/SemaCXX/cxx1z-lambda-star-this.cpp
Index: test/SemaCXX/cxx1z-lambda-star-this.cpp
===================================================================
--- test/SemaCXX/cxx1z-lambda-star-this.cpp
+++ test/SemaCXX/cxx1z-lambda-star-this.cpp
@@ -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,112 @@
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 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
} //end ns test_star_this
+
Index: lib/Sema/SemaExprCXX.cpp
===================================================================
--- lib/Sema/SemaExprCXX.cpp
+++ lib/Sema/SemaExprCXX.cpp
@@ -872,6 +872,98 @@
return false;
}
+static QualType adjustCVQualifiersForCXXThisWithinLambda(
+ ArrayRef<FunctionScopeInfo *> FunctionScopes, QualType ThisTy,
+ DeclContext *CurSemaContext, ASTContext &ASTCtx) {
+
+ QualType ClassType = ThisTy->getPointeeType();
+ bool IsFirstIteration = true;
+ 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 ClosureClass 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-- && dyn_cast<LambdaScopeInfo>(FunctionScopes[I]);
+ IsFirstIteration = false,
+ CurDC = getLambdaAwareParentOfDeclContext(CurDC)) {
+ CurLSI = cast<LambdaScopeInfo>(FunctionScopes[I]);
+ if (CurLSI->CallOperator != CurDC) {
+ assert(IsFirstIteration);
+ assert(CurLSI->CallOperator->getParent()->getParent() == CurDC);
+ CurDC = CurLSI->CallOperator;
+ }
+ 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 +994,13 @@
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;
}
@@ -951,22 +1036,36 @@
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, AdjustedThisTy, /*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);
@@ -1065,12 +1164,12 @@
"*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
@@ -1085,7 +1184,7 @@
false/*ByCopy*/);
bool isNested = NumCapturingClosures > 1;
- CSI->addThisCapture(isNested, Loc, ThisTy, ThisExpr, ByCopy);
+ CSI->addThisCapture(isNested, Loc, ThisExpr, ByCopy);
}
return false;
}
Index: lib/Sema/SemaDecl.cpp
===================================================================
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -11049,7 +11049,7 @@
} 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());
Index: include/clang/Sema/ScopeInfo.h
===================================================================
--- include/clang/Sema/ScopeInfo.h
+++ include/clang/Sema/ScopeInfo.h
@@ -500,8 +500,11 @@
/// \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 @@
/*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.
@@ -867,9 +873,9 @@
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();
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
