Re: [PATCH] c++: Fix ICE in tsubst_default_argument [PR92010]

[Patrick Palka <ppalka@redhat.com>]

On Mon, 30 Mar 2020, Jason Merrill wrote:
> On 3/30/20 3:58 PM, Patrick Palka wrote:
> > On Thu, 26 Mar 2020, Jason Merrill wrote:
> > 
> > > On 3/22/20 9:21 PM, Patrick Palka wrote:
> > > > This patch relaxes an assertion in tsubst_default_argument that exposes
> > > > a
> > > > latent
> > > > bug in how we substitute an array type into a cv-qualified wildcard
> > > > function
> > > > parameter type.  Concretely, the latent bug is that given the function
> > > > template
> > > > 
> > > >     template<typename T> void foo(const T t);
> > > > 
> > > > one would expect the type of foo<int[]> to be void(const int*), but we
> > > > (seemingly prematurely) strip function parameter types of their
> > > > top-level
> > > > cv-qualifiers when building the function's TYPE_ARG_TYPES, and instead
> > > > end
> > > > up
> > > > obtaining void(int*) as the type of foo<int[]> after substitution and
> > > > decaying.
> > > > 
> > > > We still however correctly substitute into and decay the formal
> > > > parameter
> > > > type,
> > > > obtaining const int* as the type of t after substitution.  But this then
> > > > leads
> > > > to us tripping over the assert in tsubst_default_argument that verifies
> > > > the
> > > > formal parameter type and the function type are consistent.
> > > > 
> > > > Assuming it's too late at this stage to fix the substitution bug, we can
> > > > still
> > > > relax the assertion like so.  Tested on x86_64-pc-linux-gnu, does this
> > > > look
> > > > OK?
> > > 
> > > This is core issues 1001/1322, which have not been resolved.  Clang does
> > > the
> > > substitution the way you suggest; EDG rejects the testcase because the two
> > > substitutions produce different results.  I think it would make sense to
> > > follow the EDG behavior until this issue is actually resolved.
> > 
> > Here is what I have so far towards that end.  When substituting into the
> > PARM_DECLs of a function decl, we now additionally check if the
> > aforementioned Core issues are relevant and issue a (fatal) diagnostic
> > if so.  This patch checks this in tsubst_decl <case PARM_DECL> rather
> > than in tsubst_function_decl for efficiency reasons, so that we don't
> > have to perform another traversal over the DECL_ARGUMENTS /
> > TYPE_ARG_TYPES just to implement this check.
> 
> Hmm, this seems like writing more complicated code for a very marginal
> optimization; how many function templates have so many parameters that walking
> over them once to compare types will have any effect on compile time?

Good point... though I just tried implementing this check in
tsubst_function_decl, and it seems it might be just as complicated to
implement it there instead, at least if we want to handle function
parameter packs correctly.

If we were to implement this check in tsubst_function_decl, then since
we have access to the instantiated function, it would presumably suffice
to compare its substituted DECL_ARGUMENTS with its substituted
TYPE_ARG_TYPES to see if they're consistent.  Doing so would certainly
catch the original testcase, i.e.

  template<typename T>
    void foo(const T);
  int main() { foo<int[]>(0); }

because the DECL_ARGUMENTS of foo<int[]> would be {const int*} and its
TYPE_ARG_TYPES would be {int*}.  But apparently it doesn't catch the
corresponding testcase that uses a function parameter pack, i.e.

  template<typename... Ts>
    void foo(const Ts...);
  int main() { foo<int[]>(0); }

because it turns out we don't strip top-level cv-qualifiers from
function parameter packs from TYPE_ARG_TYPES at declaration time, as we
do with regular function parameters.  So in this second testcase both
DECL_ARGUMENTS and TYPE_ARG_TYPES of foo<int[]> would be {const int*},
and yet we would (presumably) want to reject this instantiation too.

So it seems comparing TYPE_ARG_TYPES and DECL_ARGUMENTS from
tsubst_function_decl would not suffice, and we would still need to do a
variant of the trick that's done in this patch, i.e. substitute into
each dependent parameter type stripped of its top-level cv-qualifiers,
to see if these cv-qualifiers make a material difference in the
resulting function type.  Or maybe there's yet another way to detect
this?

> 
> > Is something like this what you have in mind?
> > 
> > -- >8 --
> > 
> > Subject: [PATCH] c++: Reject some instantiations that depend on resolution
> > of
> >   Core issues 1001/1322
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	Core issues 1001 and 1322
> > 	PR c++/92010
> > 	* pt.c (tsubst_decl) <case PARM_DECL>: Detect and reject the case
> > where
> > 	the function type depends on whether we strip top-level qualifiers
> > from
> > 	the type of T before or after substitution.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 	Core issues 1001 and 1322
> > 	PR c++/92010
> > 	* g++.dg/template/array33.C: New test.
> > 	* g++.dg/template/array34.C: New test.
> > ---
> >   gcc/cp/pt.c                             | 70 ++++++++++++++++++++++++-
> >   gcc/testsuite/g++.dg/template/array33.C | 39 ++++++++++++++
> >   gcc/testsuite/g++.dg/template/array34.C | 63 ++++++++++++++++++++++
> >   3 files changed, 171 insertions(+), 1 deletion(-)
> >   create mode 100644 gcc/testsuite/g++.dg/template/array33.C
> >   create mode 100644 gcc/testsuite/g++.dg/template/array34.C
> > 
> > diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
> > index 8564eb11df4..fd99053df36 100644
> > --- a/gcc/cp/pt.c
> > +++ b/gcc/cp/pt.c
> > @@ -14072,9 +14072,77 @@ tsubst_decl (tree t, tree args, tsubst_flags_t
> > complain)
> >                 /* We're dealing with a normal parameter.  */
> >                 type = tsubst (TREE_TYPE (t), args, complain, in_decl);
> >   +	    const int type_quals = cp_type_quals (type);
> > +
> > +	    /* Determine whether the function type after substitution into the
> > +	       type of the function parameter T depends on the resolution of
> > +	       Core issues 1001/1322, which have not been resolved.  In
> > +	       particular, the following detects the case where the resulting
> > +	       function type depends on whether we strip top-level qualifiers
> > +	       from the type of T before or after substitution.
> > +
> > +	       This can happen only when the dependent type of T is a
> > +	       cv-qualified wildcard type, and substitution yields a
> > +	       (cv-qualified) array type before array-to-pointer conversion.
> > */
> > +	    tree unqual_expanded_types = NULL_TREE;
> > +	    if (TREE_CODE (type) == ARRAY_TYPE
> > +		&& (type_quals & (TYPE_QUAL_CONST|TYPE_QUAL_VOLATILE))
> > +		&& DECL_FUNCTION_SCOPE_P (t)
> > +		&& !DECL_TEMPLATE_PARM_P (t))
> > +	      {
> > +		tree type_pattern = (PACK_EXPANSION_P (TREE_TYPE (t))
> > +				     ? PACK_EXPANSION_PATTERN (TREE_TYPE (t))
> > +				     : TREE_TYPE (t));
> > +		if (WILDCARD_TYPE_P (type_pattern)
> > +		    && cv_qualified_p (type_pattern))
> > +		  {
> > +		    /* Substitute into the corresponding wildcard type that is
> > +		       stripped of its own top-level cv-qualifiers.  */
> > +		    tree unqual_type;
> > +		    if (PACK_EXPANSION_P (TREE_TYPE (t)))
> > +		      {
> > +			if (unqual_expanded_types == NULL_TREE)
> > +			  {
> > +			    tree unqual_pack_expansion
> > +			      = copy_node (TREE_TYPE (t));
> > +			    PACK_EXPANSION_PATTERN (unqual_pack_expansion)
> > +			      = cv_unqualified (type_pattern);
> > +			    unqual_expanded_types
> > +			      = tsubst_pack_expansion (unqual_pack_expansion,
> > +						       args, tf_none,
> > in_decl);
> > +			  }
> > +			unqual_type = TREE_VEC_ELT (unqual_expanded_types, i);
> > +		      }
> > +		    else
> > +		      {
> > +			tree unqual_type_pattern = cv_unqualified
> > (type_pattern);
> > +			unqual_type = tsubst (unqual_type_pattern, args,
> > +					      tf_none, in_decl);
> > +		      }
> > +		    /* Check if the top-level cv-qualifiers on the wildcard
> > type
> > +		       make a difference in the resulting type.  */
> > +		    int unqual_type_quals = cp_type_quals (unqual_type);
> > +		    if (type_quals & ~unqual_type_quals
> > +			& (TYPE_QUAL_CONST|TYPE_QUAL_VOLATILE))
> > +		      {
> > +			sorry ("the type of function %qD after substitution "
> > +			       "depends on the resolution of Core issues "
> > +			       "1001 and 1332",
> > +			       DECL_NAME (DECL_CONTEXT (t)));
> > +			if (PACK_EXPANSION_P (TREE_TYPE (t)))
> > +			  inform (input_location, "when substituting into the
> > "
> > +				  "function parameter pack %q#D", t);
> > +			else
> > +			  inform (input_location, "when substituting into the
> > "
> > +				  "function parameter %q#D", t);
> > +			type = error_mark_node;
> > +		      }
> > +		  }
> > +	      }
> > +
> >               type = type_decays_to (type);
> >               TREE_TYPE (r) = type;
> > -            cp_apply_type_quals_to_decl (cp_type_quals (type), r);
> > +	    cp_apply_type_quals_to_decl (type_quals, r);
> >                 if (DECL_INITIAL (r))
> >                 {
> > diff --git a/gcc/testsuite/g++.dg/template/array33.C
> > b/gcc/testsuite/g++.dg/template/array33.C
> > new file mode 100644
> > index 00000000000..20b005bc865
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/template/array33.C
> > @@ -0,0 +1,39 @@
> > +// Reject some instantiations whose result depend on the resolution of //
> > +// Core issues 1001 and 1332, which are not yet resolved.
> > +// { dg-do compile }
> > +// { dg-additional-options "-Wno-volatile" }
> > +
> > +template<typename T>
> > +void foo0(T t = 0);			// { dg-bogus "" }
> > +
> > +template<typename T>
> > +void foo1(const T = 0);			// { dg-message "sorry,
> > unimplemented" }
> > +
> > +template<typename T>
> > +void foo2(volatile T t = 0);		// { dg-message "sorry, unimplemented"
> > }
> > +
> > +template<typename T>
> > +void foo3(const volatile T t = 0);	// { dg-message "sorry, unimplemented"
> > }
> > +
> > +int main()
> > +{
> > +  foo0<char[]>();			// { dg-bogus "" }
> > +  foo0<const char[]>();			// { dg-bogus "" }
> > +  foo0<volatile char[]>();		// { dg-bogus "" }
> > +  foo0<const volatile char[]>();	// { dg-bogus "" }
> > +
> > +  foo1<char[]>();			// { dg-message "required from here" }
> > +  foo1<const char[]>();			// { dg-bogus "" }
> > +  foo1<volatile char[]>();		// { dg-message "required from here" }
> > +  foo1<const volatile char[]>();	// { dg-bogus "" }
> > +
> > +  foo2<char[]>();			// { dg-message "required from here" }
> > +  foo2<const char[]>();			// { dg-message "required from
> > here" }
> > +  foo2<volatile char[]>();		// { dg-bogus "" }
> > +  foo2<const volatile char[]>();	// { dg-bogus "" }
> > +
> > +  foo3<char[]>();			// { dg-message "required from here" }
> > +  foo3<const char[]>();			// { dg-message "required from
> > here" }
> > +  foo3<volatile char[]>();		// { dg-message "required from here" }
> > +  foo3<const volatile char[]>();	// { dg-bogus "" }
> > +}
> > diff --git a/gcc/testsuite/g++.dg/template/array34.C
> > b/gcc/testsuite/g++.dg/template/array34.C
> > new file mode 100644
> > index 00000000000..316ea2f6407
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/template/array34.C
> > @@ -0,0 +1,63 @@
> > +// Reject some instantiations whose result depend on the resolution of
> > +// Core issues 1001 and 1332, which are not yet resolved.
> > +// { dg-do compile { target c++11 } }
> > +// { dg-additional-options "-Wno-volatile" }
> > +
> > +template<typename... Ts>
> > +void foo0(Ts... t);			// { dg-bogus "" }
> > +
> > +template<typename... Ts>
> > +void foo1(const Ts... t);		// { dg-message "sorry, unimplemented"
> > }
> > +
> > +template<typename... Ts>
> > +void foo2(volatile Ts... t);		// { dg-message "sorry, unimplemented"
> > }
> > +
> > +template<typename... Ts>
> > +void foo3(const volatile Ts... t);	// { dg-message "sorry, unimplemented"
> > }
> > +
> > +template<typename... Ts>
> > +void bar0()
> > +{
> > +  foo0<int, Ts..., int>(0, 0, 0);
> > +}
> > +
> > +template<typename... Ts>
> > +void bar1()
> > +{
> > +  foo1<int, Ts..., int>(0, 0, 0);
> > +}
> > +
> > +template<typename... Ts>
> > +void bar2()
> > +{
> > +  foo2<int, Ts..., int>(0, 0, 0);
> > +}
> > +
> > +template<typename... Ts>
> > +void bar3()
> > +{
> > +  foo3<int, Ts..., int>(0, 0, 0);
> > +}
> > +
> > +int main()
> > +{
> > +  bar0<char[]>();			// { dg-bogus "" }
> > +  bar0<const char[]>();			// { dg-bogus "" }
> > +  bar0<volatile char[]>();		// { dg-bogus "" }
> > +  bar0<const volatile char[]>();	// { dg-bogus "" }
> > +
> > +  bar1<char[]>();			// { dg-message "required from here" }
> > +  bar1<const char[]>();			// { dg-bogus "" }
> > +  bar1<volatile char[]>();		// { dg-message "required from here" }
> > +  bar1<const volatile char[]>();	// { dg-bogus "" }
> > +
> > +  bar2<char[]>();			// { dg-message "required from here" }
> > +  bar2<const char[]>();			// { dg-message "required from
> > here" }
> > +  bar2<volatile char[]>();		// { dg-bogus "" }
> > +  bar2<const volatile char[]>();	// { dg-bogus "" }
> > +
> > +  bar3<char[]>();			// { dg-message "required from here" }
> > +  bar3<const char[]>();			// { dg-message "required from
> > here" }
> > +  bar3<volatile char[]>();		// { dg-message "required from here" }
> > +  bar3<const volatile char[]>();	// { dg-bogus "" }
> > +}
> > 
> 
>