[clang] acb767f - [clang] fix profiling of template arguments of template and declaration kind

[Matheus Izvekov via cfe-commits]

Author: Matheus Izvekov
Date: 2022-09-06T18:27:39+02:00
New Revision: acb767f5cda59302aa9100afcf6133d66779d42c

URL: https://github.com/llvm/llvm-project/commit/acb767f5cda59302aa9100afcf6133d66779d42c
DIFF: https://github.com/llvm/llvm-project/commit/acb767f5cda59302aa9100afcf6133d66779d42c.diff

LOG: [clang] fix profiling of template arguments of template and declaration kind

Template arguments of template and declaration kind were being profiled
only by their canonical properties, which would cause incorrect
uniquing of constrained AutoTypes, leading to a crash in some cases.

This exposed some places in CheckTemplateArgumentList where non-canonical
arguments where being pushed into the resulting converted list.

We also throw in some asserts to catch early and explain the crashes.

Note that the fix for the 'declaration' kind is untestable at this point,
because there should be no cases right now in the AST where we try
to unique a non-canonical converted template argument.

This fixes GH55567.

Signed-off-by: Matheus Izvekov <mizvekov at gmail.com>

Differential Revision: https://reviews.llvm.org/D133072

Added: 
    

Modified: 
    clang/lib/AST/ASTContext.cpp
    clang/lib/AST/TemplateBase.cpp
    clang/lib/Sema/SemaTemplate.cpp
    clang/test/SemaTemplate/concepts.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index 20fcc8fea4b79..28f4e19893326 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -5116,7 +5116,9 @@ ASTContext::getDependentTemplateSpecializationType(
                                                    CanonArgs);
 
     // Find the insert position again.
-    DependentTemplateSpecializationTypes.FindNodeOrInsertPos(ID, InsertPos);
+    [[maybe_unused]] auto *Nothing =
+        DependentTemplateSpecializationTypes.FindNodeOrInsertPos(ID, InsertPos);
+    assert(!Nothing && "canonical type broken");
   }
 
   void *Mem = Allocate((sizeof(DependentTemplateSpecializationType) +
@@ -5731,7 +5733,9 @@ QualType ASTContext::getAutoTypeInternal(
         Canon = getAutoTypeInternal(QualType(), Keyword, IsDependent, IsPack,
                                     TypeConstraintConcept, CanonArgs, true);
         // Find the insert position again.
-        AutoTypes.FindNodeOrInsertPos(ID, InsertPos);
+        [[maybe_unused]] auto *Nothing =
+            AutoTypes.FindNodeOrInsertPos(ID, InsertPos);
+        assert(!Nothing && "canonical type broken");
       }
     } else {
       Canon = DeducedType.getCanonicalType();

diff  --git a/clang/lib/AST/TemplateBase.cpp b/clang/lib/AST/TemplateBase.cpp
index e0f5916a9a0b7..6d9ab2b8ca718 100644
--- a/clang/lib/AST/TemplateBase.cpp
+++ b/clang/lib/AST/TemplateBase.cpp
@@ -321,26 +321,15 @@ void TemplateArgument::Profile(llvm::FoldingSetNodeID &ID,
 
   case Declaration:
     getParamTypeForDecl().Profile(ID);
-    ID.AddPointer(getAsDecl()? getAsDecl()->getCanonicalDecl() : nullptr);
+    ID.AddPointer(getAsDecl());
     break;
 
+  case TemplateExpansion:
+    ID.AddInteger(TemplateArg.NumExpansions);
+    LLVM_FALLTHROUGH;
   case Template:
-  case TemplateExpansion: {
-    TemplateName Template = getAsTemplateOrTemplatePattern();
-    if (TemplateTemplateParmDecl *TTP
-          = dyn_cast_or_null<TemplateTemplateParmDecl>(
-                                                Template.getAsTemplateDecl())) {
-      ID.AddBoolean(true);
-      ID.AddInteger(TTP->getDepth());
-      ID.AddInteger(TTP->getPosition());
-      ID.AddBoolean(TTP->isParameterPack());
-    } else {
-      ID.AddBoolean(false);
-      ID.AddPointer(Context.getCanonicalTemplateName(Template)
-                                                          .getAsVoidPointer());
-    }
+    getAsTemplateOrTemplatePattern().Profile(ID);
     break;
-  }
 
   case Integral:
     getAsIntegral().Profile(ID);

diff  --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index b06612d02797b..e9076bb6633af 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -5643,7 +5643,8 @@ bool Sema::CheckTemplateArgument(NamedDecl *Param,
     if (CheckTemplateTemplateArgument(TempParm, Params, Arg))
       return true;
 
-    Converted.push_back(Arg.getArgument());
+    Converted.push_back(
+        Context.getCanonicalTemplateArgument(Arg.getArgument()));
     break;
 
   case TemplateArgument::Expression:
@@ -5813,13 +5814,14 @@ bool Sema::CheckTemplateArgumentList(
         if (!ArgumentPack.empty()) {
           // If we were part way through filling in an expanded parameter pack,
           // fall back to just producing individual arguments.
-          Converted.insert(Converted.end(),
-                           ArgumentPack.begin(), ArgumentPack.end());
+          for (const TemplateArgument &I : ArgumentPack)
+            Converted.push_back(Context.getCanonicalTemplateArgument(I));
           ArgumentPack.clear();
         }
 
         while (ArgIdx < NumArgs) {
-          Converted.push_back(NewArgs[ArgIdx].getArgument());
+          Converted.push_back(Context.getCanonicalTemplateArgument(
+              NewArgs[ArgIdx].getArgument()));
           ++ArgIdx;
         }
 
@@ -5952,7 +5954,8 @@ bool Sema::CheckTemplateArgumentList(
   if (ArgIdx < NumArgs && CurrentInstantiationScope &&
       CurrentInstantiationScope->getPartiallySubstitutedPack()) {
     while (ArgIdx < NumArgs && NewArgs[ArgIdx].getArgument().isPackExpansion())
-      Converted.push_back(NewArgs[ArgIdx++].getArgument());
+      Converted.push_back(Context.getCanonicalTemplateArgument(
+          NewArgs[ArgIdx++].getArgument()));
   }
 
   // If we have any leftover arguments, then there were too many arguments.

diff  --git a/clang/test/SemaTemplate/concepts.cpp b/clang/test/SemaTemplate/concepts.cpp
index 23c79421bb6f8..332eed56c5634 100644
--- a/clang/test/SemaTemplate/concepts.cpp
+++ b/clang/test/SemaTemplate/concepts.cpp
@@ -256,3 +256,9 @@ C auto **j1 = g();   // expected-error {{deduced type 'int' does not satisfy 'C'
 C auto **&j2 = g();  // expected-error {{deduced type 'int' does not satisfy 'C'}}
 C auto **&&j3 = g(); // expected-error {{deduced type 'int' does not satisfy 'C'}}
 }
+
+namespace GH55567 {
+template<class, template <class> class> concept C = true;
+template <class> struct S {};
+void f(C<GH55567::S> auto);
+} // namespace GH55567