[PATCH] PR c/17896 Check for missplaced bitwise op

[Jeff Law]

On 5/24/19 6:53 AM, Rafael Tsuha wrote:
> This patch adds a function to warn when there's a bitwise operation
> between a boolean and any other type. This kind of operation is
> probably a programmer mistake that may lead to unexpected behavior
> because possibily the logical operation was intended.
> The test was adapted from PR c/17896.
> 
> gcc/c-family/ChangeLog
> 2019-05-24  Rafael Tsuha <rafael.tsuha@usp.br>
> 
>     * c-warn.c (warn_logical_operator): Check for missplaced bitwise op.
> 
> gcc/testsuite/ChangeLog
> 2019-05-24  Rafael Tsuha <rafael.tsuha@usp.br>
> 
>     PR c/17896
>     * gcc.dg/boolean-bitwise.c: New test.
> 
> 
> bitwiseWarning.patch
> 
> Index: gcc/c-family/c-warn.c
> ===================================================================
> --- gcc/c-family/c-warn.c	(revision 268782)
> +++ gcc/c-family/c-warn.c	(working copy)
> @@ -167,10 +167,10 @@
>  }
>  
>  /* Warn about uses of logical || / && operator in a context where it
> -   is likely that the bitwise equivalent was intended by the
> -   programmer.  We have seen an expression in which CODE is a binary
> -   operator used to combine expressions OP_LEFT and OP_RIGHT, which before folding
> -   had CODE_LEFT and CODE_RIGHT, into an expression of type TYPE.  */
> +   is likely that the bitwise equivalent was intended by the programmer or vice
> +   versa.  We have seen an expression in which CODE is a binary operator used to
> +   combine expressions OP_LEFT and OP_RIGHT, which before folding had CODE_LEFT
> +   and CODE_RIGHT, into an expression of type TYPE.  */
>  
>  void
>  warn_logical_operator (location_t location, enum tree_code code, tree type,
> @@ -178,6 +178,7 @@
>  		       enum tree_code ARG_UNUSED (code_right), tree op_right)
>  {
>    int or_op = (code == TRUTH_ORIF_EXPR || code == TRUTH_OR_EXPR);
> +  int and_op = (code == TRUTH_ANDIF_EXPR || code == TRUTH_AND_EXPR);
>    int in0_p, in1_p, in_p;
>    tree low0, low1, low, high0, high1, high, lhs, rhs, tem;
>    bool strict_overflow_p = false;
> @@ -188,7 +189,9 @@
>    if (code != TRUTH_ANDIF_EXPR
>        && code != TRUTH_AND_EXPR
>        && code != TRUTH_ORIF_EXPR
> -      && code != TRUTH_OR_EXPR)
> +      && code != TRUTH_OR_EXPR
> +      && code != BIT_AND_EXPR
> +      && code != BIT_IOR_EXPR)
>      return;
>  
>    /* We don't want to warn if either operand comes from a macro
> @@ -219,7 +222,7 @@
>  	  if (or_op)
>  	    warning_at (location, OPT_Wlogical_op, "logical %<or%>"
>  			" applied to non-boolean constant");
> -	  else
> +	  else if (and_op)
>  	    warning_at (location, OPT_Wlogical_op, "logical %<and%>"
>  			" applied to non-boolean constant");
>  	  TREE_NO_WARNING (op_left) = true;
> @@ -227,6 +230,26 @@
>  	}
>      }
>  
> +  /* Warn if &/| are being used in a context where it is
> +     likely that the logical equivalent was intended by the
> +     programmer. That is, an expression such as op_1 & op_2
> +     where op_n should not be any boolean expression. */
> +  if ( TREE_CODE (TREE_TYPE (op_right)) == BOOLEAN_TYPE
> +      || TREE_CODE (TREE_TYPE (op_left)) == BOOLEAN_TYPE 
> +	  || COMPARISON_CLASS_P ((op_left))
> +	  || COMPARISON_CLASS_P ((op_right)))
> +    {
> +      tree folded_op_right = fold_for_warn (op_right);
So you've got an unused variable here.   If you just want the side
effect of calling fold_for_warn, then just call it, but don't assign its
result to a variable.

The existence of an unused variable would have triggered a bootstrap
failure.  This tells me you didn't bootstrap the change.  It's also
likely you didn't regression test the change.

If the bootstrap fails because we have instance of bitwise operation
between booleans, then standard practice would be to fix those too.  You
may want to consider submitting those fixes as a separate patch if
there's many of them (hopefully there's none :-)


jeff