[PATCH] D87853: [Sema] Update specialization iterator after template argument deduction.
Richard Smith - zygoloid via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Sep 17 14:42:14 PDT 2020
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
More information about the cfe-commits
mailing list