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

Message ID 1513797978-14728-1-git-send-email-dmalcolm@redhat.com
State New
Headers show
Series
  • Untitled series #247
Related show

Commit Message

David Malcolm Dec. 20, 2017, 7:26 p.m.
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?

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(-)

-- 
1.8.5.3

Comments

Jason Merrill Dec. 21, 2017, 4:57 a.m. | #1
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

Patch

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);
 	      }