C++: handle locations wrappers when calling warn_for_memset

Message ID 1513429958-46043-1-git-send-email-dmalcolm@redhat.com
State New
Headers show
Series
  • C++: handle locations wrappers when calling warn_for_memset
Related show

Commit Message

David Malcolm Dec. 16, 2017, 1:12 p.m.
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(-)

-- 
1.8.5.3

Comments

Martin Sebor Dec. 16, 2017, 8:01 p.m. | #1
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).

Martin

> +		arg1 = fold_for_warn (arg1);

> +		arg2 = fold_for_warn (arg2);

>  		if (TREE_CODE (arg2) == CONST_DECL)

>  		  arg2 = DECL_INITIAL (arg2);

>  		warn_for_memset (input_location, arg0, arg2, literal_mask);

>
Jason Merrill Dec. 19, 2017, 8:02 p.m. | #2
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.

Jason


Jason

Patch

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));
+		arg1 = fold_for_warn (arg1);
+		arg2 = fold_for_warn (arg2);
 		if (TREE_CODE (arg2) == CONST_DECL)
 		  arg2 = DECL_INITIAL (arg2);
 		warn_for_memset (input_location, arg0, arg2, literal_mask);