[Clang] Fix dependent local class instantiation bugs #134038
Merged
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
zyn0217
added
the
clang:frontend
erichkeane
reviewed
Apr 2, 2025
Yeah, admittedly this is another workaround of not having that mechanism. But the bug was a 15 regression, (which means we don't have to rely on that for it to work!) and due to some historic incorrect patches (which I'm fixing here) it ends up becoming hard to get the behavior correct as 14. |
Oh, totally understood. This is just a 'stuck between a rock and a hard place' sort of situation, and I'm pretty uncomfortable here again. I wouldn't be able to convince you to start work on the delayed lambda body instantiation instead, would I ? :-P |
AFAIK @LYP951018 is already working on this region. Though I haven't test that patch with it yet 🥲 For Xref: #105953 |
|
I talked to @erichkeane and updated the approach to make it feel less like a workaround and independent of lambda instantiation. As noted, both cases worked prior to clang 15, which means they didn’t rely on deferred lambda body instantiation to compile correctly. Regarding the lambda side of things - since @mizvekov is already working on a major ContextDecl refactoring in that area, I don’t think it's appropriate to initiate another large-scale change simultaneously. That would result in a lot of conflicts, as I can expect. It's also expected that much of the present issues will be alleviated once that gets landed and it's not too late for us to start working on the lambda deferral afterwards. That said, this patch is aimed at fixing a regression and doesn't actually touch the lambda code itself (even though the bug involves lambdas), so it shouldn't become a burden that complicates future work on deferred lambda body instantiation. In fact, I think this should actually improve our test coverage overall. There's little impact on the performance: |
|
@llvm/pr-subscribers-clang Author: Younan Zhang (zyn0217) ChangesThis 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/134038.diff 4 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index e409f206f6eae..6107fd7667306 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -348,6 +348,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/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp
index 00dcadb41e8fb..5b502b494e410 100644
--- a/clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -4264,9 +4264,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 8aaaea0bcdd66..a8d23fb2be6d5 100644
--- a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -5590,7 +5590,62 @@ 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 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 CorrectNameLoc = [&] {
+ 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.
+ Function->setDeclarationNameLoc(PatternNameLoc);
+ return;
+ }
+
+ TypeSourceInfo *TSI = Function->getNameInfo().getNamedTypeInfo();
+ // !!! When could TSI be null?
+ 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.
+ Function->setDeclarationNameLoc(PatternNameLoc);
+ return;
+ }
+
+ 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;
+ TLB.pushTrivial(
+ Context, InstT,
+ PatternNameLoc.getNamedTypeInfo()->getTypeLoc().getBeginLoc());
+ Function->setDeclarationNameLoc(DeclarationNameLoc::makeNamedTypeLoc(
+ TLB.getTypeSourceInfo(Context, InstT)));
+ };
+
+ CorrectNameLoc();
EnterExpressionEvaluationContext EvalContext(
*this, Sema::ExpressionEvaluationContext::PotentiallyEvaluated);
diff --git a/clang/test/SemaTemplate/instantiate-local-class.cpp b/clang/test/SemaTemplate/instantiate-local-class.cpp
index 298233739900f..bd6ef84a2bc0a 100644
--- a/clang/test/SemaTemplate/instantiate-local-class.cpp
+++ b/clang/test/SemaTemplate/instantiate-local-class.cpp
@@ -1,6 +1,9 @@
// RUN: %clang_cc1 -verify -std=c++11 %s
// RUN: %clang_cc1 -verify -std=c++11 -fdelayed-template-parsing %s
// RUN: %clang_cc1 -verify -std=c++20 -fsyntax-only %s
+// RUN: %clang_cc1 -std=c++17 %s -DCodeGen -emit-llvm -triple %itanium_abi_triple -o - | FileCheck %s -dump-input=always
+
+#ifndef CodeGen
template<typename T>
void f0() {
@@ -60,7 +63,7 @@ namespace PR8801 {
class X;
typedef int (X::*pmf_type)();
class X : public T { };
-
+
pmf_type pmf = &T::foo;
}
@@ -535,3 +538,72 @@ void foo() {
} // namespace local_recursive_lambda
#endif
+
+#endif // CodeGen
+
+#ifdef CodeGen
+
+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
+
+#endif
|
erichkeane
reviewed
Apr 3, 2025
|
Review ping |
erichkeane
approved these changes
Apr 14, 2025
|
This looks to be the likely culprit for a clang crash in the Fuchsia build: Does it seem likely this PR is the culprit? If so, can this be fixed forward or reverted? |
|
Can you send reproducer and / or stack trace? FYI you can use |
|
Here's a link to a reproducer tar; I was able to verify that the issue reproduces locally using our toolchain from CI with the tarball. |
|
Yeah, I confirm this crashes clang, and reverting this commit does fix the crash. For reference, this is the stack trace: @zyn0217 FYI |
mizvekov
added a commit
that referenced
this pull request
Apr 15, 2025
Reverts #134038 This crashes clang as reported here: #134038 (comment)
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Apr 15, 2025
…gs" (#135870) Reverts llvm/llvm-project#134038 This crashes clang as reported here: llvm/llvm-project#134038 (comment)
|
Thanks for the revert. I added an assertion to I need a reduced version to figure out what happened. |
zyn0217
added a commit
that referenced
this pull request
Apr 17, 2025
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. --- 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 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.
var-const
pushed a commit
to ldionne/llvm-project
that referenced
this pull request
Apr 17, 2025
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 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. Fixes llvm#59734 Fixes llvm#132208
var-const
pushed a commit
to ldionne/llvm-project
that referenced
this pull request
Apr 17, 2025
…35870) Reverts llvm#134038 This crashes clang as reported here: llvm#134038 (comment)
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 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.
Fixes #59734
Fixes #132208