[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

4485

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/lib/Sema/SemaTemplate.cpp

if (Matched.size() >= 1) {
     //      from the primary template.
     // InstantiationPattern = Template->getTemplatedDecl();
   }
 
   // 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(
+  // Search for the variable template specialization declaration that
+  // corresponds to these arguments again to get an up-to-date InsertPos.
+  // The template argument deduction in between may update the underlying
+  // specialization container and invalidate previous iterators.
+  VarTemplateSpecializationDecl *Decl =
+      Template->findSpecialization(Converted, InsertPos);
+  // Assert we should not have a variable template specialization, otherwise we
+  // should have returned early.
+  assert(!Decl);
+  Decl = BuildVarTemplateInstantiation(
       Template, InstantiationPattern, *InstantiationArgs, TemplateArgs,
       Converted, TemplateNameLoc, InsertPos /*, 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;