[PATCH] D133072: WIP: [clang] fix profiling of template arguments of template and declaration kind

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov created this revision.
Herald added projects: clang, libc++, All.
Herald added subscribers: libcxx-commits, cfe-commits.
Herald added a reviewer: libc++.
mizvekov requested review of this revision.

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 CheckTemplateArguments 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 <mizve...@gmail.com>


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D133072

Files:
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/TemplateBase.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/SemaTemplate/concepts.cpp
  libcxx/DELETE.ME


Index: clang/test/SemaTemplate/concepts.cpp
===================================================================
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -256,3 +256,9 @@
 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
Index: clang/lib/Sema/SemaTemplate.cpp
===================================================================
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -5643,7 +5643,8 @@
     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 @@
         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 @@
   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.
Index: clang/lib/AST/TemplateBase.cpp
===================================================================
--- clang/lib/AST/TemplateBase.cpp
+++ clang/lib/AST/TemplateBase.cpp
@@ -321,26 +321,15 @@
 
   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);
Index: clang/lib/AST/ASTContext.cpp
===================================================================
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -5116,7 +5116,9 @@
                                                    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 @@
         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();

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits