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  <jakub@redhat.com>

	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.

diff --git a/gcc/c-family/c-warn.c b/gcc/c-family/c-warn.c
index 2347e0b2e5d64ee869da32f6c85cc1211e8ae67e..534e4f3aae361baf31ef3a9b6423e0b1daa8705c 100644 (file)
--- 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 2685afb2af2ea4fa6e090d28a6856d6cfa921f93..21eab00d4b372e22a0e7323cf629b0c6a34b2368 100644 (file)
--- 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 390b8aa4325f1013140c6a3bbf2420382fcad702..bab0c89d6c72f1c0d00d586b4f5a1b6d71d15ead 100644 (file)
--- 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 2ea32101f07f9e81de34de05aaba6b7db01d7f05..d1051139e7006314363e8b94c18e62d9dce8ebca 100644 (file)
--- 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 1ebc73d065a5d952fa94606f4a9e9951cd8859cc..4c48d7076e796b89188d1fb6a66fa5527a18f2fb 100644 (file)
--- 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 (file)
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 (file)
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 d2e6c895e42f971628656c7b9bbdb00ebd537a58..07ddf91a2303fed06bfc332c4476090f0bb01723 100644 (file)
--- 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.