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

[Nathaniel Shead <nathanieloshead@gmail.com>]

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.

> >  	    {
> >  	      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.

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 <nathanieloshead@gmail.com>
Reviewed-by: Patrick Palka <ppalka@redhat.com>
Reviewed-by: Jason Merrill <jason@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