On 8/16/17, David Malcolm <dmalc...@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 <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 >