[clang] ODR hashes depth+index and not name of TemplateTypeParm #144796
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
Change the ODR hashing logic to use the depth+index indices instead of template parameter names. This prevents spurious ODR errors in header module builds when the type canonicalization picks up different expressions - with different template parameter names (profiling used for canonicalization ignores template parameter names). This fixes llvm#143152. This commit removes test assertions from the regression test of llvm#108866 - the compiler errors asserted by that test stop firing with this commit. It's questionable whether these diagnostics were correct. We add a FIXME to find a better regression test for that fix.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Maksim Ivanov (emaxx-google) ChangesChange the ODR hashing logic to use the depth+index indices instead of template parameter names. This prevents spurious ODR errors in header module builds when the type canonicalization picks up different expressions - with different template parameter names (profiling used for canonicalization ignores template parameter names). This fixes #143152. This commit removes test assertions from the regression test of #108866 - the compiler errors asserted by that test stop firing with this commit. It's questionable whether these diagnostics were correct. We add a FIXME to find a better regression test for that fix. Full diff: https://github.com/llvm/llvm-project/pull/144796.diff 3 Files Affected:
diff --git a/clang/lib/AST/ODRHash.cpp b/clang/lib/AST/ODRHash.cpp
index f8446dfbc6859..84fac90a03e87 100644
--- a/clang/lib/AST/ODRHash.cpp
+++ b/clang/lib/AST/ODRHash.cpp
@@ -828,7 +828,23 @@ void ODRHash::AddDecl(const Decl *D) {
return;
}
- AddDeclarationName(ND->getDeclName());
+ // For template parameters, use depth+index instead of name, because type
+ // canonicalization can change the name of the template parameter.
+ if (auto *TTPD = dyn_cast<TemplateTypeParmDecl>(ND)) {
+ ID.AddInteger(TTPD->getDepth());
+ ID.AddInteger(TTPD->getIndex());
+ AddBoolean(TTPD->isParameterPack());
+ } else if (auto *NTTPD = dyn_cast<NonTypeTemplateParmDecl>(ND)) {
+ ID.AddInteger(NTTPD->getDepth());
+ ID.AddInteger(NTTPD->getIndex());
+ AddBoolean(NTTPD->isParameterPack());
+ } else if (auto *TTPD = dyn_cast<TemplateTemplateParmDecl>(ND)) {
+ ID.AddInteger(TTPD->getDepth());
+ ID.AddInteger(TTPD->getIndex());
+ AddBoolean(TTPD->isParameterPack());
+ } else {
+ AddDeclarationName(ND->getDeclName());
+ }
// If this was a specialization we should take into account its template
// arguments. This helps to reduce collisions coming when visiting template
diff --git a/clang/test/Modules/odr_hash.cpp b/clang/test/Modules/odr_hash.cpp
index da24b1fe8729a..8ef53e32f2e95 100644
--- a/clang/test/Modules/odr_hash.cpp
+++ b/clang/test/Modules/odr_hash.cpp
@@ -5164,6 +5164,29 @@ namespace A {
A::X x;
#endif
+namespace TemplateDecltypeOperator {
+
+#if defined(FIRST) || defined(SECOND)
+template <class T6>
+T6 func();
+#endif
+
+#if defined(SECOND)
+template <class UnrelatedT>
+using UnrelatedAlias = decltype(func<UnrelatedT>())();
+#endif
+
+#if defined(FIRST) || defined(SECOND)
+class A {
+ template <class T6>
+ operator decltype(func<T6>()) () {}
+};
+#else
+A a;
+#endif
+
+}
+
// Keep macros contained to one file.
#ifdef FIRST
#undef FIRST
diff --git a/clang/test/PCH/race-condition.cpp b/clang/test/PCH/race-condition.cpp
index 752b0cc3ff628..666d92ed2ce8b 100644
--- a/clang/test/PCH/race-condition.cpp
+++ b/clang/test/PCH/race-condition.cpp
@@ -31,11 +31,12 @@ constexpr enable_if_t<meta<F>::value == 2, void> midpoint(F) {}
#else
-// expected-error@27{{'N::midpoint' has different definitions in different modules; defined here first difference is 1st parameter with type 'F'}}
-// expected-error@24{{'N::midpoint' has different definitions in different modules; defined here first difference is 1st parameter with type 'U'}}
-// expected-note@21{{but in '' found 1st parameter with type 'T'}}
+// FIXME: Change the test to trigger a suitable error: previously the test
+// asserted the ODR error ("'N::midpoint' has different definitions in different
+// modules"), which isn't fully correct as there's only one module, and since a
+// change in the ODR hash calculation this error isn't triggered anymore.
+
int x = N::something;
-// expected-error@37{{no member named 'something' in namespace 'N'}}
-// expected-note@21{{but in '' found 1st parameter with type 'T'}}
+// expected-error@38{{no member named 'something' in namespace 'N'}}
#endif
|
|
@llvm/pr-subscribers-clang-modules Author: Maksim Ivanov (emaxx-google) ChangesChange the ODR hashing logic to use the depth+index indices instead of template parameter names. This prevents spurious ODR errors in header module builds when the type canonicalization picks up different expressions - with different template parameter names (profiling used for canonicalization ignores template parameter names). This fixes #143152. This commit removes test assertions from the regression test of #108866 - the compiler errors asserted by that test stop firing with this commit. It's questionable whether these diagnostics were correct. We add a FIXME to find a better regression test for that fix. Full diff: https://github.com/llvm/llvm-project/pull/144796.diff 3 Files Affected:
diff --git a/clang/lib/AST/ODRHash.cpp b/clang/lib/AST/ODRHash.cpp
index f8446dfbc6859..84fac90a03e87 100644
--- a/clang/lib/AST/ODRHash.cpp
+++ b/clang/lib/AST/ODRHash.cpp
@@ -828,7 +828,23 @@ void ODRHash::AddDecl(const Decl *D) {
return;
}
- AddDeclarationName(ND->getDeclName());
+ // For template parameters, use depth+index instead of name, because type
+ // canonicalization can change the name of the template parameter.
+ if (auto *TTPD = dyn_cast<TemplateTypeParmDecl>(ND)) {
+ ID.AddInteger(TTPD->getDepth());
+ ID.AddInteger(TTPD->getIndex());
+ AddBoolean(TTPD->isParameterPack());
+ } else if (auto *NTTPD = dyn_cast<NonTypeTemplateParmDecl>(ND)) {
+ ID.AddInteger(NTTPD->getDepth());
+ ID.AddInteger(NTTPD->getIndex());
+ AddBoolean(NTTPD->isParameterPack());
+ } else if (auto *TTPD = dyn_cast<TemplateTemplateParmDecl>(ND)) {
+ ID.AddInteger(TTPD->getDepth());
+ ID.AddInteger(TTPD->getIndex());
+ AddBoolean(TTPD->isParameterPack());
+ } else {
+ AddDeclarationName(ND->getDeclName());
+ }
// If this was a specialization we should take into account its template
// arguments. This helps to reduce collisions coming when visiting template
diff --git a/clang/test/Modules/odr_hash.cpp b/clang/test/Modules/odr_hash.cpp
index da24b1fe8729a..8ef53e32f2e95 100644
--- a/clang/test/Modules/odr_hash.cpp
+++ b/clang/test/Modules/odr_hash.cpp
@@ -5164,6 +5164,29 @@ namespace A {
A::X x;
#endif
+namespace TemplateDecltypeOperator {
+
+#if defined(FIRST) || defined(SECOND)
+template <class T6>
+T6 func();
+#endif
+
+#if defined(SECOND)
+template <class UnrelatedT>
+using UnrelatedAlias = decltype(func<UnrelatedT>())();
+#endif
+
+#if defined(FIRST) || defined(SECOND)
+class A {
+ template <class T6>
+ operator decltype(func<T6>()) () {}
+};
+#else
+A a;
+#endif
+
+}
+
// Keep macros contained to one file.
#ifdef FIRST
#undef FIRST
diff --git a/clang/test/PCH/race-condition.cpp b/clang/test/PCH/race-condition.cpp
index 752b0cc3ff628..666d92ed2ce8b 100644
--- a/clang/test/PCH/race-condition.cpp
+++ b/clang/test/PCH/race-condition.cpp
@@ -31,11 +31,12 @@ constexpr enable_if_t<meta<F>::value == 2, void> midpoint(F) {}
#else
-// expected-error@27{{'N::midpoint' has different definitions in different modules; defined here first difference is 1st parameter with type 'F'}}
-// expected-error@24{{'N::midpoint' has different definitions in different modules; defined here first difference is 1st parameter with type 'U'}}
-// expected-note@21{{but in '' found 1st parameter with type 'T'}}
+// FIXME: Change the test to trigger a suitable error: previously the test
+// asserted the ODR error ("'N::midpoint' has different definitions in different
+// modules"), which isn't fully correct as there's only one module, and since a
+// change in the ODR hash calculation this error isn't triggered anymore.
+
int x = N::something;
-// expected-error@37{{no member named 'something' in namespace 'N'}}
-// expected-note@21{{but in '' found 1st parameter with type 'T'}}
+// expected-error@38{{no member named 'something' in namespace 'N'}}
#endif
|
emaxx-google
commented
Jun 18, 2025
| #endif | ||
|
|
||
| #if defined(FIRST) || defined(SECOND) | ||
| class A { |
There was a problem hiding this comment.
Without the changes in this PR, this test would fail with this message:
'TemplateDecltypeOperator::A' with definition in module 'SecondModule' has different definitions in different modules; first difference is this function template
but in 'FirstModule' found different function template
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.
Change the ODR hashing logic to use the depth+index indices instead of template parameter names. This prevents spurious ODR errors in header module builds when the type canonicalization picks up different expressions - with different template parameter names (profiling used for canonicalization ignores template parameter names). This fixes #143152.
This commit removes test assertions from the regression test of #108866 - the compiler errors asserted by that test stop firing with this commit. It's questionable whether these diagnostics were correct. We add a FIXME to find a better regression test for that fix.