Re: [PATCH] c++: Don't set DECL_CONTEXT to nested template-template parameters [PR98881]

[Patrick Palka <ppalka@redhat.com>]

On Tue, 5 Mar 2024, Nathaniel Shead wrote:

> On Mon, Mar 04, 2024 at 10:07:33PM -0500, Patrick Palka wrote:
> > On Tue, 5 Mar 2024, Nathaniel Shead wrote:
> > 
> > > 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 <nathanieloshead@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.
> > 
> > It might be hard to come up with such a testcase since DECL_CONTEXT
> > has been unreliably set for ttps probably since inception (for example
> > for level-lowered ttps), so most logic is already robust to context-less
> > ttps.  But I suppose we might as well stream it anyway.
> 
> Fair enough. I've tried for a while but I haven't been able to work
> anything out; maybe another time.
> 
> Otherwise here's an updated version of the patch. Bootstrapped and
> regtested (so far just modules.exp) on x86_64-pc-linux-gnu: OK for
> trunk if full regtest succeeds?

LGTM!  Obliviously streaming the context seems like the best way to go
in light of the quirks about when/how it's set.

> 
> -- >8 --
> 
> Subject: [PATCH] c++: Stream 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. Rather
> than trying to determine the assumptions that need to hold, it seems
> simpler to just always re-stream the DECL_CONTEXT as needed for now.
> 
> 	PR c++/98881
> 
> gcc/cp/ChangeLog:
> 
> 	* module.cc (trees_out::tpl_parms_fini): Stream out DECL_CONTEXT
> 	for template template parameters.
> 	(trees_in::tpl_parms_fini): Read it.
> 
> 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 <nathanieloshead@gmail.com>
> Reviewed-by: Patrick Palka <ppalka@redhat.com>
> Reviewed-by: Jason Merrill <jason@redhat.com>
> ---
>  gcc/cp/module.cc                              | 41 +++++--------------
>  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, 47 insertions(+), 31 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..fcb449563ab 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -10126,23 +10126,12 @@ trees_out::tpl_parms_fini (tree tmpl, unsigned tpl_levels)
>  	  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);
> -		}
> -	    }
> +	  /* Template template parameters need a context of their owning
> +	     template. This is quite tricky to infer correctly on stream-in
> +	     (see PR c++/98881) so we'll just provide it directly.  */
> +	  tree decl = TREE_VALUE (parm);
> +	  if (TREE_CODE (decl) == TEMPLATE_DECL)
> +	    tree_node (DECL_CONTEXT (decl));
>  	}
>      }
>  }
> @@ -10160,24 +10149,14 @@ trees_in::tpl_parms_fini (tree tmpl, unsigned tpl_levels)
>  	{
>  	  tree parm = TREE_VEC_ELT (vec, ix);
>  	  tree dflt = tree_node ();
> -	  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;
> -	    }
> +	    DECL_CONTEXT (decl) = tree_node ();
> +
> +	  if (get_overrun ())
> +	    return false;
>  	}
>      }
>    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..b0dcf353c23
> --- /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;
> +template<template<class> class UU> struct Y { };
> 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
> 
>