On Mon, Mar 04, 2024 at 09:26:00PM -0500, Patrick Palka wrote:
> On Tue, 5 Mar 2024, Nathaniel Shead wrote:
>
> > On Mon, Mar 04, 2024 at 07:14:54PM -0500, Patrick Palka wrote:
> > > On Sat, 2 Mar 2024, Nathaniel Shead wrote:
> > >
> > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
> > > >
> > > > -- >8 --
> > > >
> > > > When streaming in a nested template-template parameter as in the
> > > > attached testcase, we end up reaching the containing template-template
> > > > parameter in 'tpl_parms_fini'. We should not set the DECL_CONTEXT to
> > > > this (nested) template-template parameter, as it should already be the
> > > > struct that the outer template-template parameter is declared on.
> > > >
> > > > PR c++/98881
> > > >
> > > > gcc/cp/ChangeLog:
> > > >
> > > > * module.cc (trees_out::tpl_parms_fini): Clarify logic purely
> > > > for checking purposes. Don't consider a template template
> > > > parameter as the owning template.
> > > > (trees_in::tpl_parms_fini): Don't consider a template template
> > > > parameter as the owning template.
> > > >
> > > > gcc/testsuite/ChangeLog:
> > > >
> > > > * g++.dg/modules/tpl-tpl-parm-3_a.H: New test.
> > > > * g++.dg/modules/tpl-tpl-parm-3_b.C: New test.
> > > >
> > > > Signed-off-by: Nathaniel Shead <nathanielosh...@gmail.com>
> > > > ---
> > > > gcc/cp/module.cc | 17 ++++++++++++-----
> > > > gcc/testsuite/g++.dg/modules/tpl-tpl-parm-3_a.H | 11 +++++++++++
> > > > gcc/testsuite/g++.dg/modules/tpl-tpl-parm-3_b.C | 13 +++++++++++++
> > > > 3 files changed, 36 insertions(+), 5 deletions(-)
> > > > create mode 100644 gcc/testsuite/g++.dg/modules/tpl-tpl-parm-3_a.H
> > > > create mode 100644 gcc/testsuite/g++.dg/modules/tpl-tpl-parm-3_b.C
> > > >
> > > > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> > > > index 67f132d28d7..5663d01ed9c 100644
> > > > --- a/gcc/cp/module.cc
> > > > +++ b/gcc/cp/module.cc
> > > > @@ -10126,10 +10126,14 @@ trees_out::tpl_parms_fini (tree tmpl,
> > > > unsigned tpl_levels)
> > > > tree dflt = TREE_PURPOSE (parm);
> > > > tree_node (dflt);
> > > >
> > > > - if (streaming_p ())
> > > > + if (CHECKING_P && streaming_p ())
> > > > {
> > > > + /* Sanity check that the DECL_CONTEXT we'll infer when
> > > > + streaming in is correct. */
> > > > tree decl = TREE_VALUE (parm);
> > > > - if (TREE_CODE (decl) == TEMPLATE_DECL)
> > > > + if (TREE_CODE (decl) == TEMPLATE_DECL
> > > > + /* A template template parm is not the owning
> > > > template. */
> > > > + && !DECL_TEMPLATE_TEMPLATE_PARM_P (tmpl))
> > > > {
> > > > tree ctx = DECL_CONTEXT (decl);
> > > > tree inner = DECL_TEMPLATE_RESULT (decl);
> > > > @@ -10164,8 +10168,13 @@ trees_in::tpl_parms_fini (tree tmpl, unsigned
> > > > tpl_levels)
> > > > return false;
> > > > TREE_PURPOSE (parm) = dflt;
> > > >
> > > > + /* Original template template parms have a context
> > > > + of their owning template. Reduced ones do not.
> > > > + But if TMPL is itself a template template parm
> > > > + then it cannot be the owning template. */
> > > > tree decl = TREE_VALUE (parm);
> > > > - if (TREE_CODE (decl) == TEMPLATE_DECL)
> > > > + if (TREE_CODE (decl) == TEMPLATE_DECL
> > > > + && !DECL_TEMPLATE_TEMPLATE_PARM_P (tmpl))
> > >
> > > IIUC a TEMPLATE_DECL inside a template parameter list always represents
> > > a template template parm, so won't this effectively disable the
> > > DECL_CONTEXT setting logic?
> >
> > This is only when 'tmpl' (i.e. the containing TEMPLATE_DECL that we're
> > streaming) is itself a template template parm.
>
> D'oh, makes sense.
>
> >
> > > > {
> > > > tree inner = DECL_TEMPLATE_RESULT (decl);
> > > > tree tpi = (TREE_CODE (inner) == TYPE_DECL
> > > > @@ -10173,8 +10182,6 @@ trees_in::tpl_parms_fini (tree tmpl, unsigned
> > > > tpl_levels)
> > > > : DECL_INITIAL (inner));
> > > > bool original = (TEMPLATE_PARM_LEVEL (tpi)
> > > > == TEMPLATE_PARM_ORIG_LEVEL (tpi));
> > > > - /* Original template template parms have a context
> > > > - of their owning template. Reduced ones do not. */
> > > > if (original)
> > > > DECL_CONTEXT (decl) = tmpl;
> > > > }
> > > > diff --git a/gcc/testsuite/g++.dg/modules/tpl-tpl-parm-3_a.H
> > > > b/gcc/testsuite/g++.dg/modules/tpl-tpl-parm-3_a.H
> > > > new file mode 100644
> > > > index 00000000000..21bbc054fa3
> > > > --- /dev/null
> > > > +++ b/gcc/testsuite/g++.dg/modules/tpl-tpl-parm-3_a.H
> > > > @@ -0,0 +1,11 @@
> > > > +// PR c++/98881
> > > > +// { dg-additional-options "-fmodule-header" }
> > > > +// { dg-module-cmi {} }
> > > > +
> > > > +template <typename P> struct X {};
> > > > +
> > > > +template<template <typename> typename TT>
> > > > +struct X<TT<int>> {
> > > > + template<template <typename> typename UU>
> > > > + void f (X<UU<int>>&);
> > > > +};
> > >
> > > I wonder why the partial specialization is relevant here? I can't
> > > seem to trigger the ICE without the partial specialization.
> > > Slightly further reduced to not use bound ttps:
> > >
> > > template<template<class> class TT>
> > > struct X { };
> > >
> > > template<template<class> class TT> requires true
> > > struct X<TT> {
> > > template<template<class> class UU>
> > > void f(X<UU>);
> > > };
> > >
> > > Maybe the expectation is that tpl_parms_fini for UU should be called
> > > with tpl_levels=1 (so that we stream only its own template parameters)
> > > but it's instead called with tpl_levels=3 for some reason?
> > >
> > > IIUC that assert should always hold in the first iteration of the loop
> > > (for the ttp's own template parameters). Perhaps for subsequent
> > > iterations we need to actually stream the contexts?
> > >
> > > Ah, but we also ICE in the same spot with:
> > >
> > > template<template<class> class TT> struct X; // #1
> > > template<template<class> class UU> struct X { }; // #2
> > >
> > > because DECL_CONTEXT of UU is #1 but tmpl is #2 (which seem like a
> > > bug)... But maybe this suggests we should just stream the DECL_CONTEXT
> > > of each ttp as-is instead of trying to avoid it via an apparently
> > > fragile assumption?
> > >
> >
> > That seems reasonable to me. Probably better to aim for correctness at
> > this stage rather than relying on things I don't understand for slight
> > performance gains.
> >
> > I started putting together a patch to always restream the DECL_CONTEXT,
> > but as I was doing so I realised that actually, we stream the context
> > anyway, and this value should be correct to start with? I don't think
> > deduplication should affect this, but I've added to the test to make
> > sure that there's no issues here that I can see.
>
> Where do we otherwise stream the context? In trees_out::core_vals we
> seem to avoid streaming the context for template parms (including ttps):
>
> if (!DECL_TEMPLATE_PARM_P (t))
> WT (t->decl_minimal.context);
Ah whoops, I was looking at the wrong 'context' (type_common.context). I
guess I need to find a testcase that actually relies on DECL_CONTEXT
being set correctly for template-template parameters.
> >
> > So maybe something like this? Or might there be something I'm missing?
> >
> > Bootstrapped and regtested (only modules.exp so far) on
> > x86_64-pc-linux-gnu, OK for trunk if full regtest succeeds?
> >
> > -- >8 --
> >
> > Subject: [PATCH] c++: Remove special handling of DECL_CONTEXT for template
> > template parms [PR98881]
> >
> > When streaming in a nested template-template parameter as in the
> > attached testcase, we end up reaching the containing template-template
> > parameter in 'tpl_parms_fini'. We should not set the DECL_CONTEXT to
> > this (nested) template-template parameter, as it should already be the
> > struct that the outer template-template parameter is declared on.
> >
> > The precise logic for what DECL_CONTEXT should be for a template
> > template parameter in various situations seems rather obscure. However,
> > it turns out in this case that the correct solution is to just do less
> > work: normal streaming already streams the correct value of DECL_CONTEXT
> > anyway, so there should be no need to redetermine them here.
> >
> > PR c++/98881
> >
> > gcc/cp/ChangeLog:
> >
> > * module.cc (trees_out::tpl_parms_fini): Remove special handling
> > for DECL_CONTEXT of template template parameters.
> > (trees_in::tpl_parms_fini): Likewise.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * g++.dg/modules/tpl-tpl-parm-3.h: New test.
> > * g++.dg/modules/tpl-tpl-parm-3_a.H: New test.
> > * g++.dg/modules/tpl-tpl-parm-3_b.C: New test.
> > * g++.dg/modules/tpl-tpl-parm-3_c.C: New test.
> >
> > Signed-off-by: Nathaniel Shead <nathanielosh...@gmail.com>
> > Reviewed-by: Patrick Palka <ppa...@redhat.com>
> > Reviewed-by: Jason Merrill <ja...@redhat.com>
> > ---
> > gcc/cp/module.cc | 33 -------------------
> > gcc/testsuite/g++.dg/modules/tpl-tpl-parm-3.h | 12 +++++++
> > .../g++.dg/modules/tpl-tpl-parm-3_a.H | 5 +++
> > .../g++.dg/modules/tpl-tpl-parm-3_b.C | 5 +++
> > .../g++.dg/modules/tpl-tpl-parm-3_c.C | 15 +++++++++
> > 5 files changed, 37 insertions(+), 33 deletions(-)
> > create mode 100644 gcc/testsuite/g++.dg/modules/tpl-tpl-parm-3.h
> > create mode 100644 gcc/testsuite/g++.dg/modules/tpl-tpl-parm-3_a.H
> > create mode 100644 gcc/testsuite/g++.dg/modules/tpl-tpl-parm-3_b.C
> > create mode 100644 gcc/testsuite/g++.dg/modules/tpl-tpl-parm-3_c.C
> >
> > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> > index 67f132d28d7..dbb851baa40 100644
> > --- a/gcc/cp/module.cc
> > +++ b/gcc/cp/module.cc
> > @@ -10125,24 +10125,6 @@ trees_out::tpl_parms_fini (tree tmpl, unsigned
> > tpl_levels)
> > tree parm = TREE_VEC_ELT (vec, ix);
> > tree dflt = TREE_PURPOSE (parm);
> > tree_node (dflt);
> > -
> > - if (streaming_p ())
> > - {
> > - tree decl = TREE_VALUE (parm);
> > - if (TREE_CODE (decl) == TEMPLATE_DECL)
> > - {
> > - tree ctx = DECL_CONTEXT (decl);
> > - tree inner = DECL_TEMPLATE_RESULT (decl);
> > - tree tpi = (TREE_CODE (inner) == TYPE_DECL
> > - ? TEMPLATE_TYPE_PARM_INDEX (TREE_TYPE (decl))
> > - : DECL_INITIAL (inner));
> > - bool original = (TEMPLATE_PARM_LEVEL (tpi)
> > - == TEMPLATE_PARM_ORIG_LEVEL (tpi));
> > - /* Original template template parms have a context
> > - of their owning template. Reduced ones do not. */
> > - gcc_checking_assert (original ? ctx == tmpl : !ctx);
> > - }
> > - }
> > }
> > }
> > }
> > @@ -10163,21 +10145,6 @@ trees_in::tpl_parms_fini (tree tmpl, unsigned
> > tpl_levels)
> > if (get_overrun ())
> > return false;
> > TREE_PURPOSE (parm) = dflt;
> > -
> > - tree decl = TREE_VALUE (parm);
> > - if (TREE_CODE (decl) == TEMPLATE_DECL)
> > - {
> > - tree inner = DECL_TEMPLATE_RESULT (decl);
> > - tree tpi = (TREE_CODE (inner) == TYPE_DECL
> > - ? TEMPLATE_TYPE_PARM_INDEX (TREE_TYPE (decl))
> > - : DECL_INITIAL (inner));
> > - bool original = (TEMPLATE_PARM_LEVEL (tpi)
> > - == TEMPLATE_PARM_ORIG_LEVEL (tpi));
> > - /* Original template template parms have a context
> > - of their owning template. Reduced ones do not. */
> > - if (original)
> > - DECL_CONTEXT (decl) = tmpl;
> > - }
> > }
> > }
> > return true;
> > diff --git a/gcc/testsuite/g++.dg/modules/tpl-tpl-parm-3.h
> > b/gcc/testsuite/g++.dg/modules/tpl-tpl-parm-3.h
> > new file mode 100644
> > index 00000000000..2fc088f0f39
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/modules/tpl-tpl-parm-3.h
> > @@ -0,0 +1,12 @@
> > +// PR c++/98881
> > +
> > +template <typename P> struct X {};
> > +
> > +template<template <typename> typename TT>
> > +struct X<TT<int>> {
> > + template<template <typename> typename UU>
> > + void f (X<UU<int>>&);
> > +};
> > +
> > +template<template<class> class TT> struct Y; // #1
> > +template<template<class> class UU> struct Y { }; // #2
> > diff --git a/gcc/testsuite/g++.dg/modules/tpl-tpl-parm-3_a.H
> > b/gcc/testsuite/g++.dg/modules/tpl-tpl-parm-3_a.H
> > new file mode 100644
> > index 00000000000..47464081f25
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/modules/tpl-tpl-parm-3_a.H
> > @@ -0,0 +1,5 @@
> > +// PR c++/98881
> > +// { dg-additional-options "-fmodule-header" }
> > +// { dg-module-cmi {} }
> > +
> > +#include "tpl-tpl-parm-3.h"
> > diff --git a/gcc/testsuite/g++.dg/modules/tpl-tpl-parm-3_b.C
> > b/gcc/testsuite/g++.dg/modules/tpl-tpl-parm-3_b.C
> > new file mode 100644
> > index 00000000000..1e3cc6c444f
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/modules/tpl-tpl-parm-3_b.C
> > @@ -0,0 +1,5 @@
> > +// PR c++/98881
> > +// { dg-additional-options "-fmodules-ts -fno-module-lazy" }
> > +
> > +#include "tpl-tpl-parm-3.h"
> > +import "tpl-tpl-parm-3_a.H";
> > diff --git a/gcc/testsuite/g++.dg/modules/tpl-tpl-parm-3_c.C
> > b/gcc/testsuite/g++.dg/modules/tpl-tpl-parm-3_c.C
> > new file mode 100644
> > index 00000000000..5edba6cc64f
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/modules/tpl-tpl-parm-3_c.C
> > @@ -0,0 +1,15 @@
> > +// PR c++/98881
> > +// { dg-additional-options "-fmodules-ts" }
> > +
> > +import "tpl-tpl-parm-3_a.H";
> > +
> > +template <typename T> struct A {};
> > +template <typename T> struct B {};
> > +
> > +void foo() {
> > + X<A<int>> a;
> > + X<B<int>> b;
> > + a.f(b);
> > +
> > + Y<A> y;
> > +}
> > --
> > 2.43.2
> >
> >
>
