[PATCH] D112481: [Sema] fix nondeterminism in ASTContext::getDeducedTemplateSpecializationType
This revision was automatically updated to reflect the committed changes.
Closed by commit rGa075d6722283: [Sema] fix nondeterminism in
ASTContext::getDeducedTemplateSpecializationType (authored by weiwang).
Changed prior to commit:
https://reviews.llvm.org/D112481?vs=382375=388616#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112481/new/
https://reviews.llvm.org/D112481
Files:
clang/include/clang/AST/Type.h
clang/lib/AST/ASTContext.cpp
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -5676,6 +5676,9 @@
auto *DTST = new (*this, TypeAlignment)
DeducedTemplateSpecializationType(Template, DeducedType, IsDependent);
+ llvm::FoldingSetNodeID TempID;
+ DTST->Profile(TempID);
+ assert(ID == TempID && "ID does not match");
Types.push_back(DTST);
DeducedTemplateSpecializationTypes.InsertNode(DTST, InsertPos);
return QualType(DTST, 0);
Index: clang/include/clang/AST/Type.h
===
--- clang/include/clang/AST/Type.h
+++ clang/include/clang/AST/Type.h
@@ -5073,8 +5073,10 @@
static void Profile(llvm::FoldingSetNodeID , TemplateName Template,
QualType Deduced, bool IsDependent) {
Template.Profile(ID);
-ID.AddPointer(Deduced.getAsOpaquePtr());
-ID.AddBoolean(IsDependent);
+QualType CanonicalType =
+Deduced.isNull() ? Deduced : Deduced.getCanonicalType();
+ID.AddPointer(CanonicalType.getAsOpaquePtr());
+ID.AddBoolean(IsDependent || Template.isDependent());
}
static bool classof(const Type *T) {
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -5676,6 +5676,9 @@
auto *DTST = new (*this, TypeAlignment)
DeducedTemplateSpecializationType(Template, DeducedType, IsDependent);
+ llvm::FoldingSetNodeID TempID;
+ DTST->Profile(TempID);
+ assert(ID == TempID && "ID does not match");
Types.push_back(DTST);
DeducedTemplateSpecializationTypes.InsertNode(DTST, InsertPos);
return QualType(DTST, 0);
Index: clang/include/clang/AST/Type.h
===
--- clang/include/clang/AST/Type.h
+++ clang/include/clang/AST/Type.h
@@ -5073,8 +5073,10 @@
static void Profile(llvm::FoldingSetNodeID , TemplateName Template,
QualType Deduced, bool IsDependent) {
Template.Profile(ID);
-ID.AddPointer(Deduced.getAsOpaquePtr());
-ID.AddBoolean(IsDependent);
+QualType CanonicalType =
+Deduced.isNull() ? Deduced : Deduced.getCanonicalType();
+ID.AddPointer(CanonicalType.getAsOpaquePtr());
+ID.AddBoolean(IsDependent || Template.isDependent());
}
static bool classof(const Type *T) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D112481: [Sema] fix nondeterminism in ASTContext::getDeducedTemplateSpecializationType
bruno accepted this revision. bruno added a comment. This revision is now accepted and ready to land. This is good to go, sorry for the delay. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112481/new/ https://reviews.llvm.org/D112481 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D112481: [Sema] fix nondeterminism in ASTContext::getDeducedTemplateSpecializationType
sugak added a comment. ping @rsmith Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112481/new/ https://reviews.llvm.org/D112481 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D112481: [Sema] fix nondeterminism in ASTContext::getDeducedTemplateSpecializationType
weiwang added a comment. This issue has been blocking our internal module re-enablement for some time now, and we really appreciate any feedback. We also wonder if only `DeducedTemplateSpecializationType` is affected or it could also happen to other types. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112481/new/ https://reviews.llvm.org/D112481 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D112481: [Sema] fix nondeterminism in ASTContext::getDeducedTemplateSpecializationType
sugak updated this revision to Diff 382375.
sugak added a comment.
Updated following @bruno's suggestion, and fixed tests (thanks @weiwang)!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112481/new/
https://reviews.llvm.org/D112481
Files:
clang/include/clang/AST/Type.h
clang/lib/AST/ASTContext.cpp
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -5644,6 +5644,9 @@
auto *DTST = new (*this, TypeAlignment)
DeducedTemplateSpecializationType(Template, DeducedType, IsDependent);
+ llvm::FoldingSetNodeID TempID;
+ DTST->Profile(TempID);
+ assert(ID == TempID && "ID does not match");
Types.push_back(DTST);
if (InsertPos)
DeducedTemplateSpecializationTypes.InsertNode(DTST, InsertPos);
Index: clang/include/clang/AST/Type.h
===
--- clang/include/clang/AST/Type.h
+++ clang/include/clang/AST/Type.h
@@ -5071,8 +5071,10 @@
static void Profile(llvm::FoldingSetNodeID , TemplateName Template,
QualType Deduced, bool IsDependent) {
Template.Profile(ID);
-ID.AddPointer(Deduced.getAsOpaquePtr());
-ID.AddBoolean(IsDependent);
+QualType CanonicalType =
+Deduced.isNull() ? Deduced : Deduced.getCanonicalType();
+ID.AddPointer(CanonicalType.getAsOpaquePtr());
+ID.AddBoolean(IsDependent || Template.isDependent());
}
static bool classof(const Type *T) {
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -5644,6 +5644,9 @@
auto *DTST = new (*this, TypeAlignment)
DeducedTemplateSpecializationType(Template, DeducedType, IsDependent);
+ llvm::FoldingSetNodeID TempID;
+ DTST->Profile(TempID);
+ assert(ID == TempID && "ID does not match");
Types.push_back(DTST);
if (InsertPos)
DeducedTemplateSpecializationTypes.InsertNode(DTST, InsertPos);
Index: clang/include/clang/AST/Type.h
===
--- clang/include/clang/AST/Type.h
+++ clang/include/clang/AST/Type.h
@@ -5071,8 +5071,10 @@
static void Profile(llvm::FoldingSetNodeID , TemplateName Template,
QualType Deduced, bool IsDependent) {
Template.Profile(ID);
-ID.AddPointer(Deduced.getAsOpaquePtr());
-ID.AddBoolean(IsDependent);
+QualType CanonicalType =
+Deduced.isNull() ? Deduced : Deduced.getCanonicalType();
+ID.AddPointer(CanonicalType.getAsOpaquePtr());
+ID.AddBoolean(IsDependent || Template.isDependent());
}
static bool classof(const Type *T) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D112481: [Sema] fix nondeterminism in ASTContext::getDeducedTemplateSpecializationType
sugak added inline comments.
Comment at: clang/lib/AST/ASTContext.cpp:5640
+ DeducedTemplateSpecializationType::Profile(
+ ID, Template, DeducedType, IsDependent || Template.isDependent());
if (DeducedTemplateSpecializationType *DTST =
bruno wrote:
> Should this be done in the implementation (like done for the ctor)?
> ```
> static void Profile(llvm::FoldingSetNodeID , TemplateName Template,
> QualType Deduced, bool IsDependent) {
> Template.Profile(ID);
> ID.AddPointer(Deduced.getAsOpaquePtr());
> ID.AddBoolean(IsDependent || Template.isDependent());
> }
> ```
Thanks for the suggestion! Indeed that look better.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112481/new/
https://reviews.llvm.org/D112481
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D112481: [Sema] fix nondeterminism in ASTContext::getDeducedTemplateSpecializationType
bruno added a reviewer: bruno.
bruno added a comment.
Nice catch, thanks for working on this!
Comment at: clang/lib/AST/ASTContext.cpp:5640
+ DeducedTemplateSpecializationType::Profile(
+ ID, Template, DeducedType, IsDependent || Template.isDependent());
if (DeducedTemplateSpecializationType *DTST =
Should this be done in the implementation (like done for the ctor)?
```
static void Profile(llvm::FoldingSetNodeID , TemplateName Template,
QualType Deduced, bool IsDependent) {
Template.Profile(ID);
ID.AddPointer(Deduced.getAsOpaquePtr());
ID.AddBoolean(IsDependent || Template.isDependent());
}
```
Comment at: clang/lib/AST/ASTContext.cpp:5649
+ DTST->Profile(TempID);
+ assert(ID == TempID && "ID does not match");
Types.push_back(DTST);
Seems like this assertion is failing in some of the tests above!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112481/new/
https://reviews.llvm.org/D112481
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D112481: [Sema] fix nondeterminism in ASTContext::getDeducedTemplateSpecializationType
sugak updated this revision to Diff 382109. sugak added a comment. apply clang-format suggestion Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112481/new/ https://reviews.llvm.org/D112481 Files: clang/lib/AST/ASTContext.cpp Index: clang/lib/AST/ASTContext.cpp === --- clang/lib/AST/ASTContext.cpp +++ clang/lib/AST/ASTContext.cpp @@ -5636,14 +5636,17 @@ // Look in the folding set for an existing type. void *InsertPos = nullptr; llvm::FoldingSetNodeID ID; - DeducedTemplateSpecializationType::Profile(ID, Template, DeducedType, - IsDependent); + DeducedTemplateSpecializationType::Profile( + ID, Template, DeducedType, IsDependent || Template.isDependent()); if (DeducedTemplateSpecializationType *DTST = DeducedTemplateSpecializationTypes.FindNodeOrInsertPos(ID, InsertPos)) return QualType(DTST, 0); auto *DTST = new (*this, TypeAlignment) DeducedTemplateSpecializationType(Template, DeducedType, IsDependent); + llvm::FoldingSetNodeID TempID; + DTST->Profile(TempID); + assert(ID == TempID && "ID does not match"); Types.push_back(DTST); if (InsertPos) DeducedTemplateSpecializationTypes.InsertNode(DTST, InsertPos); Index: clang/lib/AST/ASTContext.cpp === --- clang/lib/AST/ASTContext.cpp +++ clang/lib/AST/ASTContext.cpp @@ -5636,14 +5636,17 @@ // Look in the folding set for an existing type. void *InsertPos = nullptr; llvm::FoldingSetNodeID ID; - DeducedTemplateSpecializationType::Profile(ID, Template, DeducedType, - IsDependent); + DeducedTemplateSpecializationType::Profile( + ID, Template, DeducedType, IsDependent || Template.isDependent()); if (DeducedTemplateSpecializationType *DTST = DeducedTemplateSpecializationTypes.FindNodeOrInsertPos(ID, InsertPos)) return QualType(DTST, 0); auto *DTST = new (*this, TypeAlignment) DeducedTemplateSpecializationType(Template, DeducedType, IsDependent); + llvm::FoldingSetNodeID TempID; + DTST->Profile(TempID); + assert(ID == TempID && "ID does not match"); Types.push_back(DTST); if (InsertPos) DeducedTemplateSpecializationTypes.InsertNode(DTST, InsertPos); ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D112481: [Sema] fix nondeterminism in ASTContext::getDeducedTemplateSpecializationType
sugak created this revision.
sugak added a reviewer: rsmith.
Herald added a subscriber: mgrang.
sugak requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
`DeducedTemplateSpecializationTypes` is a
`llvm::FoldingSet` [1], where
`FoldingSetNodeID` is based on the values: `TemplateName`, `QualType`,
`IsDeducedAsDependent`, those values are also used
`DeducedTemplateSpecializationType` constructor arguments.
The problem is that `FoldingSetNodeID` created by the static
`DeducedTemplateSpecializationType::Profile` may not be equal
to`FoldingSetNodeID` created by a member
`DeducedTemplateSpecializationType::Profile` of instance created with the same
`TemplateName`, `QualType`, `IsDeducedAsDependent`, which makes
`DeducedTemplateSpecializationTypes`.
Specifically, while `IsDeducedAsDependent` value is passes to the constructor,
`IsDependent()` method on the created instance may return a different value,
because `IsDependent` is not saved as is:
name=clang/include/clang/AST/Type.h
DeducedTemplateSpecializationType(TemplateName Template, QualType
DeducedAsType, bool IsDeducedAsDependent)
: DeducedType(DeducedTemplateSpecialization, DeducedAsType,
toTypeDependence(Template.getDependence()) | // <~ also
considers `TemplateName` parameter
(IsDeducedAsDependent ?
TypeDependence::DependentInstantiation : TypeDependence::None)),
For example, if an instance A with key `FoldingSetNodeID {A, B, false}` is
inserted. Then a key `FoldingSetNodeID {A, B, true}` is probed: if it happens
to correspond to the same bucket in `FoldingSet` as the first key, and
`A.Profile()` returns `FoldingSetNodeID {A, B, true}`, then we have a cache
hit. If the bucket for the second key is different from the first key, it's a
no hit, even if `A.Profile()` returns `FoldingSetNodeID {A, B, true}`. Since
`TemplateName`, `QualType` parameter values involve memory pointers, the query
result depend on allocator, and may differ from run to run.
When this is used as part of modules compilation, it may result in "module out
of date" errors, if imported modules are built on different machines.
1. https://llvm.org/docs/ProgrammersManual.html#llvm-adt-foldingset-h
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D112481
Files:
clang/lib/AST/ASTContext.cpp
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -5491,13 +5491,16 @@
void *InsertPos = nullptr;
llvm::FoldingSetNodeID ID;
DeducedTemplateSpecializationType::Profile(ID, Template, DeducedType,
- IsDependent);
+ IsDependent ||
Template.isDependent());
if (DeducedTemplateSpecializationType *DTST =
DeducedTemplateSpecializationTypes.FindNodeOrInsertPos(ID,
InsertPos))
return QualType(DTST, 0);
auto *DTST = new (*this, TypeAlignment)
DeducedTemplateSpecializationType(Template, DeducedType, IsDependent);
+ llvm::FoldingSetNodeID TempID;
+ DTST->Profile(TempID);
+ assert(ID == TempID && "ID does not match");
Types.push_back(DTST);
if (InsertPos)
DeducedTemplateSpecializationTypes.InsertNode(DTST, InsertPos);
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -5491,13 +5491,16 @@
void *InsertPos = nullptr;
llvm::FoldingSetNodeID ID;
DeducedTemplateSpecializationType::Profile(ID, Template, DeducedType,
- IsDependent);
+ IsDependent || Template.isDependent());
if (DeducedTemplateSpecializationType *DTST =
DeducedTemplateSpecializationTypes.FindNodeOrInsertPos(ID, InsertPos))
return QualType(DTST, 0);
auto *DTST = new (*this, TypeAlignment)
DeducedTemplateSpecializationType(Template, DeducedType, IsDependent);
+ llvm::FoldingSetNodeID TempID;
+ DTST->Profile(TempID);
+ assert(ID == TempID && "ID does not match");
Types.push_back(DTST);
if (InsertPos)
DeducedTemplateSpecializationTypes.InsertNode(DTST, InsertPos);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
