[SemaTemplate] Stop passing insertion position around during VarTemplate instantiation

[bruno]

Summary

They can get stale at use time because of updates from other recursive specializations. Instead, rely on the existence of previous declarations to add the specialization.

[hoy]

The bug is exposed by instantiating a large variadic template. I haven't managed to get down to a decent size of regression test.

[rsmith]

Thanks, this is pretty subtle. Here's a small C++20 testcase that invalidates InsertPos while matching partial specializations:

template<typename T, int N> constexpr bool v = true;
template<int N> requires v<int, N-1> constexpr bool v<int, N> = true;
template<> constexpr bool v<int, 0> = true;
int k = v<int, 500>;

clang/lib/Sema/SemaTemplate.cpp

4587

All the handling of InsertPos in BuildVarTemplateInstantiation and below looks wrong to me, with the same bug that this code had. We end up in VisitVarTemplateSpecializationDecl which does this:

// Do substitution on the type of the declaration
TypeSourceInfo *DI =
    SemaRef.SubstType(D->getTypeSourceInfo(), TemplateArgs,
                      D->getTypeSpecStartLoc(), D->getDeclName());
if (!DI)
  return nullptr;

if (DI->getType()->isFunctionType()) {
  SemaRef.Diag(D->getLocation(), diag::err_variable_instantiates_to_function)
      << D->isStaticDataMember() << DI->getType();
  return nullptr;
}

// Build the instantiated declaration
VarTemplateSpecializationDecl *Var = VarTemplateSpecializationDecl::Create(
    SemaRef.Context, Owner, D->getInnerLocStart(), D->getLocation(),
    VarTemplate, DI->getType(), DI, D->getStorageClass(), Converted);
Var->setTemplateArgsInfo(TemplateArgsInfo);
if (InsertPos)
  VarTemplate->AddSpecialization(Var, InsertPos);

But the call to SubstType can absolutely invalidate InsertPos; here's a test case for a crash caused by that:

template<typename T, int N> T v;
template<int N> decltype(v<int, N-1>) v<int, N>;
template<> int v<int, 0>;
int k = v<int, 500>;

So I think we should remove the InsertPos parameter from this entire cluster of functions. No use of it is correct.

With InsertPos removed, we still need to know whether to call AddSpecialization or not. We should in principle do that if and only if !PrevDecl, but the call to VisitVarTemplateSpecializationDecl in InstantiateVariableDefinition doesn't pass in a PrevDecl even though it has one (OldVar). I wonder if we can change InstantiateVariableDefinition to pass OldVar in as the PrevDecl and remove its call to MergeVarDecl and probably also its call to InstantiateVariableInitializer -- since BuildVariableInstantiation should then do all of that for us.

[bruno]

I'm taking this over from Hongtao (w/ his blessing :)

[bruno]

Hi Richard. Thanks for the quick feedback and for the testcase!

The updated patch implements your suggestions. BuildVariableInstantiation seems to cover for MergeVarDecl however it doesn't seem to be enough for InstantiateVariableInitializer (breaks several tests) most likely because InstantiatingSpecFromTemplate prevents it to be called in BuildVariableInstantiation.

[bruno]

Add context

[bruno]

Ping!

[bruno]

Ping!

[rsmith]

Thanks!

clang/include/clang/Sema/Sema.h

void InstantiateFunctionDefinition(SourceLocation PointOfInstantiation,
                                      bool Recursive = false,
                                      bool DefinitionRequired = false,
                                      bool AtEndOfTU = false);
   VarTemplateSpecializationDecl *BuildVarTemplateInstantiation(
       VarTemplateDecl *VarTemplate, VarDecl *FromVar,
       const TemplateArgumentList &TemplateArgList,
       const TemplateArgumentListInfo &TemplateArgsInfo,
       SmallVectorImpl<TemplateArgument> &Converted,
-      SourceLocation PointOfInstantiation, void *InsertPos,
+      SourceLocation PointOfInstantiation,
       LateInstantiatedAttrVec *LateAttrs = nullptr,
       LocalInstantiationScope *StartingScope = nullptr);
   VarTemplateSpecializationDecl *CompleteVarTemplateSpecializationDecl(
       VarTemplateSpecializationDecl *VarSpec, VarDecl *PatternDecl,
       const MultiLevelTemplateArgumentList &TemplateArgs);
   void
   BuildVariableInstantiation(VarDecl *NewVar, VarDecl *OldVar,
                              const MultiLevelTemplateArgumentList &TemplateArgs,

clang/include/clang/Sema/Template.h

TemplateParameterList *
       SubstTemplateParams(TemplateParameterList *List);
 
     bool SubstQualifier(const DeclaratorDecl *OldDecl,
                         DeclaratorDecl *NewDecl);
     bool SubstQualifier(const TagDecl *OldDecl,
                         TagDecl *NewDecl);
 
     Decl *VisitVarTemplateSpecializationDecl(
-        VarTemplateDecl *VarTemplate, VarDecl *FromVar, void *InsertPos,
+        VarTemplateDecl *VarTemplate, VarDecl *FromVar,
         const TemplateArgumentListInfo &TemplateArgsInfo,
         ArrayRef<TemplateArgument> Converted,
         VarTemplateSpecializationDecl *PrevDecl = nullptr);
 
     Decl *InstantiateTypedefNameDecl(TypedefNameDecl *D, bool IsTypeAlias);
     ClassTemplatePartialSpecializationDecl *
     InstantiateClassTemplatePartialSpecialization(
                                               ClassTemplateDecl *ClassTemplate,

clang/lib/Sema/SemaTemplate.cpp

Sema::CheckVarTemplateId(VarTemplateDecl *Template, SourceLocation TemplateLoc,
   }
 
   // 2. Create the canonical declaration.
   // Note that we do not instantiate a definition until we see an odr-use
   // in DoMarkVarDeclReferenced().
   // FIXME: LateAttrs et al.?
   VarTemplateSpecializationDecl *Decl = BuildVarTemplateInstantiation(
       Template, InstantiationPattern, *InstantiationArgs, TemplateArgs,
-      Converted, TemplateNameLoc, InsertPos /*, LateAttrs, StartingScope*/);
+      Converted, TemplateNameLoc /*, LateAttrs, StartingScope*/);
   if (!Decl)
     return true;
 
   if (AmbiguousPartialSpec) {
     // Partial ordering did not produce a clear winner. Complain.
     Decl->setInvalidDecl();
     Diag(PointOfInstantiation, diag::err_partial_spec_ordering_ambiguous)
         << Decl;

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

Decl *TemplateDeclInstantiator::VisitVarTemplateSpecializationDecl(
   bool Ignored;
   if (PrevDecl && SemaRef.CheckSpecializationInstantiationRedecl(
                       D->getLocation(), D->getSpecializationKind(), PrevDecl,
                       PrevDecl->getSpecializationKind(),
                       PrevDecl->getPointOfInstantiation(), Ignored))
     return nullptr;
 
   return VisitVarTemplateSpecializationDecl(
-      InstVarTemplate, D, InsertPos, VarTemplateArgsInfo, Converted, PrevDecl);
+      InstVarTemplate, D, VarTemplateArgsInfo, Converted, PrevDecl);
 }
 
 Decl *TemplateDeclInstantiator::VisitVarTemplateSpecializationDecl(
-    VarTemplateDecl *VarTemplate, VarDecl *D, void *InsertPos,
+    VarTemplateDecl *VarTemplate, VarDecl *D,
     const TemplateArgumentListInfo &TemplateArgsInfo,
     ArrayRef<TemplateArgument> Converted,
     VarTemplateSpecializationDecl *PrevDecl) {
 
   // Do substitution on the type of the declaration
   TypeSourceInfo *DI =
       SemaRef.SubstType(D->getTypeSourceInfo(), TemplateArgs,
                         D->getTypeSpecStartLoc(), D->getDeclName());
   if (!DI)
     return nullptr;
 
   if (DI->getType()->isFunctionType()) {
     SemaRef.Diag(D->getLocation(), diag::err_variable_instantiates_to_function)
         << D->isStaticDataMember() << DI->getType();
     return nullptr;
   }
 
   // Build the instantiated declaration
   VarTemplateSpecializationDecl *Var = VarTemplateSpecializationDecl::Create(
       SemaRef.Context, Owner, D->getInnerLocStart(), D->getLocation(),
       VarTemplate, DI->getType(), DI, D->getStorageClass(), Converted);
   Var->setTemplateArgsInfo(TemplateArgsInfo);
-  if (InsertPos)
+  if (!PrevDecl) {
+    void *InsertPos = nullptr;
+    VarTemplate->findSpecialization(Converted, InsertPos);
     VarTemplate->AddSpecialization(Var, InsertPos);
+  }
 
   if (SemaRef.getLangOpts().OpenCL)
     SemaRef.deduceOpenCLAddressSpace(Var);
 
   // Substitute the nested name specifier, if any.
   if (SubstQualifier(D, Var))
     return nullptr;
 
void Sema::InstantiateFunctionDefinition(SourceLocation PointOfInstantiation,
   GlobalInstantiations.perform();
 }
 
 VarTemplateSpecializationDecl *Sema::BuildVarTemplateInstantiation(
     VarTemplateDecl *VarTemplate, VarDecl *FromVar,
     const TemplateArgumentList &TemplateArgList,
     const TemplateArgumentListInfo &TemplateArgsInfo,
     SmallVectorImpl<TemplateArgument> &Converted,
-    SourceLocation PointOfInstantiation, void *InsertPos,
+    SourceLocation PointOfInstantiation,
     LateInstantiatedAttrVec *LateAttrs,
     LocalInstantiationScope *StartingScope) {
   if (FromVar->isInvalidDecl())
     return nullptr;
 
   InstantiatingTemplate Inst(*this, PointOfInstantiation, FromVar);
   if (Inst.isInvalid())
     return nullptr;
VarTemplateSpecializationDecl *Sema::BuildVarTemplateInstantiation(
   MultiLevelTemplateArgumentList MultiLevelList(TemplateArgList);
   TemplateDeclInstantiator Instantiator(*this, FromVar->getDeclContext(),
                                         MultiLevelList);
 
   // TODO: Set LateAttrs and StartingScope ...
 
   return cast_or_null<VarTemplateSpecializationDecl>(
       Instantiator.VisitVarTemplateSpecializationDecl(
-          VarTemplate, FromVar, InsertPos, TemplateArgsInfo, Converted));
+          VarTemplate, FromVar, TemplateArgsInfo, Converted));
 }
 
 /// Instantiates a variable template specialization by completing it
 /// with appropriate type information and initializer.
 VarTemplateSpecializationDecl *Sema::CompleteVarTemplateSpecializationDecl(
     VarTemplateSpecializationDecl *VarSpec, VarDecl *PatternDecl,
     const MultiLevelTemplateArgumentList &TemplateArgs) {
   assert(PatternDecl->isThisDeclarationADefinition() &&
void Sema::InstantiateVariableDefinition(SourceLocation PointOfInstantiation,
   } else if (Var->isStaticDataMember() &&
              Var->getLexicalDeclContext()->isRecord()) {
     // We need to instantiate the definition of a static data member template,
     // and all we have is the in-class declaration of it. Instantiate a separate
     // declaration of the definition.
     TemplateDeclInstantiator Instantiator(*this, Var->getDeclContext(),
                                           TemplateArgs);
     Var = cast_or_null<VarDecl>(Instantiator.VisitVarTemplateSpecializationDecl(
-        VarSpec->getSpecializedTemplate(), Def, nullptr,
-        VarSpec->getTemplateArgsInfo(), VarSpec->getTemplateArgs().asArray()));
+        VarSpec->getSpecializedTemplate(), Def, VarSpec->getTemplateArgsInfo(),
+        VarSpec->getTemplateArgs().asArray(), VarSpec));
     if (Var) {
       llvm::PointerUnion<VarTemplateDecl *,
                          VarTemplatePartialSpecializationDecl *> PatternPtr =
           VarSpec->getSpecializedTemplateOrPartial();
       if (VarTemplatePartialSpecializationDecl *Partial =
           PatternPtr.dyn_cast<VarTemplatePartialSpecializationDecl *>())
         cast<VarTemplateSpecializationDecl>(Var)->setInstantiationOf(
             Partial, &VarSpec->getTemplateInstantiationArgs());
 
-      // Merge the definition with the declaration.
-      LookupResult R(*this, Var->getDeclName(), Var->getLocation(),
-                     LookupOrdinaryName, forRedeclarationInCurContext());
-      R.addDecl(OldVar);
-      MergeVarDecl(Var, R);
-
       // Attach the initializer.
       InstantiateVariableInitializer(Var, Def, TemplateArgs);
     }
   } else
     // Complete the existing variable's definition with an appropriately
     // substituted type and initializer.
     Var = CompleteVarTemplateSpecializationDecl(VarSpec, Def, TemplateArgs);
 

clang/test/SemaTemplate/instantiate-var-template.cpp

namespace InstantiationDependent {
   }
 }
 
 namespace PR24483 {
   template<typename> struct A;
   template<typename... T> A<T...> models;
   template<> struct B models<>; // expected-error {{incomplete type 'struct B'}} expected-note {{forward declaration}}
 }
+
+namespace InvalidInsertPos {
+  template<typename T, int N> T v;
+  template<int N> decltype(v<int, N-1>) v<int, N>;
+  template<> int v<int, 0>;
+  int k = v<int, 500>;
+}