Reapply "[Clang] Fix dependent local class instantiation bugs" #135914
Conversation
This file contains hidden or 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
|
The Linux CI failure seems unrelated because I saw the same from @cor3ntin's PR: https://buildkite.com/llvm-project/github-pull-requests/builds/168960 |
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Younan Zhang (zyn0217) ChangesThis reapplies #134038 Since the last patch, this fixes a null pointer dereference where the TSI of the destructor wasn't properly propagated into the DeclarationNameInfo. We now construct a LocInfoType for dependent cases, as done elsewhere in getDestructorName, such that GetTypeFromParser can correctly obtain the TSI. Previous PR body: This patch fixes two long-standing bugs that prevent Clang from instantiating local class members inside a dependent context. These bugs were introduced in commits 21eb1af and 919df9d. 21eb1af introduced a concept called eligible methods such that it did an attempt to skip past ineligible method instantiation when instantiating class members. Unfortunately, this broke the instantiation chain for local classes - getTemplateInstantiationPattern() would fail to find the correct definition pattern if the class was defined within a partially transformed dependent context. 919df9d introduced a separate issue by incorrectly copying the DeclarationNameInfo during function definition instantiation from the template pattern, even though that DNI might contain a transformed TypeSourceInfo. Since that TSI was already updated when the declaration was instantiated, this led to inconsistencies. As a result, the final instantiated function could lose track of the transformed declarations, hence we crash: https://compiler-explorer.com/z/vjvoG76Tf. This PR corrects them by
Full diff: https://github.com/llvm/llvm-project/pull/135914.diff 6 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 84ad253c1ec4f..87c2f72d20a46 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -458,6 +458,7 @@ Bug Fixes to C++ Support
by template argument deduction.
- Clang is now better at instantiating the function definition after its use inside
of a constexpr lambda. (#GH125747)
+- Fixed a local class member function instantiation bug inside dependent lambdas. (#GH59734), (#GH132208)
- Clang no longer crashes when trying to unify the types of arrays with
certain differences in qualifiers (this could happen during template argument
deduction or when building a ternary operator). (#GH97005)
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index 8df590fa624cf..f83c05ac053ed 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -347,7 +347,7 @@ ParsedType Sema::getDestructorName(const IdentifierInfo &II,
QualType T =
CheckTypenameType(ElaboratedTypeKeyword::None, SourceLocation(),
SS.getWithLocInContext(Context), II, NameLoc);
- return ParsedType::make(T);
+ return CreateParsedType(T, Context.getTrivialTypeSourceInfo(T, NameLoc));
}
// The remaining cases are all non-standard extensions imitating the behavior
diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp
index d2408a94ad0ab..0e81804f8c1e7 100644
--- a/clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -4126,9 +4126,6 @@ Sema::InstantiateClassMembers(SourceLocation PointOfInstantiation,
if (FunctionDecl *Pattern =
Function->getInstantiatedFromMemberFunction()) {
- if (Function->isIneligibleOrNotSelected())
- continue;
-
if (Function->getTrailingRequiresClause()) {
ConstraintSatisfaction Satisfaction;
if (CheckFunctionConstraints(Function, Satisfaction) ||
diff --git a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
index 5c80077f294c6..5f370c933f834 100644
--- a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -5597,7 +5597,61 @@ void Sema::InstantiateFunctionDefinition(SourceLocation PointOfInstantiation,
Function->setLocation(PatternDecl->getLocation());
Function->setInnerLocStart(PatternDecl->getInnerLocStart());
Function->setRangeEnd(PatternDecl->getEndLoc());
- Function->setDeclarationNameLoc(PatternDecl->getNameInfo().getInfo());
+ // Let the instantiation use the Pattern's DeclarationNameLoc, due to the
+ // following awkwardness:
+ //
+ // 1. There are out-of-tree users of getNameInfo().getSourceRange(), who
+ // expect the source range of the instantiated declaration to be set to
+ // point to the definition.
+ //
+ // 2. That getNameInfo().getSourceRange() might return the TypeLocInfo's
+ // location it tracked.
+ //
+ // 3. Function might come from an (implicit) declaration, while the pattern
+ // comes from a definition. In these cases, we need the PatternDecl's source
+ // location.
+ //
+ // To that end, we need to more or less tweak the DeclarationNameLoc. However,
+ // we can't blindly copy the DeclarationNameLoc from the PatternDecl to the
+ // function, since it contains associated TypeLocs that should have already
+ // been transformed. So, we rebuild the TypeLoc for that purpose. Technically,
+ // we should create a new function declaration and assign everything we need,
+ // but InstantiateFunctionDefinition updates the declaration in place.
+ auto NameLocPointsToPattern = [&] {
+ DeclarationNameInfo PatternName = PatternDecl->getNameInfo();
+ DeclarationNameLoc PatternNameLoc = PatternName.getInfo();
+ switch (PatternName.getName().getNameKind()) {
+ case DeclarationName::CXXConstructorName:
+ case DeclarationName::CXXDestructorName:
+ case DeclarationName::CXXConversionFunctionName:
+ break;
+ default:
+ // Cases where DeclarationNameLoc doesn't matter, as it merely contains a
+ // source range.
+ return PatternNameLoc;
+ }
+
+ TypeSourceInfo *TSI = Function->getNameInfo().getNamedTypeInfo();
+ // TSI might be null if the function is named by a constructor template id.
+ // E.g. S<T>() {} for class template S with a template parameter T.
+ if (!TSI) {
+ // We don't care about the DeclarationName of the instantiated function,
+ // but only the DeclarationNameLoc. So if the TypeLoc is absent, we do
+ // nothing.
+ return PatternNameLoc;
+ }
+
+ QualType InstT = TSI->getType();
+ // We want to use a TypeLoc that reflects the transformed type while
+ // preserving the source location from the pattern.
+ TypeLocBuilder TLB;
+ TypeSourceInfo *PatternTSI = PatternName.getNamedTypeInfo();
+ assert(PatternTSI && "Pattern is supposed to have an associated TSI");
+ TLB.pushTrivial(Context, InstT, PatternTSI->getTypeLoc().getBeginLoc());
+ return DeclarationNameLoc::makeNamedTypeLoc(
+ TLB.getTypeSourceInfo(Context, InstT));
+ };
+ Function->setDeclarationNameLoc(NameLocPointsToPattern());
EnterExpressionEvaluationContext EvalContext(
*this, Sema::ExpressionEvaluationContext::PotentiallyEvaluated);
diff --git a/clang/test/CodeGenCXX/local-class-instantiation.cpp b/clang/test/CodeGenCXX/local-class-instantiation.cpp
new file mode 100644
index 0000000000000..34103a1ee55ef
--- /dev/null
+++ b/clang/test/CodeGenCXX/local-class-instantiation.cpp
@@ -0,0 +1,64 @@
+// RUN: %clang_cc1 -std=c++17 %s -emit-llvm -triple %itanium_abi_triple -o - | FileCheck %s
+
+namespace LambdaContainingLocalClasses {
+
+template <typename F>
+void GH59734() {
+ [&](auto param) {
+ struct Guard {
+ Guard() {
+ // Check that we're able to create DeclRefExpr to param at this point.
+ static_assert(__is_same(decltype(param), int), "");
+ }
+ ~Guard() {
+ static_assert(__is_same(decltype(param), int), "");
+ }
+ operator decltype(param)() {
+ return decltype(param)();
+ }
+ };
+ Guard guard;
+ param = guard;
+ }(42);
+}
+
+// Guard::Guard():
+// CHECK-DAG: define {{.*}} @_ZZZN28LambdaContainingLocalClasses7GH59734IiEEvvENKUlT_E_clIiEEDaS1_EN5GuardC2Ev
+// Guard::operator int():
+// CHECK-DAG: define {{.*}} @_ZZZN28LambdaContainingLocalClasses7GH59734IiEEvvENKUlT_E_clIiEEDaS1_EN5GuardcviEv
+// Guard::~Guard():
+// CHECK-DAG: define {{.*}} @_ZZZN28LambdaContainingLocalClasses7GH59734IiEEvvENKUlT_E_clIiEEDaS1_EN5GuardD2Ev
+
+struct S {};
+
+template <class T = void>
+auto GH132208 = [](auto param) {
+ struct OnScopeExit {
+ OnScopeExit() {
+ static_assert(__is_same(decltype(param), S), "");
+ }
+ ~OnScopeExit() {
+ static_assert(__is_same(decltype(param), S), "");
+ }
+ operator decltype(param)() {
+ return decltype(param)();
+ }
+ } pending;
+
+ param = pending;
+};
+
+void bar() {
+ GH59734<int>();
+
+ GH132208<void>(S{});
+}
+
+// OnScopeExit::OnScopeExit():
+// CHECK-DAG: define {{.*}} @_ZZNK28LambdaContainingLocalClasses8GH132208IvEMUlT_E_clINS_1SEEEDaS2_EN11OnScopeExitC2Ev
+// OnScopeExit::operator S():
+// CHECK-DAG: define {{.*}} @_ZZNK28LambdaContainingLocalClasses8GH132208IvEMUlT_E_clINS_1SEEEDaS2_EN11OnScopeExitcvS5_Ev
+// OnScopeExit::~OnScopeExit():
+// CHECK-DAG: define {{.*}} @_ZZNK28LambdaContainingLocalClasses8GH132208IvEMUlT_E_clINS_1SEEEDaS2_EN11OnScopeExitD2Ev
+
+} // namespace LambdaContainingLocalClasses
diff --git a/clang/test/SemaTemplate/instantiate-local-class.cpp b/clang/test/SemaTemplate/instantiate-local-class.cpp
index 298233739900f..810ce9329eae7 100644
--- a/clang/test/SemaTemplate/instantiate-local-class.cpp
+++ b/clang/test/SemaTemplate/instantiate-local-class.cpp
@@ -535,3 +535,37 @@ void foo() {
} // namespace local_recursive_lambda
#endif
+
+namespace PR134038_Regression {
+
+template <class T> class G {
+public:
+ template <class> class Iter {
+ public:
+ Iter();
+ ~Iter();
+
+ operator G<T>();
+ };
+};
+
+template <class ObserverType>
+template <class ContainerType>
+G<ObserverType>::Iter<ContainerType>::Iter() {}
+
+template <class ObserverType>
+template <class ContainerType>
+G<ObserverType>::Iter<ContainerType>::~Iter() {}
+
+template <class ObserverType>
+template <class ContainerType>
+G<ObserverType>::Iter<ContainerType>::operator G<ObserverType>() {
+ return G<ObserverType>{};
+}
+
+void NotifySettingChanged() {
+ G<int>::Iter<int> Iter;
+ G<int> g = Iter;
+}
+
+}
|
|
@mysterymath In the meantime, can you please help us test the Fuchsia CI with this patch? I appreciate your help :) |
erichkeane
approved these changes
Apr 16, 2025
We're not really set up to run the full Fuchsia CI with specific upstream patches, and it's a bit too flaky and expensive to be much good for that anyway. For now, it's better as post-submit than pre-submit, I'm afraid. So long as this fixes the issue in the reproducer, and the LLVM tests pass, that should be good enough to go in IMO. |
mizvekov
reviewed
Apr 16, 2025
I tested the reproducer locally and it passed with this patch. Now that the CI is green, I'll go ahead and merge it - post-commit reviews are welcome |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/59/builds/16182 Here is the relevant piece of the build log for the reference |
var-const
pushed a commit
to ldionne/llvm-project
that referenced
this pull request
Apr 17, 2025
…135914) This reapplies llvm#134038 Since the last patch, this fixes a null pointer dereference where the TSI of the destructor wasn't properly propagated into the DeclarationNameInfo. We now construct a LocInfoType for dependent cases, as done elsewhere in getDestructorName, such that GetTypeFromParser can correctly obtain the TSI. --- This patch fixes two long-standing bugs that prevent Clang from instantiating local class members inside a dependent context. These bugs were introduced in commits llvm@21eb1af and llvm@919df9d. llvm@21eb1af introduced a concept called eligible methods such that it did an attempt to skip past ineligible method instantiation when instantiating class members. Unfortunately, this broke the instantiation chain for local classes - getTemplateInstantiationPattern() would fail to find the correct definition pattern if the class was defined within a partially transformed dependent context. llvm@919df9d introduced a separate issue by incorrectly copying the DeclarationNameInfo during function definition instantiation from the template pattern, even though that DNI might contain a transformed TypeSourceInfo. Since that TSI was already updated when the declaration was instantiated, this led to inconsistencies. As a result, the final instantiated function could lose track of the transformed declarations, hence we crash: https://compiler-explorer.com/z/vjvoG76Tf. This PR corrects them by 1. Removing the bypass logic for method instantiation. The eligible flag is independent of instantiation and can be updated properly afterward, so skipping instantiation is unnecessary. 2. Carefully handling TypeSourceInfo by creating a new instance that preserves the pattern's source location while using the already transformed type.
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 reapplies #134038
Since the last patch, this fixes a null pointer dereference where the TSI of the destructor wasn't properly propagated into the DeclarationNameInfo. We now construct a LocInfoType for dependent cases, as done elsewhere in getDestructorName, such that GetTypeFromParser can correctly obtain the TSI.
Previous PR body:
This patch fixes two long-standing bugs that prevent Clang from instantiating local class members inside a dependent context. These bugs were introduced in commits 21eb1af and 919df9d.
21eb1af introduced a concept called eligible methods such that it did an attempt to skip past ineligible method instantiation when instantiating class members. Unfortunately, this broke the instantiation chain for local classes - getTemplateInstantiationPattern() would fail to find the correct definition pattern if the class was defined within a partially transformed dependent context.
919df9d introduced a separate issue by incorrectly copying the DeclarationNameInfo during function definition instantiation from the template pattern, even though that DNI might contain a transformed TypeSourceInfo. Since that TSI was already updated when the declaration was instantiated, this led to inconsistencies. As a result, the final instantiated function could lose track of the transformed declarations, hence we crash: https://compiler-explorer.com/z/vjvoG76Tf.
This PR corrects them by
Removing the bypass logic for method instantiation. The eligible flag is independent of instantiation and can be updated properly afterward, so skipping instantiation is unnecessary.
Carefully handling TypeSourceInfo by creating a new instance that preserves the pattern's source location while using the already transformed type.