diff mbox

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

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

Commit Message

Patrick Palka Jan. 22, 2016, 4:17 p.m. UTC
On Thu, 21 Jan 2016, Patrick Palka wrote:

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

Well, I tried it and the result is really ugly and it only "somewhat"
works.  (Maybe I'm just missing something obvious though.)  The ugliness
comes from the fact that decaying an array parameter type of a function
type embedded deep within some tree structure requires rebuilding all
the tree structures in between to avoid issues due to tree sharing.
This approach only "somewhat" works because it currently looks through
function, pointer, reference and array types.  And I just noticed that
this approach does not work at all for USING_DECLs because no PARM_DECL
is ever retained anyway in that case.

I think a better, complete fix for this issue would be to, one way or
another, be able to get at the PARM_DECLs that correspond to a given
FUNCTION_TYPE.  Say, if, the TREE_CHAIN of a FUNCTION_TYPE optionally
pointed to its PARM_DECLs, or something.  What do you think?

In the meantime, at this stage, I am personally most comfortable with
the previous patch (the one that XFAILs unify17.C).

Comments

Jason Merrill Jan. 22, 2016, 9:05 p.m. UTC | #1
On 01/22/2016 11:17 AM, Patrick Palka wrote:
> On Thu, 21 Jan 2016, Patrick Palka wrote:
>> 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.
>
> Well, I tried it and the result is really ugly and it only "somewhat"
> works.  (Maybe I'm just missing something obvious though.)  The ugliness
> comes from the fact that decaying an array parameter type of a function
> type embedded deep within some tree structure requires rebuilding all
> the tree structures in between to avoid issues due to tree sharing.

Yes, that does complicate things.

> This approach only "somewhat" works because it currently looks through
> function, pointer, reference and array types.

Right, you would need to handle template arguments as well.

> And I just noticed that
> this approach does not work at all for USING_DECLs because no PARM_DECL
> is ever retained anyway in that case.

I don't understand what you mean about USING_DECLs.

> I think a better, complete fix for this issue would be to, one way or
> another, be able to get at the PARM_DECLs that correspond to a given
> FUNCTION_TYPE.  Say, if, the TREE_CHAIN of a FUNCTION_TYPE optionally
> pointed to its PARM_DECLs, or something.  What do you think?

Hmm.  So void(int[5]) and void(int*) would be distinct types, but they 
would share TYPE_CANONICAL, as though one is a typedef of the other? 
Interesting, but I'm not sure how that would interact with template 
argument canonicalization.  Well, that can probably be made to work by 
treating dependent template arguments as distinct more frequently.

Another thought: What if we keep a list of arrays we need to substitute 
into for a particular function template?

> In the meantime, at this stage, I am personally most comfortable with
> the previous patch (the one that XFAILs unify17.C).

I don't think that's a good tradeoff, sorry.  For the moment, let's 
revert your earlier patch.

Jason
Patrick Palka Jan. 22, 2016, 10:30 p.m. UTC | #2
On Fri, 22 Jan 2016, Jason Merrill wrote:

> On 01/22/2016 11:17 AM, Patrick Palka wrote:
>> On Thu, 21 Jan 2016, Patrick Palka wrote:
>>> 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.
>> 
>> Well, I tried it and the result is really ugly and it only "somewhat"
>> works.  (Maybe I'm just missing something obvious though.)  The ugliness
>> comes from the fact that decaying an array parameter type of a function
>> type embedded deep within some tree structure requires rebuilding all
>> the tree structures in between to avoid issues due to tree sharing.
>
> Yes, that does complicate things.
>
>> This approach only "somewhat" works because it currently looks through
>> function, pointer, reference and array types.
>
> Right, you would need to handle template arguments as well.
>
>> And I just noticed that
>> this approach does not work at all for USING_DECLs because no PARM_DECL
>> is ever retained anyway in that case.
>
> I don't understand what you mean about USING_DECLs.

I just meant that we fail and would continue to fail to diagnose an
"array of void" error in the following test case:

    template <typename T>
    using X = void (T[5]);

    void foo (X<void>);

>
>> I think a better, complete fix for this issue would be to, one way or
>> another, be able to get at the PARM_DECLs that correspond to a given
>> FUNCTION_TYPE.  Say, if, the TREE_CHAIN of a FUNCTION_TYPE optionally
>> pointed to its PARM_DECLs, or something.  What do you think?
>
> Hmm.  So void(int[5]) and void(int*) would be distinct types, but they would 
> share TYPE_CANONICAL, as though one is a typedef of the other? Interesting, 
> but I'm not sure how that would interact with template argument 
> canonicalization.  Well, that can probably be made to work by treating 
> dependent template arguments as distinct more frequently.
>
> Another thought: What if we keep a list of arrays we need to substitute into 
> for a particular function template?

That approach definitely seems easier to reason about.  And it could
properly handle "using" templates as well as variable templates -- any
TEMPLATE_DECL, I think.

>
>> In the meantime, at this stage, I am personally most comfortable with
>> the previous patch (the one that XFAILs unify17.C).
>
> I don't think that's a good tradeoff, sorry.  For the moment, let's revert 
> your earlier patch.

Okay, will do.

>
> Jason
>
>
Jason Merrill Jan. 25, 2016, 3:11 p.m. UTC | #3
On 01/22/2016 05:30 PM, Patrick Palka wrote:
> On Fri, 22 Jan 2016, Jason Merrill wrote:
>> On 01/22/2016 11:17 AM, Patrick Palka wrote:
>>> On Thu, 21 Jan 2016, Patrick Palka wrote:
>>>> 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.
>>>
>>> Well, I tried it and the result is really ugly and it only "somewhat"
>>> works.  (Maybe I'm just missing something obvious though.)  The ugliness
>>> comes from the fact that decaying an array parameter type of a function
>>> type embedded deep within some tree structure requires rebuilding all
>>> the tree structures in between to avoid issues due to tree sharing.
>>
>> Yes, that does complicate things.
>>
>>> This approach only "somewhat" works because it currently looks through
>>> function, pointer, reference and array types.
>>
>> Right, you would need to handle template arguments as well.
>>
>>> And I just noticed that
>>> this approach does not work at all for USING_DECLs because no PARM_DECL
>>> is ever retained anyway in that case.
>>
>> I don't understand what you mean about USING_DECLs.
>
> I just meant that we fail and would continue to fail to diagnose an
> "array of void" error in the following test case:
>
>     template <typename T>
>     using X = void (T[5]);
>
>     void foo (X<void>);

True.  I think here we want to get the error when instantiating X<void>.

>>> I think a better, complete fix for this issue would be to, one way or
>>> another, be able to get at the PARM_DECLs that correspond to a given
>>> FUNCTION_TYPE.  Say, if, the TREE_CHAIN of a FUNCTION_TYPE optionally
>>> pointed to its PARM_DECLs, or something.  What do you think?
>>
>> Hmm.  So void(int[5]) and void(int*) would be distinct types, but they
>> would share TYPE_CANONICAL, as though one is a typedef of the other?
>> Interesting, but I'm not sure how that would interact with template
>> argument canonicalization.  Well, that can probably be made to work by
>> treating dependent template arguments as distinct more frequently.
>>
>> Another thought: What if we keep a list of arrays we need to
>> substitute into for a particular function template?
>
> That approach definitely seems easier to reason about.  And it could
> properly handle "using" templates as well as variable templates -- any
> TEMPLATE_DECL, I think.

Agreed.

Jason
diff mbox

Patch

diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index f4604b6..c70eb12 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -9082,6 +9082,92 @@  check_var_type (tree identifier, tree type)
    return type;
  }

+/* Given a type T, return a copy of T within which each array parameter type of
+   each function type embedded in T has been decayed to the corresponding
+   pointer type.  If no decaying was done, return T.
+
+   For example, if T corresponds to the type
+
+       void (** (char, void (&) (int[5]), char[7])) (int (*) (long[10]));
+                                 ~~~~~~   ~~~~~~~             ~~~~~~~
+
+   then the type returned by this function corresponds to
+
+       void (** (char, void (&) (int *), char *)) (int (*) (long *))
+                                 ~~~~~   ~~~~~~             ~~~~~~
+*/
+
+static tree
+decay_array_parms_r (tree t)
+{
+  if (t == NULL_TREE)
+    return t;
+
+  if (FUNC_OR_METHOD_TYPE_P (t))
+    {
+      auto_vec<tree> new_types;
+      new_types.reserve (8);
+      bool any_changed_p = false;
+
+      for (tree arg = TYPE_ARG_TYPES (t);
+	   arg != NULL_TREE && arg != void_list_node;
+	   arg = TREE_CHAIN (arg))
+	{
+	  tree old_type = TREE_VALUE (arg);
+	  tree new_type;
+
+	  if (TREE_CODE (old_type) == ARRAY_TYPE)
+	    new_type = decay_array_parms_r (type_decays_to (old_type));
+	  else
+	    new_type = decay_array_parms_r (old_type);
+
+	  if (old_type != new_type)
+	    any_changed_p = true;
+
+	  new_types.safe_push (new_type);
+	}
+
+      tree old_ret_type = TREE_TYPE (t);
+      tree new_ret_type = decay_array_parms_r (old_ret_type);
+      if (old_ret_type != new_ret_type)
+	any_changed_p = true;
+
+      if (!any_changed_p)
+	return t;
+
+      tree new_type_arg_types = NULL_TREE;
+      tree arg;
+      int i = 0;
+      for (arg = TYPE_ARG_TYPES (t);
+	   arg != NULL_TREE && arg != void_list_node;
+	   arg = TREE_CHAIN (arg), i++)
+	new_type_arg_types = tree_cons (TREE_PURPOSE (arg),
+					new_types[i],
+					new_type_arg_types);
+      new_type_arg_types = nreverse (new_type_arg_types);
+      if (arg == void_list_node)
+	new_type_arg_types = chainon (new_type_arg_types, void_list_node);
+
+      return build_function_type (new_ret_type, new_type_arg_types);
+    }
+  else if (TREE_CODE (t) == POINTER_TYPE)
+    {
+      tree old_type_type = TREE_TYPE (t);
+      tree new_type_type = decay_array_parms_r (old_type_type);
+      if (old_type_type != new_type_type)
+	return build_pointer_type (new_type_type);
+    }
+  else if (TREE_CODE (t) == REFERENCE_TYPE)
+    {
+      tree old_type_type = TREE_TYPE (t);
+      tree new_type_type = decay_array_parms_r (old_type_type);
+      if (old_type_type != new_type_type)
+	return cp_build_reference_type (new_type_type, TYPE_REF_IS_RVALUE (t));
+    }
+
+  return t;
+}
+
  /* Given declspecs and a declarator (abstract or otherwise), determine
     the name and type of the object declared and construct a DECL node
     for it.
@@ -10213,6 +10299,15 @@  grokdeclarator (const cp_declarator *declarator,

  	    type = build_function_type (type, arg_types);

+	    /* Decay all the (dependent) array parameter types embedded in this
+	       function type into their corresponding pointer types.  This is
+	       only done once, recursively, at the top-level declarator,
+	       not when processing a nested function declarator, designated by
+	       DECL_CONTEXT == PARM, so that the PARM_DECLS built will
+	       contain the un-decayed types.  */
+	    if (decl_context != PARM && processing_template_decl)
+	      type = decay_array_parms_r (type);
+
  	    tree attrs = declarator->std_attributes;
  	    if (tx_qual)
  	      {
diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 3733ee0..596b129 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -11795,6 +11795,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;
@@ -17797,23 +17801,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
@@ -18252,8 +18239,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;
@@ -20287,9 +20272,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;
@@ -20575,10 +20557,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" } }