On Wed, 1 Jan 2025, A J Ryan Solutions Ltd wrote:
> Hi and happy new year, this patch is to fix a compiler seg-fault as
> encountered in the following example:
Hi, thanks for the patch! Your fix makes sense to me, and I believe it
also fixes the testcases from PR102626 and at least some of its related
PRs.
>
> template<class> struct Class1{};
>
> template<auto...Tn> class Class2;
> template<typename...V,Class1<V>...Un> class Class2<Un...>
> {
> public: void apply(){}
> };
>
> Class1<bool> class1_bool;
> Class1<char> class1_char;
>
> int main()
> {
> Class2<class1_bool,class1_char> class2;
> class2.apply();
> }
> where it ends up populating the V argument in the instantiated template at
> the class2 declaration with the unknown placeholder and the segfault is when
> it later tries to get the name for an incompletely
> resolved type.
Here's a more reduced example that's fixed by your patch:
template<auto> struct A { };
template<class... Ts, Ts... Vs>
void f(A<Vs>...);
int main() {
f(A<0>{}, A<1u>{}, A<2l>{}); // OK, Ts deduced to {int,unsigned,long}
}
A related PR that isn't and shouldn't be fixed by this patch is PR84796
where the type parameter pack is declared at a different level than
the non-type parameter pack, e.g.:
template<auto...>
struct A { };
template<class... Ts>
struct B {
template<Ts... Vs>
static void f(A<Vs...>);
};
int main() {
A<1, true, 'c'> a;
B<int, bool, char>::f(a);
}
Before your patch we would ICE on this valid testcase, after we now reject it
with a bogus error:
<stdin>: In function ‘int main()’:
<stdin>:12:24: error: no matching function for call to ‘B<int, bool,
char>::f(A<1, true, 'c'>&)’
<stdin>:12:24: note: there is 1 candidate
<stdin>:7:15: note: candidate 1: ‘template<Ts ...Vs> static void B<Ts>::f(A<Vs
...>) [with Ts ...Vs = {Vs ...}; Ts = {int, bool, char}]’
<stdin>:7:15: note: template argument deduction/substitution failed:
<stdin>:12:24: note: ‘Vs’ is not equivalent to ‘1’
Which is no worse than ICEing, I suppose :)
>
> The incorrect processing happens in unify_pack_expansion where it is calling
> unify_one_argument
> here(https://gcc.gnu.org/git?p=gcc.git;a=blob;f=gcc/cp/pt.cc;h=0ffa0b53e26a87df3ed2c7202446c68d2800db64;hb=HEAD#l24718)
> for each argument of the argument pack and recursively unifying the inner
> arguments. In
> this example the inner argument against V (bool/char) is set in targs however
> it is not seen as a pack argument in unify_pack_expansion because in the
> unfixed code it does not reach and link this associated
> pack and later on when processing the collected arguments to substitute it in
> in instantiate_class_template the argument has not been set as an argument
> pack in targs and it doesn't match the the template
> specialisation parameter which is a parameter pack and it falls back to the
> unknown placeholder.
> The parameter packs are originally linked in make_pack_expansion
> here(https://gcc.gnu.org/git?p=gcc.git;a=blob;f=gcc/cp/pt.cc;h=0ffa0b53e26a87df3ed2c7202446c68d2800db64;hb=HEAD#l4246)
> and the change is in
> this tree walk so that in the example above V is chained to Un and after
> calling unify_one_argument it loops over both packs and extracts their
> associated value set in targs and puts it into it's pack
> argument collection.
>
> Regards
> Adam Ryan
>
>
> Subject: [PATCH] Add tree walk case to obtain A pack in
> <typename...A,Class<A>...B>.
>
> For non-type parameter packs when unifying the arguments in
> unify_pack_expansion
> it iterates over the associated packs of a param so that when it recursivly
> unifies the param with the arguments it knows which targs have been populated
> with parameter pack arguments that it can then collect up. This change adds a
> tree walk so that in the example above it reaches ...A and adds it to the
> associated packs for ...B and therefore knows it will have been set in targs
> in unify_pack_expansion and processes it as per other pack arguments.
>
> PR gcc/118265
PR c++/118265 rather
A ChangeLog entry is missing from the commit message as per
https://gcc.gnu.org/contribute.html#patches
Here's an example ChangeLog we can use for this patch:
gcc/cp/ChangeLog:
* pt.cc (find_parameter_packs_r) <case TEMPLATE_PARM_INDEX>:
Walk into the type of a parameter pack.
>
> Signed-off-by: Adam J Ryan <gcc.gnu....@ajryansolutions.co.uk>
> ---
> gcc/cp/pt.cc | 5 +++++
> gcc/testsuite/g++.dg/pr118265.C | 17 +++++++++++++++++
> 2 files changed, 22 insertions(+)
> create mode 100644 gcc/testsuite/g++.dg/pr118265.C
>
> diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> index 0ffa0b53e26..22f5d4b1875 100644
> --- a/gcc/cp/pt.cc
> +++ b/gcc/cp/pt.cc
> @@ -4012,6 +4012,11 @@ find_parameter_packs_r (tree *tp, int *walk_subtrees,
> void* data)
> &find_parameter_packs_r, ppd, ppd->visited);
> return NULL_TREE;
>
> + case TEMPLATE_PARM_INDEX:
> + if (parameter_pack_p)
> + WALK_SUBTREE ( TREE_TYPE(t) );
There should be a space before the ( starting a function call not
after (as per the GNU coding style), so this should be
"WALK_SUBTREE (TREE_TYPE (t));"
> + return NULL_TREE;
> +
> case DECL_EXPR:
> {
> tree decl = DECL_EXPR_DECL (t);
> diff --git a/gcc/testsuite/g++.dg/pr118265.C b/gcc/testsuite/g++.dg/pr118265.C
> new file mode 100644
> index 00000000000..00bda1b5b48
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/pr118265.C
> @@ -0,0 +1,17 @@
> +// { dg-do compile { target c++20 } }
> +template<class> struct Class1{};
> +
> +template<auto...Tn> class Class2;
> +template<typename...V,Class1<V>...Un> class Class2<Un...>
> +{
> + public: void apply(){}
> +};
> +
> +Class1<bool> class1_bool;
> +Class1<char> class1_char;
> +
> +int main()
> +{
> + Class2<class1_bool,class1_char> class2;
> + class2.apply();
> +}
> \ No newline at end of file
> --
> 2.43.0
>
