Re: [PATCH] c++: Fix ICE in tsubst_default_argument [PR92010]

[Jason Merrill <jason@redhat.com>]

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.

Jason