| Message ID | 20170816142917.GB17069@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <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
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 >
On Wed, Aug 16, 2017 at 11:07:36AM -0400, David Malcolm 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? Just my past experience. Sometimes you can't really control the macro and then you get annoying warnings. E.g. I had to tweak the warning that warns about if (i == i) to not warn about #define N 2 if (a[N] == a[2]) {} because that gave bogus warning during bootstrap, if I recall well. > 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? I'm glad you think so. More than happy to make it an expected warning. > > 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? I feel like we should, but some might feel otherwise. Thanks, Marek
Ping. On Wed, Aug 16, 2017 at 05:24:56PM +0200, Marek Polacek wrote: > On Wed, Aug 16, 2017 at 11:07:36AM -0400, David Malcolm 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? > > Just my past experience. Sometimes you can't really control the macro > and then you get annoying warnings. > > E.g. I had to tweak the warning that warns about if (i == i) to not warn about > > #define N 2 > if (a[N] == a[2]) {} > > because that gave bogus warning during bootstrap, if I recall well. > > > 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? > > I'm glad you think so. More than happy to make it an expected warning. > > > > 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? > > I feel like we should, but some might feel otherwise. > > Thanks, > > Marek Marek
Ping. On Fri, Aug 25, 2017 at 02:47:45PM +0200, Marek Polacek wrote: > Ping. > > On Wed, Aug 16, 2017 at 05:24:56PM +0200, Marek Polacek wrote: > > On Wed, Aug 16, 2017 at 11:07:36AM -0400, David Malcolm 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? > > > > Just my past experience. Sometimes you can't really control the macro > > and then you get annoying warnings. > > > > E.g. I had to tweak the warning that warns about if (i == i) to not warn about > > > > #define N 2 > > if (a[N] == a[2]) {} > > > > because that gave bogus warning during bootstrap, if I recall well. > > > > > 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? > > > > I'm glad you think so. More than happy to make it an expected warning. > > > > > > 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? > > > > I feel like we should, but some might feel otherwise. > > > > Thanks, > > > > Marek > > Marek Marek
On 08/16/2017 08:29 AM, 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. > > 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>. > > 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. OK. jeff
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; +}
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. 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>. 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. Marek