rsmith added a comment.
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>;
================
Comment at: clang/lib/Sema/SemaTemplate.cpp:4485
Template, InstantiationPattern, *InstantiationArgs, TemplateArgs,
Converted, TemplateNameLoc, InsertPos /*, LateAttrs, StartingScope*/);
if (!Decl)
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87853/new/
https://reviews.llvm.org/D87853
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
