diff mbox

Fix the remaining PR c++/24666 blockers (arrays decay to pointers too early)

Message ID alpine.DEB.2.20.9.1601192049020.13332@idea
State New
Headers show

Commit Message

Patrick Palka Jan. 20, 2016, 3:30 a.m. UTC 
On Tue, 19 Jan 2016, Patrick Palka wrote:

> On Mon, Jan 18, 2016 at 4:04 PM, Jason Merrill <jason@redhat.com> wrote:
>> On 01/18/2016 02:12 PM, Patrick Palka wrote:
>>>
>>> On Mon, Jan 18, 2016 at 10:34 AM, Jason Merrill <jason@redhat.com> wrote:
>>>>
>>>> On 12/25/2015 12:37 PM, Patrick Palka wrote:
>>>>>
>>>>>
>>>>> That alone would not be sufficient because more_specialized_fn()
>>>>> doesn't call maybe_adjust_types_for_deduction() beforehand, yet we
>>>>> have to do the decaying there too (and on both types, not just one of
>>>>> them).
>>>>>
>>>>> And maybe_adjust_types_for_deduction() seems to operate on the
>>>>> presumption that one type is the parameter type and one is the
>>>>> argument type. But in more_specialized_fn() and in get_bindings() we
>>>>> are really working with two parameter types and have to decay them
>>>>> both. So sometimes we have to decay one of the types that are
>>>>> eventually going to get passed to unify(), and other times we want to
>>>>> decay both types that are going to get passed to unify().
>>>>> maybe_adjust_types_for_deduction() seems to only expect the former
>>>>> case.
>>>>>
>>>>> Finally, maybe_adjust_types_for_deduction() is not called when
>>>>> unifying a nested function declarator (because it is guarded by the
>>>>> subr flag in unify_one_argument), so doing it there we would also
>>>>> regress in the following test case:
>>>>
>>>>
>>>>
>>>> Ah, that makes sense.
>>>>
>>>> How about keeping the un-decayed type in the PARM_DECLs, so that we get
>>>> the
>>>> substitution failure in instantiate_template, but having the decayed type
>>>> in
>>>> the TYPE_ARG_TYPES, probably by doing the decay in grokparms, so it's
>>>> already decayed when we're doing unification?
>>>
>>>
>>> I just tried this, and it works well! With this approach, all but one
>>> of the test cases pass.  The failing test case is unify17.C:
>>>
>>> -- 8< --
>>>
>>> void foo (int *);
>>>
>>> template <typename T>
>>> void bar (void (T[5])); // { dg-error "array of 'void'" }
>>>
>>> void
>>> baz (void)
>>> {
>>>    bar<void> (0); // { dg-error "no matching function" }
>>> }
>>>
>>> -- 8< --
>>>
>>> Here, we don't get a substitution failure because we don't have a
>>> corresponding FUNCTION_DECL for the nested function specifier, only a
>>> FUNCTION_TYPE. So there is no PARM_DECL to recurse into during
>>> substitution, that retains the un-decayed argument type "T[5]" of the
>>> nested function specifier.
>>
>>
>> Then your original patch is OK.  Thanks for indulging me.
>
> I have committed the original patch, but I just noticed that it has
> the unintended effect of changing whether certain template
> declarations are considered duplicates or not.  For instance, in the
> following test case we no longer consider the three declarations of
> "foo" to be duplicates and we instead register three overloads for
> foo, so that when we go to use foo later on we now get an ambiguity
> error:
>
> $ cat hmmmmm.cc
> template <typename T>
> int foo (T [4]);
>
> template <typename T>
> int foo (T [3]);
>
> template <typename T>
> int foo (T *);
>
> int x = foo<int> (0);
> $ g++ hmmmmm.cc
> hmmmmm.cc:10:20: error: call of overloaded ‘foo(int)’ is ambiguous
> int x = foo<int> (0);
>                    ^
> hmmmmm.cc:2:5: note: candidate: int foo(T [4]) [with T = int]
> int foo (T [4]);
>     ^~~
> hmmmmm.cc:5:5: note: candidate: int foo(T [3]) [with T = int]
> int foo (T [3]);
>     ^~~
> hmmmmm.cc:8:5: note: candidate: int foo(T*) [with T = int]
> int foo (T *);
>     ^~~
>
>
> Before the patch, this test case would have compiled cleanly because
> each subsequent declaration of foo would have been considered a
> duplicate of the first one (since we would have decayed the array
> parameter types early on).
>
> I am not sure whether the previous or current behavior is correct, or
> if maybe the first two decls ought to be considered duplicates and the
> last one not.  What do you think?
>

A much more significant side effect of this patch is that it changes the
mangling of function templates that have dependent array parameters.
For the following test case:

     template <typename T, int I>
     int foo (T [I]);

     int x = foo<int, 5> (0);

We now output

   _Z3fooIiLi5EEiAT0__T_ (aka "int foo<int, 5>(int [5])")

instead of

   _Z3fooIiLi5EEiPT_ (aka "int foo<int, 5>(int*)")

because when mangling a specialized FUNCTION_DECL the mangler uses the
undecayed TYPE_ARG_TYPES (TREE_TYPE (...)) of the corresponding
TEMPLATE_DECL instead of using the decayed
TYPE_ARG_TYPES (TREE_TYPE (...)) of that FUNCTION_DECL.

In light of this, I am inclined to instead use your suggested approach
(to decay the parameter types in grokparms, which become the
FUNCTION_TYPE's TYPE_ARG_TYPES, while retaining the un-decayed types in
the PARM_DECLs) since that approach does not exhibit this mangling issue
or the above mentioned overloading issue.  Here is a patch for that.

-- 8< --

Subject: [PATCH] Use a different approach for avoiding to decay array
  parameters too early

gcc/cp/ChangeLog:

 	PR c++/11858
 	PR c++/24663
 	PR c++/24664
 	* decl.c (grokparms): Decay each parameter type after stripping
 	its qualifiers.
 	* pt.c (tsubst_decl) [FUNCTION_DECL]: Return error_mark_node if
 	substition of the DECL_ARGUMENTS fails.
 	(decay_dependent_array_parm_type): Delete.
 	(type_unification_real): Remove calls to
 	decay_dependent_array_parm_type.
 	(more_specialized_fn): Likewise.
 	(get_bindings): Likewise.

gcc/testsuite/ChangeLog:

 	PR c++/11858
 	PR c++/24663
 	PR c++/24664
 	* g++.dg/template/mangle2.C: New test.
 	* g++.dg/template/unify17.C: XFAIL.
---
  gcc/cp/decl.c                           |  1 +
  gcc/cp/pt.c                             | 31 +++++--------------------------
  gcc/testsuite/g++.dg/template/mangle2.C | 22 ++++++++++++++++++++++
  gcc/testsuite/g++.dg/template/unify17.C |  4 ++--
  4 files changed, 30 insertions(+), 28 deletions(-)
  create mode 100644 gcc/testsuite/g++.dg/template/mangle2.C

Comments

Jason Merrill Jan. 21, 2016, 6:43 p.m. UTC | #1 
On 01/19/2016 10:30 PM, Patrick Palka wrote:
>      * g++.dg/template/unify17.C: XFAIL.

Hmm, I'm not comfortable with deliberately regressing this testcase.

>   template <typename T>
> -void bar (void (T[5])); // { dg-error "array of 'void'" }
> +void bar (void (T[5])); // { dg-error "array of 'void'" "" { xfail
> *-*-* } }

Can we work it so that T[5] also is un-decayed in the DECL_ARGUMENTS of 
bar, but decayed in the TYPE_ARG_TYPES?

Jason
Patrick Palka Jan. 21, 2016, 9:45 p.m. UTC | #2 
On Thu, 21 Jan 2016, Jason Merrill wrote:

> On 01/19/2016 10:30 PM, Patrick Palka wrote:
>>      * g++.dg/template/unify17.C: XFAIL.
>
> Hmm, I'm not comfortable with deliberately regressing this testcase.
>
>>   template <typename T>
>> -void bar (void (T[5])); // { dg-error "array of 'void'" }
>> +void bar (void (T[5])); // { dg-error "array of 'void'" "" { xfail
>> *-*-* } }
>
> Can we work it so that T[5] also is un-decayed in the DECL_ARGUMENTS of bar, 
> but decayed in the TYPE_ARG_TYPES?

I think so, I'll try it.

>
> Jason
>
>
diff mbox

Patch

diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index ceeef60..d681281 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -11702,6 +11702,7 @@  grokparms (tree parmlist, tree *parms)
  	  /* Top-level qualifiers on the parameters are
  	     ignored for function types.  */
  	  type = strip_top_quals (type);
+	  type = type_decays_to (type);

  	  if (TREE_CODE (type) == METHOD_TYPE)
  	    {
diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index fdef601..a7084a1 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -11740,6 +11740,10 @@  tsubst_decl (tree t, tree args, tsubst_flags_t complain)
  				     complain, t);
  	DECL_RESULT (r) = NULL_TREE;

+	for (tree parm = DECL_ARGUMENTS (r); parm; parm = DECL_CHAIN (parm))
+	  if (TREE_TYPE (parm) == error_mark_node)
+	    RETURN (error_mark_node);
+
  	TREE_STATIC (r) = 0;
  	TREE_PUBLIC (r) = TREE_PUBLIC (t);
  	DECL_EXTERNAL (r) = 1;
@@ -17742,23 +17746,6 @@  fn_type_unification (tree fn,
    return r;
  }

-/* TYPE is the type of a function parameter.  If TYPE is a (dependent)
-   ARRAY_TYPE, return the corresponding POINTER_TYPE to which it decays.
-   Otherwise return TYPE.  (We shouldn't see non-dependent ARRAY_TYPE
-   parameters because they get decayed as soon as they are declared.)  */
-
-static tree
-decay_dependent_array_parm_type (tree type)
-{
-  if (TREE_CODE (type) == ARRAY_TYPE)
-    {
-      gcc_assert (uses_template_parms (type));
-      return type_decays_to (type);
-    }
-
-  return type;
-}
-
  /* Adjust types before performing type deduction, as described in
     [temp.deduct.call] and [temp.deduct.conv].  The rules in these two
     sections are symmetric.  PARM is the type of a function parameter
@@ -18197,8 +18184,6 @@  type_unification_real (tree tparms,
        arg = args[ia];
        ++ia;

-      parm = decay_dependent_array_parm_type (parm);
-
        if (unify_one_argument (tparms, targs, parm, arg, subr, strict,
  			      explain_p))
  	return 1;
@@ -20232,9 +20217,6 @@  more_specialized_fn (tree pat1, tree pat2, int len)
            len = 0;
          }

-      arg1 = decay_dependent_array_parm_type (arg1);
-      arg2 = decay_dependent_array_parm_type (arg2);
-
        if (TREE_CODE (arg1) == REFERENCE_TYPE)
  	{
  	  ref1 = TYPE_REF_IS_RVALUE (arg1) + 1;
@@ -20520,10 +20502,7 @@  get_bindings (tree fn, tree decl, tree explicit_args, bool check_rettype)
    for (arg = decl_arg_types, ix = 0;
         arg != NULL_TREE && arg != void_list_node;
         arg = TREE_CHAIN (arg), ++ix)
-    {
-      args[ix] = TREE_VALUE (arg);
-      args[ix] = decay_dependent_array_parm_type (args[ix]);
-    }
+    args[ix] = TREE_VALUE (arg);

    if (fn_type_unification (fn, explicit_args, targs,
  			   args, ix,
diff --git a/gcc/testsuite/g++.dg/template/mangle2.C b/gcc/testsuite/g++.dg/template/mangle2.C
new file mode 100644
index 0000000..f9ea040
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/mangle2.C
@@ -0,0 +1,22 @@ 
+template <typename T, int I>
+int foo (T [I]);
+
+template <typename T>
+int bar (T [7]);
+
+template <int I>
+int baz (int [I]);
+
+extern int x, y, z;
+
+int
+main ()
+{
+  x = foo<int, 5> (0);
+  y = bar<char> (0);
+  z = baz<9> (0);
+}
+
+// { dg-final { scan-assembler "_Z3fooIiLi5EEiPT_" } }
+// { dg-final { scan-assembler "_Z3barIcEiPT_" } }
+// { dg-final { scan-assembler "_Z3bazILi9EEiPi" } }
diff --git a/gcc/testsuite/g++.dg/template/unify17.C b/gcc/testsuite/g++.dg/template/unify17.C
index 2da8553..3ae7699 100644
--- a/gcc/testsuite/g++.dg/template/unify17.C
+++ b/gcc/testsuite/g++.dg/template/unify17.C
@@ -1,11 +1,11 @@ 
  void foo (int *);

  template <typename T>
-void bar (void (T[5])); // { dg-error "array of 'void'" }
+void bar (void (T[5])); // { dg-error "array of 'void'" "" { xfail *-*-* } }

  void
  baz (void)
  {
    bar (foo); // { dg-bogus "" }
-  bar<void> (0); // { dg-error "no matching function" }
+  bar<void> (0); // { dg-error "no matching function" "" { xfail *-*-* } }
  }