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