| Message ID | 20151118160348.GC21807@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 18 Nov 2015, Marek Polacek wrote: > diff --git gcc/c/c-typeck.c gcc/c/c-typeck.c > index c18c307..48c1a98 100644 > --- gcc/c/c-typeck.c > +++ gcc/c/c-typeck.c > @@ -3512,7 +3512,11 @@ parser_build_binary_op (location_t location, enum tree_code code, > code1, arg1.value, code2, arg2.value); > > if (warn_tautological_compare) > - warn_tautological_cmp (location, code, arg1.value, arg2.value); > + warn_tautological_cmp (location, code, > + /* This function will try to fold both > + args, so don't leak C_MAYBE_CONST_EXPR. */ > + remove_c_maybe_const_expr (arg1.value), > + remove_c_maybe_const_expr (arg2.value)); remove_c_maybe_const_expr doesn't seem to be quite what you want. Apart from this not being a case covered by the comment on the function, you're ignoring the possibility of the side effects in the C_MAYBE_CONST_EXPR_PRE. So I think you want something else: if either argument is a C_MAYBE_CONST_EXPR whose C_MAYBE_CONST_EXPR_PRE is non-NULL and has side-effects, don't run the comparison, and otherwise it's OK to go down to the C_MAYBE_CONST_EXPR_EXPR.
diff --git gcc/c/c-typeck.c gcc/c/c-typeck.c index c18c307..48c1a98 100644 --- gcc/c/c-typeck.c +++ gcc/c/c-typeck.c @@ -3512,7 +3512,11 @@ parser_build_binary_op (location_t location, enum tree_code code, code1, arg1.value, code2, arg2.value); if (warn_tautological_compare) - warn_tautological_cmp (location, code, arg1.value, arg2.value); + warn_tautological_cmp (location, code, + /* This function will try to fold both + args, so don't leak C_MAYBE_CONST_EXPR. */ + remove_c_maybe_const_expr (arg1.value), + remove_c_maybe_const_expr (arg2.value)); if (warn_logical_not_paren && TREE_CODE_CLASS (code) == tcc_comparison diff --git gcc/testsuite/gcc.dg/pr68412-2.c gcc/testsuite/gcc.dg/pr68412-2.c index e69de29..be1dcfa 100644 --- gcc/testsuite/gcc.dg/pr68412-2.c +++ gcc/testsuite/gcc.dg/pr68412-2.c @@ -0,0 +1,15 @@ +/* PR c/68412 */ +/* { dg-do compile } */ +/* { dg-options "-Wall -Wextra" } */ + +int +fn1 (int i) +{ + return ({ i; }) == ({ i; }); /* { dg-warning "self-comparison always evaluates to true" } */ +} + +int +fn2 (int i) +{ + return ({ i + 1; }) != ({ i + 1; }); /* { dg-warning "self-comparison always evaluates to false" } */ +} diff --git gcc/testsuite/gcc.dg/pr68412.c gcc/testsuite/gcc.dg/pr68412.c index e69de29..825eb63 100644 --- gcc/testsuite/gcc.dg/pr68412.c +++ gcc/testsuite/gcc.dg/pr68412.c @@ -0,0 +1,41 @@ +/* PR c/68412 */ +/* { dg-do compile } */ +/* { dg-options "-Wall -Wextra" } */ + +#define M (sizeof (int) * __CHAR_BIT__) + +int +fn1 (int i) +{ + return i == (-1 << 8); /* { dg-warning "left shift of negative value" } */ +} + +int +fn2 (int i) +{ + return i == (1 << M); /* { dg-warning "left shift count" } */ +} + +int +fn3 (int i) +{ + return i == 10 << (M - 1); /* { dg-warning "requires" } */ +} + +int +fn4 (int i) +{ + return i == 1 << -1; /* { dg-warning "left shift count" } */ +} + +int +fn5 (int i) +{ + return i == 1 >> M; /* { dg-warning "right shift count" } */ +} + +int +fn6 (int i) +{ + return i == 1 >> -1; /* { dg-warning "right shift count" } */ +}
Since C++ delayed folding, warn_tautological_cmp needs to fold its arguments. But sometimes this function gets C_MAYBE_CONST_EXPR from the C FE, and fold() duly ICEs. I was torn if I should just return from warn_tautological_cmp and not warn when it gets C_MAYBE_CONST_EXPR as an argument, or if I should strip C_MAYBE_CONST_EXPR before calling warn_tautological_cmp. I decided for the latter since I think warn_tautological_cmp probably should warn on code such as int i = 10; bool b = ({ i; }) == ({ i; }); (ugh). Bootstrapped/regtested on x86_64-linux, ok for trunk? 2015-11-18 Marek Polacek <polacek@redhat.com> PR c/68412 * c-typeck.c (parser_build_binary_op): Strip C_MAYBE_CONST_EXPR when passing arguments to warn_tautological_compare. * gcc.dg/pr68412.c: New test. * gcc.dg/pr68412-2.c: New test. Marek