| Message ID | CAHMCd+VzCAzH3LheeairBb4CO2c_tv30f7MFARqmMC0Vk8uAKg@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | PR c/17896 Check for missplaced bitwise op | expand |
ping Em sex, 24 de mai de 2019 às 09:53, Rafael Tsuha <rafael.tsuha@usp.br> escreveu: > > 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.
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
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); + if (code == BIT_IOR_EXPR) + warning_at (location, OPT_Wlogical_op, "bitwise %<or%>" + " applied to boolean type"); + else if (code == BIT_AND_EXPR) + warning_at (location, OPT_Wlogical_op, "bitwise %<and%>" + " applied to boolean type"); + TREE_NO_WARNING (op_left) = true; + return; + } + /* We do not warn for constants because they are typical of macro expansions that test for features. */ if (CONSTANT_CLASS_P (fold_for_warn (op_left)) Index: gcc/testsuite/gcc.dg/boolean-bitwise.c =================================================================== --- gcc/testsuite/gcc.dg/boolean-bitwise.c (nonexistent) +++ gcc/testsuite/gcc.dg/boolean-bitwise.c (working copy) @@ -0,0 +1,28 @@ +/* Test operation of -Wparentheses. booleans joined by & or | */ + +/* { dg-do compile } */ +/* { dg-options "-Wlogical-op" } */ + +int foo (int); + +int +bar (int a, int b, int c) +{ + foo (a == b & b == c); /* { dg-warning "boolean" "correct warning" } */ + foo (a == b & (b == c)); /* { dg-warning "boolean" "correct warning" } */ + foo ((a == b) & b == c); /* { dg-warning "boolean" "correct warning" } */ + foo (++a == b & b == c); /* { dg-warning "boolean" "correct warning" } */ + foo ((a == b) & (b == c)); /* { dg-warning "boolean" "correct warning" } */ + foo (a == b && b == c); + foo (a & b); + + foo (a == b | b == c); /* { dg-warning "boolean" "correct warning" } */ + foo (a == b | (b == c)); /* { dg-warning "boolean" "correct warning" } */ + foo ((a == b) | b == c); /* { dg-warning "boolean" "correct warning" } */ + foo (++a == b | b == c); /* { dg-warning "boolean" "correct warning" } */ + foo ((a == b) | (b == c)); /* { dg-warning "boolean" "correct warning" } */ + foo (a == b || b == c); + foo (a | b); + + return 0; +}
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.