On 3/16/21 11:56 AM, Jakub Jelinek via Gcc-patches wrote:
Hi!
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.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
2021-03-16 Jakub Jelinek <ja...@redhat.com>
PR c++/99565
* tree-core.h (enum operand_equal_flag): Add OEP_DUPLICATED_BRANCHES.
* fold-const.c (operand_compare::operand_equal_p): Don't compare
field offsets if OEP_DUPLICATED_BRANCHES.
* c-warn.c (do_warn_duplicated_branches): Pass also
OEP_DUPLICATED_BRANCHES to operand_equal_p.
* c-typeck.c (build_conditional_expr): Pass OEP_DUPLICATED_BRANCHES
to operand_equal_p.
* call.c (build_conditional_expr_1): Pass OEP_DUPLICATED_BRANCHES
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/tree-core.h.jj 2021-01-16 22:52:33.654413400 +0100
+++ gcc/tree-core.h 2021-03-16 16:24:06.136829900 +0100
@@ -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,
+ /* Considers different fields different even when they have
+ the same offset. */
+ OEP_DUPLICATED_BRANCHES = 256
It seems sort of "inverted:" I'd expect OEP_LEXICOGRAPHIC on its
own to do a lexicographical comparison, without having to set
an additional bit to ask for it. If a more refined form of
a lexicographical comparison is needed that considers
lexicographically distinct names equal then adding a new bit
for that would seem like the right design.
I'd also expect the name of the new bit to be independent of
the context where it's used (like -Wduplicated-branches), in case
it also needs to be used in an unrelated warning. (E.g., the new
-Wvla-parameter also uses OEP_LEXICOGRAPHICAL and if it does need
to set a new bit it would look odd if that bit had the name of
an unrelated warning in it.)
Martin
};
/* Enum and arrays used for tree allocation stats.
--- gcc/fold-const.c.jj 2021-02-24 12:10:17.571212194 +0100
+++ gcc/fold-const.c 2021-03-16 16:19:16.327018956 +0100
@@ -3317,7 +3317,7 @@ operand_compare::operand_equal_p (const_
flags &= ~OEP_ADDRESS_OF;
if (!OP_SAME (1))
{
- if (compare_address)
+ if (compare_address && (flags & OEP_DUPLICATED_BRANCHES) == 0)
{
if (TREE_OPERAND (arg0, 2)
|| TREE_OPERAND (arg1, 2))
--- gcc/c-family/c-warn.c.jj 2021-02-12 23:57:30.459142340 +0100
+++ gcc/c-family/c-warn.c 2021-03-16 16:24:19.224685926 +0100
@@ -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_DUPLICATED_BRANCHES)
/* 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,
--- gcc/c/c-typeck.c.jj 2021-02-18 22:17:44.184657291 +0100
+++ gcc/c/c-typeck.c 2021-03-16 16:23:58.335915719 +0100
@@ -5490,7 +5490,7 @@ build_conditional_expr (location_t colon
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_DUPLICATED_BRANCHES)))
warning_at (EXPR_LOCATION (ret), OPT_Wduplicated_branches,
"this condition has identical branches");
--- gcc/cp/call.c.jj 2021-03-12 10:11:17.642122302 +0100
+++ gcc/cp/call.c 2021-03-16 16:23:49.647011304 +0100
@@ -5798,7 +5798,8 @@ build_conditional_expr_1 (const op_locat
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_DUPLICATED_BRANCHES)))
warning_at (EXPR_LOCATION (result), OPT_Wduplicated_branches,
"this condition has identical branches");
--- gcc/cp/cvt.c.jj 2021-03-04 16:03:40.089323550 +0100
+++ gcc/cp/cvt.c 2021-03-16 16:36:26.903680722 +0100
@@ -1198,8 +1198,8 @@ convert_to_void (tree expr, impl_conv_vo
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_vo
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;
}
--- gcc/testsuite/g++.dg/warn/Wduplicated-branches6.C.jj 2021-03-16 14:44:06.655822730 +0100
+++ gcc/testsuite/g++.dg/warn/Wduplicated-branches6.C 2021-03-16
14:42:01.804198665 +0100
@@ -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; }
+};
--- gcc/testsuite/g++.dg/warn/Wduplicated-branches7.C.jj 2021-03-16
16:18:37.157450078 +0100
+++ gcc/testsuite/g++.dg/warn/Wduplicated-branches7.C 2021-03-16
16:18:07.437777203 +0100
@@ -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"
}
+}
Jakub
