On Tue, Mar 16, 2021 at 06:56:47PM +0100, Jakub Jelinek 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
> };
>
> /* 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)
This change isn't strictly necessary, OEP_DUPLICATED_BRANCHES is needed
mainly in build_conditional_expr*, right? (It makes sense to me though,
so let's leave it in.)
> /* 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");
The c/ and c-family/ parts are ok, thanks.
Marek
