Re: [PATCH] C++: handle locations wrappers when calling warn_for_memset

[Martin Sebor <msebor@gmail.com>]

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