Page MenuHomePhabricator

[SemaTemplate] Stop passing insertion position around during VarTemplate instantiation
ClosedPublic

Authored by bruno on Sep 17 2020, 12:54 PM.

Details

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.

Diff Detail

Event Timeline

hoy created this revision.Sep 17 2020, 12:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 17 2020, 12:54 PM
hoy requested review of this revision.Sep 17 2020, 12:54 PM
hoy added a comment.Sep 17 2020, 1:35 PM

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

hoy edited the summary of this revision. (Show Details)Sep 17 2020, 1:37 PM
hoy added reviewers: rsmith, bruno, wenlei.

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 commandeered this revision.Sep 21 2020, 11:34 PM
bruno edited reviewers, added: hoy; removed: bruno.

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

bruno updated this revision to Diff 293351.Sep 21 2020, 11:38 PM
bruno retitled this revision from [Sema] Update specialization iterator after template argument deduction. to [SemaTemplate] Stop passing insertion position around during VarTemplate instantiation.
bruno edited the summary of this revision. (Show Details)

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 updated this revision to Diff 293356.Sep 21 2020, 11:52 PM

Add context

rsmith accepted this revision.Tue, Oct 6, 2:11 PM

Thanks!

This revision is now accepted and ready to land.Tue, Oct 6, 2:11 PM