From: Jason Merrill <jason@redhat.com>
To: Patrick Palka <ppalka@redhat.com>
Cc: gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] c++: Fix ICE in tsubst_default_argument [PR92010]
Date: Wed, 1 Apr 2020 18:29:56 -0400 [thread overview]
Message-ID: <a4822427-7724-713a-e19c-9c8f114417a7@redhat.com> (raw)
In-Reply-To: <alpine.DEB.2.22.413.2003311527330.3094475@idea>
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.
Jason
next prev parent reply other threads:[~2020-04-01 22:30 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-23 1:21 Patrick Palka
2020-03-23 3:11 ` Patrick Palka
2020-03-26 19:29 ` Jason Merrill
2020-03-30 19:58 ` Patrick Palka
2020-03-30 20:15 ` Patrick Palka
2020-03-30 20:42 ` Jason Merrill
2020-03-30 22:46 ` Patrick Palka
2020-03-31 17:13 ` Jason Merrill
2020-03-31 19:50 ` Patrick Palka
2020-04-01 22:29 ` Jason Merrill [this message]
2020-04-01 22:37 ` Jason Merrill
2020-04-06 15:45 ` Patrick Palka
2020-04-06 21:33 ` Jason Merrill
2020-04-07 17:40 ` Patrick Palka
2020-04-07 20:26 ` Patrick Palka
2020-04-07 21:21 ` Jason Merrill
2020-04-08 14:18 ` Patrick Palka
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a4822427-7724-713a-e19c-9c8f114417a7@redhat.com \
--to=jason@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=ppalka@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).