C++ PATCH for c++/57891, narrowing conversions in non-type template arguments
Jason Merrill
jason@redhat.com
Sun Aug 5 14:02:00 GMT 2018
On Tue, Jul 24, 2018 at 6:49 AM, Marek Polacek <polacek@redhat.com> wrote:
> On Tue, Jul 03, 2018 at 04:27:33PM -0400, Jason Merrill wrote:
>> On Tue, Jul 3, 2018 at 3:41 PM, Jason Merrill <jason@redhat.com> wrote:
>> > On Tue, Jul 3, 2018 at 2:58 PM, Marek Polacek <polacek@redhat.com> wrote:
>> >> On Tue, Jul 03, 2018 at 12:40:51PM -0400, Jason Merrill wrote:
>> >>> On Fri, Jun 29, 2018 at 3:58 PM, Marek Polacek <polacek@redhat.com> wrote:
>> >>> > On Wed, Jun 27, 2018 at 07:35:15PM -0400, Jason Merrill wrote:
>> >>> >> On Wed, Jun 27, 2018 at 12:53 PM, Marek Polacek <polacek@redhat.com> wrote:
>> >>> >> > This PR complains about us accepting invalid code like
>> >>> >> >
>> >>> >> > template<unsigned int> struct A {};
>> >>> >> > A<-1> a;
>> >>> >> >
>> >>> >> > Where we should detect the narrowing: [temp.arg.nontype] says
>> >>> >> > "A template-argument for a non-type template-parameter shall be a converted
>> >>> >> > constant expression ([expr.const]) of the type of the template-parameter."
>> >>> >> > and a converted constant expression can contain only
>> >>> >> > - integral conversions other than narrowing conversions,
>> >>> >> > - [...]."
>> >>> >> > It spurred e.g.
>> >>> >> > <https://stackoverflow.com/questions/28184888/how-implicit-conversion-works-for-non-type-template-parameters>
>> >>> >> > and has >=3 dups so it has some visibility.
>> >>> >> >
>> >>> >> > I think build_converted_constant_expr needs to set check_narrowing.
>> >>> >> > check_narrowing also always mentions that it's in { } but that is no longer
>> >>> >> > true; in the future it will also apply to <=>. We'd probably have to add a new
>> >>> >> > flag to struct conversion if wanted to distinguish between these.
>> >>> >> >
>> >>> >> > This does not yet fix detecting narrowing in function templates (78244).
>> >>> >> >
>> >>> >> > Bootstrapped/regtested on x86_64-linux, ok for trunk?
>> >>> >> >
>> >>> >> > 2018-06-27 Marek Polacek <polacek@redhat.com>
>> >>> >> >
>> >>> >> > PR c++/57891
>> >>> >> > * call.c (build_converted_constant_expr): Set check_narrowing.
>> >>> >> > * decl.c (compute_array_index_type): Add warning sentinel. Use
>> >>> >> > input_location.
>> >>> >> > * pt.c (convert_nontype_argument): Return NULL_TREE if any errors
>> >>> >> > were reported.
>> >>> >> > * typeck2.c (check_narrowing): Don't mention { } in diagnostic.
>> >>> >> >
>> >>> >> > * g++.dg/cpp0x/Wnarrowing6.C: New test.
>> >>> >> > * g++.dg/cpp0x/Wnarrowing7.C: New test.
>> >>> >> > * g++.dg/cpp0x/Wnarrowing8.C: New test.
>> >>> >> > * g++.dg/cpp0x/constexpr-data2.C: Add dg-error.
>> >>> >> > * g++.dg/init/new43.C: Adjust dg-error.
>> >>> >> > * g++.dg/other/fold1.C: Likewise.
>> >>> >> > * g++.dg/parse/array-size2.C: Likewise.
>> >>> >> > * g++.dg/other/vrp1.C: Add dg-error.
>> >>> >> > * g++.dg/template/char1.C: Likewise.
>> >>> >> > * g++.dg/ext/builtin12.C: Likewise.
>> >>> >> > * g++.dg/template/dependent-name3.C: Adjust dg-error.
>> >>> >> >
>> >>> >> > diff --git gcc/cp/call.c gcc/cp/call.c
>> >>> >> > index 209c1fd2f0e..956c7b149dc 100644
>> >>> >> > --- gcc/cp/call.c
>> >>> >> > +++ gcc/cp/call.c
>> >>> >> > @@ -4152,7 +4152,10 @@ build_converted_constant_expr (tree type, tree expr, tsubst_flags_t complain)
>> >>> >> > }
>> >>> >> >
>> >>> >> > if (conv)
>> >>> >> > - expr = convert_like (conv, expr, complain);
>> >>> >> > + {
>> >>> >> > + conv->check_narrowing = !processing_template_decl;
>> >>> >>
>> >>> >> Why !processing_template_decl? This needs a comment.
>> >>> >
>> >>> > Otherwise we'd warn for e.g.
>> >>> >
>> >>> > template<int N> struct S { char a[N]; };
>> >>> > S<1> s;
>> >>> >
>> >>> > where compute_array_index_type will try to convert the size of the array (which
>> >>> > is a template_parm_index of type int when parsing the template) to size_type.
>> >>> > So I guess I can say that we need to wait for instantiation?
>> >>>
>> >>> We certainly shouldn't give a narrowing diagnostic about a
>> >>> value-dependent expression. It probably makes sense to check that at
>> >>> the top of check_narrowing, with all the other early exit conditions.
>> >>> But if we do know the constant value in the template, it's good to
>> >>> complain then rather than wait for instantiation.
>> >>
>> >> Makes sense; how about this then? (Regtest/bootstrap running.)
>> >>
>> >> 2018-07-03 Marek Polacek <polacek@redhat.com>
>> >>
>> >> PR c++/57891
>> >> * call.c (build_converted_constant_expr): Set check_narrowing.
>> >> * decl.c (compute_array_index_type): Add warning sentinel. Use
>> >> input_location.
>> >> * pt.c (convert_nontype_argument): Return NULL_TREE if any errors
>> >> were reported.
>> >> * typeck2.c (check_narrowing): Don't warn for instantiation-dependent
>> >> expressions or non-constants in a template. Don't mention { } in
>> >> diagnostic.
>> >>
>> >> * g++.dg/cpp0x/Wnarrowing6.C: New test.
>> >> * g++.dg/cpp0x/Wnarrowing7.C: New test.
>> >> * g++.dg/cpp0x/Wnarrowing8.C: New test.
>> >> * g++.dg/cpp0x/Wnarrowing9.C: New test.
>> >> * g++.dg/cpp0x/Wnarrowing10.C: New test.
>> >> * g++.dg/cpp0x/constexpr-data2.C: Add dg-error.
>> >> * g++.dg/init/new43.C: Adjust dg-error.
>> >> * g++.dg/other/fold1.C: Likewise.
>> >> * g++.dg/parse/array-size2.C: Likewise.
>> >> * g++.dg/other/vrp1.C: Add dg-error.
>> >> * g++.dg/template/char1.C: Likewise.
>> >> * g++.dg/ext/builtin12.C: Likewise.
>> >> * g++.dg/template/dependent-name3.C: Adjust dg-error.
>> >>
>> >> diff --git gcc/cp/call.c gcc/cp/call.c
>> >> index 209c1fd2f0e..4fb0fa8774b 100644
>> >> --- gcc/cp/call.c
>> >> +++ gcc/cp/call.c
>> >> @@ -4152,7 +4152,10 @@ build_converted_constant_expr (tree type, tree expr, tsubst_flags_t complain)
>> >> }
>> >>
>> >> if (conv)
>> >> - expr = convert_like (conv, expr, complain);
>> >> + {
>> >> + conv->check_narrowing = true;
>> >> + expr = convert_like (conv, expr, complain);
>> >> + }
>> >> else
>> >> expr = error_mark_node;
>> >>
>> >> diff --git gcc/cp/decl.c gcc/cp/decl.c
>> >> index c04b9b7d457..8da63fa2aaa 100644
>> >> --- gcc/cp/decl.c
>> >> +++ gcc/cp/decl.c
>> >> @@ -9508,6 +9508,8 @@ compute_array_index_type (tree name, tree size, tsubst_flags_t complain)
>> >> else
>> >> {
>> >> size = instantiate_non_dependent_expr_sfinae (size, complain);
>> >> + /* Don't warn about narrowing for VLAs. */
>> >> + warning_sentinel s (warn_narrowing, !TREE_CONSTANT (osize));
>> >> size = build_converted_constant_expr (size_type_node, size, complain);
>> >
>> > Hmm, perhaps the underlying issue is that we only want
>> > build_converted_constant_expr to check for narrowing of constant
>> > values; if the value isn't constant, it isn't any kind of constant
>> > expression. So perhaps the checking needs to happen as part of
>> > constexpr evaluation.
>>
>> On the other hand, check_narrowing itself forces constexpr evaluation,
>> so this could be handled all in the conversion code; we just need to
>> tell check_narrowing to allow non-constant values in that case.
> OK -- see the patch below. Now, I'm not crazy about adding another bit
> to struct conversion, but reusing any of the other bits didn't seem
> safe/possible. Maybe it'll come in handy when dealing with this problem
> for function templates.
Looks good.
>> >> @@ -6669,9 +6669,12 @@ convert_nontype_argument (tree type, tree expr, tsubst_flags_t complain)
>> >> /* C++17: A template-argument for a non-type template-parameter shall
>> >> be a converted constant expression (8.20) of the type of the
>> >> template-parameter. */
>> >> + int errs = errorcount;
>> >> expr = build_converted_constant_expr (type, expr, complain);
>> >> if (expr == error_mark_node)
>> >> - return error_mark_node;
>> >> + /* Make sure we return NULL_TREE only if we have really issued
>> >> + an error, as described above. */
>> >> + return errorcount > errs ? NULL_TREE : error_mark_node;
>> >
>> > Is this still needed?
>>
>> Earlier you wrote,
>>
>> > Checking complain doesn't work because that doesn't
>> > say if we have really issued an error. If we have not, and we return
>> > NULL_TREE anyway, we hit this assert:
>> > 8517 gcc_assert (!(complain & tf_error) || seen_error ());
>>
>> If (complain & tf_error), we shouldn't see error_mark_node without an
>> error having been issued. Why is build_converted_constant_expr
>> returning error_mark_node without giving an error when (complain &
>> tf_error)?
>
> This can happen on invalid code; e.g. tests nontype13.C and crash87.C.
> What happens there is that we're trying to convert a METHOD_TYPE to bool,
> which doesn't work: in standard_conversion we go into the
> else if (tcode == BOOLEAN_TYPE)
> block, which returns NULL, implicit_conversion also then returns NULL, yet
> no error has been issued.
Yes, that's normal. The problem is in build_converted_constant_expr:
if (conv)
expr = convert_like (conv, expr, complain);
else
expr = error_mark_node;
Here when setting expr to error_mark_node I forgot to give an error if
(complain & tf_error).
>> >> + /* If we're in a template and we know the constant value, we can
>> >> + warn. Otherwise wait for instantiation. */
>> >> + || (processing_template_decl && !TREE_CONSTANT (init)))
>> >
>> > I don't think we want this condition. If the value is non-constant
>> > but also not dependent, it'll never be constant, so we can go ahead
>> > and complain.
>
> (This to be investigated later.)
Why later?
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
>
> 2018-07-23 Marek Polacek <polacek@redhat.com>
>
> PR c++/57891
> * call.c (struct conversion): Add check_narrowing_const_only.
> (build_converted_constant_expr): Set check_narrowing and
> check_narrowing_const_only.
> (convert_like_real): Pass it to check_narrowing.
> * cp-tree.h (check_narrowing): Add a default parameter.
> * decl.c (compute_array_index_type): Use input_location.
> * pt.c (convert_nontype_argument): Return NULL_TREE if any errors
> were reported.
> * typeck2.c (check_narrowing): Don't warn for instantiation-dependent
> expressions or non-constants in a template. Don't mention { } in
> diagnostic. Only check narrowing for constants if CONST_ONLY.
>
> --- gcc/cp/decl.c
> +++ gcc/cp/decl.c
> @@ -9581,7 +9581,7 @@ compute_array_index_type (tree name, tree size, tsubst_flags_t complain)
> {
> tree folded = cp_fully_fold (size);
> if (TREE_CODE (folded) == INTEGER_CST)
> - pedwarn (location_of (size), OPT_Wpedantic,
> + pedwarn (input_location, OPT_Wpedantic,
It should work to use location_of (osize) here.
Jason
More information about the Gcc-patches
mailing list