[PATCH] D155705: [clang] Fix specialization of non-templated member classes of class templates
Mariya Podchishchaeva via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Jul 20 02:29:11 PDT 2023
Fznamznon added a comment.
> Can you explain why the fix fixes the bug?
Sure, let me try. To be honest, I'm not very familiar with the code either, so I'll describe how I came to the fix. I'll be using added test `clang/test/SemaTemplate/gh61159.cpp` as example of failing code. The issue happens when instantiating `X<int>::impl::f`.
So, the original assertion reported by https://github.com/llvm/llvm-project/issues/61159 happens in a function `Sema::BuildExpressionFromIntegralTemplateArgument`, because it expects non-type integral argument, but receives type argument:
clang/lib/Sema/SemaTemplate.cpp
7955 Sema::BuildExpressionFromIntegralTemplateArgument(const TemplateArgument &Arg,
7956 SourceLocation Loc) {
7957 assert(Arg.getKind() == TemplateArgument::Integral &&
7958 "Operation is only valid for integral template arguments");
(gdb) p Arg.dump()
int // that is type argument `int` from outer specialization of struct X.
So, let's see where the argument comes from, for this I went two frames of back upper, to function `TemplateInstantiator::TransformTemplateParmRefExpr`:
clang/lib/Sema/SemaTemplateInstantiate.cpp
1789 TemplateArgument Arg = TemplateArgs(NTTP->getDepth(), NTTP->getPosition());
NTTP is a NonTypeTemplateParmDecl that represents template parameter ct of function f
(gdb) p NTTP->dump()
NonTypeTemplateParmDecl 0x14b76380 <t.cpp:8:14, col:18> col:18 referenced 'int' depth 0 index 0 ct
TemplateArgs is a MultiLevelTemplateArgumentList - data structure containing arguments for current instantiation
(gdb) p TemplateArgs.dump()
NumRetainedOuterLevels: 0
0: <int>
1: <17>
We see it has two arguments - "int" and "17", we should be processing "17" but instead end up with "int". Both have same indexes since there is only one argument possible, so they should have different depth. For "17" we have zero depth.
(gdb) p NTTP->getDepth()
$8 = 0
So, after that I went to the place where NonTypeTemplateParmDecl is created to see why it has depth "0" instead of "1".
(gdb) b NonTypeTemplateParmDecl::Create
(gdb) up several times to see where Depth parameters come from
and it is here:
clang/lib/Parse/ParseTemplate.cpp
130 if (ParseTemplateParameters(TemplateParamScopes,
131 CurTemplateDepthTracker.getDepth(),
132 TemplateParams, LAngleLoc, RAngleLoc)) {
133 // Skip until the semi-colon or a '}'.
134 SkipUntil(tok::r_brace, StopAtSemi | StopBeforeMatch);
135 TryConsumeToken(tok::semi);
136 return nullptr;
137 }
CurTemplateDepthTracker.getDepth() is increased only if template parameter list of the declaration being parsed is not empty, i.e. when declaration we have is not an explicit specialization:
139 ExprResult OptionalRequiresClauseConstraintER;
140 if (!TemplateParams.empty()) {
141 isSpecialization = false;
142 ++CurTemplateDepthTracker;
But this is exactly what we have! So we end up having zero depth for our NTTP.
I tried increasing depth unconditionally but it broke handful of tests and seems zero depth for cases like this is on purpose so that is not the solution. So I figured similar test case which doesn't crash:
template <>
struct X<int> {
struct impl {
template<int ct>
int f() { return ct; };
};
};
}
And went through the clang code that was failing for the original example:
7954 ExprResult
7955 Sema::BuildExpressionFromIntegralTemplateArgument(const TemplateArgument &Arg,
7956 SourceLocation Loc) {
7957 assert(Arg.getKind() == TemplateArgument::Integral &&
(gdb) p Arg.dump()
17 // correct!
Let's go see corresponding NTTP and MultiLevelTemplateArgumentList :
clang/lib/Sema/SemaTemplateInstantiate.cpp
1789 TemplateArgument Arg = TemplateArgs(NTTP->getDepth(), NTTP->getPosition());
(gdb) p NTTP->dump()
NonTypeTemplateParmDecl 0x14b761b8 <t1.cpp:8:11, col:15> col:15 referenced 'int' depth 0 index 0 ct // all same
(gdb) p TemplateArgs.dump()
NumRetainedOuterLevels: 0
0: <17>
TemplateArgs doesn't have "int" paramter at all so all is correct.
So, after that I went to see where `TemplateArgs` is formed. For this I went several stack frames and came to
clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
5075 MultiLevelTemplateArgumentList TemplateArgs = getTemplateInstantiationArgs(
5076 Function, /*Final=*/false, nullptr, false, PatternDecl);
So, what `getTemplateInstantiationArgs` does, it goes from the declaration we're instantiating `X<int>::impl::f` to the outer contexts gathering required template arguments, it does so in a loop:
334 while (!CurDecl->isFileContextDecl()) {
...
343 } else if (const auto *ClassTemplSpec =
344 dyn_cast<ClassTemplateSpecializationDecl>(CurDecl)) {
345 R = HandleClassTemplateSpec(ClassTemplSpec, Result,
346 SkipForSpecialization);
347 } else if (const auto *Function = dyn_cast<FunctionDecl>(CurDecl)) {
348 R = HandleFunction(Function, Result, Pattern, RelativeToPrimary,
349 ForConstraintInstantiation);
350 } else if (const auto *Rec = dyn_cast<CXXRecordDecl>(CurDecl)) {
351 R = HandleRecordDecl(Rec, Result, Context, ForConstraintInstantiation);
352 } else if (const auto *CSD =
353 dyn_cast<ImplicitConceptSpecializationDecl>(CurDecl)) {
354 R = HandleImplicitConceptSpecializationDecl(CSD, Result);
355 } else if (const auto *FTD = dyn_cast<FunctionTemplateDecl>(CurDecl)) {
356 R = HandleFunctionTemplateDecl(FTD, Result);
So there is a bunch of handlers for each type of declaration met, for non-failing example we go first through `HandleFunctionTemplateDecl` for `f` itself and add "17" to the resulting argument list:
(gdb) p Result.dump()
NumRetainedOuterLevels: 0
0: <17>
then on next step `CurDecl` is `impl`:
(gdb) p CurDecl->dump()
CXXRecordDecl 0x14b76468 <t1.cpp:9:3, line:12:3> line:9:10 referenced struct impl definition
We go to `HandleRecordDecl` and add nothing since there is no template arguments.
Next `CurDecl` is `X`:
(gdb) p CurDecl->dump()
ClassTemplateSpecializationDecl 0x14b761c8 <t1.cpp:7:1, line:13:1> line:8:8 struct X definition
We go to `HandleClassTemplateSpec` and add no arguments to the resulting list because what we have is explicit specialization of X:
SemaTemplateInstantiate.cpp
145 // Add template arguments from a class template instantiation.
146 Response
147 HandleClassTemplateSpec(const ClassTemplateSpecializationDecl *ClassTemplSpec,
148 MultiLevelTemplateArgumentList &Result,
149 bool SkipForSpecialization) {
150 if (!ClassTemplSpec->isClassScopeExplicitSpecialization()) {
151 // We're done when we hit an explicit specialization.
152 if (ClassTemplSpec->getSpecializationKind() == TSK_ExplicitSpecialization &&
153 !isa<ClassTemplatePartialSpecializationDecl>(ClassTemplSpec)) // THIS IS TRUE
154 return Response::Done(); // AND WE EXIT HERE WITHOUT ADDING "INT"
(gdb) p ClassTemplSpec->getSpecializationKind()
$8 = clang::TSK_ExplicitSpecialization
Ok, how the failing case is different? We end up in `HandleClassTemplateSpec` in the same way, but the difference is when you do
template <>
struct X<int>::impl {
instead of
template <>
struct X<int> {
struct impl {
specialization of X<int> becomes implicit:
(gdb) p ClassTemplSpec->getSpecializationKind()
$3 = clang::TSK_ImplicitInstantiation
And we don't exit, so adding "int" to resulting template argument list:
clang/lib/Sema/SemaTemplateInstantiate.cpp
156 if (!SkipForSpecialization)
157 Result.addOuterTemplateArguments(
158 const_cast<ClassTemplateSpecializationDecl *>(ClassTemplSpec),
159 ClassTemplSpec->getTemplateInstantiationArgs().asArray(),
160 /*Final=*/false);
(gdb) p Result.dump()
NumRetainedOuterLevels: 0
0: <int>
1: <17>
And I think, we shouldn't be adding "int" to the resulting list like we do in non-failing case. I don't think it brings any value to the instantiation, it just breaks the system. So I figured, we probably should exit somehow. And I noticed there is `CXXRecordDecl::getMemberSpecializationInfo` for the case of a non-template class member of a class template which we have in failing example. And what I understood from looking at it, `MemberSpecializationInfo` returned by it normally has `MSInfo->getTemplateSpecializationKind() = TSK_ExplicitSpecialization` if the outer template class was explicitly specialized somehow. So I inserted an early exit when we process `impl` struct, we don't process `X` for the failing case and the bug becomes fixed.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D155705/new/
https://reviews.llvm.org/D155705
More information about the cfe-commits
mailing list