gcc: Add tree walk case to reach A pack from B in <typename...A,Class<A>...B>. [PR118265]

Hi and happy new year, this patch is to fix a compiler seg-fault as encountered in the following example:

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.

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

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.org@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
>

On 1/27/25 1:26 PM, Patrick Palka wrote:
> 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.

Yes, thanks!

In addition to Patrick's comments, all of which I agree with, the 
subject line should start with "c++" rather than "gcc".  We also leave 
out a closing period.

The subject line, and the rest of the commit message, must have lines no 
longer than 76 characters.

For the subject line specifically, under 70 characters is preferable, 
for the sake of git log --oneline.

Also, the testcase should not go in the main g++.dg/ directory, but a 
subdirectory, with a filename indicating what it tests.  Perhaps 
cpp1z/variadic-nontype1.C

>> 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.org@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

On Monday, 27 January 2025 at 19:48, Jason Merrill - jason at redhat.com <jason@redhat.com> wrote:
>
> On 1/27/25 1:26 PM, Patrick Palka wrote:
>
> > 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.
>
>
> Yes, thanks!
>
> In addition to Patrick's comments, all of which I agree with, the
> subject line should start with "c++" rather than "gcc". We also leave
> out a closing period.
>
> The subject line, and the rest of the commit message, must have lines no
> longer than 76 characters.
>
> For the subject line specifically, under 70 characters is preferable,
> for the sake of git log --oneline.
>
> Also, the testcase should not go in the main g++.dg/ directory, but a
> subdirectory, with a filename indicating what it tests. Perhaps
> cpp1z/variadic-nontype1.C
>
> > > 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 :)

It's interesting you've raised this as along with this bug I also investigated what I thought was a related failure in PR78216 and in the end that was due to the issue your mentioning here with multilevel parameter packs. I created a fix for it but when I combined and tested it with this one it led to an ICE further down the line and it led me to consider if this fix is correct or if it is leading to the construction of invalid chains / nodes. Anyway I had a chance to look at it again today and found it was exposing another fault in the multi-level param handling. I'll look at putting that one up next.

> >
> > > 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.org@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
>
>

Thanks for all the feedback, I've submitted a version 2.