Implement -Wduplicated-branches (PR c/64279)
This patch introduces a new warning, -Wduplicated-branches. Its purpose is to warn for code such as if (cond) return 0; else return 0; as well as e.g. r = i ? *p : *p; The approach I took was to use inchash::add_expr to hash the expressions and then compare the hashes. But of course nothing's that easy. The warning should stay quiet for various macro usages so I've introduced expr_from_macro_expansion_r. 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.) add_expr didn't handle error_mark_node that can get there from the FEs (an undeclared variable, etc.), so that's why the change. But maybe it'd be better to walk the tree in do_warn_duplicated_branches, and if if sees an error node, don't attempt to hash the expr. Bootstrapped/regtested on x86_64-linux and ppc64-linux, ok for trunk? 2016-10-19 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. diff --git gcc/c-family/c-common.h gcc/c-family/c-common.h index bfdbda0..67e 100644 --- gcc/c-family/c-common.h +++ gcc/c-family/c-common.h @@ -1530,6 +1530,8 @@ 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 *); +extern void do_warn_duplicated_branches (tree); /* 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) &pset); } + if (warn_duplicated_branches) +walk_tree_without_duplicates (&DECL_SAVED_TREE (fndecl), + do_warn_duplicated_branches_r, NULL); + /* Dump the C-specific tree IR. */ dump_orig = get_dump_info (TDI_original, &local_dump_flags); if (dump_orig) diff --git gcc/c-family/c-warn.c gcc/c-family/c-warn.c index 904f6d3..f6e8e71 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 br
Re: Implement -Wduplicated-branches (PR c/64279)
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
Re: Implement -Wduplicated-branches (PR c/64279)
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)
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)
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)
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)
--- 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)
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)
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)
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)
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)
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)
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)
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) (v2)
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) &pset); } + if (warn_duplicated_branches) +walk_tree_without_duplicates (&DECL_SAVED_TREE (fndecl), + do_warn_duplicated_branches_r, NULL); + /* Dump the C-specific tree IR. */ dump_orig = get_dump_info (TDI_original, &local_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. */ + && !walk_tree_with
Re: Implement -Wduplicated-branches (PR c/64279) (v3)
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) &pset); } + if (warn_duplicated_branches) +walk_tree_without_duplicates (&DECL_SAVED_TREE (fndecl), + do_warn_duplicated_branches_r, NULL); + /* Dump the C-specific tree IR. */ dump_orig = get_dump_info (TDI_original, &local_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); + inchash::ad
Re: Implement -Wduplicated-branches (PR c/64279) (v3)
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)
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)
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)
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)
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)
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) &pset); } + if (warn_duplicated_branches) +walk_tree_without_duplicates (&DECL_SAVED_TREE (fndecl), + do_warn_duplicated_branches_r, NULL); + /* Dump the C-specific tree IR. */ dump_orig
Re: Implement -Wduplicated-branches (PR c/64279) (v3)
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 > **); > extern bool
Re: Implement -Wduplicated-branches (PR c/64279) (v3)
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)
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)
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)
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, COND_EXPR, result_typ
Re: Implement -Wduplicated-branches (PR c/64279) (v3)
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)
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)
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 do_warn_duplicated_branch
Re: Implement -Wduplicated-branches (PR c/64279) (v3)
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)
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)
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)
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)
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)
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