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

[Patrick Palka <ppalka@redhat.com>]

On Wed, 1 Apr 2020, Jason Merrill wrote:

> On 4/1/20 6:29 PM, Jason Merrill wrote:
> > On 3/31/20 3:50 PM, Patrick Palka wrote:
> > > On Tue, 31 Mar 2020, Jason Merrill wrote:
> > > 
> > > > On 3/30/20 6:46 PM, Patrick Palka wrote:
> > > > > 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?
> > > > 
> > > > I think let's go ahead with comparing TYPE_ARG_TYPES and DECL_ARGUMENTS;
> > > > the
> > > > problem comes when they disagree.  If we're handling pack expansions
> > > > wrong,
> > > > that's a separate issue.
> > > 
> > > Hm, comparing TYPE_ARG_TYPES and DECL_ARGUMENTS for compatibility seems
> > > to be exposing a latent bug with how we handle lambdas that appear in
> > > function parameter types.  Take g++.dg/cpp2a/lambda-uneval3.C for
> > > example:
> > > 
> > >      template <class T> void spam(decltype([]{}) (*s)[sizeof(T)]) {}
> > >      int main() { spam<char>(nullptr); }
> > > 
> > > According to tsubst_function_decl in current trunk, the type of the
> > > function paremeter 's' of spam<char> according to its TYPE_ARG_TYPES is
> > >      struct ._anon_4[1] *
> > > and according to its DECL_ARGUMENTS the type of 's' is
> > >      struct ._anon_5[1] *
> > > 
> > > The disagreement happens because we call tsubst_lambda_expr twice during
> > > substitution and thereby generate two distinct lambda types, one when
> > > substituting into the TYPE_ARG_TYPES and another when substituting into
> > > the DECL_ARGUMENTS.  I'm not sure how to work around this
> > > bug/false-positive..
> > 
> > Oof.
> > 
> > I think probably the right answer is to rebuild TYPE_ARG_TYPES from
> > DECL_ARGUMENTS if they don't match.
> 
> ...and treat that as a resolution of 1001/1322, so not giving an error.

Is something like this what you have in mind?  Bootstrap and testing in
progress.

-- >8 --

Subject: [PATCH] c++: Rebuild function type when it disagrees with formal
 parameter types [PR92010]

gcc/cp/ChangeLog:

	Core issues 1001 and 1322
	PR c++/92010
	* pt.c (maybe_rebuild_function_type): New function.
	(tsubst_function_decl): Use it.

gcc/testsuite/ChangeLog:

	Core issues 1001 and 1322
	PR c++/92010
	* g++.dg/cpp2a/lambda-uneval11.c: New test.
	* g++.dg/template/array33.C: New test.
	* g++.dg/template/array34.C: New test.
	* g++.dg/template/defarg22.C: New test.
---
 gcc/cp/pt.c                                  | 55 +++++++++++++++++
 gcc/testsuite/g++.dg/cpp2a/lambda-uneval11.C | 10 ++++
 gcc/testsuite/g++.dg/template/array33.C      | 63 ++++++++++++++++++++
 gcc/testsuite/g++.dg/template/array34.C      | 63 ++++++++++++++++++++
 gcc/testsuite/g++.dg/template/defarg22.C     | 13 ++++
 5 files changed, 204 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/cpp2a/lambda-uneval11.C
 create mode 100644 gcc/testsuite/g++.dg/template/array33.C
 create mode 100644 gcc/testsuite/g++.dg/template/array34.C
 create mode 100644 gcc/testsuite/g++.dg/template/defarg22.C

diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 041ce35a31c..fc0df790c0f 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -13475,6 +13475,59 @@ lookup_explicit_specifier (tree v)
   return *explicit_specifier_map->get (v);
 }
 
+/* Check if the function type of DECL, a FUNCTION_DECL, agrees with the type of
+   each of its formal parameters.  If there is a disagreement then rebuild
+   DECL's function type according to its formal parameter types, as part of a
+   resolution for Core issues 1001/1322.  */
+
+static void
+maybe_rebuild_function_decl_type (tree decl)
+{
+  bool function_type_needs_rebuilding = false;
+  if (tree parm_list = FUNCTION_FIRST_USER_PARM (decl))
+    {
+      tree parm_type_list = FUNCTION_FIRST_USER_PARMTYPE (decl);
+      while (parm_type_list && parm_type_list != void_list_node)
+	{
+	  tree parm_type = TREE_VALUE (parm_type_list);
+	  tree formal_parm_type_unqual = strip_top_quals (TREE_TYPE (parm_list));
+	  if (!same_type_p (parm_type, formal_parm_type_unqual))
+	    {
+	      function_type_needs_rebuilding = true;
+	      break;
+	    }
+
+	  parm_list = DECL_CHAIN (parm_list);
+	  parm_type_list = TREE_CHAIN (parm_type_list);
+	}
+    }
+
+  if (!function_type_needs_rebuilding)
+    return;
+
+  const tree new_arg_types = copy_list (TYPE_ARG_TYPES (TREE_TYPE (decl)));
+
+  tree parm_list = FUNCTION_FIRST_USER_PARM (decl);
+  tree old_parm_type_list = FUNCTION_FIRST_USER_PARMTYPE (decl);
+  tree new_parm_type_list = skip_artificial_parms_for (decl, new_arg_types);
+  while (old_parm_type_list && old_parm_type_list != void_list_node)
+    {
+      tree *new_parm_type = &TREE_VALUE (new_parm_type_list);
+      tree formal_parm_type_unqual = strip_top_quals (TREE_TYPE (parm_list));
+      if (!same_type_p (*new_parm_type, formal_parm_type_unqual))
+	*new_parm_type = formal_parm_type_unqual;
+
+      if (TREE_CHAIN (old_parm_type_list) == void_list_node)
+	TREE_CHAIN (new_parm_type_list) = void_list_node;
+      parm_list = DECL_CHAIN (parm_list);
+      old_parm_type_list = TREE_CHAIN (old_parm_type_list);
+      new_parm_type_list = TREE_CHAIN (new_parm_type_list);
+    }
+
+  TREE_TYPE (decl)
+    = build_function_type (TREE_TYPE (TREE_TYPE (decl)), new_arg_types);
+}
+
 /* Subroutine of tsubst_decl for the case when T is a FUNCTION_DECL.  */
 
 static tree
@@ -13665,6 +13718,8 @@ tsubst_function_decl (tree t, tree args, tsubst_flags_t complain,
   DECL_ARGUMENTS (r) = parms;
   DECL_RESULT (r) = NULL_TREE;
 
+  maybe_rebuild_function_decl_type (r);
+
   TREE_STATIC (r) = 0;
   TREE_PUBLIC (r) = TREE_PUBLIC (t);
   DECL_EXTERNAL (r) = 1;
diff --git a/gcc/testsuite/g++.dg/cpp2a/lambda-uneval11.C b/gcc/testsuite/g++.dg/cpp2a/lambda-uneval11.C
new file mode 100644
index 00000000000..a04262494c7
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/lambda-uneval11.C
@@ -0,0 +1,10 @@
+// PR c++/92010
+// { dg-do compile { target c++2a } }
+
+template <class T> void spam(decltype([]{}) (*s)[sizeof(T)] = nullptr)
+{ }
+
+void foo()
+{
+  spam<int>();
+}
diff --git a/gcc/testsuite/g++.dg/template/array33.C b/gcc/testsuite/g++.dg/template/array33.C
new file mode 100644
index 00000000000..0aa587351b4
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/array33.C
@@ -0,0 +1,63 @@
+// Verify that top-level cv-qualifiers on parameter types are considered
+// when determining the function type of an instantiated function template.
+// This resolves a part of Core issues 1001/1322.
+// { dg-do compile }
+// { dg-additional-options "-Wno-volatile" }
+
+template<typename T>
+void foo0(T t = 0);
+
+template<typename T>
+void foo1(const T = 0);
+
+template<typename T>
+void foo2(volatile T t = 0);
+
+template<typename T>
+void foo3(const volatile T t = 0);
+
+#if __cplusplus >= 201103L
+#define SA(X) static_assert(X,#X)
+SA(__is_same(decltype(foo0<char[]>), void(char*)));
+SA(__is_same(decltype(foo0<const char[]>), void(const char*)));
+SA(__is_same(decltype(foo0<volatile char[]>), void(volatile char*)));
+SA(__is_same(decltype(foo0<const volatile char[]>), void(const volatile char*)));
+
+SA(__is_same(decltype(foo1<char[]>), void(const char*)));
+SA(__is_same(decltype(foo1<const char[]>), void(const char*)));
+SA(__is_same(decltype(foo1<volatile char[]>), void(const volatile char*)));
+SA(__is_same(decltype(foo1<const volatile char[]>), void(const volatile char*)));
+
+SA(__is_same(decltype(foo2<char[]>), void(volatile char*)));
+SA(__is_same(decltype(foo2<const char[]>), void(const volatile char*)));
+SA(__is_same(decltype(foo2<volatile char[]>), void(volatile char*)));
+SA(__is_same(decltype(foo2<const volatile char[]>), void(const volatile char*)));
+
+SA(__is_same(decltype(foo3<char[]>), void(const volatile char*)));
+SA(__is_same(decltype(foo3<const char[]>), void(const volatile char*)));
+SA(__is_same(decltype(foo3<volatile char[]>), void(const volatile char*)));
+SA(__is_same(decltype(foo3<const volatile char[]>), void(const volatile char*)));
+#endif
+
+int main()
+{
+  foo0<char[]>();
+  foo0<const char[]>();
+  foo0<volatile char[]>();
+  foo0<const volatile char[]>();
+
+  foo1<char[]>();
+  foo1<const char[]>();
+  foo1<volatile char[]>();
+  foo1<const volatile char[]>();
+
+  foo2<char[]>();
+  foo2<const char[]>();
+  foo2<volatile char[]>();
+  foo2<const volatile char[]>();
+
+  foo3<char[]>();
+  foo3<const char[]>();
+  foo3<volatile char[]>();
+  foo3<const volatile char[]>();
+}
diff --git a/gcc/testsuite/g++.dg/template/array34.C b/gcc/testsuite/g++.dg/template/array34.C
new file mode 100644
index 00000000000..38c06401974
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/array34.C
@@ -0,0 +1,63 @@
+// Verify that top-level cv-qualifiers on parameter types are considered
+// when determining the function type of an instantiated function template.
+// This resolves a part of Core issues 1001/1322.
+// { dg-do compile { target c++11 } }
+// { dg-additional-options "-Wno-volatile" }
+
+template<typename... Ts>
+void foo0(Ts... t);
+
+template<typename... Ts>
+void foo1(const Ts... t);
+
+template<typename... Ts>
+void foo2(volatile Ts... t);
+
+template<typename... Ts>
+void foo3(const volatile Ts... t);
+
+#if __cplusplus >= 201103L
+#define SA(X) static_assert(X,#X)
+SA(__is_same(decltype(foo0<char[]>), void(char*)));
+SA(__is_same(decltype(foo0<const char[]>), void(const char*)));
+SA(__is_same(decltype(foo0<volatile char[]>), void(volatile char*)));
+SA(__is_same(decltype(foo0<const volatile char[]>), void(const volatile char*)));
+
+SA(__is_same(decltype(foo1<char[]>), void(const char*)));
+SA(__is_same(decltype(foo1<const char[]>), void(const char*)));
+SA(__is_same(decltype(foo1<volatile char[]>), void(const volatile char*)));
+SA(__is_same(decltype(foo1<const volatile char[]>), void(const volatile char*)));
+
+SA(__is_same(decltype(foo2<char[]>), void(volatile char*)));
+SA(__is_same(decltype(foo2<const char[]>), void(const volatile char*)));
+SA(__is_same(decltype(foo2<volatile char[]>), void(volatile char*)));
+SA(__is_same(decltype(foo2<const volatile char[]>), void(const volatile char*)));
+
+SA(__is_same(decltype(foo3<char[]>), void(const volatile char*)));
+SA(__is_same(decltype(foo3<const char[]>), void(const volatile char*)));
+SA(__is_same(decltype(foo3<volatile char[]>), void(const volatile char*)));
+SA(__is_same(decltype(foo3<const volatile char[]>), void(const volatile char*)));
+#endif
+
+int main()
+{
+  foo0<char[]>(0);
+  foo0<const char[]>(0);
+  foo0<volatile char[]>(0);
+  foo0<const volatile char[]>(0);
+
+  foo1<char[]>(0);
+  foo1<const char[]>(0);
+  foo1<volatile char[]>(0);
+  foo1<const volatile char[]>(0);
+
+  foo2<char[]>(0);
+  foo2<const char[]>(0);
+  foo2<volatile char[]>(0);
+  foo2<const volatile char[]>(0);
+
+  foo3<char[]>(0);
+  foo3<const char[]>(0);
+  foo3<volatile char[]>(0);
+  foo3<const volatile char[]>(0);
+}
diff --git a/gcc/testsuite/g++.dg/template/defarg22.C b/gcc/testsuite/g++.dg/template/defarg22.C
new file mode 100644
index 00000000000..599061cedb0
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/defarg22.C
@@ -0,0 +1,13 @@
+// PR c++/92010
+// { dg-do compile { target c++11 } }
+
+template <typename T = char[3]>
+void foo(const T t = "; ")
+{
+}
+
+int main()
+{
+  foo ();
+}
+
-- 
2.26.0.106.g9fadedd637