c-family PATCH to improve -Wtautological-compare (PR c/81783)

[Eric Gallager]

On 8/16/17, David Malcolm <dmalcolm@redhat.com> wrote:
> On Wed, 2017-08-16 at 16:29 +0200, Marek Polacek wrote:
>> This patch improves -Wtautological-compare so that it also detects
>> bitwise comparisons involving & and | that are always true or false,
>> e.g.
>>
>>   if ((a & 16) == 10)
>>     return 1;
>>
>> can never be true.  Note that e.g. "(a & 9) == 8" is *not* always
>> false
>> or true.
>>
>> I think it's pretty straightforward with one snag: we shouldn't warn
>> if
>> the constant part of the bitwise operation comes from a macro, but
>> currently
>> that's not possible, so I XFAILed this in the new test.
>
> Maybe I'm missing something here, but why shouldn't it warn when the
> constant comes from a macro?
>
> At the end of your testcase you have this example:
>
> #define N 0x10
>   if ((a & N) == 10) /* { dg-bogus "bitwise comparison always evaluates to
> false" "" { xfail *-*-* } } */
>      return 1;
>   if ((a | N) == 10) /* { dg-bogus "bitwise comparison always evaluates to
> false" "" { xfail *-*-* } } */
>    return 1;
>
> That code looks bogus to me (and if the defn of "N" is further away,
> it's harder to spot that it's wrong): shouldn't we warn about it?
>

What about:

#ifdef SOME_PREPROCESSOR_CONDITIONAL
# define N 0x10
#else
# define N 0x11
#endif

or

#define N __LINE__

or

#define N __COUNTER__

or something else like that?

>>
>> This has found one issue in the GCC codebase and it's a genuine bug:
>> <https://gcc.gnu.org/ml/gcc-patches/2017-08/msg00757.html>.
>
> In this example GOVD_WRITTEN is from an enum, not a macro, but if
> GOVD_WRITTEN had been a macro, shouldn't we still issue a warning?
>
> Hope this is constructive
> Dave
>
>> Bootstrapped/regtested on x86_64-linux, ok for trunk?
>>
>> 2017-08-16  Marek Polacek  <polacek@redhat.com>
>>
>> 	PR c/81783
>> 	* c-warn.c (warn_tautological_bitwise_comparison): New
>> function.
>> 	(warn_tautological_cmp): Call it.
>>
>> 	* doc/invoke.texi: Update -Wtautological-compare documentation.
>>
>> 	* c-c++-common/Wtautological-compare-5.c: New test.
>>
>> diff --git gcc/c-family/c-warn.c gcc/c-family/c-warn.c
>> index 9c3073444cf..0749d16a50f 100644
>> --- gcc/c-family/c-warn.c
>> +++ gcc/c-family/c-warn.c
>> @@ -321,6 +321,59 @@ find_array_ref_with_const_idx_r (tree *expr_p,
>> int *, void *)
>>    return NULL_TREE;
>>  }
>>
>> +/* Subroutine of warn_tautological_cmp.  Warn about bitwise
>> comparison
>> +   that always evaluate to true or false.  LOC is the location of
>> the
>> +   ==/!= comparison specified by CODE; LHS and RHS are the usual
>> operands
>> +   of this comparison.  */
>> +
>> +static void
>> +warn_tautological_bitwise_comparison (location_t loc, tree_code
>> code,
>> +				      tree lhs, tree rhs)
>> +{
>> +  if (code != EQ_EXPR && code != NE_EXPR)
>> +    return;
>> +
>> +  /* Extract the operands from e.g. (x & 8) == 4.  */
>> +  tree bitop;
>> +  tree cst;
>> +  if ((TREE_CODE (lhs) == BIT_AND_EXPR
>> +       || TREE_CODE (lhs) == BIT_IOR_EXPR)
>> +      && TREE_CODE (rhs) == INTEGER_CST)
>> +    bitop = lhs, cst = rhs;
>> +  else if ((TREE_CODE (rhs) == BIT_AND_EXPR
>> +	    || TREE_CODE (rhs) == BIT_IOR_EXPR)
>> +	   && TREE_CODE (lhs) == INTEGER_CST)
>> +    bitop = rhs, cst = lhs;
>> +  else
>> +    return;
>> +
>> +  tree bitopcst;
>> +  if (TREE_CODE (TREE_OPERAND (bitop, 0)) == INTEGER_CST)
>> +    bitopcst = TREE_OPERAND (bitop, 0);
>> +  else if (TREE_CODE (TREE_OPERAND (bitop, 1)) == INTEGER_CST)
>> +    bitopcst = TREE_OPERAND (bitop, 1);
>> +  else
>> +    return;
>> +
>> +  wide_int res;
>> +  if (TREE_CODE (bitop) == BIT_AND_EXPR)
>> +    res = wi::bit_and (bitopcst, cst);
>> +  else
>> +    res = wi::bit_or (bitopcst, cst);
>> +
>> +  /* For BIT_AND only warn if (CST2 & CST1) != CST1, and
>> +     for BIT_OR only if (CST2 | CST1) != CST1.  */
>> +  if (res == cst)
>> +    return;
>> +
>> +  if (code == EQ_EXPR)
>> +    warning_at (loc, OPT_Wtautological_compare,
>> +		"bitwise comparison always evaluates to false");
>> +  else
>> +    warning_at (loc, OPT_Wtautological_compare,
>> +		"bitwise comparison always evaluates to true");
>> +}
>> +
>>  /* Warn if a self-comparison always evaluates to true or false.  LOC
>>     is the location of the comparison with code CODE, LHS and RHS are
>>     operands of the comparison.  */
>> @@ -337,6 +390,8 @@ warn_tautological_cmp (location_t loc, enum
>> tree_code code, tree lhs, tree rhs)
>>        || from_macro_expansion_at (EXPR_LOCATION (rhs)))
>>      return;
>>
>> +  warn_tautological_bitwise_comparison (loc, code, lhs, rhs);
>> +
>>    /* We do not warn for constants because they are typical of macro
>>       expansions that test for features, sizeof, and similar.  */
>>    if (CONSTANT_CLASS_P (fold_for_warn (lhs))
>> diff --git gcc/doc/invoke.texi gcc/doc/invoke.texi
>> index ec29f1d629e..72a16a19711 100644
>> --- gcc/doc/invoke.texi
>> +++ gcc/doc/invoke.texi
>> @@ -5484,6 +5484,14 @@ int i = 1;
>>  @dots{}
>>  if (i > i) @{ @dots{} @}
>>  @end smallexample
>> +
>> +This warning also warns about bitwise comparisons that always
>> evaluate
>> +to true or false, for instance:
>> +@smallexample
>> +if ((a & 16) == 10) @{ @dots{} @}
>> +@end smallexample
>> +will always be false.
>> +
>>  This warning is enabled by @option{-Wall}.
>>
>>  @item -Wtrampolines
>> diff --git gcc/testsuite/c-c++-common/Wtautological-compare-5.c
>> gcc/testsuite/c-c++-common/Wtautological-compare-5.c
>> index e69de29bb2d..4664bfdeae6 100644
>> --- gcc/testsuite/c-c++-common/Wtautological-compare-5.c
>> +++ gcc/testsuite/c-c++-common/Wtautological-compare-5.c
>> @@ -0,0 +1,106 @@
>> +/* PR c/81783 */
>> +/* { dg-do compile } */
>> +/* { dg-options "-Wtautological-compare" } */
>> +
>> +enum E { FOO = 128 };
>> +
>> +int
>> +f (int a)
>> +{
>> +  if ((a & 16) == 10) /* { dg-warning "bitwise comparison always
>> evaluates to false" } */
>> +    return 1;
>> +  if ((16 & a) == 10) /* { dg-warning "bitwise comparison always
>> evaluates to false" } */
>> +    return 1;
>> +  if (10 == (a & 16)) /* { dg-warning "bitwise comparison always
>> evaluates to false" } */
>> +    return 1;
>> +  if (10 == (16 & a)) /* { dg-warning "bitwise comparison always
>> evaluates to false" } */
>> +    return 1;
>> +
>> +  if ((a & 16) != 10) /* { dg-warning "bitwise comparison always
>> evaluates to true" } */
>> +    return 1;
>> +  if ((16 & a) != 10) /* { dg-warning "bitwise comparison always
>> evaluates to true" } */
>> +    return 1;
>> +  if (10 != (a & 16)) /* { dg-warning "bitwise comparison always
>> evaluates to true" } */
>> +    return 1;
>> +  if (10 != (16 & a)) /* { dg-warning "bitwise comparison always
>> evaluates to true" } */
>> +    return 1;
>> +
>> +  if ((a & 9) == 8)
>> +    return 1;
>> +  if ((9 & a) == 8)
>> +    return 1;
>> +  if (8 == (a & 9))
>> +    return 1;
>> +  if (8 == (9 & a))
>> +    return 1;
>> +
>> +  if ((a & 9) != 8)
>> +    return 1;
>> +  if ((9 & a) != 8)
>> +    return 1;
>> +  if (8 != (a & 9))
>> +    return 1;
>> +  if (8 != (9 & a))
>> +    return 1;
>> +
>> +  if ((a | 16) == 10) /* { dg-warning "bitwise comparison always
>> evaluates to false" } */
>> +    return 1;
>> +  if ((16 | a) == 10) /* { dg-warning "bitwise comparison always
>> evaluates to false" } */
>> +    return 1;
>> +  if (10 == (a | 16)) /* { dg-warning "bitwise comparison always
>> evaluates to false" } */
>> +    return 1;
>> +  if (10 == (16 | a)) /* { dg-warning "bitwise comparison always
>> evaluates to false" } */
>> +    return 1;
>> +
>> +  if ((a | 16) != 10) /* { dg-warning "bitwise comparison always
>> evaluates to true" } */
>> +    return 1;
>> +  if ((16 | a) != 10) /* { dg-warning "bitwise comparison always
>> evaluates to true" } */
>> +    return 1;
>> +  if (10 != (a | 16)) /* { dg-warning "bitwise comparison always
>> evaluates to true" } */
>> +    return 1;
>> +  if (10 != (16 | a)) /* { dg-warning "bitwise comparison always
>> evaluates to true" } */
>> +    return 1;
>> +
>> +  if ((a | 9) == 8) /* { dg-warning "bitwise comparison always
>> evaluates to false" } */
>> +    return 1;
>> +  if ((9 | a) == 8) /* { dg-warning "bitwise comparison always
>> evaluates to false" } */
>> +    return 1;
>> +  if (8 == (a | 9)) /* { dg-warning "bitwise comparison always
>> evaluates to false" } */
>> +    return 1;
>> +  if (8 == (9 | a)) /* { dg-warning "bitwise comparison always
>> evaluates to false" } */
>> +    return 1;
>> +
>> +  if ((a | 9) != 8) /* { dg-warning "bitwise comparison always
>> evaluates to true" } */
>> +    return 1;
>> +  if ((9 | a) != 8) /* { dg-warning "bitwise comparison always
>> evaluates to true" } */
>> +    return 1;
>> +  if (8 != (a | 9)) /* { dg-warning "bitwise comparison always
>> evaluates to true" } */
>> +    return 1;
>> +  if (8 != (9 | a)) /* { dg-warning "bitwise comparison always
>> evaluates to true" } */
>> +    return 1;
>> +
>> +  if ((a & 128) != 1) /* { dg-warning "bitwise comparison always
>> evaluates to true" } */
>> +    return 1;
>> +  if ((128 & a) != 1) /* { dg-warning "bitwise comparison always
>> evaluates to true" } */
>> +    return 1;
>> +  if ((a & FOO) != 1) /* { dg-warning "bitwise comparison always
>> evaluates to true" } */
>> +    return 1;
>> +  if ((FOO & a) != 1) /* { dg-warning "bitwise comparison always
>> evaluates to true" } */
>> +    return 1;
>> +  if ((a & 128) == 1) /* { dg-warning "bitwise comparison always
>> evaluates to false" } */
>> +    return 1;
>> +  if ((128 & a) == 1) /* { dg-warning "bitwise comparison always
>> evaluates to false" } */
>> +    return 1;
>> +  if ((a & FOO) == 1) /* { dg-warning "bitwise comparison always
>> evaluates to false" } */
>> +    return 1;
>> +  if ((FOO & a) == 1) /* { dg-warning "bitwise comparison always
>> evaluates to false" } */
>> +    return 1;
>> +
>> +#define N 0x10
>> +  if ((a & N) == 10) /* { dg-bogus "bitwise comparison always
>> evaluates to false" "" { xfail *-*-* } } */
>> +    return 1;
>> +  if ((a | N) == 10) /* { dg-bogus "bitwise comparison always
>> evaluates to false" "" { xfail *-*-* } } */
>> +    return 1;
>> +
>> +  return 0;
>> +}
>>
>> 	Marek
>