On Thu, May 4, 2017 at 12:27 PM, Marek Polacek <pola...@redhat.com> wrote:
> This PR points out a missing -Wlogical-op warning (unless you use -fwrapv).
>
> We end up calling warn_logical_operator with op_left that is
> C_M_C_E <a + 1> != 0
> and op_right that is
> a + 1
>
> But make_range just cannot handle C_M_C_Es right; for exprs it simply picks
> the
> first operand and that doesn't work with C_M_C_E, where we'd need to use
> C_MAYBE_CONST_EXPR_EXPR. warn_logical_operator has code that strips C_M_C_E
> but that is insufficient. I think we have to use walk_tree, because as this
> testcase shows, those C_M_C_Es might not be the outermost expression.
>
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
>
> 2017-05-03 Marek Polacek <pola...@redhat.com>
>
> PR c/80525
> * c-warn.c (unwrap_c_maybe_const): New.
> (warn_logical_operator): Call it.
>
> * c-c++-common/Wlogical-op-1.c: Don't use -fwrapv anymore.
> * c-c++-common/Wlogical-op-2.c: New test.
>
> diff --git gcc/c-family/c-warn.c gcc/c-family/c-warn.c
> index 45dd583..0f17bc5 100644
> --- gcc/c-family/c-warn.c
> +++ gcc/c-family/c-warn.c
> @@ -112,6 +112,21 @@ overflow_warning (location_t loc, tree value)
> }
> }
>
> +/* Helper function for walk_tree. Unwrap C_MAYBE_CONST_EXPRs in an
> expression
> + pointed to by TP. */
> +
> +static tree
> +unwrap_c_maybe_const (tree *tp, int *walk_subtrees, void *)
> +{
> + if (TREE_CODE (*tp) == C_MAYBE_CONST_EXPR)
> + {
> + *tp = C_MAYBE_CONST_EXPR_EXPR (*tp);
> + /* C_MAYBE_CONST_EXPRs don't nest. */
> + *walk_subtrees = false;
This changes trees in-place -- do you need to operate on a copy?
> + }
> + return NULL_TREE;
> +}
> +
> /* 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
> @@ -189,11 +204,10 @@ warn_logical_operator (location_t location, enum
> tree_code code, tree type,
> (with OR) or trivially false (with AND). If so, do not warn.
> This is a common idiom for testing ranges of data types in
> portable code. */
> + walk_tree_without_duplicates (&op_left, unwrap_c_maybe_const, NULL);
> lhs = make_range (op_left, &in0_p, &low0, &high0, &strict_overflow_p);
> if (!lhs)
> return;
> - if (TREE_CODE (lhs) == C_MAYBE_CONST_EXPR)
> - lhs = C_MAYBE_CONST_EXPR_EXPR (lhs);
>
> /* If this is an OR operation, invert both sides; now, the result
> should be always false to get a warning. */
> @@ -204,11 +218,10 @@ warn_logical_operator (location_t location, enum
> tree_code code, tree type,
> if (tem && integer_zerop (tem))
> return;
>
> + walk_tree_without_duplicates (&op_right, unwrap_c_maybe_const, NULL);
> rhs = make_range (op_right, &in1_p, &low1, &high1, &strict_overflow_p);
> if (!rhs)
> return;
> - if (TREE_CODE (rhs) == C_MAYBE_CONST_EXPR)
> - rhs = C_MAYBE_CONST_EXPR_EXPR (rhs);
>
> /* If this is an OR operation, invert both sides; now, the result
> should be always false to get a warning. */
> diff --git gcc/testsuite/c-c++-common/Wlogical-op-1.c
> gcc/testsuite/c-c++-common/Wlogical-op-1.c
> index e89a35a..c5f992a 100644
> --- gcc/testsuite/c-c++-common/Wlogical-op-1.c
> +++ gcc/testsuite/c-c++-common/Wlogical-op-1.c
> @@ -1,8 +1,6 @@
> /* PR c/63357 */
> /* { dg-do compile } */
> -/* For -fwrapv see PR80525, xfailing the subtest isn't possible as it passes
> - with the C++ FE which doesn't have maybe_const_expr. */
> -/* { dg-options "-fwrapv -Wlogical-op" } */
> +/* { dg-options "-Wlogical-op" } */
>
> #ifndef __cplusplus
> # define bool _Bool
> diff --git gcc/testsuite/c-c++-common/Wlogical-op-2.c
> gcc/testsuite/c-c++-common/Wlogical-op-2.c
> index e69de29..6360ef9 100644
> --- gcc/testsuite/c-c++-common/Wlogical-op-2.c
> +++ gcc/testsuite/c-c++-common/Wlogical-op-2.c
> @@ -0,0 +1,12 @@
> +/* PR c/80525 */
> +/* { dg-do compile } */
> +/* { dg-options "-Wlogical-op" } */
> +
> +int
> +fn (int a, int b)
> +{
> + if ((a + 1) && (a + 1)) /* { dg-warning "logical .and. of equal
> expressions" } */
> + return a;
> + if ((a + 1) || (a + 1)) /* { dg-warning "logical .or. of equal
> expressions" } */
> + return b;
> +}
>
> Marek
