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

[Patrick Palka <ppalka@redhat.com>]

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.

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 "" }
+}
-- 
2.26.0.106.g9fadedd637