From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 93695 invoked by alias); 5 Aug 2018 14:02:56 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 92980 invoked by uid 89); 5 Aug 2018 14:02:56 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-23.9 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 spammy=ahead, investigated, templates, jasonredhatcom X-HELO: mail-oi0-f65.google.com Received: from mail-oi0-f65.google.com (HELO mail-oi0-f65.google.com) (209.85.218.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sun, 05 Aug 2018 14:02:53 +0000 Received: by mail-oi0-f65.google.com with SMTP id 8-v6so17819371oip.0 for ; Sun, 05 Aug 2018 07:02:53 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:ac9:531d:0:0:0:0:0 with HTTP; Sun, 5 Aug 2018 07:02:31 -0700 (PDT) In-Reply-To: <20180723204912.GH29486@redhat.com> References: <20180627165343.GC4896@redhat.com> <20180629195834.GB5927@redhat.com> <20180703185805.GH5927@redhat.com> <20180723204912.GH29486@redhat.com> From: Jason Merrill Date: Sun, 05 Aug 2018 14:02:00 -0000 Message-ID: Subject: Re: C++ PATCH for c++/57891, narrowing conversions in non-type template arguments To: Marek Polacek Cc: GCC Patches Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2018-08/txt/msg00364.txt.bz2 On Tue, Jul 24, 2018 at 6:49 AM, Marek Polacek 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 wrote: >> > On Tue, Jul 3, 2018 at 2:58 PM, Marek Polacek 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 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 wrote: >> >>> >> > This PR complains about us accepting invalid code like >> >>> >> > >> >>> >> > template 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. >> >>> >> > >> >>> >> > 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 >> >>> >> > >> >>> >> > 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 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 >> >> >> >> 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 > > 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