https://github.com/hahnjo updated
https://github.com/llvm/llvm-project/pull/133057
>From e5f64e795f10a81a009b4562db0de692deb4c0f7 Mon Sep 17 00:00:00 2001
From: Jonas Hahnfeld <jonas.hahnf...@cern.ch>
Date: Tue, 25 Mar 2025 11:27:59 +0100
Subject: [PATCH 1/5] [Serialization] Hash inner template arguments
The code is applied from ODRHash::AddDecl with the reasoning given
in the comment, to reduce collisions. This was particularly visible
with STL types templated on std::pair where its template arguments
were not taken into account.
---
.../lib/Serialization/TemplateArgumentHasher.cpp | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/clang/lib/Serialization/TemplateArgumentHasher.cpp
b/clang/lib/Serialization/TemplateArgumentHasher.cpp
index 3e8ffea78c2f1..d2aef73dec723 100644
--- a/clang/lib/Serialization/TemplateArgumentHasher.cpp
+++ b/clang/lib/Serialization/TemplateArgumentHasher.cpp
@@ -202,6 +202,21 @@ void TemplateArgumentHasher::AddDecl(const Decl *D) {
}
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
+ // specialization types (eg. when processing type template arguments).
+ ArrayRef<TemplateArgument> Args;
+ if (auto *CTSD = dyn_cast<ClassTemplateSpecializationDecl>(D))
+ Args = CTSD->getTemplateArgs().asArray();
+ else if (auto *VTSD = dyn_cast<VarTemplateSpecializationDecl>(D))
+ Args = VTSD->getTemplateArgs().asArray();
+ else if (auto *FD = dyn_cast<FunctionDecl>(D))
+ if (FD->getTemplateSpecializationArgs())
+ Args = FD->getTemplateSpecializationArgs()->asArray();
+
+ for (auto &TA : Args)
+ AddTemplateArgument(TA);
}
void TemplateArgumentHasher::AddQualType(QualType T) {
>From e0dbf8e9c8ac320555181591ad4e30cb1ee5a926 Mon Sep 17 00:00:00 2001
From: Jonas Hahnfeld <jonas.hahnf...@cern.ch>
Date: Tue, 25 Mar 2025 11:31:23 +0100
Subject: [PATCH 2/5] [Serialization] Complete only needed partial
specializations
It is unclear (to me) why this needs to be done "for safety", but
this change significantly improves the effectiveness of lazy loading.
---
clang/lib/Serialization/ASTReader.cpp | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/clang/lib/Serialization/ASTReader.cpp
b/clang/lib/Serialization/ASTReader.cpp
index e8dddda584a9b..f7c4e1846393b 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -8089,14 +8089,8 @@ void ASTReader::CompleteRedeclChain(const Decl *D) {
}
}
- if (Template) {
- // For partitial specialization, load all the specializations for safety.
- if (isa<ClassTemplatePartialSpecializationDecl,
- VarTemplatePartialSpecializationDecl>(D))
- Template->loadLazySpecializationsImpl();
- else
- Template->loadLazySpecializationsImpl(Args);
- }
+ if (Template)
+ Template->loadLazySpecializationsImpl(Args);
}
CXXCtorInitializer **
>From 9325b153a46e272f2627a341097ff9527a8837b4 Mon Sep 17 00:00:00 2001
From: Jonas Hahnfeld <jonas.hahnf...@cern.ch>
Date: Tue, 25 Mar 2025 11:42:20 +0100
Subject: [PATCH 3/5] [Serialization] Load only needed partial specializations
Similar as the last commit, it is unclear why we need to load all
specializations, including non-partial ones, when we have a TPL.
---
clang/lib/AST/DeclTemplate.cpp | 6 ------
1 file changed, 6 deletions(-)
diff --git a/clang/lib/AST/DeclTemplate.cpp b/clang/lib/AST/DeclTemplate.cpp
index 8a6fb3391c13f..6b918c2d75abb 100644
--- a/clang/lib/AST/DeclTemplate.cpp
+++ b/clang/lib/AST/DeclTemplate.cpp
@@ -368,12 +368,6 @@ bool RedeclarableTemplateDecl::loadLazySpecializationsImpl(
if (!ExternalSource)
return false;
- // If TPL is not null, it implies that we're loading specializations for
- // partial templates. We need to load all specializations in such cases.
- if (TPL)
- return
ExternalSource->LoadExternalSpecializations(this->getCanonicalDecl(),
- /*OnlyPartial=*/false);
-
return ExternalSource->LoadExternalSpecializations(this->getCanonicalDecl(),
Args);
}
>From 3d95698ac96a9db6e99b02741d998fcce2a98735 Mon Sep 17 00:00:00 2001
From: Jonas Hahnfeld <jonas.hahnf...@cern.ch>
Date: Tue, 25 Mar 2025 11:59:46 +0100
Subject: [PATCH 4/5] [Serialization] Remove bail-out logic in
TemplateArgumentHasher
While it is correct to assign a single fixed hash to all template
arguments, it can reduce the effectiveness of lazy loading and is
not actually needed: we are allowed to ignore parts that cannot be
handled because they will be analogously ignored by all hashings.
---
.../Serialization/TemplateArgumentHasher.cpp | 36 ++-----------------
1 file changed, 3 insertions(+), 33 deletions(-)
diff --git a/clang/lib/Serialization/TemplateArgumentHasher.cpp
b/clang/lib/Serialization/TemplateArgumentHasher.cpp
index d2aef73dec723..70f963b63509c 100644
--- a/clang/lib/Serialization/TemplateArgumentHasher.cpp
+++ b/clang/lib/Serialization/TemplateArgumentHasher.cpp
@@ -22,17 +22,6 @@ using namespace clang;
namespace {
class TemplateArgumentHasher {
- // If we bail out during the process of calculating hash values for
- // template arguments for any reason. We're allowed to do it since
- // TemplateArgumentHasher are only required to give the same hash value
- // for the same template arguments, but not required to give different
- // hash value for different template arguments.
- //
- // So in the worst case, it is still a valid implementation to give all
- // inputs the same BailedOutValue as output.
- bool BailedOut = false;
- static constexpr unsigned BailedOutValue = 0x12345678;
-
llvm::FoldingSetNodeID ID;
public:
@@ -42,14 +31,7 @@ class TemplateArgumentHasher {
void AddInteger(unsigned V) { ID.AddInteger(V); }
- unsigned getValue() {
- if (BailedOut)
- return BailedOutValue;
-
- return ID.computeStableHash();
- }
-
- void setBailedOut() { BailedOut = true; }
+ unsigned getValue() { return ID.computeStableHash(); }
void AddType(const Type *T);
void AddQualType(QualType T);
@@ -95,8 +77,7 @@ void
TemplateArgumentHasher::AddTemplateArgument(TemplateArgument TA) {
case TemplateArgument::Expression:
// If we meet expression in template argument, it implies
// that the template is still dependent. It is meaningless
- // to get a stable hash for the template. Bail out simply.
- BailedOut = true;
+ // to get a stable hash for the template.
break;
case TemplateArgument::Pack:
AddInteger(TA.pack_size());
@@ -113,10 +94,9 @@ void TemplateArgumentHasher::AddStructuralValue(const
APValue &Value) {
// 'APValue::Profile' uses pointer values to make hash for LValue and
// MemberPointer, but they differ from one compiler invocation to another.
- // It may be difficult to handle such cases. Bail out simply.
+ // It may be difficult to handle such cases.
if (Kind == APValue::LValue || Kind == APValue::MemberPointer) {
- BailedOut = true;
return;
}
@@ -138,14 +118,11 @@ void TemplateArgumentHasher::AddTemplateName(TemplateName
Name) {
case TemplateName::DependentTemplate:
case TemplateName::SubstTemplateTemplateParm:
case TemplateName::SubstTemplateTemplateParmPack:
- BailedOut = true;
break;
case TemplateName::UsingTemplate: {
UsingShadowDecl *USD = Name.getAsUsingShadowDecl();
if (USD)
AddDecl(USD->getTargetDecl());
- else
- BailedOut = true;
break;
}
case TemplateName::DeducedTemplate:
@@ -170,7 +147,6 @@ void
TemplateArgumentHasher::AddDeclarationName(DeclarationName Name) {
case DeclarationName::ObjCZeroArgSelector:
case DeclarationName::ObjCOneArgSelector:
case DeclarationName::ObjCMultiArgSelector:
- BailedOut = true;
break;
case DeclarationName::CXXConstructorName:
case DeclarationName::CXXDestructorName:
@@ -197,7 +173,6 @@ void
TemplateArgumentHasher::AddDeclarationName(DeclarationName Name) {
void TemplateArgumentHasher::AddDecl(const Decl *D) {
const NamedDecl *ND = dyn_cast<NamedDecl>(D);
if (!ND) {
- BailedOut = true;
return;
}
@@ -221,7 +196,6 @@ void TemplateArgumentHasher::AddDecl(const Decl *D) {
void TemplateArgumentHasher::AddQualType(QualType T) {
if (T.isNull()) {
- BailedOut = true;
return;
}
SplitQualType split = T.split();
@@ -231,7 +205,6 @@ void TemplateArgumentHasher::AddQualType(QualType T) {
// Process a Type pointer. Add* methods call back into TemplateArgumentHasher
// while Visit* methods process the relevant parts of the Type.
-// Any unhandled type will make the hash computation bail out.
class TypeVisitorHelper : public TypeVisitor<TypeVisitorHelper> {
typedef TypeVisitor<TypeVisitorHelper> Inherited;
llvm::FoldingSetNodeID &ID;
@@ -263,9 +236,6 @@ class TypeVisitorHelper : public
TypeVisitor<TypeVisitorHelper> {
void Visit(const Type *T) { Inherited::Visit(T); }
- // Unhandled types. Bail out simply.
- void VisitType(const Type *T) { Hash.setBailedOut(); }
-
void VisitAdjustedType(const AdjustedType *T) {
AddQualType(T->getOriginalType());
}
>From 5b5e242027e18205240546f20738698118829500 Mon Sep 17 00:00:00 2001
From: Jonas Hahnfeld <jonas.hahnf...@cern.ch>
Date: Tue, 19 Aug 2025 14:25:13 +0200
Subject: [PATCH 5/5] [DO NOT MERGE] Add failing reproducer
---
clang/test/Modules/pr133057.cpp | 143 ++++++++++++++++++++++++++++++++
1 file changed, 143 insertions(+)
create mode 100644 clang/test/Modules/pr133057.cpp
diff --git a/clang/test/Modules/pr133057.cpp b/clang/test/Modules/pr133057.cpp
new file mode 100644
index 0000000000000..b273fc318058e
--- /dev/null
+++ b/clang/test/Modules/pr133057.cpp
@@ -0,0 +1,143 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -xc++ -std=c++20 -emit-module -fmodule-name=hf
-fno-cxx-modules -fmodules -fno-implicit-modules %t/CMO.cppmap -o %t/WI9.pcm
+// RUN: %clang_cc1 -xc++ -std=c++20 -emit-module -fmodule-name=g
-fno-cxx-modules -fmodules -fno-implicit-modules -fmodule-file=%t/WI9.pcm
%t/E6H.cppmap -o %t/4BK.pcm
+// RUN: %clang_cc1 -xc++ -std=c++20 -emit-module -fmodule-name=r
-fno-cxx-modules -fmodules -fno-implicit-modules -fmodule-file=%t/WI9.pcm
%t/HMT.cppmap -o %t/LUM.pcm
+// RUN: %clang_cc1 -xc++ -std=c++20 -emit-module -fmodule-name=q
-fno-cxx-modules -fmodules -fno-implicit-modules -fmodule-file=%t/LUM.pcm
-fmodule-file=%t/4BK.pcm %t/JOV.cppmap -o %t/9VX.pcm
+// RUN: %clang_cc1 -xc++ -std=c++20 -verify -fsyntax-only -fno-cxx-modules
-fmodules -fno-implicit-modules -fmodule-file=%t/9VX.pcm %t/XFD.cc
+
+//--- 2OT.h
+#include "LQ1.h"
+
+namespace ciy {
+namespace xqk {
+template <typename>
+class vum {
+ public:
+ using sc = std::C::wmd;
+ friend bool operator==(vum, vum);
+};
+template <typename>
+class me {
+ public:
+ using vbh = vum<me>;
+ using sc = std::C::vy<vbh>::sc;
+ template <typename db>
+ operator db() { return {}; }
+};
+} // namespace xqk
+template <typename vus>
+xqk::me<vus> uvo(std::C::wmd, vus);
+} // namespace ciy
+
+class ua {
+ std::C::wmd kij() {
+ ciy::uvo(kij(), '-');
+ return {};
+ }
+};
+
+//--- 9KF.h
+#include "LQ1.h"
+#include "2OT.h"
+namespace {
+void al(std::C::wmd lou) { std::C::jv<std::C::wmd> yt = ciy::uvo(lou, '/'); }
+} // namespace
+
+//--- CMO.cppmap
+module "hf" {
+header "LQ1.h"
+}
+
+
+//--- E6H.cppmap
+module "g" {
+export *
+header "2OT.h"
+}
+
+
+//--- HMT.cppmap
+module "r" {
+header "2OT.h"
+}
+
+
+//--- JOV.cppmap
+module "q" {
+header "9KF.h"
+}
+
+
+//--- LQ1.h
+namespace std {
+namespace C {
+template <class zd>
+struct vy : zd {};
+template <class ub>
+struct vy<ub*> {
+ typedef ub jz;
+};
+struct wmd {};
+template <class uo, class zt>
+void sk(uo k, zt gf) {
+ (void)(k != gf);
+}
+template <class uo>
+class fm {
+ public:
+ fm(uo);
+};
+template <class kj, class kju>
+bool operator==(kj, kju);
+template <class epn>
+void afm(epn) {
+ using yp = vy<epn>;
+ if (__is_trivially_copyable(yp)) {
+ sk(fm(epn()), nullptr);
+ }
+}
+template <class ub>
+class jv {
+ public:
+ constexpr void gq();
+ ub *nef;
+};
+template <class ub>
+constexpr void jv<ub>::gq() {
+ afm(nef);
+}
+} // namespace C
+} // namespace std
+namespace ciy {
+} // namespace ciy
+
+//--- XFD.cc
+// expected-no-diagnostics
+#include "LQ1.h"
+#include "2OT.h"
+class wiy {
+ public:
+ std::C::wmd eyb();
+};
+template <typename wpa>
+void i(wpa fg) {
+ std::C::jv<std::C::wmd> zs;
+ zs = ciy::uvo(fg.eyb(), '\n');
+}
+namespace ciy {
+namespace xqk {
+struct sbv;
+std::C::jv<sbv> ns() {
+ std::C::jv<sbv> ubs;
+ ubs.gq();
+ return ubs;
+}
+} // namespace xqk
+} // namespace ciy
+void s() {
+ wiy fg;
+ i(fg);
+}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
