[PATCH] c++: Fix ICE in tsubst_default_argument [PR92010]
Jason Merrill
jason@redhat.com
Mon Apr 6 21:33:23 GMT 2020
On 4/6/20 11:45 AM, Patrick Palka wrote:
> 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.
Yes, thanks.
> -- >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);
> + }
The usual pattern for this sort of thing is to use a tree* to track the
end of the new list, which should also avoid making a garbage copy of
void_list_node. e.g. from tsubst_attribute:
> tree list = NULL_TREE;
> tree *q = &list;
> for (int i = 0; i < len; ++i)
> {
> tree elt = TREE_VEC_ELT (pack, i);
> *q = build_tree_list (purp, elt);
> q = &TREE_CHAIN (*q);
> }
Jason
More information about the Gcc-patches
mailing list