From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.81]) by sourceware.org (Postfix) with ESMTP id D202B385BF83 for ; Mon, 6 Apr 2020 21:33:30 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org D202B385BF83 Received: from mail-qk1-f200.google.com (mail-qk1-f200.google.com [209.85.222.200]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-451-aAoWKMyEMwaV0HY86-PZgQ-1; Mon, 06 Apr 2020 17:33:28 -0400 X-MC-Unique: aAoWKMyEMwaV0HY86-PZgQ-1 Received: by mail-qk1-f200.google.com with SMTP id h9so1232862qkm.5 for ; Mon, 06 Apr 2020 14:33:28 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=mYe3ahowdSQY+dK7dIcN5jemSNcZyJnhSLw9MCw4saE=; b=uIYgGuZoow/jxeGdK7PDSWnqdPFhENr34CU0aBol4gasArsr4QGhO12L4Y+IfDKf7C j7b9ZXzb8lBeUmw0osG4uaFQT8xBVSPYnSiLG2rbaGGob8eO/oJhlgr4bvMtsQ6GkFCm C4aKQ4avaDvAipCgLM5D15SIXGTGf8xQK/9F76GZDfJf021asAP27rzFztidtzdS5BoZ P/QTGZVEY4nw3XqqXW3lhVv/oxzrRSuBjxCxFDCizTtbNf5/lspifLCvh8XHsttZ8gxh D6z+0LJmxIqztohGRNr3LRGD/7opDtZU8vkYa6NuhandsFRx6KzC9QuvR5nXoBo5nTsw AxzA== X-Gm-Message-State: AGi0PuZGK02vTFeizgAPpD6vFKdKHx/bkiTp7CtwMPM0dChRZkG69rsf 3qRZT+fOYE7XAqYOIC91kIreDNhdxQnav/+OxSN1pxvG8N8sJ1rXPIyjqK+8PeKRxqwwwGNFm+h bFpwp66oy25B5+7uc5Q== X-Received: by 2002:a37:9a11:: with SMTP id c17mr646292qke.456.1586208807442; Mon, 06 Apr 2020 14:33:27 -0700 (PDT) X-Google-Smtp-Source: APiQypIbHu75MoQz9P41Ao+AH0sn7gOURO9uYz8MgQvqnSN3LoHKAhrV+26qSngxeB6BVZ/0q06Irw== X-Received: by 2002:a37:9a11:: with SMTP id c17mr646266qke.456.1586208806886; Mon, 06 Apr 2020 14:33:26 -0700 (PDT) Received: from [192.168.1.148] (209-6-216-142.s141.c3-0.smr-cbr1.sbo-smr.ma.cable.rcncustomer.com. [209.6.216.142]) by smtp.gmail.com with ESMTPSA id v76sm15699179qka.32.2020.04.06.14.33.24 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 06 Apr 2020 14:33:24 -0700 (PDT) Subject: Re: [PATCH] c++: Fix ICE in tsubst_default_argument [PR92010] To: Patrick Palka Cc: gcc-patches@gcc.gnu.org References: <20200323012105.3692086-1-ppalka@redhat.com> <0c702b38-1fca-d0ac-c1ee-f7e080d9367c@redhat.com> From: Jason Merrill Message-ID: <6716ec8e-7c6d-04b0-a545-b0b0e8248669@redhat.com> Date: Mon, 6 Apr 2020 17:33:23 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.3.0 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-28.5 required=5.0 tests=BAYES_00, BODY_8BITS, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 06 Apr 2020 21:33:32 -0000 On 4/6/20 11:45 AM, Patrick Palka wrote: > On Wed, 1 Apr 2020, Jason Merrill wrote: >=20 >> 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.=C2=A0 Concretely, the latent bug is that given t= he >>>>>>>>>> function >>>>>>>>>> template >>>>>>>>>> >>>>>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 template void foo(co= nst T t); >>>>>>>>>> >>>>>>>>>> one would expect the type of foo 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 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.=C2=A0 = 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.=C2=A0 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.=C2=A0 I think it would m= ake >>>>>>>>> sense >>>>>>>>> to >>>>>>>>> follow the EDG behavior until this issue is actually resolved. >>>>>>>> >>>>>>>> Here is what I have so far towards that end.=C2=A0 When substituti= ng >>>>>>>> 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.=C2=A0 This patch checks this in tsubst_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 t= o >>>>>> 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 sin= ce >>>>>> 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.=C2=A0 Doing so would ce= rtainly >>>>>> catch the original testcase, i.e. >>>>>> >>>>>> =C2=A0=C2=A0=C2=A0 template >>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 void foo(const T); >>>>>> =C2=A0=C2=A0=C2=A0 int main() { foo(0); } >>>>>> >>>>>> because the DECL_ARGUMENTS of foo would be {const int*} and i= ts >>>>>> TYPE_ARG_TYPES would be {int*}.=C2=A0 But apparently it doesn't catc= h the >>>>>> corresponding testcase that uses a function parameter pack, i.e. >>>>>> >>>>>> =C2=A0=C2=A0=C2=A0 template >>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 void foo(const Ts...); >>>>>> =C2=A0=C2=A0=C2=A0 int main() { foo(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.=C2=A0 So in this second testcas= e both >>>>>> DECL_ARGUMENTS and TYPE_ARG_TYPES of foo 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 d= o >>>>>> 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-qualifier= s, >>>>>> to see if these cv-qualifiers make a material difference in the >>>>>> resulting function type.=C2=A0 Or maybe there's yet another way to d= etect >>>>>> this? >>>>> >>>>> I think let's go ahead with comparing TYPE_ARG_TYPES and DECL_ARGUMEN= TS; >>>>> the >>>>> problem comes when they disagree.=C2=A0 If we're handling pack expans= ions >>>>> wrong, >>>>> that's a separate issue. >>>> >>>> Hm, comparing TYPE_ARG_TYPES and DECL_ARGUMENTS for compatibility seem= s >>>> to be exposing a latent bug with how we handle lambdas that appear in >>>> function parameter types.=C2=A0 Take g++.dg/cpp2a/lambda-uneval3.C for >>>> example: >>>> >>>> =C2=A0=C2=A0=C2=A0=C2=A0 template void spam(decltype([]{}) = (*s)[sizeof(T)]) {} >>>> =C2=A0=C2=A0=C2=A0=C2=A0 int main() { spam(nullptr); } >>>> >>>> According to tsubst_function_decl in current trunk, the type of the >>>> function paremeter 's' of spam according to its TYPE_ARG_TYPES i= s >>>> =C2=A0=C2=A0=C2=A0=C2=A0 struct ._anon_4[1] * >>>> and according to its DECL_ARGUMENTS the type of 's' is >>>> =C2=A0=C2=A0=C2=A0=C2=A0 struct ._anon_5[1] * >>>> >>>> The disagreement happens because we call tsubst_lambda_expr twice duri= ng >>>> substitution and thereby generate two distinct lambda types, one when >>>> substituting into the TYPE_ARG_TYPES and another when substituting int= o >>>> the DECL_ARGUMENTS.=C2=A0 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. >=20 > Is something like this what you have in mind? Bootstrap and testing in > progress. Yes, thanks. > -- >8 -- >=20 > Subject: [PATCH] c++: Rebuild function type when it disagrees with formal > parameter types [PR92010] >=20 > gcc/cp/ChangeLog: >=20 > =09Core issues 1001 and 1322 > =09PR c++/92010 > =09* pt.c (maybe_rebuild_function_type): New function. > =09(tsubst_function_decl): Use it. >=20 > gcc/testsuite/ChangeLog: >=20 > =09Core issues 1001 and 1322 > =09PR c++/92010 > =09* g++.dg/cpp2a/lambda-uneval11.c: New test. > =09* g++.dg/template/array33.C: New test. > =09* g++.dg/template/array34.C: New test. > =09* 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 >=20 > 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); > } > =20 > +/* 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 rebui= ld > + 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 =3D false; > + if (tree parm_list =3D FUNCTION_FIRST_USER_PARM (decl)) > + { > + tree parm_type_list =3D FUNCTION_FIRST_USER_PARMTYPE (decl); > + while (parm_type_list && parm_type_list !=3D void_list_node) > +=09{ > +=09 tree parm_type =3D TREE_VALUE (parm_type_list); > +=09 tree formal_parm_type_unqual =3D strip_top_quals (TREE_TYPE (parm_l= ist)); > +=09 if (!same_type_p (parm_type, formal_parm_type_unqual)) > +=09 { > +=09 function_type_needs_rebuilding =3D true; > +=09 break; > +=09 } > + > +=09 parm_list =3D DECL_CHAIN (parm_list); > +=09 parm_type_list =3D TREE_CHAIN (parm_type_list); > +=09} > + } > + > + if (!function_type_needs_rebuilding) > + return; > + > + const tree new_arg_types =3D copy_list (TYPE_ARG_TYPES (TREE_TYPE (dec= l))); > + > + tree parm_list =3D FUNCTION_FIRST_USER_PARM (decl); > + tree old_parm_type_list =3D FUNCTION_FIRST_USER_PARMTYPE (decl); > + tree new_parm_type_list =3D skip_artificial_parms_for (decl, new_arg_t= ypes); > + while (old_parm_type_list && old_parm_type_list !=3D void_list_node) > + { > + tree *new_parm_type =3D &TREE_VALUE (new_parm_type_list); > + tree formal_parm_type_unqual =3D strip_top_quals (TREE_TYPE (parm_= list)); > + if (!same_type_p (*new_parm_type, formal_parm_type_unqual)) > +=09*new_parm_type =3D formal_parm_type_unqual; > + > + if (TREE_CHAIN (old_parm_type_list) =3D=3D void_list_node) > +=09TREE_CHAIN (new_parm_type_list) =3D void_list_node; > + parm_list =3D DECL_CHAIN (parm_list); > + old_parm_type_list =3D TREE_CHAIN (old_parm_type_list); > + new_parm_type_list =3D TREE_CHAIN (new_parm_type_list); > + } The usual pattern for this sort of thing is to use a tree* to track the=20 end of the new list, which should also avoid making a garbage copy of=20 void_list_node. e.g. from tsubst_attribute: > tree list =3D NULL_TREE; > tree *q =3D &list; > for (int i =3D 0; i < len; ++i) > { > tree elt =3D TREE_VEC_ELT (pack, i); > *q =3D build_tree_list (purp, elt); > q =3D &TREE_CHAIN (*q); > } Jason