[PATCH v2] c++: ICE with real-to-int conversion in template [PR97973]

[Jason Merrill]

On 3/8/21 7:52 PM, Marek Polacek wrote:
> On Fri, Mar 05, 2021 at 05:15:49PM -0500, Jason Merrill via Gcc-patches wrote:
>> On 3/3/21 7:55 PM, Marek Polacek wrote:
>>> In this test we are building a call in a template, but since neither
>>> the function nor any of its arguments are dependent, we go down the
>>> normal path in finish_call_expr.  convert_arguments sees that we're
>>> binding a reference to int to double and therein convert_to_integer
>>> creates a FIX_TRUNC_EXPR.  Later, we call check_function_arguments
>>> which folds the arguments, and, in a template, fold_for_warn calls
>>> fold_non_dependent_expr.  But tsubst_copy_and_build should not see
>>> a FIX_TRUNC_EXPR (see the patch discussed in
>>> <https://gcc.gnu.org/pipermail/gcc-patches/2018-March/496183.html>)
>>> or we crash.
>>
>> This again, sigh.  Let me take a step back.
> 
> :-(
>   
>> So basically, the output of convert_like_real in a template is a mix of
>> template and non-template trees, and thus unsuitable for consumption by
>> anything other than grabbing its type and throwing it away, as most callers
>> do.
>>
>> The problem here is that cp_build_function_call_vec calls
>> check_function_arguments with these trees.  build_over_call, however, does
>> not call check_function_arguments in a template.  Preventing that call in a
>> template also fixes the testcase, though it regresses diagnostic location in
>> Wnonnull5.C (which it shouldn't, that's a separate bug).
> 
> Yeah, that does strike me as wrong.  So I've tried
> 
> --- a/gcc/cp/typeck.c
> +++ b/gcc/cp/typeck.c
> @@ -4038,8 +4038,10 @@ cp_build_function_call_vec (tree function, vec<tree, va_gc> **params,
> 
>     /* Check for errors in format strings and inappropriately
>        null parameters.  */
> -  bool warned_p = check_function_arguments (input_location, fndecl, fntype,
> -                       nargs, argarray, NULL);
> +  bool warned_p
> +    = (!processing_template_decl
> +       && check_function_arguments (input_location, fndecl, fntype,
> +                   nargs, argarray, NULL));
> 
>     ret = build_cxx_call (function, nargs, argarray, complain, orig_fndecl);
> 
> and saw no failures with it (with/out my patch).  In fact, I'd like to
> apply both patches.
>   
>> IMPLICIT_CONV_EXPR is a way to represent the conversion so that the result
>> is still a template tree, and therefore suitable for fold_for_warn, which
>> allows us to warn when parsing the template, which is generally desirable.
> 
> That sounds like a fair assessment.
>   
>> I think the approach of expanding IMPLICIT_CONV_EXPR is probably the right
>> choice, but perhaps we should expand it to all non-trivial conversions, not
>> just those that would use problematic tree codes.
> 
> Yeah; it's worked pretty well for classes after we've dealt with the
> initial fallout.  I've factored the check into a new function, and also
> extended it with the case where we'd create a FLOAT_EXPR.  I don't know
> if that covers all non-trivial conversions.

By non-trivial I was thinking pretty much any non-identity conversion. 
But your patch is probably safer for trunk/10.  OK.

> Do we want to assert that FLOAT_EXPR never makes its way into tsubst now?

Sure.

> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10?
> 
> -- >8 --
> In this test we are building a call in a template, but since neither
> the function nor any of its arguments are dependent, we go down the
> normal path in finish_call_expr.  convert_arguments sees that we're
> binding a reference to int to double and therein convert_to_integer
> creates a FIX_TRUNC_EXPR.  Later, we call check_function_arguments
> which folds the arguments, and, in a template, fold_for_warn calls
> fold_non_dependent_expr.  But tsubst_copy_and_build should not see
> a FIX_TRUNC_EXPR (see the patch discussed in
> <https://gcc.gnu.org/pipermail/gcc-patches/2018-March/496183.html>)
> or we crash.
> 
> So let's not create a FIX_TRUNC_EXPR in a template in the first place
> and instead use IMPLICIT_CONV_EXPR.
> 
> gcc/cp/ChangeLog:
> 
> 	PR c++/97973
> 	* call.c (conv_unsafe_in_template_p): New.
> 	(convert_like): Use it.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR c++/97973
> 	* g++.dg/conversion/real-to-int1.C: New test.
> ---
>   gcc/cp/call.c                                 | 23 ++++++++++++++++++-
>   .../g++.dg/conversion/real-to-int1.C          | 17 ++++++++++++++
>   2 files changed, 39 insertions(+), 1 deletion(-)
>   create mode 100644 gcc/testsuite/g++.dg/conversion/real-to-int1.C
> 
> diff --git a/gcc/cp/call.c b/gcc/cp/call.c
> index 7d12fea60f2..f450691d3f6 100644
> --- a/gcc/cp/call.c
> +++ b/gcc/cp/call.c
> @@ -8048,6 +8048,27 @@ convert_like_internal (conversion *convs, tree expr, tree fn, int argnum,
>     return expr;
>   }
>   
> +/* Return true if converting FROM to TO is unsafe in a template.  */
> +
> +static bool
> +conv_unsafe_in_template_p (tree to, tree from)
> +{
> +  /* Converting classes involves TARGET_EXPR.  */
> +  if (CLASS_TYPE_P (to) || CLASS_TYPE_P (from))
> +    return true;
> +
> +  /* Converting real to integer produces FIX_TRUNC_EXPR which tsubst
> +     doesn't handle.  */
> +  if (SCALAR_FLOAT_TYPE_P (from) && INTEGRAL_OR_ENUMERATION_TYPE_P (to))
> +    return true;
> +
> +  /* Converting integer to real isn't a trivial conversion, either.  */
> +  if (INTEGRAL_OR_ENUMERATION_TYPE_P (from) && SCALAR_FLOAT_TYPE_P (to))
> +    return true;
> +
> +  return false;
> +}
> +
>   /* Wrapper for convert_like_internal that handles creating
>      IMPLICIT_CONV_EXPR.  */
>   
> @@ -8064,7 +8085,7 @@ convert_like (conversion *convs, tree expr, tree fn, int argnum,
>     tree conv_expr = NULL_TREE;
>     if (processing_template_decl
>         && convs->kind != ck_identity
> -      && (CLASS_TYPE_P (convs->type) || CLASS_TYPE_P (TREE_TYPE (expr))))
> +      && conv_unsafe_in_template_p (convs->type, TREE_TYPE (expr)))
>       {
>         conv_expr = build1 (IMPLICIT_CONV_EXPR, convs->type, expr);
>         if (convs->kind != ck_ref_bind)
> diff --git a/gcc/testsuite/g++.dg/conversion/real-to-int1.C b/gcc/testsuite/g++.dg/conversion/real-to-int1.C
> new file mode 100644
> index 00000000000..f7b990b3f4b
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/conversion/real-to-int1.C
> @@ -0,0 +1,17 @@
> +// PR c++/97973
> +
> +void (*foo[1])(const int &);
> +void (*foo2[1])(const double &);
> +
> +template<typename>
> +void f ()
> +{
> +  (foo[0])(1.1);
> +  (foo2[0])(1);
> +}
> +
> +void
> +g ()
> +{
> +  f<char> ();
> +}
> 
> base-commit: bd85b4dd2dd7b00b6342ed1e33fb48035a3dcb61
>