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?
gcc/c-family/ChangeLog:
* c-warn.c (warn_for_memset): Call fold_for_warn on the arguments.
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,
eliminating the double logical negation cast to bool.
Eliminate the special-casing for CONST_DECL in favor of the
fold_for_warn within warn_for_memset.
---
gcc/c-family/c-warn.c | 3 +++
gcc/cp/parser.c | 16 ++++++++++++----
2 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/gcc/c-family/c-warn.c b/gcc/c-family/c-warn.c
index 6045d6e..baaf37e 100644
--- a/gcc/c-family/c-warn.c
+++ b/gcc/c-family/c-warn.c
@@ -1865,6 +1865,9 @@ void
warn_for_memset (location_t loc, tree arg0, tree arg2,
int literal_zero_mask)
{
+ arg0 = fold_for_warn (arg0);
+ arg2 = fold_for_warn (arg2);
+
if (warn_memset_transposed_args
&& integer_zerop (arg2)
&& (literal_zero_mask & (1 << 2)) != 0
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index d15769a..adfca60 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 true if EXPR is the integer constant zero or a complex constant
+ of zero, without any folding, but ignoring location wrappers. */
+
+static bool
+literal_integer_zerop (const_tree expr)
+{
+ STRIP_ANY_LOCATION_WRAPPER (expr);
+ return integer_zerop (expr);
+}
+
/* Parse a postfix-expression.
postfix-expression:
@@ -7168,10 +7178,8 @@ 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));
- if (TREE_CODE (arg2) == CONST_DECL)
- arg2 = DECL_INITIAL (arg2);
+ int literal_mask = ((literal_integer_zerop (arg1) << 1)
+ | (literal_integer_zerop (arg2) << 2));
warn_for_memset (input_location, arg0, arg2, literal_mask);
}
--
1.8.5.3
