[Clang][Sema] Fix crash when 'this' is used in a dependent class scope function template specialization that instantiates to a static member function #87541
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Krystian Stasiowski (sdkrystian) ChangesThis patch fixes a crash that happens when ' template<typename T>
struct A
{
template<typename U>
static void f();
template<>
void f<int>()
{
this; // causes crash during instantiation
}
};
template struct A<int>;This happens because during instantiation of the function body, Full diff: https://github.com/llvm/llvm-project/pull/87541.diff 7 Files Affected:
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 8c98d8c7fef7a7..696736152ca4e3 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -5627,7 +5627,8 @@ class Sema final {
ExprResult BuildDeclarationNameExpr(const CXXScopeSpec &SS, LookupResult &R,
bool NeedsADL,
- bool AcceptInvalidDecl = false);
+ bool AcceptInvalidDecl = false,
+ bool NeedUnresolved = false);
ExprResult BuildDeclarationNameExpr(
const CXXScopeSpec &SS, const DeclarationNameInfo &NameInfo, NamedDecl *D,
NamedDecl *FoundD = nullptr,
@@ -6779,7 +6780,10 @@ class Sema final {
SourceLocation RParenLoc);
//// ActOnCXXThis - Parse 'this' pointer.
- ExprResult ActOnCXXThis(SourceLocation loc);
+ ExprResult ActOnCXXThis(SourceLocation Loc);
+
+ /// Check whether the type of 'this' is valid in the current context.
+ bool CheckCXXThisType(SourceLocation Loc, QualType Type);
/// Build a CXXThisExpr and mark it referenced in the current context.
Expr *BuildCXXThisExpr(SourceLocation Loc, QualType Type, bool IsImplicit);
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 80b4257d9d83ed..7fd72dc5ac2a40 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -3442,10 +3442,11 @@ static bool ShouldLookupResultBeMultiVersionOverload(const LookupResult &R) {
ExprResult Sema::BuildDeclarationNameExpr(const CXXScopeSpec &SS,
LookupResult &R, bool NeedsADL,
- bool AcceptInvalidDecl) {
+ bool AcceptInvalidDecl,
+ bool NeedUnresolved) {
// If this is a single, fully-resolved result and we don't need ADL,
// just build an ordinary singleton decl ref.
- if (!NeedsADL && R.isSingleResult() &&
+ if (!NeedUnresolved && !NeedsADL && R.isSingleResult() &&
!R.getAsSingle<FunctionTemplateDecl>() &&
!ShouldLookupResultBeMultiVersionOverload(R))
return BuildDeclarationNameExpr(SS, R.getLookupNameInfo(), R.getFoundDecl(),
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index 76bb78aa8b5458..83bb0c911dfd78 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -1415,26 +1415,42 @@ bool Sema::CheckCXXThisCapture(SourceLocation Loc, const bool Explicit,
}
ExprResult Sema::ActOnCXXThis(SourceLocation Loc) {
- /// C++ 9.3.2: In the body of a non-static member function, the keyword this
- /// is a non-lvalue expression whose value is the address of the object for
- /// which the function is called.
+ // C++20 [expr.prim.this]p1:
+ // The keyword this names a pointer to the object for which an
+ // implicit object member function is invoked or a non-static
+ // data member's initializer is evaluated.
QualType ThisTy = getCurrentThisType();
- if (ThisTy.isNull()) {
- DeclContext *DC = getFunctionLevelDeclContext();
+ if (CheckCXXThisType(Loc, ThisTy))
+ return ExprError();
- if (const auto *Method = dyn_cast<CXXMethodDecl>(DC);
- Method && Method->isExplicitObjectMemberFunction()) {
- return Diag(Loc, diag::err_invalid_this_use) << 1;
- }
+ return BuildCXXThisExpr(Loc, ThisTy, /*IsImplicit=*/false);
+}
- if (isLambdaCallWithExplicitObjectParameter(CurContext))
- return Diag(Loc, diag::err_invalid_this_use) << 1;
+bool Sema::CheckCXXThisType(SourceLocation Loc, QualType Type) {
+ if (!Type.isNull())
+ return false;
- return Diag(Loc, diag::err_invalid_this_use) << 0;
+ // C++20 [expr.prim.this]p3:
+ // If a declaration declares a member function or member function template
+ // of a class X, the expression this is a prvalue of type
+ // �pointer to cv-qualifier-seq X� wherever X is the current class between
+ // the optional cv-qualifier-seq and the end of the function-definition,
+ // member-declarator, or declarator. It shall not appear within the
+ // declaration of either a static member function or an explicit object
+ // member function of the current class (although its type and value
+ // category are defined within such member functions as they are within
+ // an implicit object member function).
+ DeclContext *DC = getFunctionLevelDeclContext();
+ if (const auto *Method = dyn_cast<CXXMethodDecl>(DC);
+ Method && Method->isExplicitObjectMemberFunction()) {
+ Diag(Loc, diag::err_invalid_this_use) << 1;
+ } else if (isLambdaCallWithExplicitObjectParameter(CurContext)) {
+ Diag(Loc, diag::err_invalid_this_use) << 1;
+ } else {
+ Diag(Loc, diag::err_invalid_this_use) << 0;
}
-
- return BuildCXXThisExpr(Loc, ThisTy, /*IsImplicit=*/false);
+ return true;
}
Expr *Sema::BuildCXXThisExpr(SourceLocation Loc, QualType Type,
diff --git a/clang/lib/Sema/SemaExprMember.cpp b/clang/lib/Sema/SemaExprMember.cpp
index 32998ae60eafe2..438f7dc24481c7 100644
--- a/clang/lib/Sema/SemaExprMember.cpp
+++ b/clang/lib/Sema/SemaExprMember.cpp
@@ -61,6 +61,9 @@ enum IMAKind {
/// The reference is a contextually-permitted abstract member reference.
IMA_Abstract,
+ /// Whether the context is static is dependent on the enclosing template (i.e. in a dependent class scope explicit specialization).
+ IMA_Dependent,
+
/// The reference may be to an unresolved using declaration and the
/// context is not an instance method.
IMA_Unresolved_StaticOrExplicitContext,
@@ -91,10 +94,20 @@ static IMAKind ClassifyImplicitMemberAccess(Sema &SemaRef,
DeclContext *DC = SemaRef.getFunctionLevelDeclContext();
- bool isStaticOrExplicitContext =
- SemaRef.CXXThisTypeOverride.isNull() &&
- (!isa<CXXMethodDecl>(DC) || cast<CXXMethodDecl>(DC)->isStatic() ||
- cast<CXXMethodDecl>(DC)->isExplicitObjectMemberFunction());
+ bool couldInstantiateToStatic = false;
+ bool isStaticOrExplicitContext = SemaRef.CXXThisTypeOverride.isNull();
+
+ if (auto *MD = dyn_cast<CXXMethodDecl>(DC)) {
+ if (!MD->isStatic() && !MD->isExplicitObjectMemberFunction())
+ isStaticOrExplicitContext = false;
+
+ // A dependent class scope function template explicit specialization
+ // which has no explicit object parameter and is declared without 'static'
+ // could instantiate to either a static or non-static member function.
+ if (MD->getDependentSpecializationInfo() &&
+ !MD->isStatic() && !MD->isExplicitObjectMemberFunction())
+ couldInstantiateToStatic = true;
+ }
if (R.isUnresolvableResult())
return isStaticOrExplicitContext ? IMA_Unresolved_StaticOrExplicitContext
@@ -123,6 +136,9 @@ static IMAKind ClassifyImplicitMemberAccess(Sema &SemaRef,
if (Classes.empty())
return IMA_Static;
+ if (couldInstantiateToStatic)
+ return IMA_Dependent;
+
// C++11 [expr.prim.general]p12:
// An id-expression that denotes a non-static data member or non-static
// member function of a class can only be used:
@@ -268,27 +284,26 @@ ExprResult Sema::BuildPossibleImplicitMemberExpr(
const CXXScopeSpec &SS, SourceLocation TemplateKWLoc, LookupResult &R,
const TemplateArgumentListInfo *TemplateArgs, const Scope *S,
UnresolvedLookupExpr *AsULE) {
- switch (ClassifyImplicitMemberAccess(*this, R)) {
+ switch (IMAKind Classification = ClassifyImplicitMemberAccess(*this, R)) {
case IMA_Instance:
- return BuildImplicitMemberExpr(SS, TemplateKWLoc, R, TemplateArgs, true, S);
-
case IMA_Mixed:
case IMA_Mixed_Unrelated:
case IMA_Unresolved:
- return BuildImplicitMemberExpr(SS, TemplateKWLoc, R, TemplateArgs, false,
- S);
-
+ return BuildImplicitMemberExpr(SS, TemplateKWLoc, R, TemplateArgs,
+ /*IsKnownInstance=*/Classification == IMA_Instance, S);
case IMA_Field_Uneval_Context:
Diag(R.getNameLoc(), diag::warn_cxx98_compat_non_static_member_use)
<< R.getLookupNameInfo().getName();
[[fallthrough]];
case IMA_Static:
case IMA_Abstract:
+ case IMA_Dependent:
case IMA_Mixed_StaticOrExplicitContext:
case IMA_Unresolved_StaticOrExplicitContext:
if (TemplateArgs || TemplateKWLoc.isValid())
- return BuildTemplateIdExpr(SS, TemplateKWLoc, R, false, TemplateArgs);
- return AsULE ? AsULE : BuildDeclarationNameExpr(SS, R, false);
+ return BuildTemplateIdExpr(SS, TemplateKWLoc, R, /*RequiresADL=*/false, TemplateArgs);
+ return AsULE ? AsULE : BuildDeclarationNameExpr(SS, R, /*NeedsADL=*/false, /*AcceptInvalidDecl=*/false,
+ /*NeedUnresolved=*/Classification == IMA_Dependent);
case IMA_Error_StaticOrExplicitContext:
case IMA_Error_Unrelated:
diff --git a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
index dc972018e7b281..ae3c3caf9c5c8d 100644
--- a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -5084,6 +5084,14 @@ void Sema::InstantiateFunctionDefinition(SourceLocation PointOfInstantiation,
EnterExpressionEvaluationContext EvalContext(
*this, Sema::ExpressionEvaluationContext::PotentiallyEvaluated);
+ Qualifiers ThisTypeQuals;
+ CXXRecordDecl *ThisContext = nullptr;
+ if (CXXMethodDecl *Method = dyn_cast<CXXMethodDecl>(Function)) {
+ ThisContext = Method->getParent();
+ ThisTypeQuals = Method->getMethodQualifiers();
+ }
+ CXXThisScopeRAII ThisScope(*this, ThisContext, ThisTypeQuals);
+
// Introduce a new scope where local variable instantiations will be
// recorded, unless we're actually a member function within a local
// class, in which case we need to merge our results with the parent
diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h
index a2568ad0f82cc2..a11709af80b640 100644
--- a/clang/lib/Sema/TreeTransform.h
+++ b/clang/lib/Sema/TreeTransform.h
@@ -3301,12 +3301,13 @@ class TreeTransform {
/// Build a new C++ "this" expression.
///
- /// By default, builds a new "this" expression without performing any
- /// semantic analysis. Subclasses may override this routine to provide
- /// different behavior.
+ /// By default, performs semantic analysis to build a new "this" expression.
+ /// Subclasses may override this routine to provide different behavior.
ExprResult RebuildCXXThisExpr(SourceLocation ThisLoc,
QualType ThisType,
bool isImplicit) {
+ if (getSema().CheckCXXThisType(ThisLoc, ThisType))
+ return ExprError();
return getSema().BuildCXXThisExpr(ThisLoc, ThisType, isImplicit);
}
diff --git a/clang/test/SemaTemplate/ms-function-specialization-class-scope.cpp b/clang/test/SemaTemplate/ms-function-specialization-class-scope.cpp
index dcab9bfaeabcb0..e72a98b637da5b 100644
--- a/clang/test/SemaTemplate/ms-function-specialization-class-scope.cpp
+++ b/clang/test/SemaTemplate/ms-function-specialization-class-scope.cpp
@@ -1,7 +1,6 @@
-// RUN: %clang_cc1 -fms-extensions -fsyntax-only -verify %s
-// RUN: %clang_cc1 -fms-extensions -fdelayed-template-parsing -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fms-extensions -fsyntax-only -Wno-unused-value -verify %s
+// RUN: %clang_cc1 -fms-extensions -fdelayed-template-parsing -fsyntax-only -Wno-unused-value -verify %s
-// expected-no-diagnostics
class A {
public:
template<class U> A(U p) {}
@@ -76,3 +75,36 @@ struct S {
int f<0>(int);
};
}
+
+namespace UsesThis {
+ template<typename T>
+ struct A {
+ int x;
+
+ template<typename U>
+ static void f();
+
+ template<>
+ void f<int>() {
+ this->x; // expected-error {{invalid use of 'this' outside of a non-static member function}}
+ x; // expected-error {{invalid use of member 'x' in static member function}}
+ A::x; // expected-error {{invalid use of member 'x' in static member function}}
+ +x; // expected-error {{invalid use of member 'x' in static member function}}
+ +A::x; // expected-error {{invalid use of member 'x' in static member function}}
+ }
+
+ template<typename U>
+ void g();
+
+ template<>
+ void g<int>() {
+ this->x;
+ x;
+ A::x;
+ +x;
+ +A::x;
+ }
+ };
+
+ template struct A<int>; // expected-note {{in instantiation of function template specialization}}
+}
|
sdkrystian
force-pushed
the
expl-spec-this
branch
2 times, most recently
from
aa722c5
to
5869c87
Compare
|
Still need to add a release note |
cor3ntin
reviewed
Apr 4, 2024
clang/lib/Sema/SemaExprMember.cpp
Outdated
Comment on lines
98
to
109
| bool couldInstantiateToStatic = false; | ||
| bool isStaticOrExplicitContext = SemaRef.CXXThisTypeOverride.isNull(); | ||
|
|
||
| if (auto *MD = dyn_cast<CXXMethodDecl>(DC)) { | ||
| if (!MD->isStatic() && !MD->isExplicitObjectMemberFunction()) | ||
| isStaticOrExplicitContext = false; | ||
|
|
||
| // A dependent class scope function template explicit specialization | ||
| // which has no explicit object parameter and is declared without 'static' | ||
| // could instantiate to either a static or non-static member function. | ||
| if (MD->getDependentSpecializationInfo() && !MD->isStatic() && | ||
| !MD->isExplicitObjectMemberFunction()) | ||
| couldInstantiateToStatic = true; | ||
| } |
There was a problem hiding this comment.
Suggested change
| bool couldInstantiateToStatic = false; | |
| bool isStaticOrExplicitContext = SemaRef.CXXThisTypeOverride.isNull(); | |
| if (auto *MD = dyn_cast<CXXMethodDecl>(DC)) { | |
| if (!MD->isStatic() && !MD->isExplicitObjectMemberFunction()) | |
| isStaticOrExplicitContext = false; | |
| // A dependent class scope function template explicit specialization | |
| // which has no explicit object parameter and is declared without 'static' | |
| // could instantiate to either a static or non-static member function. | |
| if (MD->getDependentSpecializationInfo() && !MD->isStatic() && | |
| !MD->isExplicitObjectMemberFunction()) | |
| couldInstantiateToStatic = true; | |
| } | |
| bool couldInstantiateToStatic = false; | |
| bool isStaticOrExplicitContext = SemaRef.CXXThisTypeOverride.isNull(); | |
| if (auto *MD = dyn_cast<CXXMethodDecl>(DC)) { | |
| if (MD->isImplicitObjectMemberFunction()) | |
| isStaticOrExplicitContext = false; | |
| // A dependent class scope function template explicit specialization | |
| // which has no explicit object parameter and is declared without 'static' | |
| // could instantiate to either a static or non-static member function. | |
| if (MD->getDependentSpecializationInfo() && !isStaticOrExplicitContext) | |
| couldInstantiateToStatic = true; | |
| } |
There was a problem hiding this comment.
@cor3ntin I simplified a bit further:
bool couldInstantiateToStatic = false;
bool isStaticOrExplicitContext = SemaRef.CXXThisTypeOverride.isNull();
if (auto *MD = dyn_cast<CXXMethodDecl>(DC)) {
if (MD->isImplicitObjectMemberFunction()) {
isStaticOrExplicitContext = false;
// A dependent class scope function template explicit specialization
// that is neither declared 'static' nor with an explicit object
// parameter could instantiate to a static or non-static member function.
couldInstantiateToStatic = MD->getDependentSpecializationInfo();
}
}
sdkrystian
force-pushed
the
expl-spec-this
branch
from
0121194
to
4450a35
Compare
|
@cor3ntin Could you elaborate as to which NFC changes should be split into separate commits? There aren't any changes included that are needed for the patch.. |
|
Ping @erichkeane :) |
|
I'm OK with this, but would like @cor3ntin to take another look since he engaged earlier. |
…e function template specialization that instantiates to a static member function
sdkrystian
force-pushed
the
expl-spec-this
branch
from
4450a35
to
2b01663
Compare
|
This commit appears to regress an example like this: template <typename T>
struct Foo {
template <typename X>
int bar(X x) {
return 0;
}
template <>
int bar(int x) {
return bar(5.0);
}
};
void call() {
Foo<double> f;
f.bar(1);
}Used to compile, now results in an error: Is that intended? |
|
@rupprecht That's unintended, will revert and address it. |
sdkrystian
added a commit
to sdkrystian/llvm-project
that referenced
this pull request
Apr 10, 2024
…lass scope function template specialization that instantiates to a static member function (llvm#87541)" This patch fixes a crash that happens when '`this`' is referenced (implicitly or explicitly) in a dependent class scope function template specialization that instantiates to a static member function. For example: ``` template<typename T> struct A { template<typename U> static void f(); template<> void f<int>() { this; // causes crash during instantiation } }; template struct A<int>; ``` This happens because during instantiation of the function body, `Sema::getCurrentThisType` will return a null `QualType` which we rebuild the `CXXThisExpr` with. A similar problem exists for implicit class member access expressions in such contexts (which shouldn't really happen within templates anyways per [class.mfct.non.static] p2, but changing that is non-trivial). This patch fixes the crash by building `UnresolvedLookupExpr`s instead of `MemberExpr`s for these implicit member accesses, which will then be correctly rebuilt as `MemberExpr`s during instantiation.
metaflow
added a commit
that referenced
this pull request
Apr 15, 2024
…endent class scope function template specialization that instantiates to a static member function (#87541)" (#88311)" This reverts commit aa80f3e. See #88311 (comment). There is a fix forward proposed but I am reverting this commit to fix trunk.
sdkrystian
added a commit
to sdkrystian/llvm-project
that referenced
this pull request
Apr 15, 2024
…lass scope function template specialization that instantiates to a static member function (llvm#87541, llvm#88311)" Reapplies llvm#87541 and llvm#88311 addressing the bug which caused expressions naming overload sets to be incorrectly rebuilt, as well as the bug which caused base class members to always be treated as overload sets.
bazuzi
pushed a commit
to bazuzi/llvm-project
that referenced
this pull request
Apr 15, 2024
…endent class scope function template specialization that instantiates to a static member function (llvm#87541)" (llvm#88311)" This reverts commit aa80f3e. See llvm#88311 (comment). There is a fix forward proposed but I am reverting this commit to fix trunk.
aniplcc
pushed a commit
to aniplcc/llvm-project
that referenced
this pull request
Apr 15, 2024
…endent class scope function template specialization that instantiates to a static member function (llvm#87541)" (llvm#88311)" This reverts commit aa80f3e. See llvm#88311 (comment). There is a fix forward proposed but I am reverting this commit to fix trunk.
sdkrystian
added a commit
to sdkrystian/llvm-project
that referenced
this pull request
Apr 18, 2024
…lass scope function template specialization that instantiates to a static member function (llvm#87541, llvm#88311)" Reapplies llvm#87541 and llvm#88311 addressing the bug which caused expressions naming overload sets to be incorrectly rebuilt, as well as the bug which caused base class members to always be treated as overload sets.
sdkrystian
added a commit
to sdkrystian/llvm-project
that referenced
this pull request
Apr 22, 2024
…lass scope function template specialization that instantiates to a static member function (llvm#87541, llvm#88311)" Reapplies llvm#87541 and llvm#88311 addressing the bug which caused expressions naming overload sets to be incorrectly rebuilt, as well as the bug which caused base class members to always be treated as overload sets.
sdkrystian
added a commit
that referenced
this pull request
Apr 22, 2024
…lass scope function template specialization that instantiates to a static member function (#87541, #88311)" (#88731) Reapplies #87541 and #88311 (again) addressing the bug which caused expressions naming overload sets to be incorrectly rebuilt, as well as the bug which caused base class members to always be treated as overload sets. The primary change since #88311 is `UnresolvedLookupExpr::Create` is called directly in `BuildPossibleImplicitMemberExpr` with `KnownDependent` as `true` (which causes the expression type to be set to `ASTContext::DependentTy`). This ensures that any further semantic analysis involving the type of the potentially implicit class member access expression is deferred until instantiation.
This was referenced Apr 22, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch fixes a crash that happens when '
this' is referenced (implicitly or explicitly) in a dependent class scope function template specialization that instantiates to a static member function. For example (godbolt):This happens because during instantiation of the function body,
Sema::getCurrentThisTypewill return a nullQualTypewhich we rebuild theCXXThisExprwith. A similar problem exists for implicit class member access expressions in such contexts (which shouldn't really happen within templates anyways per [class.mfct.non.static] p2, but changing that is non-trivial). This patch fixes the crash by buildingUnresolvedLookupExprs instead ofMemberExprs for these implicit member accesses, which will then be correctly rebuilt asMemberExprs during instantiation.