From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 115713 invoked by alias); 21 Dec 2017 04:58:12 -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 115657 invoked by uid 89); 21 Dec 2017 04:58:11 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.2 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 spammy= X-HELO: mail-it0-f66.google.com Received: from mail-it0-f66.google.com (HELO mail-it0-f66.google.com) (209.85.214.66) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 21 Dec 2017 04:58:10 +0000 Received: by mail-it0-f66.google.com with SMTP id u62so9215335ita.2 for ; Wed, 20 Dec 2017 20:58:09 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=Q3Xq+omf9ie3gS3nv77ELGPgFlIEraAqc+ElhVaQuEo=; b=o52Yx6uADp2bMzPBs6tuhNPv3m/l7RrZWvoeCO0gV0YfWNYUsXEUCm7ngbwtXdG1Z+ OqAZ/GyQ26FHkzcoiV0EDhQIdd/uvojAi+vrJl53SLMxz1Kb7DGP4e/eRcmdj3HPWD3D KsG6k7pUFwggK1lVjid2KJN72b5xWzPnl9vPNOU9gldniaYKkj2Q8qBQT9Q2AcdnhoD2 dIMWvDLdABGcI0RGXiv5sEfeBXca/qwduIWJlDlH7GjUVNoLEZn55+kQcfdqek/zdyDz uJgO/ZSNsb9fFSZjVRSokoC2H72dkt0ym/6WPAlcQdgHVFNXINsyjpB04TJoS+QgFKmT mhGg== X-Gm-Message-State: AKGB3mJgcE4Oes/8xNYIW5b07UTLle5iemCzUu/hD/JrxbnU5Wrnn4jn T68kQRfv5BVybGqr37ljGZdzhDoVtcUvnb3w7mkWyQ== X-Google-Smtp-Source: ACJfBotHJolUAANEz3SIjF7shk3l18TerjI62sCw02gf8+xxae/Tbs2craqzQtxjo6+DV7l7KMaUpupAqcUQVhhD8wE= X-Received: by 10.36.120.11 with SMTP id p11mr11297635itc.10.1513832288349; Wed, 20 Dec 2017 20:58:08 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.141.20 with HTTP; Wed, 20 Dec 2017 20:57:47 -0800 (PST) In-Reply-To: <1513797978-14728-1-git-send-email-dmalcolm@redhat.com> References: <1513797978-14728-1-git-send-email-dmalcolm@redhat.com> From: Jason Merrill Date: Thu, 21 Dec 2017 04:58:00 -0000 Message-ID: Subject: Re: [v3 of 05/14] C++: handle locations wrappers when calling warn_for_memset To: David Malcolm Cc: Martin Sebor , Nathan Sidwell , Jakub Jelinek , Richard Biener , gcc-patches List Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2017-12/txt/msg01407.txt.bz2 On Wed, Dec 20, 2017 at 2:26 PM, David Malcolm wrote: > On Tue, 2017-12-19 at 15:02 -0500, Jason Merrill wrote: >> On 12/16/2017 03:01 PM, Martin Sebor wrote: >> > On 12/16/2017 06:12 AM, David Malcolm wrote: >> > > On Mon, 2017-12-11 at 18:36 -0500, Jason Merrill wrote: >> > > > On 11/10/2017 04:45 PM, David Malcolm wrote: >> > > > > We need to strip away location wrappers in tree.c predicates >> > > > > like >> > > > > integer_zerop, otherwise they fail when they're called on >> > > > > wrapped INTEGER_CST; an example can be seen for >> > > > > c-c++-common/Wmemset-transposed-args1.c >> > > > > in g++.sum, where the warn_for_memset fails to detect integer >> > > > > zero >> > > > > if the location wrappers aren't stripped. >> > > > >> > > > These shouldn't be needed; callers should have folded away >> > > > location >> > > > wrappers. I would hope for STRIP_ANY_LOCATION_WRAPPER to be >> > > > almost >> > > > never needed. >> > > > >> > > > warn_for_memset may be missing some calls to fold_for_warn. >> > > >> > > It is, thanks. >> > > >> > > Here's a revised fix for e.g. Wmemset-transposed-args1.c, >> > > replacing >> > > "[PATCH 05/14] tree.c: strip location wrappers from integer_zerop >> > > etc" >> > > and >> > > "[PATCH 10/14] warn_for_memset: handle location wrappers" >> > > >> > > This version of the patch simply calls fold_for_warn on each of >> > > the >> > > arguments before calling warn_for_memset. This ensures that >> > > warn_for_memset detects integer zero. It also adds a >> > > literal_integer_zerop to deal with location wrapper nodes when >> > > building "literal_mask" for the call, since this must be called >> > > *before* the fold_for_warn calls. >> > > >> > > Successfully bootstrapped®rtested on x86_64-pc-linux-gnu, as >> > > part of the kit. >> > > >> > > Is this OK for trunk, once the rest of the kit is approved? >> > > >> > > gcc/cp/ChangeLog: >> > > * parser.c (literal_integer_zerop): New function. >> > > (cp_parser_postfix_expression): When calling warn_for_memset, >> > > replace integer_zerop calls with literal_integer_zerop, and >> > > call fold_for_warn on the arguments. >> > > --- >> > > gcc/cp/parser.c | 18 ++++++++++++++++-- >> > > 1 file changed, 16 insertions(+), 2 deletions(-) >> > > >> > > diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c >> > > index d15769a..7bca460 100644 >> > > --- a/gcc/cp/parser.c >> > > +++ b/gcc/cp/parser.c >> > > @@ -6621,6 +6621,16 @@ cp_parser_compound_literal_p (cp_parser >> > > *parser) >> > > return compound_literal_p; >> > > } >> > > >> > > +/* Return 1 if EXPR is the integer constant zero or a complex >> > > constant >> > > + of zero, without any folding, but ignoring location >> > > wrappers. */ >> > > + >> > > +static int >> > > +literal_integer_zerop (const_tree expr) >> > > +{ >> > > + STRIP_ANY_LOCATION_WRAPPER (expr); >> > > + return integer_zerop (expr); >> > > +} >> > > + >> > > /* Parse a postfix-expression. >> > > >> > > postfix-expression: >> > > @@ -7168,8 +7178,12 @@ cp_parser_postfix_expression (cp_parser >> > > *parser, bool address_p, bool cast_p, >> > > tree arg0 = (*args)[0]; >> > > tree arg1 = (*args)[1]; >> > > tree arg2 = (*args)[2]; >> > > - int literal_mask = ((!!integer_zerop (arg1) << 1) >> > > - | (!!integer_zerop (arg2) << 2)); >> > > + /* Handle any location wrapper nodes. */ >> > > + arg0 = fold_for_warn (arg0); >> > > + int literal_mask = ((!!literal_integer_zerop (arg1) << >> > > 1) >> > > + | (!!literal_integer_zerop (arg2) << 2)); >> > >> > The double negation jumped out at me. The integer_zerop() function >> > looks like a predicate that hasn't yet been converted to return >> > bool. >> > It seems that new predicates that are implemented on top of it >> > could >> > return bool and their callers avoid this conversion. (At some >> > point >> > in the future it would also be nice to convert the existing >> > predicates to return bool). >> >> Agreed. And since integer_zerop in fact returns boolean values, >> there >> seems to be no need for the double negative in the first place. >> >> So, please make the new function return bool and remove the !!s. >> >> And I think the fold_for_warn call should be in warn_for_memset, and >> it >> should be called for arg2 as well instead of specifically handling >> CONST_DECL Here. > > Thanks. > > Here's an updated version of the patch which I believe covers all > of the above. > > Successfully bootstrapped®rtested on x86_64-pc-linux-gnu, as > part of the kit. > > OK for trunk once the rest of the kit is approved? OK. Jason