Skip to content

[clang] ODR hashes depth+index and not name of TemplateTypeParm #144796

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+46 −6
Open

[clang] ODR hashes depth+index and not name of TemplateTypeParm #144796

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2fd016f
[clang] ODR hashes depth+index and not name of TemplateTypeParm
emaxx-google Jun 18, 2025
4f800c8
fix assert line number
emaxx-google Jun 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion clang/lib/AST/ODRHash.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you split the variadic templates into a separate change?

This looks like an independent issue (the code below should detect an ODR violation, but it doesn't) and actually opens a whole other can of worms wrt to what needs to be checked (e.g. checking types of non-type template parameters, template headers of template template parameters, etc).

namespace mytest {
#ifdef FIRST
template <class ...T>
struct FooTypePack {};

template <int ...I>
struct FooIntPack {};

template <template<class> class ...TT>
struct FooTemplateTemplatePack {};
#endif

#ifdef SECOND
template <class T>
struct FooTypePack {};

template <int I>
struct FooIntPack {};

template <template<class> class TT>
struct FooTemplateTemplatePack {};
#endif

#ifdef ACCESS
FooTypePack<int> a;
FooIntPack<0> b;
FooTemplateTemplatePack<FooTypePack> c;
#endif
}

} 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
Expand Down
23 changes: 23 additions & 0 deletions clang/test/Modules/odr_hash.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a comment to this test that points to this PR/GitHub issue and mentions that we need to make sure despite the fact that decltype(e) gets canonicalized to decltype(e_0) where e_0 can potentially have different template parameter names. Therefore, we need to ensure their ODR hashes are the same across multiple modules?

It might prove helpful in the future if somebody decides to reassess the choices made here and choose different trade-offs.

template <class T6>
operator decltype(func<T6>()) () {}
};
#else
A a;
#endif

}

// Keep macros contained to one file.
#ifdef FIRST
#undef FIRST
Expand Down
11 changes: 6 additions & 5 deletions clang/test/PCH/race-condition.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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@39{{no member named 'something' in namespace 'N'}}

#endif
Loading