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

[Patrick Palka]

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