[clang] Do not substitute parameter pack while retaining the pack expansion #108197
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Utkarsh Saxena (usx95) ChangesConsider when Input[I] is a VarDecl with parameter pack. We would have already expanded the pack before the code change in the loop Now in This leads to use of an invalid pack substitution index in This change set There are other instances of Full diff: https://github.com/llvm/llvm-project/pull/108197.diff 1 Files Affected:
diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h
index 0daf620b4123e4..0de43d2127b12f 100644
--- a/clang/lib/Sema/TreeTransform.h
+++ b/clang/lib/Sema/TreeTransform.h
@@ -4361,7 +4361,8 @@ bool TreeTransform<Derived>::TransformExprs(Expr *const *Inputs,
// forgetting the partially-substituted parameter pack.
if (RetainExpansion) {
ForgetPartiallySubstitutedPackRAII Forget(getDerived());
-
+ // Simple transform producing another pack expansion.
+ Sema::ArgumentPackSubstitutionIndexRAII SubstIndex(getSema(), -1);
ExprResult Out = getDerived().TransformExpr(Pattern);
if (Out.isInvalid())
return true;
|
|
Other than the test, the idea of the change does seem correct, here's the relevant comment from /// The current index into pack expansion arguments that will be
/// used for substitution of parameter packs.
///
/// The pack expansion index will be -1 to indicate that parameter packs
/// should be instantiated as themselves. Otherwise, the index specifies
/// which argument within the parameter pack will be used for substitution.
int ArgumentPackSubstitutionIndex;It looks clear that when Maybe there are certain intricacies I am not getting, but I believe your direction to put this into cc @zygoloid @shafik @cor3ntin in case they want to chime in. |
|
I have spent some time poking at the code and looking at the debugger and came up with a smaller repro, see https://gcc.godbolt.org/z/6ccPPd6hz: int bar(...);
template <int> struct Int {};
template <class ...T>
constexpr auto foo(T... x) -> decltype(bar(T(x)...)) { return 1; }
template <class ...T>
constexpr auto baz(Int<foo<T>(T())>... x) -> int { return 1; }
static_assert(baz<Int<1>, Int<2>, Int<3>>(Int<1>(), Int<2>(), Int<3>(), Int<4>()) == 1);I hope this helps. |
|
Thanks. I have moved this to |
|
Thanks for the review. Landing now. For other reviewers, feel free to drop more comments, and I would be happy to address in a followup. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/154/builds/4277 Here is the relevant piece of the build log for the reference |
Consider when
Input[I]is aVarDeclwith parameter pack. We would have already expanded the pack before the code change in the loopfor (unsigned I = 0; I != *NumExpansions; ++I) {.Now in
if (RetainExpansion) {, without this change, we continue to substitute the pack in the pattern even when we do not have meaningfulArgumentPackSubstitutionIndexset.This leads to use of an invalid pack substitution index in
TemplateInstantiator::TransformFunctionParmPackRefExprinTransformedDecl = (*Pack)[getSema().ArgumentPackSubstitutionIndex];This change sets
ArgumentPackSubstitutionIndexto-1while retaining expansion to instructTransformFunctionParmPackRefExprto buildFunctionParmPackExprinstead of substituting the param pack.There are other instances of
RetainExpansionand IIUC, they should also unset theArgumentPackSubstitutionIndex. It would be great if someone can verify my understanding. If this is correct then we could instead have aArgumentPackSubstitutionIndexRAIIas part ofForgetPartiallySubstitutedPackRAII.EDIT: I have moved this to
ForgetPartiallySubstitutedPackRAII.Fixes #63819
Fixes #107560