Re: Implement -Wduplicated-branches (PR c/64279) (v3)

[David Edelsohn]

This patch introduced a new testsuite failure:

FAIL: g++.dg/warn/Wduplicated-branches1.C  -std=gnu++98 (test for excess errors)
Excess errors:
/nasfarm/edelsohn/src/src/gcc/testsuite/g++.dg/warn/Wduplicated-branches1.C:9:3:
 warning: this condition has identical branches [-Wduplicated-branches]
/nasfarm/edelsohn/src/src/gcc/testsuite/g++.dg/warn/Wduplicated-branches1.C:9:3:
 warning: this condition has identical branches [-Wduplicated-branches]

Thanks, David

Re: Implement -Wduplicated-branches (PR c/64279) (v3)

[Marek Polacek]

On Fri, Jan 20, 2017 at 10:05:22AM -0500, David Edelsohn wrote:
> This patch introduced a new testsuite failure:
> 
> FAIL: g++.dg/warn/Wduplicated-branches1.C  -std=gnu++98 (test for excess 
> errors)
> Excess errors:
> /nasfarm/edelsohn/src/src/gcc/testsuite/g++.dg/warn/Wduplicated-branches1.C:9:3:
>  warning: this condition has identical branches [-Wduplicated-branches]
> /nasfarm/edelsohn/src/src/gcc/testsuite/g++.dg/warn/Wduplicated-branches1.C:9:3:
>  warning: this condition has identical branches [-Wduplicated-branches]

Sorry about that.  I'll investigate.

Marek

Re: Implement -Wduplicated-branches (PR c/64279) (v3)

[Marek Polacek]

On Mon, Jan 16, 2017 at 03:32:59PM -0700, Jeff Law wrote:
> s/indentical/identical in the doc/invoke.texi changes.
 
Fixed.

> > diff --git gcc/c/c-typeck.c gcc/c/c-typeck.c
> > index 96e7351..ed8ffe4 100644
> > --- gcc/c/c-typeck.c
> > +++ gcc/c/c-typeck.c
> > @@ -5193,6 +5193,15 @@ build_conditional_expr (location_t colon_loc, tree 
> > ifexp, bool ifexp_bcp,
> >  ret = build1 (EXCESS_PRECISION_EXPR, semantic_result_type, ret);
> > 
> >protected_set_expr_location (ret, colon_loc);
> > +
> > +  /* If the OP1 and OP2 are the same and don't have side-effects,
> > + 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)))
> Did you want OEP_LEXICOGRAPHIC here, or in this context do we not have to
> worry about the more complex forms?  What about a statement expressions?
> Have we lowered those at this point already?
 
I think we do not want OEP_LEXICOGRAPHIC here, because if the op0 or op1
of ?: have side-effects, they'll survive until c_genericize, so the warning
will warn.  With OEP_LEXICOGRAPHIC, it'd warn twice.

Simple statement expressions are handled (there are a few tests).  More
complicated ({})s are represented as BIND_EXPRs and those aren't handled.

> > diff --git gcc/cp/call.c gcc/cp/call.c
> > index e431221..84931fb 100644
> > --- gcc/cp/call.c
> > +++ gcc/cp/call.c
> > @@ -5302,6 +5302,13 @@ build_conditional_expr_1 (location_t loc, tree arg1, 
> > tree arg2, tree arg3,
> >   valid_operands:
> >result = build3_loc (loc, COND_EXPR, result_type, arg1, arg2, arg3);
> > 
> > +  /* If the ARG2 and ARG3 are the same and don't have side-effects,
> > + warn here, because the COND_EXPR will be turned into ARG2.  */
> > +  if (warn_duplicated_branches
> > +  && (arg2 == arg3 || operand_equal_p (arg2, arg3, 0)))
> Likewise.
 
Same as above.

> So, typo fix in invoke.texi and change to use OEP_LEXICOGRAPHIC in those two
> spots if needed and then OK for the trunk.

Thanks!

This is the patch with the typo fixed.  I'll commit it today after regtesting.

2017-01-20  Marek Polacek  

PR c/64279
* c-common.h (do_warn_duplicated_branches_r): Declare.
* c-gimplify.c (c_genericize): Walk the function tree calling
do_warn_duplicated_branches_r.
* c-warn.c (expr_from_macro_expansion_r): New.
(do_warn_duplicated_branches): New.
(do_warn_duplicated_branches_r): New.
* c.opt (Wduplicated-branches): New option.

* c-typeck.c (build_conditional_expr): Warn about duplicated branches.

* call.c (build_conditional_expr_1): Warn about duplicated branches.
* semantics.c (finish_expr_stmt): Build statement using the proper
location.

* doc/invoke.texi: Document -Wduplicated-branches.
* fold-const.c (operand_equal_p): Handle MODIFY_EXPR, INIT_EXPR,
COMPOUND_EXPR, PREDECREMENT_EXPR, PREINCREMENT_EXPR,
POSTDECREMENT_EXPR, POSTINCREMENT_EXPR, CLEANUP_POINT_EXPR, EXPR_STMT,
STATEMENT_LIST, and RETURN_EXPR.  For non-pure non-const functions
return 0 only when not OEP_LEXICOGRAPHIC.
(fold_build_cleanup_point_expr): Use the expression
location when building CLEANUP_POINT_EXPR.
* tree-core.h (enum operand_equal_flag): Add OEP_LEXICOGRAPHIC.
* tree.c (add_expr): Handle error_mark_node.

* c-c++-common/Wduplicated-branches-1.c: New test.
* c-c++-common/Wduplicated-branches-10.c: New test.
* c-c++-common/Wduplicated-branches-11.c: New test.
* c-c++-common/Wduplicated-branches-12.c: New test.
* c-c++-common/Wduplicated-branches-2.c: New test.
* c-c++-common/Wduplicated-branches-3.c: New test.
* c-c++-common/Wduplicated-branches-4.c: New test.
* c-c++-common/Wduplicated-branches-5.c: New test.
* c-c++-common/Wduplicated-branches-6.c: New test.
* c-c++-common/Wduplicated-branches-7.c: New test.
* c-c++-common/Wduplicated-branches-8.c: New test.
* c-c++-common/Wduplicated-branches-9.c: New test.
* c-c++-common/Wimplicit-fallthrough-7.c: Coalesce dg-warning.
* g++.dg/cpp0x/lambda/lambda-switch.C: Move dg-warning.
* g++.dg/ext/builtin-object-size3.C: Likewise.
* g++.dg/gomp/loop-1.C: Likewise.
* g++.dg/warn/Wduplicated-branches1.C: New test.
* g++.dg/warn/Wduplicated-branches2.C: New test.

diff --git gcc/c-family/c-common.h gcc/c-family/c-common.h
index b838869..06918db 100644
--- gcc/c-family/c-common.h
+++ gcc/c-family/c-common.h
@@ -1537,6 +1537,7 @@ extern void maybe_warn_bool_compare (location_t, enum 
tree_code, tree, tree);
 extern bool maybe_warn_shift_overflow (location_t, tree, tree);
 extern void warn_duplicated_cond_add_or_warn (location_t, tree, vec **);
 extern bool diagnose_mismatched_attributes (tree, tree);
+extern tree 

Re: Implement -Wduplicated-branches (PR c/64279) (v3)

[Jakub Jelinek]

On Thu, Jan 19, 2017 at 05:52:14PM +0100, Marek Polacek wrote:
> > I agree that not warning for 
> >   if (foo)
> > return NULL;
> >   else
> > return NULL;
> > is bad.  But how can I compare those expressions side-by-side?  I'm not 
> > finding
> > anything. :(
> 
> Seems like ENOTIME to address this; will you be ok with the patch as-is
> (modulo Jeff comments), if I open a PR about the above test case?

Yeah.

Jakub

Re: Implement -Wduplicated-branches (PR c/64279) (v3)

[Marek Polacek]

On Mon, Jan 09, 2017 at 02:39:30PM +0100, Marek Polacek wrote:
> On Mon, Jan 09, 2017 at 12:18:01PM +0100, Jakub Jelinek wrote:
> > On Mon, Jan 09, 2017 at 10:21:47AM +0100, Marek Polacek wrote:
> > > +/* Callback function to determine whether an expression TP or one of its
> > > +   subexpressions comes from macro expansion.  Used to suppress bogus
> > > +   warnings.  */
> > > +
> > > +static tree
> > > +expr_from_macro_expansion_r (tree *tp, int *, void *)
> > > +{
> > > +  if (CAN_HAVE_LOCATION_P (*tp)
> > > +  && from_macro_expansion_at (EXPR_LOCATION (*tp)))
> > > +return integer_zero_node;
> > > +
> > > +  return NULL_TREE;
> > > +}
> > 
> > I know this is hard issue, but won't it disable the warning way too often?
> > 
> > Perhaps it is good enough for the initial version (GCC 7), but doesn't it 
> > stop
> > whenever one uses NULL in the branches, or some other trivial macros like
> > that?  Perhaps we want to do the analysis if there is anything from macro
> > expansion side-by-side on both the expressions and if you find something
> > from a macro expansion, then still warn if both corresponding expressions
> > are from the same macro expansion (either only non-function like one, or
> > perhaps also function-like one with the same arguments, if it is possible
> > to figure out those somehow)?  And perhaps it would be nice to choose
> > warning level, whether you want to warn only under these rules (no macros
> > or something smarter if implemented) vs. some certainly non-default more
> > aggressive mode that will just warn no matter what macros there are.
> 
> I agree that not warning for 
>   if (foo)
> return NULL;
>   else
> return NULL;
> is bad.  But how can I compare those expressions side-by-side?  I'm not 
> finding
> anything. :(

Seems like ENOTIME to address this; will you be ok with the patch as-is
(modulo Jeff comments), if I open a PR about the above test case?

Thanks,

Marek

Re: Implement -Wduplicated-branches (PR c/64279) (v3)

[Jeff Law]

On 01/09/2017 02:21 AM, Marek Polacek wrote:

On Thu, Jan 05, 2017 at 04:41:28PM +0100, Jakub Jelinek wrote:

On Thu, Jan 05, 2017 at 04:39:40PM +0100, Marek Polacek wrote:

Coming back to this...



Right, after h0 == h1 is missing && operand_equal_p (thenb, elseb, 0)
or so (the exact last operand needs to be figured out).
OEP_ONLY_CONST is certainly wrong, we want the same VAR_DECLs to mean the
same thing.  0 is a tiny bit better, but still it will give up on e.g. pure
and other calls.  OEP_PURE_SAME is tiny bit better than that, but still
calls with the same arguments to the same function will not be considered
equal, plus likely operand_equal_p doesn't handle STATEMENT_LIST etc.
So maybe we need another OEP_* mode for this.


Yea, if I add "&& operand_equal_p (thenb, elseb, 0)" then this warning doesn't
trigger for certain cases, such as MODIFY_EXPR, RETURN_EXPR, probably
STATEMENT_LIST and others.  So I suppose I could introduce a new OEP_ mode for
this (names?  OEP_EXTENDED?) and then in operand_equal_p in case tcc_expression
do

  case MODIFY_EXPR:
if (flags & OEP_EXTENDED)
  // compare LHS and RHS of both

?


Yeah.  Not sure what is the best name for that.  Maybe Richi has some clever
ideas.


Here it is.  The changes in operand_equal_p should only trigger with the new
OEP_LEXICOGRAPHIC, and given the macro location issue, the warning isn't yet
enabled by neither -Wall nor -Wextra, so this all should be safe.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2017-01-09  Marek Polacek  

PR c/64279
* c-common.h (do_warn_duplicated_branches_r): Declare.
* c-gimplify.c (c_genericize): Walk the function tree calling
do_warn_duplicated_branches_r.
* c-warn.c (expr_from_macro_expansion_r): New.
(do_warn_duplicated_branches): New.
(do_warn_duplicated_branches_r): New.
* c.opt (Wduplicated-branches): New option.

* c-typeck.c (build_conditional_expr): Warn about duplicated branches.

* call.c (build_conditional_expr_1): Warn about duplicated branches.
* semantics.c (finish_expr_stmt): Build statement using the proper
location.

* doc/invoke.texi: Document -Wduplicated-branches.
* fold-const.c (operand_equal_p): Handle MODIFY_EXPR, INIT_EXPR,
COMPOUND_EXPR, PREDECREMENT_EXPR, PREINCREMENT_EXPR,
POSTDECREMENT_EXPR, POSTINCREMENT_EXPR, CLEANUP_POINT_EXPR, EXPR_STMT,
STATEMENT_LIST, and RETURN_EXPR.  For non-pure non-const functions
return 0 only when not OEP_LEXICOGRAPHIC.
(fold_build_cleanup_point_expr): Use the expression
location when building CLEANUP_POINT_EXPR.
* tree-core.h (enum operand_equal_flag): Add OEP_LEXICOGRAPHIC.
* tree.c (add_expr): Handle error_mark_node.

* c-c++-common/Wduplicated-branches-1.c: New test.
* c-c++-common/Wduplicated-branches-10.c: New test.
* c-c++-common/Wduplicated-branches-11.c: New test.
* c-c++-common/Wduplicated-branches-12.c: New test.
* c-c++-common/Wduplicated-branches-2.c: New test.
* c-c++-common/Wduplicated-branches-3.c: New test.
* c-c++-common/Wduplicated-branches-4.c: New test.
* c-c++-common/Wduplicated-branches-5.c: New test.
* c-c++-common/Wduplicated-branches-6.c: New test.
* c-c++-common/Wduplicated-branches-7.c: New test.
* c-c++-common/Wduplicated-branches-8.c: New test.
* c-c++-common/Wduplicated-branches-9.c: New test.
* c-c++-common/Wimplicit-fallthrough-7.c: Coalesce dg-warning.
* g++.dg/cpp0x/lambda/lambda-switch.C: Move dg-warning.
* g++.dg/ext/builtin-object-size3.C: Likewise.
* g++.dg/gomp/loop-1.C: Likewise.
* g++.dg/warn/Wduplicated-branches1.C: New test.
* g++.dg/warn/Wduplicated-branches2.C: New test.

s/indentical/identical in the doc/invoke.texi changes.






diff --git gcc/c/c-typeck.c gcc/c/c-typeck.c
index 96e7351..ed8ffe4 100644
--- gcc/c/c-typeck.c
+++ gcc/c/c-typeck.c
@@ -5193,6 +5193,15 @@ build_conditional_expr (location_t colon_loc, tree 
ifexp, bool ifexp_bcp,
 ret = build1 (EXCESS_PRECISION_EXPR, semantic_result_type, ret);

   protected_set_expr_location (ret, colon_loc);
+
+  /* If the OP1 and OP2 are the same and don't have side-effects,
+ 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)))
Did you want OEP_LEXICOGRAPHIC here, or in this context do we not have 
to worry about the more complex forms?  What about a statement 
expressions?  Have we lowered those at this point already?



diff --git gcc/cp/call.c gcc/cp/call.c
index e431221..84931fb 100644
--- gcc/cp/call.c
+++ gcc/cp/call.c
@@ -5302,6 +5302,13 @@ build_conditional_expr_1 (location_t loc, tree arg1, 
tree arg2, tree arg3,
  valid_operands:
   result = build3_loc (loc, 

Re: Implement -Wduplicated-branches (PR c/64279) (v3)

[Marek Polacek]

On Mon, Jan 09, 2017 at 12:18:01PM +0100, Jakub Jelinek wrote:
> On Mon, Jan 09, 2017 at 10:21:47AM +0100, Marek Polacek wrote:
> > +/* Callback function to determine whether an expression TP or one of its
> > +   subexpressions comes from macro expansion.  Used to suppress bogus
> > +   warnings.  */
> > +
> > +static tree
> > +expr_from_macro_expansion_r (tree *tp, int *, void *)
> > +{
> > +  if (CAN_HAVE_LOCATION_P (*tp)
> > +  && from_macro_expansion_at (EXPR_LOCATION (*tp)))
> > +return integer_zero_node;
> > +
> > +  return NULL_TREE;
> > +}
> 
> I know this is hard issue, but won't it disable the warning way too often?
> 
> Perhaps it is good enough for the initial version (GCC 7), but doesn't it stop
> whenever one uses NULL in the branches, or some other trivial macros like
> that?  Perhaps we want to do the analysis if there is anything from macro
> expansion side-by-side on both the expressions and if you find something
> from a macro expansion, then still warn if both corresponding expressions
> are from the same macro expansion (either only non-function like one, or
> perhaps also function-like one with the same arguments, if it is possible
> to figure out those somehow)?  And perhaps it would be nice to choose
> warning level, whether you want to warn only under these rules (no macros
> or something smarter if implemented) vs. some certainly non-default more
> aggressive mode that will just warn no matter what macros there are.

I agree that not warning for 
  if (foo)
return NULL;
  else
return NULL;
is bad.  But how can I compare those expressions side-by-side?  I'm not finding
anything. :(

As for the idea of multiple levels, sure, I could do that, although I'd prefer
to get the initial version in first.

Marek

Re: Implement -Wduplicated-branches (PR c/64279) (v3)

[Jakub Jelinek]

On Mon, Jan 09, 2017 at 10:21:47AM +0100, Marek Polacek wrote:
> +/* Callback function to determine whether an expression TP or one of its
> +   subexpressions comes from macro expansion.  Used to suppress bogus
> +   warnings.  */
> +
> +static tree
> +expr_from_macro_expansion_r (tree *tp, int *, void *)
> +{
> +  if (CAN_HAVE_LOCATION_P (*tp)
> +  && from_macro_expansion_at (EXPR_LOCATION (*tp)))
> +return integer_zero_node;
> +
> +  return NULL_TREE;
> +}

I know this is hard issue, but won't it disable the warning way too often?

Perhaps it is good enough for the initial version (GCC 7), but doesn't it stop
whenever one uses NULL in the branches, or some other trivial macros like
that?  Perhaps we want to do the analysis if there is anything from macro
expansion side-by-side on both the expressions and if you find something
from a macro expansion, then still warn if both corresponding expressions
are from the same macro expansion (either only non-function like one, or
perhaps also function-like one with the same arguments, if it is possible
to figure out those somehow)?  And perhaps it would be nice to choose
warning level, whether you want to warn only under these rules (no macros
or something smarter if implemented) vs. some certainly non-default more
aggressive mode that will just warn no matter what macros there are.

Jakub

Re: Implement -Wduplicated-branches (PR c/64279) (v3)

[Marek Polacek]

On Mon, Jan 09, 2017 at 11:57:48AM +0100, Richard Biener wrote:
> On Mon, 9 Jan 2017, Marek Polacek wrote:
> 
> > On Thu, Jan 05, 2017 at 04:41:28PM +0100, Jakub Jelinek wrote:
> > > On Thu, Jan 05, 2017 at 04:39:40PM +0100, Marek Polacek wrote:
> > > > Coming back to this...
> > > 
> > > > > Right, after h0 == h1 is missing && operand_equal_p (thenb, elseb, 0)
> > > > > or so (the exact last operand needs to be figured out).
> > > > > OEP_ONLY_CONST is certainly wrong, we want the same VAR_DECLs to mean 
> > > > > the
> > > > > same thing.  0 is a tiny bit better, but still it will give up on 
> > > > > e.g. pure
> > > > > and other calls.  OEP_PURE_SAME is tiny bit better than that, but 
> > > > > still
> > > > > calls with the same arguments to the same function will not be 
> > > > > considered
> > > > > equal, plus likely operand_equal_p doesn't handle STATEMENT_LIST etc.
> > > > > So maybe we need another OEP_* mode for this.
> > > > 
> > > > Yea, if I add "&& operand_equal_p (thenb, elseb, 0)" then this warning 
> > > > doesn't
> > > > trigger for certain cases, such as MODIFY_EXPR, RETURN_EXPR, probably
> > > > STATEMENT_LIST and others.  So I suppose I could introduce a new OEP_ 
> > > > mode for
> > > > this (names?  OEP_EXTENDED?) and then in operand_equal_p in case 
> > > > tcc_expression
> > > > do
> > > > 
> > > >   case MODIFY_EXPR:
> > > > if (flags & OEP_EXTENDED)
> > > >   // compare LHS and RHS of both
> > > >  
> > > > ?
> > > 
> > > Yeah.  Not sure what is the best name for that.  Maybe Richi has some 
> > > clever
> > > ideas.
> > 
> > Here it is.  The changes in operand_equal_p should only trigger with the new
> > OEP_LEXICOGRAPHIC, and given the macro location issue, the warning isn't yet
> > enabled by neither -Wall nor -Wextra, so this all should be safe.
> > 
> > Bootstrapped/regtested on x86_64-linux, ok for trunk?
> 
> @@ -2722,6 +2722,9 @@ combine_comparisons (location_t loc,
> If OEP_ADDRESS_OF is set, we are actually comparing addresses of
> objects,
> not values of expressions.
>  
> +   If OEP_LEXICOGRAPHIC is set, then also handle expressions such as
> +   MODIFY_EXPR, RETURN_EXPR, as well as STATEMENT_LISTs.
> +
> 
> I'd say "also handle expressions with side-effects such as ..."
> 
> otherwise the middle-end changes look good to me - I'll defer to
> C FE maintainers for the rest.

Thanks, I'll fix it up.

Marek

Re: Implement -Wduplicated-branches (PR c/64279) (v3)

[Richard Biener]

On Mon, 9 Jan 2017, Marek Polacek wrote:

> On Thu, Jan 05, 2017 at 04:41:28PM +0100, Jakub Jelinek wrote:
> > On Thu, Jan 05, 2017 at 04:39:40PM +0100, Marek Polacek wrote:
> > > Coming back to this...
> > 
> > > > Right, after h0 == h1 is missing && operand_equal_p (thenb, elseb, 0)
> > > > or so (the exact last operand needs to be figured out).
> > > > OEP_ONLY_CONST is certainly wrong, we want the same VAR_DECLs to mean 
> > > > the
> > > > same thing.  0 is a tiny bit better, but still it will give up on e.g. 
> > > > pure
> > > > and other calls.  OEP_PURE_SAME is tiny bit better than that, but still
> > > > calls with the same arguments to the same function will not be 
> > > > considered
> > > > equal, plus likely operand_equal_p doesn't handle STATEMENT_LIST etc.
> > > > So maybe we need another OEP_* mode for this.
> > > 
> > > Yea, if I add "&& operand_equal_p (thenb, elseb, 0)" then this warning 
> > > doesn't
> > > trigger for certain cases, such as MODIFY_EXPR, RETURN_EXPR, probably
> > > STATEMENT_LIST and others.  So I suppose I could introduce a new OEP_ 
> > > mode for
> > > this (names?  OEP_EXTENDED?) and then in operand_equal_p in case 
> > > tcc_expression
> > > do
> > > 
> > >   case MODIFY_EXPR:
> > > if (flags & OEP_EXTENDED)
> > >   // compare LHS and RHS of both
> > >  
> > > ?
> > 
> > Yeah.  Not sure what is the best name for that.  Maybe Richi has some clever
> > ideas.
> 
> Here it is.  The changes in operand_equal_p should only trigger with the new
> OEP_LEXICOGRAPHIC, and given the macro location issue, the warning isn't yet
> enabled by neither -Wall nor -Wextra, so this all should be safe.
> 
> Bootstrapped/regtested on x86_64-linux, ok for trunk?

@@ -2722,6 +2722,9 @@ combine_comparisons (location_t loc,
If OEP_ADDRESS_OF is set, we are actually comparing addresses of
objects,
not values of expressions.
 
+   If OEP_LEXICOGRAPHIC is set, then also handle expressions such as
+   MODIFY_EXPR, RETURN_EXPR, as well as STATEMENT_LISTs.
+

I'd say "also handle expressions with side-effects such as ..."

otherwise the middle-end changes look good to me - I'll defer to
C FE maintainers for the rest.

Thanks,
Richard.

> 2017-01-09  Marek Polacek  
> 
>   PR c/64279
>   * c-common.h (do_warn_duplicated_branches_r): Declare.
>   * c-gimplify.c (c_genericize): Walk the function tree calling
>   do_warn_duplicated_branches_r.
>   * c-warn.c (expr_from_macro_expansion_r): New.
>   (do_warn_duplicated_branches): New.
>   (do_warn_duplicated_branches_r): New.
>   * c.opt (Wduplicated-branches): New option.
> 
>   * c-typeck.c (build_conditional_expr): Warn about duplicated branches.
> 
>   * call.c (build_conditional_expr_1): Warn about duplicated branches.
>   * semantics.c (finish_expr_stmt): Build statement using the proper
>   location.
> 
>   * doc/invoke.texi: Document -Wduplicated-branches.
>   * fold-const.c (operand_equal_p): Handle MODIFY_EXPR, INIT_EXPR,
>   COMPOUND_EXPR, PREDECREMENT_EXPR, PREINCREMENT_EXPR,
>   POSTDECREMENT_EXPR, POSTINCREMENT_EXPR, CLEANUP_POINT_EXPR, EXPR_STMT,
>   STATEMENT_LIST, and RETURN_EXPR.  For non-pure non-const functions
>   return 0 only when not OEP_LEXICOGRAPHIC.
>   (fold_build_cleanup_point_expr): Use the expression
>   location when building CLEANUP_POINT_EXPR.
>   * tree-core.h (enum operand_equal_flag): Add OEP_LEXICOGRAPHIC.
>   * tree.c (add_expr): Handle error_mark_node.
> 
>   * c-c++-common/Wduplicated-branches-1.c: New test.
>   * c-c++-common/Wduplicated-branches-10.c: New test.
>   * c-c++-common/Wduplicated-branches-11.c: New test.
>   * c-c++-common/Wduplicated-branches-12.c: New test.
>   * c-c++-common/Wduplicated-branches-2.c: New test.
>   * c-c++-common/Wduplicated-branches-3.c: New test.
>   * c-c++-common/Wduplicated-branches-4.c: New test.
>   * c-c++-common/Wduplicated-branches-5.c: New test.
>   * c-c++-common/Wduplicated-branches-6.c: New test.
>   * c-c++-common/Wduplicated-branches-7.c: New test.
>   * c-c++-common/Wduplicated-branches-8.c: New test.
>   * c-c++-common/Wduplicated-branches-9.c: New test.
>   * c-c++-common/Wimplicit-fallthrough-7.c: Coalesce dg-warning.
>   * g++.dg/cpp0x/lambda/lambda-switch.C: Move dg-warning.
>   * g++.dg/ext/builtin-object-size3.C: Likewise.
>   * g++.dg/gomp/loop-1.C: Likewise.
>   * g++.dg/warn/Wduplicated-branches1.C: New test.
>   * g++.dg/warn/Wduplicated-branches2.C: New test.
> 
> diff --git gcc/c-family/c-common.h gcc/c-family/c-common.h
> index b838869..06918db 100644
> --- gcc/c-family/c-common.h
> +++ gcc/c-family/c-common.h
> @@ -1537,6 +1537,7 @@ extern void maybe_warn_bool_compare (location_t, enum 
> tree_code, tree, tree);
>  extern bool maybe_warn_shift_overflow (location_t, tree, tree);
>  extern void warn_duplicated_cond_add_or_warn (location_t, tree, vec 
> 

Re: Implement -Wduplicated-branches (PR c/64279) (v3)

[Marek Polacek]

On Thu, Jan 05, 2017 at 04:41:28PM +0100, Jakub Jelinek wrote:
> On Thu, Jan 05, 2017 at 04:39:40PM +0100, Marek Polacek wrote:
> > Coming back to this...
> 
> > > Right, after h0 == h1 is missing && operand_equal_p (thenb, elseb, 0)
> > > or so (the exact last operand needs to be figured out).
> > > OEP_ONLY_CONST is certainly wrong, we want the same VAR_DECLs to mean the
> > > same thing.  0 is a tiny bit better, but still it will give up on e.g. 
> > > pure
> > > and other calls.  OEP_PURE_SAME is tiny bit better than that, but still
> > > calls with the same arguments to the same function will not be considered
> > > equal, plus likely operand_equal_p doesn't handle STATEMENT_LIST etc.
> > > So maybe we need another OEP_* mode for this.
> > 
> > Yea, if I add "&& operand_equal_p (thenb, elseb, 0)" then this warning 
> > doesn't
> > trigger for certain cases, such as MODIFY_EXPR, RETURN_EXPR, probably
> > STATEMENT_LIST and others.  So I suppose I could introduce a new OEP_ mode 
> > for
> > this (names?  OEP_EXTENDED?) and then in operand_equal_p in case 
> > tcc_expression
> > do
> > 
> >   case MODIFY_EXPR:
> > if (flags & OEP_EXTENDED)
> >   // compare LHS and RHS of both
> >  
> > ?
> 
> Yeah.  Not sure what is the best name for that.  Maybe Richi has some clever
> ideas.

Here it is.  The changes in operand_equal_p should only trigger with the new
OEP_LEXICOGRAPHIC, and given the macro location issue, the warning isn't yet
enabled by neither -Wall nor -Wextra, so this all should be safe.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2017-01-09  Marek Polacek  

PR c/64279
* c-common.h (do_warn_duplicated_branches_r): Declare.
* c-gimplify.c (c_genericize): Walk the function tree calling
do_warn_duplicated_branches_r.
* c-warn.c (expr_from_macro_expansion_r): New.
(do_warn_duplicated_branches): New.
(do_warn_duplicated_branches_r): New.
* c.opt (Wduplicated-branches): New option.

* c-typeck.c (build_conditional_expr): Warn about duplicated branches.

* call.c (build_conditional_expr_1): Warn about duplicated branches.
* semantics.c (finish_expr_stmt): Build statement using the proper
location.

* doc/invoke.texi: Document -Wduplicated-branches.
* fold-const.c (operand_equal_p): Handle MODIFY_EXPR, INIT_EXPR,
COMPOUND_EXPR, PREDECREMENT_EXPR, PREINCREMENT_EXPR,
POSTDECREMENT_EXPR, POSTINCREMENT_EXPR, CLEANUP_POINT_EXPR, EXPR_STMT,
STATEMENT_LIST, and RETURN_EXPR.  For non-pure non-const functions
return 0 only when not OEP_LEXICOGRAPHIC.
(fold_build_cleanup_point_expr): Use the expression
location when building CLEANUP_POINT_EXPR.
* tree-core.h (enum operand_equal_flag): Add OEP_LEXICOGRAPHIC.
* tree.c (add_expr): Handle error_mark_node.

* c-c++-common/Wduplicated-branches-1.c: New test.
* c-c++-common/Wduplicated-branches-10.c: New test.
* c-c++-common/Wduplicated-branches-11.c: New test.
* c-c++-common/Wduplicated-branches-12.c: New test.
* c-c++-common/Wduplicated-branches-2.c: New test.
* c-c++-common/Wduplicated-branches-3.c: New test.
* c-c++-common/Wduplicated-branches-4.c: New test.
* c-c++-common/Wduplicated-branches-5.c: New test.
* c-c++-common/Wduplicated-branches-6.c: New test.
* c-c++-common/Wduplicated-branches-7.c: New test.
* c-c++-common/Wduplicated-branches-8.c: New test.
* c-c++-common/Wduplicated-branches-9.c: New test.
* c-c++-common/Wimplicit-fallthrough-7.c: Coalesce dg-warning.
* g++.dg/cpp0x/lambda/lambda-switch.C: Move dg-warning.
* g++.dg/ext/builtin-object-size3.C: Likewise.
* g++.dg/gomp/loop-1.C: Likewise.
* g++.dg/warn/Wduplicated-branches1.C: New test.
* g++.dg/warn/Wduplicated-branches2.C: New test.

diff --git gcc/c-family/c-common.h gcc/c-family/c-common.h
index b838869..06918db 100644
--- gcc/c-family/c-common.h
+++ gcc/c-family/c-common.h
@@ -1537,6 +1537,7 @@ extern void maybe_warn_bool_compare (location_t, enum 
tree_code, tree, tree);
 extern bool maybe_warn_shift_overflow (location_t, tree, tree);
 extern void warn_duplicated_cond_add_or_warn (location_t, tree, vec **);
 extern bool diagnose_mismatched_attributes (tree, tree);
+extern tree do_warn_duplicated_branches_r (tree *, int *, void *);
 
 /* In c-attribs.c.  */
 extern bool attribute_takes_identifier_p (const_tree);
diff --git gcc/c-family/c-gimplify.c gcc/c-family/c-gimplify.c
index c327ca7..57edb41 100644
--- gcc/c-family/c-gimplify.c
+++ gcc/c-family/c-gimplify.c
@@ -125,6 +125,10 @@ c_genericize (tree fndecl)
 );
 }
 
+  if (warn_duplicated_branches)
+walk_tree_without_duplicates (_SAVED_TREE (fndecl),
+ do_warn_duplicated_branches_r, NULL);
+
   /* Dump the C-specific tree IR.  */
  

Re: Implement -Wduplicated-branches (PR c/64279) (v3)

[Richard Biener]

On January 5, 2017 4:41:28 PM GMT+01:00, Jakub Jelinek  wrote:
>On Thu, Jan 05, 2017 at 04:39:40PM +0100, Marek Polacek wrote:
>> Coming back to this...
>
>> > Right, after h0 == h1 is missing && operand_equal_p (thenb, elseb,
>0)
>> > or so (the exact last operand needs to be figured out).
>> > OEP_ONLY_CONST is certainly wrong, we want the same VAR_DECLs to
>mean the
>> > same thing.  0 is a tiny bit better, but still it will give up on
>e.g. pure
>> > and other calls.  OEP_PURE_SAME is tiny bit better than that, but
>still
>> > calls with the same arguments to the same function will not be
>considered
>> > equal, plus likely operand_equal_p doesn't handle STATEMENT_LIST
>etc.
>> > So maybe we need another OEP_* mode for this.
>> 
>> Yea, if I add "&& operand_equal_p (thenb, elseb, 0)" then this
>warning doesn't
>> trigger for certain cases, such as MODIFY_EXPR, RETURN_EXPR, probably
>> STATEMENT_LIST and others.  So I suppose I could introduce a new OEP_
>mode for
>> this (names?  OEP_EXTENDED?) and then in operand_equal_p in case
>tcc_expression
>> do
>> 
>>   case MODIFY_EXPR:
>> if (flags & OEP_EXTENDED)
>>   // compare LHS and RHS of both
>>  
>> ?
>
>Yeah.  Not sure what is the best name for that.  Maybe Richi has some
>clever
>ideas.

OEP_LEXICOGRAPHIC?

Richard.

>
>   Jakub

Re: Implement -Wduplicated-branches (PR c/64279) (v3)

[Jakub Jelinek]

On Thu, Jan 05, 2017 at 04:39:40PM +0100, Marek Polacek wrote:
> Coming back to this...

> > Right, after h0 == h1 is missing && operand_equal_p (thenb, elseb, 0)
> > or so (the exact last operand needs to be figured out).
> > OEP_ONLY_CONST is certainly wrong, we want the same VAR_DECLs to mean the
> > same thing.  0 is a tiny bit better, but still it will give up on e.g. pure
> > and other calls.  OEP_PURE_SAME is tiny bit better than that, but still
> > calls with the same arguments to the same function will not be considered
> > equal, plus likely operand_equal_p doesn't handle STATEMENT_LIST etc.
> > So maybe we need another OEP_* mode for this.
> 
> Yea, if I add "&& operand_equal_p (thenb, elseb, 0)" then this warning doesn't
> trigger for certain cases, such as MODIFY_EXPR, RETURN_EXPR, probably
> STATEMENT_LIST and others.  So I suppose I could introduce a new OEP_ mode for
> this (names?  OEP_EXTENDED?) and then in operand_equal_p in case 
> tcc_expression
> do
> 
>   case MODIFY_EXPR:
> if (flags & OEP_EXTENDED)
>   // compare LHS and RHS of both
>  
> ?

Yeah.  Not sure what is the best name for that.  Maybe Richi has some clever
ideas.

Jakub

Re: Implement -Wduplicated-branches (PR c/64279) (v3)

[Marek Polacek]

Coming back to this...

On Thu, Nov 03, 2016 at 02:38:50PM +0100, Jakub Jelinek wrote:
> On Thu, Nov 03, 2016 at 09:27:55AM -0400, Jason Merrill wrote:
> > On Thu, Nov 3, 2016 at 7:24 AM, Marek Polacek  wrote:
> > > On Tue, Nov 01, 2016 at 02:53:58PM +0100, Jakub Jelinek wrote:
> > >> On Tue, Nov 01, 2016 at 09:41:20AM -0400, Jason Merrill wrote:
> > >> > On Tue, Oct 25, 2016 at 9:59 AM, Marek Polacek  
> > >> > wrote:
> > >> > > On Mon, Oct 24, 2016 at 04:10:21PM +0200, Marek Polacek wrote:
> > >> > >> On Thu, Oct 20, 2016 at 12:28:36PM +0200, Marek Polacek wrote:
> > >> > >> > I found a problem with this patch--we can't call 
> > >> > >> > do_warn_duplicated_branches in
> > >> > >> > build_conditional_expr, because that way the C++-specific codes 
> > >> > >> > might leak into
> > >> > >> > the hasher.  Instead, I should use operand_equal_p, I think.  Let 
> > >> > >> > me rework
> > >> > >> > that part of the patch.
> > >> >
> > >> > Hmm, is there a reason not to use operand_equal_p for
> > >> > do_warn_duplicated_branches as well?  I'm concerned about hash
> > >> > collisions leading to false positives.
> > >>
> > >> If the hashing function is iterative_hash_expr / inchash::add_expr, then
> > >> that is supposed to pair together with operand_equal_p, we even have
> > >> checking verification of that.
> > 
> > Yes, but there could still be hash collisions; we can't guarantee that
> > everything that compares unequal also hashes unequal.
> 
> Right, after h0 == h1 is missing && operand_equal_p (thenb, elseb, 0)
> or so (the exact last operand needs to be figured out).
> OEP_ONLY_CONST is certainly wrong, we want the same VAR_DECLs to mean the
> same thing.  0 is a tiny bit better, but still it will give up on e.g. pure
> and other calls.  OEP_PURE_SAME is tiny bit better than that, but still
> calls with the same arguments to the same function will not be considered
> equal, plus likely operand_equal_p doesn't handle STATEMENT_LIST etc.
> So maybe we need another OEP_* mode for this.

Yea, if I add "&& operand_equal_p (thenb, elseb, 0)" then this warning doesn't
trigger for certain cases, such as MODIFY_EXPR, RETURN_EXPR, probably
STATEMENT_LIST and others.  So I suppose I could introduce a new OEP_ mode for
this (names?  OEP_EXTENDED?) and then in operand_equal_p in case tcc_expression
do

  case MODIFY_EXPR:
if (flags & OEP_EXTENDED)
  // compare LHS and RHS of both
 
?

Marek

Re: Implement -Wduplicated-branches (PR c/64279) (v3)

[Eric Gallager]

On 11/3/16, Jakub Jelinek  wrote:
> On Thu, Nov 03, 2016 at 09:27:55AM -0400, Jason Merrill wrote:
>> On Thu, Nov 3, 2016 at 7:24 AM, Marek Polacek  wrote:
>> > On Tue, Nov 01, 2016 at 02:53:58PM +0100, Jakub Jelinek wrote:
>> >> On Tue, Nov 01, 2016 at 09:41:20AM -0400, Jason Merrill wrote:
>> >> > On Tue, Oct 25, 2016 at 9:59 AM, Marek Polacek 
>> >> > wrote:
>> >> > > On Mon, Oct 24, 2016 at 04:10:21PM +0200, Marek Polacek wrote:
>> >> > >> On Thu, Oct 20, 2016 at 12:28:36PM +0200, Marek Polacek wrote:
>> >> > >> > I found a problem with this patch--we can't call
>> >> > >> > do_warn_duplicated_branches in
>> >> > >> > build_conditional_expr, because that way the C++-specific codes
>> >> > >> > might leak into
>> >> > >> > the hasher.  Instead, I should use operand_equal_p, I think.
>> >> > >> > Let me rework
>> >> > >> > that part of the patch.
>> >> >
>> >> > Hmm, is there a reason not to use operand_equal_p for
>> >> > do_warn_duplicated_branches as well?  I'm concerned about hash
>> >> > collisions leading to false positives.
>> >>
>> >> If the hashing function is iterative_hash_expr / inchash::add_expr,
>> >> then
>> >> that is supposed to pair together with operand_equal_p, we even have
>> >> checking verification of that.
>>
>> Yes, but there could still be hash collisions; we can't guarantee that
>> everything that compares unequal also hashes unequal.
>
> Right, after h0 == h1 is missing && operand_equal_p (thenb, elseb, 0)
> or so (the exact last operand needs to be figured out).
> OEP_ONLY_CONST is certainly wrong, we want the same VAR_DECLs to mean the
> same thing.  0 is a tiny bit better, but still it will give up on e.g. pure
> and other calls.  OEP_PURE_SAME is tiny bit better than that, but still
> calls with the same arguments to the same function will not be considered
> equal, plus likely operand_equal_p doesn't handle STATEMENT_LIST etc.
> So maybe we need another OEP_* mode for this.
>
>   Jakub
>

Pinging this conversation for the new year. Any chances of
-Wduplicated-branches making it in in time for GCC 7?

Thanks,
Eric

Re: Implement -Wduplicated-branches (PR c/64279) (v3)

[Jakub Jelinek]

On Thu, Nov 03, 2016 at 09:27:55AM -0400, Jason Merrill wrote:
> On Thu, Nov 3, 2016 at 7:24 AM, Marek Polacek  wrote:
> > On Tue, Nov 01, 2016 at 02:53:58PM +0100, Jakub Jelinek wrote:
> >> On Tue, Nov 01, 2016 at 09:41:20AM -0400, Jason Merrill wrote:
> >> > On Tue, Oct 25, 2016 at 9:59 AM, Marek Polacek  
> >> > wrote:
> >> > > On Mon, Oct 24, 2016 at 04:10:21PM +0200, Marek Polacek wrote:
> >> > >> On Thu, Oct 20, 2016 at 12:28:36PM +0200, Marek Polacek wrote:
> >> > >> > I found a problem with this patch--we can't call 
> >> > >> > do_warn_duplicated_branches in
> >> > >> > build_conditional_expr, because that way the C++-specific codes 
> >> > >> > might leak into
> >> > >> > the hasher.  Instead, I should use operand_equal_p, I think.  Let 
> >> > >> > me rework
> >> > >> > that part of the patch.
> >> >
> >> > Hmm, is there a reason not to use operand_equal_p for
> >> > do_warn_duplicated_branches as well?  I'm concerned about hash
> >> > collisions leading to false positives.
> >>
> >> If the hashing function is iterative_hash_expr / inchash::add_expr, then
> >> that is supposed to pair together with operand_equal_p, we even have
> >> checking verification of that.
> 
> Yes, but there could still be hash collisions; we can't guarantee that
> everything that compares unequal also hashes unequal.

Right, after h0 == h1 is missing && operand_equal_p (thenb, elseb, 0)
or so (the exact last operand needs to be figured out).
OEP_ONLY_CONST is certainly wrong, we want the same VAR_DECLs to mean the
same thing.  0 is a tiny bit better, but still it will give up on e.g. pure
and other calls.  OEP_PURE_SAME is tiny bit better than that, but still
calls with the same arguments to the same function will not be considered
equal, plus likely operand_equal_p doesn't handle STATEMENT_LIST etc.
So maybe we need another OEP_* mode for this.

Jakub

Re: Implement -Wduplicated-branches (PR c/64279) (v3)

[Jason Merrill]

On Thu, Nov 3, 2016 at 7:24 AM, Marek Polacek  wrote:
> On Tue, Nov 01, 2016 at 02:53:58PM +0100, Jakub Jelinek wrote:
>> On Tue, Nov 01, 2016 at 09:41:20AM -0400, Jason Merrill wrote:
>> > On Tue, Oct 25, 2016 at 9:59 AM, Marek Polacek  wrote:
>> > > On Mon, Oct 24, 2016 at 04:10:21PM +0200, Marek Polacek wrote:
>> > >> On Thu, Oct 20, 2016 at 12:28:36PM +0200, Marek Polacek wrote:
>> > >> > I found a problem with this patch--we can't call 
>> > >> > do_warn_duplicated_branches in
>> > >> > build_conditional_expr, because that way the C++-specific codes might 
>> > >> > leak into
>> > >> > the hasher.  Instead, I should use operand_equal_p, I think.  Let me 
>> > >> > rework
>> > >> > that part of the patch.
>> >
>> > Hmm, is there a reason not to use operand_equal_p for
>> > do_warn_duplicated_branches as well?  I'm concerned about hash
>> > collisions leading to false positives.
>>
>> If the hashing function is iterative_hash_expr / inchash::add_expr, then
>> that is supposed to pair together with operand_equal_p, we even have
>> checking verification of that.

Yes, but there could still be hash collisions; we can't guarantee that
everything that compares unequal also hashes unequal.

Jason

Re: Implement -Wduplicated-branches (PR c/64279) (v3)

[Marek Polacek]

On Tue, Nov 01, 2016 at 02:53:58PM +0100, Jakub Jelinek wrote:
> On Tue, Nov 01, 2016 at 09:41:20AM -0400, Jason Merrill wrote:
> > On Tue, Oct 25, 2016 at 9:59 AM, Marek Polacek  wrote:
> > > On Mon, Oct 24, 2016 at 04:10:21PM +0200, Marek Polacek wrote:
> > >> On Thu, Oct 20, 2016 at 12:28:36PM +0200, Marek Polacek wrote:
> > >> > I found a problem with this patch--we can't call 
> > >> > do_warn_duplicated_branches in
> > >> > build_conditional_expr, because that way the C++-specific codes might 
> > >> > leak into
> > >> > the hasher.  Instead, I should use operand_equal_p, I think.  Let me 
> > >> > rework
> > >> > that part of the patch.
> > 
> > Hmm, is there a reason not to use operand_equal_p for
> > do_warn_duplicated_branches as well?  I'm concerned about hash
> > collisions leading to false positives.
> 
> If the hashing function is iterative_hash_expr / inchash::add_expr, then
> that is supposed to pair together with operand_equal_p, we even have
> checking verification of that.

Yes, I use inchash::add_expr.

So any opinions as to what to do with this patch?

Marek

Re: Implement -Wduplicated-branches (PR c/64279) (v3)

[Jakub Jelinek]

On Tue, Nov 01, 2016 at 09:41:20AM -0400, Jason Merrill wrote:
> On Tue, Oct 25, 2016 at 9:59 AM, Marek Polacek  wrote:
> > On Mon, Oct 24, 2016 at 04:10:21PM +0200, Marek Polacek wrote:
> >> On Thu, Oct 20, 2016 at 12:28:36PM +0200, Marek Polacek wrote:
> >> > I found a problem with this patch--we can't call 
> >> > do_warn_duplicated_branches in
> >> > build_conditional_expr, because that way the C++-specific codes might 
> >> > leak into
> >> > the hasher.  Instead, I should use operand_equal_p, I think.  Let me 
> >> > rework
> >> > that part of the patch.
> 
> Hmm, is there a reason not to use operand_equal_p for
> do_warn_duplicated_branches as well?  I'm concerned about hash
> collisions leading to false positives.

If the hashing function is iterative_hash_expr / inchash::add_expr, then
that is supposed to pair together with operand_equal_p, we even have
checking verification of that.

Jakub

Re: Implement -Wduplicated-branches (PR c/64279) (v3)

[Jason Merrill]

On Tue, Oct 25, 2016 at 9:59 AM, Marek Polacek  wrote:
> On Mon, Oct 24, 2016 at 04:10:21PM +0200, Marek Polacek wrote:
>> On Thu, Oct 20, 2016 at 12:28:36PM +0200, Marek Polacek wrote:
>> > I found a problem with this patch--we can't call 
>> > do_warn_duplicated_branches in
>> > build_conditional_expr, because that way the C++-specific codes might leak 
>> > into
>> > the hasher.  Instead, I should use operand_equal_p, I think.  Let me rework
>> > that part of the patch.

Hmm, is there a reason not to use operand_equal_p for
do_warn_duplicated_branches as well?  I'm concerned about hash
collisions leading to false positives.

Jason

Re: Implement -Wduplicated-branches (PR c/64279) (v3)

[Marek Polacek]

On Mon, Oct 24, 2016 at 04:10:21PM +0200, Marek Polacek wrote:
> On Thu, Oct 20, 2016 at 12:28:36PM +0200, Marek Polacek wrote:
> > I found a problem with this patch--we can't call 
> > do_warn_duplicated_branches in
> > build_conditional_expr, because that way the C++-specific codes might leak 
> > into
> > the hasher.  Instead, I should use operand_equal_p, I think.  Let me rework
> > that part of the patch.
> 
> Thus:

And one more thing, let's not warn for if { } else { }, either.
Thanks Tobias for testing.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2016-10-25  Marek Polacek  

PR c/64279
* c-common.h (do_warn_duplicated_branches_r,
do_warn_duplicated_branches): Declare.
* c-gimplify.c (c_genericize): Walk the function tree calling
do_warn_duplicated_branches_r.
* c-warn.c (expr_from_macro_expansion_r): New.
(do_warn_duplicated_branches): New.
(do_warn_duplicated_branches_r): New.
* c.opt (Wduplicated-branches): New option.

* c-typeck.c (build_conditional_expr): Warn about duplicated branches.

* call.c (build_conditional_expr_1): Warn about duplicated branches.
* semantics.c (finish_expr_stmt): Build statement using the proper
location.

* doc/invoke.texi: Document -Wduplicated-branches.
* fold-const.c (fold_build_cleanup_point_expr): Use the expression
location when building CLEANUP_POINT_EXPR.
* tree.c (add_expr): Handle error_mark_node.

* c-c++-common/Wduplicated-branches-1.c: New test.
* c-c++-common/Wduplicated-branches-2.c: New test.
* c-c++-common/Wduplicated-branches-3.c: New test.
* c-c++-common/Wduplicated-branches-4.c: New test.
* c-c++-common/Wduplicated-branches-5.c: New test.
* c-c++-common/Wduplicated-branches-6.c: New test.
* c-c++-common/Wduplicated-branches-7.c: New test.
* c-c++-common/Wduplicated-branches-8.c: New test.
* c-c++-common/Wduplicated-branches-9.c: New test.
* c-c++-common/Wduplicated-branches-10.c: New test.
* c-c++-common/Wimplicit-fallthrough-7.c: Coalesce dg-warning.
* g++.dg/cpp0x/lambda/lambda-switch.C: Move dg-warning.
* g++.dg/ext/builtin-object-size3.C (bar): Likewise.
* g++.dg/gomp/loop-1.C: Likewise.
* g++.dg/warn/Wduplicated-branches1.C: New test.
* g++.dg/warn/Wduplicated-branches2.C: New test.

diff --git gcc/c-family/c-common.h gcc/c-family/c-common.h
index 547bab2..46e9d2e 100644
--- gcc/c-family/c-common.h
+++ gcc/c-family/c-common.h
@@ -1530,6 +1530,7 @@ extern void maybe_warn_bool_compare (location_t, enum 
tree_code, tree, tree);
 extern bool maybe_warn_shift_overflow (location_t, tree, tree);
 extern void warn_duplicated_cond_add_or_warn (location_t, tree, vec **);
 extern bool diagnose_mismatched_attributes (tree, tree);
+extern tree do_warn_duplicated_branches_r (tree *, int *, void *);
 
 /* In c-attribs.c.  */
 extern bool attribute_takes_identifier_p (const_tree);
diff --git gcc/c-family/c-gimplify.c gcc/c-family/c-gimplify.c
index c18b057..3ed2da8 100644
--- gcc/c-family/c-gimplify.c
+++ gcc/c-family/c-gimplify.c
@@ -125,6 +125,10 @@ c_genericize (tree fndecl)
 );
 }
 
+  if (warn_duplicated_branches)
+walk_tree_without_duplicates (_SAVED_TREE (fndecl),
+ do_warn_duplicated_branches_r, NULL);
+
   /* Dump the C-specific tree IR.  */
   dump_orig = get_dump_info (TDI_original, _dump_flags);
   if (dump_orig)
diff --git gcc/c-family/c-warn.c gcc/c-family/c-warn.c
index 904f6d3..433f5c3 100644
--- gcc/c-family/c-warn.c
+++ gcc/c-family/c-warn.c
@@ -2154,3 +2154,72 @@ maybe_warn_bool_compare (location_t loc, enum tree_code 
code, tree op0,
"with boolean expression is always false", cst);
 }
 }
+
+/* Callback function to determine whether an expression TP or one of its
+   subexpressions comes from macro expansion.  Used to suppress bogus
+   warnings.  */
+
+static tree
+expr_from_macro_expansion_r (tree *tp, int *, void *)
+{
+  if (CAN_HAVE_LOCATION_P (*tp)
+  && from_macro_expansion_at (EXPR_LOCATION (*tp)))
+return integer_zero_node;
+
+  return NULL_TREE;
+}
+
+/* Possibly warn when an if-else has identical branches.  */
+
+static void
+do_warn_duplicated_branches (tree expr)
+{
+  tree thenb = COND_EXPR_THEN (expr);
+  tree elseb = COND_EXPR_ELSE (expr);
+
+  /* Don't bother if there's no else branch.  */
+  if (elseb == NULL_TREE)
+return;
+
+  /* And don't warn for empty statements.  */
+  if (TREE_CODE (thenb) == NOP_EXPR
+  && TREE_TYPE (thenb) == void_type_node
+  && TREE_OPERAND (thenb, 0) == size_zero_node)
+return;
+
+  /* ... or empty branches.  */
+  if (TREE_CODE (thenb) == STATEMENT_LIST
+  && STATEMENT_LIST_HEAD (thenb) == NULL)
+return;
+
+  /* Compute the hash of the then branch.  */
+  inchash::hash hstate0 (0);
+  

Re: Implement -Wduplicated-branches (PR c/64279)

[Martin Sebor]

On 10/24/2016 08:44 AM, Marek Polacek wrote:

On Mon, Oct 24, 2016 at 04:39:20PM +0200, Jakub Jelinek wrote:

On Mon, Oct 24, 2016 at 04:34:40PM +0200, Marek Polacek wrote:

On Mon, Oct 24, 2016 at 08:18:25AM -0600, Martin Sebor wrote:

The case above is just a case where with -O GCC can figure out what the value
of a const-qualified variable is, see decl_constant_value_for_optimization.
Since the warning is implemented even before gimplifying, optimizations like
CP don't come into play yet.


Ah, okay.  That should limit the number of these false positives.
(I saw -O2 in dg-options and assumed it was important.  It sounds
like the test case should pass even with -O1).


Yep--even -O is enough.


But even without constant propagation there will be similar cases
(though probably less pervasive).  For instance, if j were defined
to something like this:

  const int j = 4 == sizeof (size_t);


Well, I think the warning would still be desirable:

  const int j = 4 == sizeof (long);
  if (j == 0)
{
  if (i > 10) /* { dg-warning "this condition has identical branches" } */
*p = j * 2 + 1;
  else
*p = 1;
}

Given the j == 0 check, the branches really are duplicated.  This is actually
a distilled version of what I found in gcov-io.c.


Are you hashing before folding or after folding?


After, e.g. I think build_modify_expr c_fully_fold operands.


If before folding, you wouldn't complain about the gcov-io.c test.
With folding, one can imagine something like:
  const int a = sizeof (long);
  const int b = sizeof (long long);
  int c;
  if (cond)
c = a;
  else
c = b;
or similar, where a and b can be the same on some targets and different on
others.  I believe the warning is still useful, but it will probably be
never false positive free.


Unfortunately :(.  Maybe tolerable for -Wextra, though.


That's why I wondered about data.  I've seen examples like the one
above but it's hard to judge how common they are and what their
overall proportion is among all code like that where the warning
would be justified.  But to be clear, I don't raise this to suggest
the warning shouldn't be added or is never useful but rather in
hopes that the data might lead to insights into how to reduce the
false positive rate (if it's even a problem).  For instance, I
imagine it should be fairly easy to avoid warning on the simple
example above.

Martin

Re: Implement -Wduplicated-branches (PR c/64279)

[Marek Polacek]

On Mon, Oct 24, 2016 at 04:39:20PM +0200, Jakub Jelinek wrote:
> On Mon, Oct 24, 2016 at 04:34:40PM +0200, Marek Polacek wrote:
> > On Mon, Oct 24, 2016 at 08:18:25AM -0600, Martin Sebor wrote:
> > > > The case above is just a case where with -O GCC can figure out what the 
> > > > value
> > > > of a const-qualified variable is, see 
> > > > decl_constant_value_for_optimization.
> > > > Since the warning is implemented even before gimplifying, optimizations 
> > > > like
> > > > CP don't come into play yet.
> > > 
> > > Ah, okay.  That should limit the number of these false positives.
> > > (I saw -O2 in dg-options and assumed it was important.  It sounds
> > > like the test case should pass even with -O1).
> >  
> > Yep--even -O is enough.
> > 
> > > But even without constant propagation there will be similar cases
> > > (though probably less pervasive).  For instance, if j were defined
> > > to something like this:
> > > 
> > >   const int j = 4 == sizeof (size_t);
> > 
> > Well, I think the warning would still be desirable:
> > 
> >   const int j = 4 == sizeof (long);
> >   if (j == 0)
> > {   
> >   if (i > 10) /* { dg-warning "this condition has identical branches" } 
> > */
> > *p = j * 2 + 1;
> >   else
> > *p = 1;
> > }
> > 
> > Given the j == 0 check, the branches really are duplicated.  This is 
> > actually
> > a distilled version of what I found in gcov-io.c.
> 
> Are you hashing before folding or after folding?

After, e.g. I think build_modify_expr c_fully_fold operands.

> If before folding, you wouldn't complain about the gcov-io.c test.
> With folding, one can imagine something like:
>   const int a = sizeof (long);
>   const int b = sizeof (long long);
>   int c;
>   if (cond)
> c = a;
>   else
> c = b;
> or similar, where a and b can be the same on some targets and different on
> others.  I believe the warning is still useful, but it will probably be
> never false positive free.

Unfortunately :(.  Maybe tolerable for -Wextra, though.

Marek

Re: Implement -Wduplicated-branches (PR c/64279)

[Jakub Jelinek]

On Mon, Oct 24, 2016 at 04:34:40PM +0200, Marek Polacek wrote:
> On Mon, Oct 24, 2016 at 08:18:25AM -0600, Martin Sebor wrote:
> > > The case above is just a case where with -O GCC can figure out what the 
> > > value
> > > of a const-qualified variable is, see 
> > > decl_constant_value_for_optimization.
> > > Since the warning is implemented even before gimplifying, optimizations 
> > > like
> > > CP don't come into play yet.
> > 
> > Ah, okay.  That should limit the number of these false positives.
> > (I saw -O2 in dg-options and assumed it was important.  It sounds
> > like the test case should pass even with -O1).
>  
> Yep--even -O is enough.
> 
> > But even without constant propagation there will be similar cases
> > (though probably less pervasive).  For instance, if j were defined
> > to something like this:
> > 
> >   const int j = 4 == sizeof (size_t);
> 
> Well, I think the warning would still be desirable:
> 
>   const int j = 4 == sizeof (long);
>   if (j == 0)
> {   
>   if (i > 10) /* { dg-warning "this condition has identical branches" } */
> *p = j * 2 + 1;
>   else
> *p = 1;
> }
> 
> Given the j == 0 check, the branches really are duplicated.  This is actually
> a distilled version of what I found in gcov-io.c.

Are you hashing before folding or after folding?
If before folding, you wouldn't complain about the gcov-io.c test.
With folding, one can imagine something like:
  const int a = sizeof (long);
  const int b = sizeof (long long);
  int c;
  if (cond)
c = a;
  else
c = b;
or similar, where a and b can be the same on some targets and different on
others.  I believe the warning is still useful, but it will probably be
never false positive free.

Jakub

Re: Implement -Wduplicated-branches (PR c/64279)

[Marek Polacek]

On Mon, Oct 24, 2016 at 08:18:25AM -0600, Martin Sebor wrote:
> > The case above is just a case where with -O GCC can figure out what the 
> > value
> > of a const-qualified variable is, see decl_constant_value_for_optimization.
> > Since the warning is implemented even before gimplifying, optimizations like
> > CP don't come into play yet.
> 
> Ah, okay.  That should limit the number of these false positives.
> (I saw -O2 in dg-options and assumed it was important.  It sounds
> like the test case should pass even with -O1).
 
Yep--even -O is enough.

> But even without constant propagation there will be similar cases
> (though probably less pervasive).  For instance, if j were defined
> to something like this:
> 
>   const int j = 4 == sizeof (size_t);

Well, I think the warning would still be desirable:

  const int j = 4 == sizeof (long);
  if (j == 0)
{   
  if (i > 10) /* { dg-warning "this condition has identical branches" } */
*p = j * 2 + 1;
  else
*p = 1;
}

Given the j == 0 check, the branches really are duplicated.  This is actually
a distilled version of what I found in gcov-io.c.

Marek

Re: Implement -Wduplicated-branches (PR c/64279)

[Martin Sebor]

On 10/24/2016 07:59 AM, Marek Polacek wrote:

On Thu, Oct 20, 2016 at 02:21:42PM -0600, Martin Sebor wrote:

--- gcc/testsuite/c-c++-common/Wduplicated-branches-1.c
+++ gcc/testsuite/c-c++-common/Wduplicated-branches-1.c
@@ -0,0 +1,187 @@
+/* PR c/64279 */
+/* { dg-do compile } */
+/* { dg-options "-Wduplicated-branches -O2" } */
+
+extern void foo (int);
+extern int g;
+extern int a[10];
+
+int
+f (int i, int *p)
+{
+  const int j = 0;
+  if (j == 0)
+{
+  if (i > 10) /* { dg-warning "this condition has identical branches" } */
+   /* Optimizers can figure out that this is 1.  */
+   *p = j * 2 + 1;
+  else
+   *p = 1;
+}


I wonder if this test case (perhaps with a slight modification)
illustrates the concern Jeff raised.  Suppose j is an argument
to the function whose value of zero is determined by constant
propagation.  Such code is not uncommon but will presumably be
diagnosed, which in all likelihood will be considered a false
positive.  I don't have a sense of how pervasive such cases
might be.  Do you have any data from projects other than GCC?
(Since there are no fixes in the patch I assume it didn't find
any bugs in GCC itself.)


The case above is just a case where with -O GCC can figure out what the value
of a const-qualified variable is, see decl_constant_value_for_optimization.
Since the warning is implemented even before gimplifying, optimizations like
CP don't come into play yet.


Ah, okay.  That should limit the number of these false positives.
(I saw -O2 in dg-options and assumed it was important.  It sounds
like the test case should pass even with -O1).

But even without constant propagation there will be similar cases
(though probably less pervasive).  For instance, if j were defined
to something like this:

  const int j = 4 == sizeof (size_t);

Martin

Re: Implement -Wduplicated-branches (PR c/64279) (v2)

[Marek Polacek]

On Thu, Oct 20, 2016 at 12:28:36PM +0200, Marek Polacek wrote:
> I found a problem with this patch--we can't call do_warn_duplicated_branches 
> in
> build_conditional_expr, because that way the C++-specific codes might leak 
> into
> the hasher.  Instead, I should use operand_equal_p, I think.  Let me rework
> that part of the patch.

Thus:

Bootstrapped/regtested on x86_64-linux.

2016-10-24  Marek Polacek  

PR c/64279
* c-common.h (do_warn_duplicated_branches_r,
do_warn_duplicated_branches): Declare.
* c-gimplify.c (c_genericize): Walk the function tree calling
do_warn_duplicated_branches_r.
* c-warn.c (expr_from_macro_expansion_r): New.
(do_warn_duplicated_branches): New.
(do_warn_duplicated_branches_r): New.
* c.opt (Wduplicated-branches): New option.

* c-typeck.c (build_conditional_expr): Warn about duplicated branches.

* call.c (build_conditional_expr_1): Warn about duplicated branches.
* semantics.c (finish_expr_stmt): Build statement using the proper
location.

* doc/invoke.texi: Document -Wduplicated-branches.
* fold-const.c (fold_build_cleanup_point_expr): Use the expression
location when building CLEANUP_POINT_EXPR.
* tree.c (add_expr): Handle error_mark_node.

* c-c++-common/Wduplicated-branches-1.c: New test.
* c-c++-common/Wduplicated-branches-2.c: New test.
* c-c++-common/Wduplicated-branches-3.c: New test.
* c-c++-common/Wduplicated-branches-4.c: New test.
* c-c++-common/Wduplicated-branches-5.c: New test.
* c-c++-common/Wduplicated-branches-6.c: New test.
* c-c++-common/Wduplicated-branches-7.c: New test.
* c-c++-common/Wduplicated-branches-8.c: New test.
* c-c++-common/Wduplicated-branches-9.c: New test.
* c-c++-common/Wimplicit-fallthrough-7.c: Coalesce dg-warning.
* g++.dg/cpp0x/lambda/lambda-switch.C: Move dg-warning.
* g++.dg/ext/builtin-object-size3.C (bar): Likewise.
* g++.dg/gomp/loop-1.C: Likewise.
* g++.dg/warn/Wduplicated-branches1.C: New test.
* g++.dg/warn/Wduplicated-branches2.C: New test.

diff --git gcc/c-family/c-common.h gcc/c-family/c-common.h
index bfdbda0..e48f69b 100644
--- gcc/c-family/c-common.h
+++ gcc/c-family/c-common.h
@@ -1530,6 +1530,7 @@ extern void maybe_warn_bool_compare (location_t, enum 
tree_code, tree, tree);
 extern bool maybe_warn_shift_overflow (location_t, tree, tree);
 extern void warn_duplicated_cond_add_or_warn (location_t, tree, vec **);
 extern bool diagnose_mismatched_attributes (tree, tree);
+extern tree do_warn_duplicated_branches_r (tree *, int *, void *);
 
 /* In c-attribs.c.  */
 extern bool attribute_takes_identifier_p (const_tree);
diff --git gcc/c-family/c-gimplify.c gcc/c-family/c-gimplify.c
index c18b057..3ed2da8 100644
--- gcc/c-family/c-gimplify.c
+++ gcc/c-family/c-gimplify.c
@@ -125,6 +125,10 @@ c_genericize (tree fndecl)
 );
 }
 
+  if (warn_duplicated_branches)
+walk_tree_without_duplicates (_SAVED_TREE (fndecl),
+ do_warn_duplicated_branches_r, NULL);
+
   /* Dump the C-specific tree IR.  */
   dump_orig = get_dump_info (TDI_original, _dump_flags);
   if (dump_orig)
diff --git gcc/c-family/c-warn.c gcc/c-family/c-warn.c
index 904f6d3..fda9b66 100644
--- gcc/c-family/c-warn.c
+++ gcc/c-family/c-warn.c
@@ -2154,3 +2154,67 @@ maybe_warn_bool_compare (location_t loc, enum tree_code 
code, tree op0,
"with boolean expression is always false", cst);
 }
 }
+
+/* Callback function to determine whether an expression TP or one of its
+   subexpressions comes from macro expansion.  Used to suppress bogus
+   warnings.  */
+
+static tree
+expr_from_macro_expansion_r (tree *tp, int *, void *)
+{
+  if (CAN_HAVE_LOCATION_P (*tp)
+  && from_macro_expansion_at (EXPR_LOCATION (*tp)))
+return integer_zero_node;
+
+  return NULL_TREE;
+}
+
+/* Possibly warn when an if-else has identical branches.  */
+
+static void
+do_warn_duplicated_branches (tree expr)
+{
+  tree thenb = COND_EXPR_THEN (expr);
+  tree elseb = COND_EXPR_ELSE (expr);
+
+  /* Don't bother if there's no else branch.  */
+  if (elseb == NULL_TREE)
+return;
+
+  /* And don't warn for empty statements.  */
+  if (TREE_CODE (thenb) == NOP_EXPR
+  && TREE_TYPE (thenb) == void_type_node
+  && TREE_OPERAND (thenb, 0) == size_zero_node)
+return;
+
+  /* Compute the hash of the then branch.  */
+  inchash::hash hstate0 (0);
+  inchash::add_expr (thenb, hstate0);
+  hashval_t h0 = hstate0.end ();
+
+  /* Compute the hash of the else branch.  */
+  inchash::hash hstate1 (0);
+  inchash::add_expr (elseb, hstate1);
+  hashval_t h1 = hstate1.end ();
+
+  /* Compare the hashes.  */
+  if (h0 == h1
+  /* Don't warn if any of the branches or their subexpressions comes
+from a macro.  */
+  && 

Re: Implement -Wduplicated-branches (PR c/64279)

[Marek Polacek]

On Thu, Oct 20, 2016 at 12:56:12PM -0600, Jeff Law wrote:
> On 10/20/2016 08:37 AM, David Malcolm wrote:
> > On Thu, 2016-10-20 at 16:24 +0200, Marek Polacek wrote:
> > > On Thu, Oct 20, 2016 at 10:11:55AM -0400, Jason Merrill wrote:
> > > > On Wed, Oct 19, 2016 at 7:07 AM, Marek Polacek 
> > > > wrote:
> > > > > But since integer csts and various decls
> > > > > don't carry location info, this works only partially, so the
> > > > > warning
> > > > > warns even for e.g.
> > > > > 
> > > > >   if (TREE_STATIC (node) || DECL_EXTERNAL (node))
> > > > > max_align = MAX_OFILE_ALIGNMENT;
> > > > >   else
> > > > > max_align = MAX_STACK_ALIGNMENT;
> > > > > 
> > > > > when MAX_OFILE_ALIGNMENT and MAX_STACK_ALIGNMENT represent the
> > > > > same number.
> > > > > There's no way to get around this, so the warning isn't enabled
> > > > > by nor
> > > > > -Wall neither -Wextra, and can't be until we solve the pesky
> > > > > location
> > > > > horror.  (-Wduplicated-cond is off for the very same reason.)
> > > > 
> > > > Is someone working on this?
> > > 
> > > Not me, but I think David has been experimenting with this.  David,
> > > is that
> > > correct?
> > 
> > I started looking at this, but it's unlikely that I'll have something
> > ready for gcc 7 (have been focusing on the RTL frontend).
> Right.  David and I have kicked around some ideas on how we might get to a
> point where constants and decls can carry location information, but we
> agreed that it wasn't likely gcc-7 material.
> 
> So the question in my mind is does the warning make sense given we can't
> current disambiguate constants and thus are likely generating a goodly
> number of false positives.

That's the question.  It certainly can't be in -Wall/-Wextra given the
macro problem, but that is also true for other warnings.  Maybe it makes
sense to add it all the same so that people can experiment with it.

> [ Presumably it was good enough to catch the duplicated branch code Jon
> pointed out in tree-ssa-threadupdate.c recently?  Any other real bugs
> flagged? ]

Yeah, it would've found it.  I haven't found any other clear bugs yet.

Marek

Re: Implement -Wduplicated-branches (PR c/64279)

[Marek Polacek]

On Thu, Oct 20, 2016 at 02:21:42PM -0600, Martin Sebor wrote:
> > --- gcc/testsuite/c-c++-common/Wduplicated-branches-1.c
> > +++ gcc/testsuite/c-c++-common/Wduplicated-branches-1.c
> > @@ -0,0 +1,187 @@
> > +/* PR c/64279 */
> > +/* { dg-do compile } */
> > +/* { dg-options "-Wduplicated-branches -O2" } */
> > +
> > +extern void foo (int);
> > +extern int g;
> > +extern int a[10];
> > +
> > +int
> > +f (int i, int *p)
> > +{
> > +  const int j = 0;
> > +  if (j == 0)
> > +{
> > +  if (i > 10) /* { dg-warning "this condition has identical branches" 
> > } */
> > +   /* Optimizers can figure out that this is 1.  */
> > +   *p = j * 2 + 1;
> > +  else
> > +   *p = 1;
> > +}
> 
> I wonder if this test case (perhaps with a slight modification)
> illustrates the concern Jeff raised.  Suppose j is an argument
> to the function whose value of zero is determined by constant
> propagation.  Such code is not uncommon but will presumably be
> diagnosed, which in all likelihood will be considered a false
> positive.  I don't have a sense of how pervasive such cases
> might be.  Do you have any data from projects other than GCC?
> (Since there are no fixes in the patch I assume it didn't find
> any bugs in GCC itself.)

The case above is just a case where with -O GCC can figure out what the value
of a const-qualified variable is, see decl_constant_value_for_optimization.
Since the warning is implemented even before gimplifying, optimizations like
CP don't come into play yet.

I don't have data from anything else than GCC itself.  It hasn't found any
bugs in GCC (yet), but the codebase was recently scanned by the PVS tool
and the bugs were fixed.

Marek

Re: Implement -Wduplicated-branches (PR c/64279)

[Martin Sebor]

--- gcc/testsuite/c-c++-common/Wduplicated-branches-1.c
+++ gcc/testsuite/c-c++-common/Wduplicated-branches-1.c
@@ -0,0 +1,187 @@
+/* PR c/64279 */
+/* { dg-do compile } */
+/* { dg-options "-Wduplicated-branches -O2" } */
+
+extern void foo (int);
+extern int g;
+extern int a[10];
+
+int
+f (int i, int *p)
+{
+  const int j = 0;
+  if (j == 0)
+{
+  if (i > 10) /* { dg-warning "this condition has identical branches" } */
+   /* Optimizers can figure out that this is 1.  */
+   *p = j * 2 + 1;
+  else
+   *p = 1;
+}


I wonder if this test case (perhaps with a slight modification)
illustrates the concern Jeff raised.  Suppose j is an argument
to the function whose value of zero is determined by constant
propagation.  Such code is not uncommon but will presumably be
diagnosed, which in all likelihood will be considered a false
positive.  I don't have a sense of how pervasive such cases
might be.  Do you have any data from projects other than GCC?
(Since there are no fixes in the patch I assume it didn't find
any bugs in GCC itself.)

Martin

Re: Implement -Wduplicated-branches (PR c/64279)

[Jeff Law]

On 10/20/2016 08:37 AM, David Malcolm wrote:

On Thu, 2016-10-20 at 16:24 +0200, Marek Polacek wrote:

On Thu, Oct 20, 2016 at 10:11:55AM -0400, Jason Merrill wrote:

On Wed, Oct 19, 2016 at 7:07 AM, Marek Polacek 
wrote:

But since integer csts and various decls
don't carry location info, this works only partially, so the
warning
warns even for e.g.

  if (TREE_STATIC (node) || DECL_EXTERNAL (node))
max_align = MAX_OFILE_ALIGNMENT;
  else
max_align = MAX_STACK_ALIGNMENT;

when MAX_OFILE_ALIGNMENT and MAX_STACK_ALIGNMENT represent the
same number.
There's no way to get around this, so the warning isn't enabled
by nor
-Wall neither -Wextra, and can't be until we solve the pesky
location
horror.  (-Wduplicated-cond is off for the very same reason.)


Is someone working on this?


Not me, but I think David has been experimenting with this.  David,
is that
correct?


I started looking at this, but it's unlikely that I'll have something
ready for gcc 7 (have been focusing on the RTL frontend).
Right.  David and I have kicked around some ideas on how we might get to 
a point where constants and decls can carry location information, but we 
agreed that it wasn't likely gcc-7 material.


So the question in my mind is does the warning make sense given we can't 
current disambiguate constants and thus are likely generating a goodly 
number of false positives.


[ Presumably it was good enough to catch the duplicated branch code Jon 
pointed out in tree-ssa-threadupdate.c recently?  Any other real bugs

flagged? ]

jeff

Re: Implement -Wduplicated-branches (PR c/64279)

[David Malcolm]

On Thu, 2016-10-20 at 16:24 +0200, Marek Polacek wrote:
> On Thu, Oct 20, 2016 at 10:11:55AM -0400, Jason Merrill wrote:
> > On Wed, Oct 19, 2016 at 7:07 AM, Marek Polacek 
> > wrote:
> > > But since integer csts and various decls
> > > don't carry location info, this works only partially, so the
> > > warning
> > > warns even for e.g.
> > > 
> > >   if (TREE_STATIC (node) || DECL_EXTERNAL (node))
> > > max_align = MAX_OFILE_ALIGNMENT;
> > >   else
> > > max_align = MAX_STACK_ALIGNMENT;
> > > 
> > > when MAX_OFILE_ALIGNMENT and MAX_STACK_ALIGNMENT represent the
> > > same number.
> > > There's no way to get around this, so the warning isn't enabled
> > > by nor
> > > -Wall neither -Wextra, and can't be until we solve the pesky
> > > location
> > > horror.  (-Wduplicated-cond is off for the very same reason.)
> > 
> > Is someone working on this?
> 
> Not me, but I think David has been experimenting with this.  David,
> is that
> correct?

I started looking at this, but it's unlikely that I'll have something
ready for gcc 7 (have been focusing on the RTL frontend).

Re: Implement -Wduplicated-branches (PR c/64279)

[Marek Polacek]

On Thu, Oct 20, 2016 at 10:11:55AM -0400, Jason Merrill wrote:
> On Wed, Oct 19, 2016 at 7:07 AM, Marek Polacek  wrote:
> > But since integer csts and various decls
> > don't carry location info, this works only partially, so the warning
> > warns even for e.g.
> >
> >   if (TREE_STATIC (node) || DECL_EXTERNAL (node))
> > max_align = MAX_OFILE_ALIGNMENT;
> >   else
> > max_align = MAX_STACK_ALIGNMENT;
> >
> > when MAX_OFILE_ALIGNMENT and MAX_STACK_ALIGNMENT represent the same number.
> > There's no way to get around this, so the warning isn't enabled by nor
> > -Wall neither -Wextra, and can't be until we solve the pesky location
> > horror.  (-Wduplicated-cond is off for the very same reason.)
> 
> Is someone working on this?

Not me, but I think David has been experimenting with this.  David, is that
correct?

Marek

Re: Implement -Wduplicated-branches (PR c/64279)

[Jason Merrill]

On Wed, Oct 19, 2016 at 7:07 AM, Marek Polacek  wrote:
> But since integer csts and various decls
> don't carry location info, this works only partially, so the warning
> warns even for e.g.
>
>   if (TREE_STATIC (node) || DECL_EXTERNAL (node))
> max_align = MAX_OFILE_ALIGNMENT;
>   else
> max_align = MAX_STACK_ALIGNMENT;
>
> when MAX_OFILE_ALIGNMENT and MAX_STACK_ALIGNMENT represent the same number.
> There's no way to get around this, so the warning isn't enabled by nor
> -Wall neither -Wextra, and can't be until we solve the pesky location
> horror.  (-Wduplicated-cond is off for the very same reason.)

Is someone working on this?

Jason

Re: Implement -Wduplicated-branches (PR c/64279)

[Marek Polacek]

I found a problem with this patch--we can't call do_warn_duplicated_branches in
build_conditional_expr, because that way the C++-specific codes might leak into
the hasher.  Instead, I should use operand_equal_p, I think.  Let me rework
that part of the patch.

Marek