From 660eb7e9dee46ef1c986d5a4fa5cbd182b435518 Mon Sep 17 00:00:00 2001 From: Jakub Jelinek Date: Thu, 25 Mar 2021 11:33:35 +0100 Subject: [PATCH] c-family: Fix up -Wduplicated-branches for union members [PR99565] Honza has fairly recently changed operand_equal_p to compare DECL_FIELD_OFFSET for COMPONENT_REFs when comparing addresses. As the first testcase in this patch shows, while that is very nice for optimizations, for the -Wduplicated-branches warning it causes regressions. Pedantically a union in both C and C++ has only one active member at a time, so using some other union member even if it has the same type is UB, so I think the warning shouldn't warn when it sees access to different fields that happen to have the same offset and should consider them different. In my first attempt to fix this I've keyed the old behavior on OEP_LEXICOGRAPHIC, but unfortunately that has various problems, the warning has a quick non-lexicographic compare in build_conditional_expr* and another lexicographic more expensive one later during genericization and turning the first one into lexicographic would mean wasting compile time on large conditionals. So, this patch instead introduces a new OEP_ flag and makes sure to pass it to operand_equal_p in all -Wduplicated-branches cases. The cvt.c changes are because on the other testcase we were warning with UNKNOWN_LOCATION, so the user wouldn't really know where the questionable code is. 2021-03-25 Jakub Jelinek PR c++/99565 * tree-core.h (enum operand_equal_flag): Add OEP_ADDRESS_OF_SAME_FIELD. * fold-const.c (operand_compare::operand_equal_p): Don't compare field offsets if OEP_ADDRESS_OF_SAME_FIELD. * c-warn.c (do_warn_duplicated_branches): Pass also OEP_ADDRESS_OF_SAME_FIELD to operand_equal_p. * c-typeck.c (build_conditional_expr): Pass OEP_ADDRESS_OF_SAME_FIELD to operand_equal_p. * call.c (build_conditional_expr_1): Pass OEP_ADDRESS_OF_SAME_FIELD to operand_equal_p. * cvt.c (convert_to_void): Preserve location_t on COND_EXPR or or COMPOUND_EXPR. * g++.dg/warn/Wduplicated-branches6.C: New test. * g++.dg/warn/Wduplicated-branches7.C: New test. --- gcc/c-family/c-warn.c | 3 ++- gcc/c/c-typeck.c | 2 +- gcc/cp/call.c | 3 ++- gcc/cp/cvt.c | 8 ++++---- gcc/fold-const.c | 3 ++- gcc/testsuite/g++.dg/warn/Wduplicated-branches6.C | 9 +++++++++ gcc/testsuite/g++.dg/warn/Wduplicated-branches7.C | 11 +++++++++++ gcc/tree-core.h | 5 ++++- 8 files changed, 35 insertions(+), 9 deletions(-) create mode 100644 gcc/testsuite/g++.dg/warn/Wduplicated-branches6.C create mode 100644 gcc/testsuite/g++.dg/warn/Wduplicated-branches7.C diff --git a/gcc/c-family/c-warn.c b/gcc/c-family/c-warn.c index 2347e0b..534e4f3 100644 --- a/gcc/c-family/c-warn.c +++ b/gcc/c-family/c-warn.c @@ -2779,7 +2779,8 @@ do_warn_duplicated_branches (tree expr) /* Compare the hashes. */ if (h0 == h1 - && operand_equal_p (thenb, elseb, OEP_LEXICOGRAPHIC) + && operand_equal_p (thenb, elseb, OEP_LEXICOGRAPHIC + | OEP_ADDRESS_OF_SAME_FIELD) /* Don't warn if any of the branches or their subexpressions comes from a macro. */ && !walk_tree_without_duplicates (&thenb, expr_from_macro_expansion_r, diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c index 2685afb..21eab00 100644 --- a/gcc/c/c-typeck.c +++ b/gcc/c/c-typeck.c @@ -5544,7 +5544,7 @@ build_conditional_expr (location_t colon_loc, tree ifexp, bool ifexp_bcp, warn here, because the COND_EXPR will be turned into OP1. */ if (warn_duplicated_branches && TREE_CODE (ret) == COND_EXPR - && (op1 == op2 || operand_equal_p (op1, op2, 0))) + && (op1 == op2 || operand_equal_p (op1, op2, OEP_ADDRESS_OF_SAME_FIELD))) warning_at (EXPR_LOCATION (ret), OPT_Wduplicated_branches, "this condition has identical branches"); diff --git a/gcc/cp/call.c b/gcc/cp/call.c index 390b8aa..bab0c89 100644 --- a/gcc/cp/call.c +++ b/gcc/cp/call.c @@ -5798,7 +5798,8 @@ build_conditional_expr_1 (const op_location_t &loc, warn here, because the COND_EXPR will be turned into ARG2. */ if (warn_duplicated_branches && (complain & tf_warning) - && (arg2 == arg3 || operand_equal_p (arg2, arg3, 0))) + && (arg2 == arg3 || operand_equal_p (arg2, arg3, + OEP_ADDRESS_OF_SAME_FIELD))) warning_at (EXPR_LOCATION (result), OPT_Wduplicated_branches, "this condition has identical branches"); diff --git a/gcc/cp/cvt.c b/gcc/cp/cvt.c index 2ea3210..d105113 100644 --- a/gcc/cp/cvt.c +++ b/gcc/cp/cvt.c @@ -1198,8 +1198,8 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain) new_op2 = convert_to_void (op2, ICV_CAST, complain); } - expr = build3 (COND_EXPR, TREE_TYPE (new_op2), - TREE_OPERAND (expr, 0), new_op1, new_op2); + expr = build3_loc (loc, COND_EXPR, TREE_TYPE (new_op2), + TREE_OPERAND (expr, 0), new_op1, new_op2); break; } @@ -1215,8 +1215,8 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain) if (new_op1 != op1) { - tree t = build2 (COMPOUND_EXPR, TREE_TYPE (new_op1), - TREE_OPERAND (expr, 0), new_op1); + tree t = build2_loc (loc, COMPOUND_EXPR, TREE_TYPE (new_op1), + TREE_OPERAND (expr, 0), new_op1); expr = t; } diff --git a/gcc/fold-const.c b/gcc/fold-const.c index 1ebc73d..4c48d70 100644 --- a/gcc/fold-const.c +++ b/gcc/fold-const.c @@ -3317,7 +3317,8 @@ operand_compare::operand_equal_p (const_tree arg0, const_tree arg1, flags &= ~OEP_ADDRESS_OF; if (!OP_SAME (1)) { - if (compare_address) + if (compare_address + && (flags & OEP_ADDRESS_OF_SAME_FIELD) == 0) { if (TREE_OPERAND (arg0, 2) || TREE_OPERAND (arg1, 2)) diff --git a/gcc/testsuite/g++.dg/warn/Wduplicated-branches6.C b/gcc/testsuite/g++.dg/warn/Wduplicated-branches6.C new file mode 100644 index 0000000..70f0bee --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wduplicated-branches6.C @@ -0,0 +1,9 @@ +// PR c++/99565 +// { dg-do compile } +// { dg-options "-Wduplicated-branches" } + +struct A { + union { int a; int b; }; + int& foo (bool x) { return x ? a : b; } // { dg-bogus "this condition has identical branches" } + void bar (bool x, int y) { if (x) a = y; else b = y; } +}; diff --git a/gcc/testsuite/g++.dg/warn/Wduplicated-branches7.C b/gcc/testsuite/g++.dg/warn/Wduplicated-branches7.C new file mode 100644 index 0000000..bbc0793 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wduplicated-branches7.C @@ -0,0 +1,11 @@ +// PR c++/99565 +// { dg-do compile } +// { dg-options "-Wduplicated-branches" } + +int a; + +void +foo (bool x) +{ + x ? ++a : ++a; // { dg-warning "this condition has identical branches" } +} diff --git a/gcc/tree-core.h b/gcc/tree-core.h index d2e6c89..07ddf91 100644 --- a/gcc/tree-core.h +++ b/gcc/tree-core.h @@ -896,7 +896,10 @@ enum operand_equal_flag { OEP_HASH_CHECK = 32, /* Makes operand_equal_p handle more expressions: */ OEP_LEXICOGRAPHIC = 64, - OEP_BITWISE = 128 + OEP_BITWISE = 128, + /* For OEP_ADDRESS_OF of COMPONENT_REFs, only consider same fields as + equivalent rather than also different fields with the same offset. */ + OEP_ADDRESS_OF_SAME_FIELD = 256 }; /* Enum and arrays used for tree allocation stats. -- 2.7.4