On Monday, 27 January 2025 at 19:48, Jason Merrill - jason at redhat.com
<ja...@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....@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.
