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?
>
> 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 <pola...@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
