Re: [v3 of 05/14] C++: handle locations wrappers when calling warn_for_memset

[Jason Merrill <jason at redhat dot com>]

On Wed, Dec 20, 2017 at 2:26 PM, David Malcolm <dmalcolm@redhat.com> 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&regrtested 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&regrtested on x86_64-pc-linux-gnu, as
> part of the kit.
>
> OK for trunk once the rest of the kit is approved?

OK.

Jason