| Message ID | 20210304005505.667154-1-polacek@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | c++: ICE with real-to-int conversion in template [PR97973] | expand |
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). 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. 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. > So let's not create a FIX_TRUNC_EXPR in a template in the first place > and instead use IMPLICIT_CONV_EXPR. > > (I'm not sure why tsubst* also doesn't crash on a FLOAT_EXPR; it'd > make sense to me to also create an IMPLICIT_CONV_EXPR for integer > to real convs and add "case FLOAT_EXPR" just under FIX_TRUNC_EXPR. > But perhaps that's too risky to do now.) > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10? > > gcc/cp/ChangeLog: > > PR c++/97973 > * call.c (convert_like): When converting real to integer in > a template, use IMPLICIT_CONV_EXPR. > > gcc/testsuite/ChangeLog: > > PR c++/97973 > * g++.dg/conversion/real-to-int1.C: New test. > --- > gcc/cp/call.c | 7 ++++++- > gcc/testsuite/g++.dg/conversion/real-to-int1.C | 17 +++++++++++++++++ > 2 files changed, 23 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 123f06b1f2b..8074d8fdba8 100644 > --- a/gcc/cp/call.c > +++ b/gcc/cp/call.c > @@ -8062,7 +8062,12 @@ 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)))) > + && (CLASS_TYPE_P (convs->type) > + || CLASS_TYPE_P (TREE_TYPE (expr)) > + /* Converting real to integer produces FIX_TRUNC_EXPR which > + tsubst also doesn't grok. */ > + || (SCALAR_FLOAT_TYPE_P (TREE_TYPE (expr)) > + && INTEGRAL_OR_ENUMERATION_TYPE_P (convs->type)))) > { > 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: 8d57bdadd2d9c2e5c95515ca7a583d7b407b55c4 >
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. Do we want to assert that FLOAT_EXPR never makes its way into tsubst now? 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
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 >
On Wed, Mar 17, 2021 at 04:45:00PM -0400, Jason Merrill wrote: > On 3/8/21 7:52 PM, Marek Polacek wrote: > > 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. Yea, I'd leave it as-is for now. > > Do we want to assert that FLOAT_EXPR never makes its way into tsubst now? > > Sure. Ok, I'll add the assert if it passes testing. Thanks, Marek
diff --git a/gcc/cp/call.c b/gcc/cp/call.c index 123f06b1f2b..8074d8fdba8 100644 --- a/gcc/cp/call.c +++ b/gcc/cp/call.c @@ -8062,7 +8062,12 @@ 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)))) + && (CLASS_TYPE_P (convs->type) + || CLASS_TYPE_P (TREE_TYPE (expr)) + /* Converting real to integer produces FIX_TRUNC_EXPR which + tsubst also doesn't grok. */ + || (SCALAR_FLOAT_TYPE_P (TREE_TYPE (expr)) + && INTEGRAL_OR_ENUMERATION_TYPE_P (convs->type)))) { 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> (); +}