[C/C++] RFC: Implement -Wduplicated-cond (PR c/64249) (take

2)
Reply-To: 
In-Reply-To: <20150918100606.GF27588@redhat.com>

On Fri, Sep 18, 2015 at 12:06:06PM +0200, Marek Polacek wrote:
> > Since we don't know bar's side-effects we must assume they change
> > the value of a and so we must avoid diagnosing the third if.
> 
> Ok, I'm convinced now.  We have something similar in the codebase:
> libsupc++/eh_catch.cc has
> 
>   int count = header->handlerCount;
>   if (count < 0)
>     {   
>       // This exception was rethrown.  Decrement the (inverted) catch
>       // count and remove it from the chain when it reaches zero.
>       if (++count == 0)
>         globals->caughtExceptions = header->nextException;
>     }   
>   else if (--count == 0)
>     {   
>       // Handling for this exception is complete.  Destroy the object.
>       globals->caughtExceptions = header->nextException;
>       _Unwind_DeleteException (&header->unwindHeader);
>       return;
>     }   
>   else if (count < 0)
>     // A bug in the exception handling library or compiler.
>     std::terminate ();
> 
> Here all arms are reachable.  I guess I need to kill the chain of conditions
> when we find something with side-effects, exactly as you suggested.

Done in the below.  This version actually bootstraps, because I've added
-Wno-duplicated-cond for insn-dfatab.o and insn-latencytab.o (don't know
how to fix these) + I've tweaked a condition in genemit.c.  The problem
here is that we have

      if (INTVAL (x) == 0)
        printf ("const0_rtx");
      else if (INTVAL (x) == 1)
        printf ("const1_rtx");
      else if (INTVAL (x) == -1) 
        printf ("constm1_rtx");
      // ...
      else if (INTVAL (x) == STORE_FLAG_VALUE)
        printf ("const_true_rtx");

and STORE_FLAG_VALUE happens to be 1, so we have two same conditions.
STORE_FLAG_VALUE is 1 or -1, but according to the documentation it can
also be some other number so we should keep this if statement.  I've
avoided the warning by adding STORE_FLAG_VALUE > 1 check.

How does this look like now?

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

2015-09-18  Marek Polacek  <polacek@redhat.com>

	PR c/64249
	* c-common.c (warn_duplicated_cond_add_or_warn): New function.
	* c-common.h (warn_duplicated_cond_add_or_warn): Declare.
	* c.opt (Wduplicated-cond): New option.

	* c-parser.c (c_parser_statement_after_labels): Add CHAIN parameter
	and pass it down to c_parser_if_statement.
	(c_parser_else_body): Add CHAIN parameter and pass it down to
	c_parser_statement_after_labels.
	(c_parser_if_statement): Add CHAIN parameter.  Add code to warn about
	duplicated if-else-if conditions.

	* parser.c (cp_parser_statement): Add CHAIN parameter and pass it
	down to cp_parser_selection_statement.
	(cp_parser_selection_statement): Add CHAIN parameter.  Add code to
	warn about duplicated if-else-if conditions.
	(cp_parser_implicitly_scoped_statement): Add CHAIN parameter and pass
	it down to cp_parser_statement.

	* doc/invoke.texi: Document -Wduplicated-cond.
	* Makefile.in (insn-latencytab.o): Use -Wno-duplicated-cond.
	(insn-dfatab.o): Likewise.
	* genemit.c (gen_exp): Rewrite condition to avoid -Wduplicated-cond
	warning.

	* c-c++-common/Wduplicated-cond-1.c: New test.
	* c-c++-common/Wduplicated-cond-2.c: New test.
	* c-c++-common/Wduplicated-cond-3.c: New test.
	* c-c++-common/Wduplicated-cond-4.c: New test.
	* c-c++-common/Wmisleading-indentation.c (fn_37): Avoid
	-Wduplicated-cond warning.


	Marek

> Done in the below.  This version actually bootstraps, because I've added
> -Wno-duplicated-cond for insn-dfatab.o and insn-latencytab.o (don't know
> how to fix these) + I've tweaked a condition in genemit.c.  The problem
> here is that we have
>
>        if (INTVAL (x) == 0)
>          printf ("const0_rtx");
>        else if (INTVAL (x) == 1)
>          printf ("const1_rtx");
>        else if (INTVAL (x) == -1)
>          printf ("constm1_rtx");
>        // ...
>        else if (INTVAL (x) == STORE_FLAG_VALUE)
>          printf ("const_true_rtx");
>
> and STORE_FLAG_VALUE happens to be 1, so we have two same conditions.
> STORE_FLAG_VALUE is 1 or -1, but according to the documentation it can
> also be some other number so we should keep this if statement.  I've
> avoided the warning by adding STORE_FLAG_VALUE > 1 check.

Binutils and GLIBC also fail to build due to similar problems (in
addition to several errors triggered by the new -Wunused-const-variable
warning).

The one in GLIBC is trivial to fix by guarding the code with
#if N != 1:

In file included from ../sysdeps/x86_64/ldbl2mpn.c:1:0:
../sysdeps/x86_64/../i386/ldbl2mpn.c: In function 
‘__mpn_extract_long_double’:
../sysdeps/x86_64/../i386/ldbl2mpn.c:89:24: error: duplicated ‘if’ 
condition [-Werror=duplicated-cond]
     else if (res_ptr[0] != 0)
                         ^
../sysdeps/x86_64/../i386/ldbl2mpn.c:74:23: note: previously used here
     if (res_ptr[N - 1] != 0)
                        ^

The one in Binutils is pretty easy to fix too:

In function ‘stab_int_type’:
/home/msebor/scm/fsf/binutils-gdb/binutils/wrstabs.c:665:18: error: 
duplicated if’ condition [-Werror=duplicated-cond]
     else if (size == 8)
                   ^
/home/msebor/scm/fsf/binutils-gdb/binutils/wrstabs.c:663:18: note: 
previously used here
     else if (size == sizeof (long))
                   ^

but it makes me wonder how common this pattern is in portable
code and whether adding workarounds for it is the right solution
(or if it might prompt people to disable the warning, which would
be a shame).

As an aside, I would have expected the change you implemented
in GCC to get around this to trigger some other warning (such
as -Wtautological-compare) e.g., if (a > 1 && a == -1), but it
doesn't seem to, either in GCC or Clang.

>
> How does this look like now?

If no one else is concerned about the above it looks good to
me. I was hoping to see the warning emitted for conditional
expressions as well but that can be considered an enhancement.

FWIW, while testing the patch I noticed the following bug: 67629.
It seems like the same logic was we discussed in this context is
needed there as well.

Martin

On 18/09/15 18:45, Martin Sebor wrote:
> but it makes me wonder how common this pattern is in portable
> code and whether adding workarounds for it is the right solution
> (or if it might prompt people to disable the warning, which would
> be a shame).

Perhaps if we are going to warn, we could look for sizeof() and virtual 
locations in the operands, and skip the warning. It would be nice to find a 
heuristic that allows warning in most cases but skip those that appear often in 
common code.

Another alternative is to have a more heuristic version of operand_equal(), if 
two operands are equal because of "compilation-dependent" code (macro 
expansion, sizeof, etc), then they are not considered equal. that is 
operand_equal_2(sizeof(long), sizeof(long)) returns true, but 
operand_equal_2(8, sizeof(long)) returns false.

I have no idea whether it is possible to implement operand_equal_2 in a sane way.

Cheers,

Manuel.

On Fri, Sep 18, 2015 at 10:45:33AM -0600, Martin Sebor wrote:
> >Done in the below.  This version actually bootstraps, because I've added
> >-Wno-duplicated-cond for insn-dfatab.o and insn-latencytab.o (don't know
> >how to fix these) + I've tweaked a condition in genemit.c.  The problem
> >here is that we have
> >
> >       if (INTVAL (x) == 0)
> >         printf ("const0_rtx");
> >       else if (INTVAL (x) == 1)
> >         printf ("const1_rtx");
> >       else if (INTVAL (x) == -1)
> >         printf ("constm1_rtx");
> >       // ...
> >       else if (INTVAL (x) == STORE_FLAG_VALUE)
> >         printf ("const_true_rtx");
> >
> >and STORE_FLAG_VALUE happens to be 1, so we have two same conditions.
> >STORE_FLAG_VALUE is 1 or -1, but according to the documentation it can
> >also be some other number so we should keep this if statement.  I've
> >avoided the warning by adding STORE_FLAG_VALUE > 1 check.
> 
> Binutils and GLIBC also fail to build due to similar problems (in
> addition to several errors triggered by the new -Wunused-const-variable
> warning).
 
Thanks for doing this.  I've been meaning to compile a kernel with this
new warning on but I've never gotten to do it.

> The one in GLIBC is trivial to fix by guarding the code with
> #if N != 1:
> 
> In file included from ../sysdeps/x86_64/ldbl2mpn.c:1:0:
> ../sysdeps/x86_64/../i386/ldbl2mpn.c: In function
> ‘__mpn_extract_long_double’:
> ../sysdeps/x86_64/../i386/ldbl2mpn.c:89:24: error: duplicated ‘if’ condition
> [-Werror=duplicated-cond]
>     else if (res_ptr[0] != 0)
>                         ^
> ../sysdeps/x86_64/../i386/ldbl2mpn.c:74:23: note: previously used here
>     if (res_ptr[N - 1] != 0)
>                        ^
> 
> The one in Binutils is pretty easy to fix too:
> 
> In function ‘stab_int_type’:
> /home/msebor/scm/fsf/binutils-gdb/binutils/wrstabs.c:665:18: error:
> duplicated if’ condition [-Werror=duplicated-cond]
>     else if (size == 8)
>                   ^
> /home/msebor/scm/fsf/binutils-gdb/binutils/wrstabs.c:663:18: note:
> previously used here
>     else if (size == sizeof (long))
>                   ^
> 
> but it makes me wonder how common this pattern is in portable
> code and whether adding workarounds for it is the right solution
> (or if it might prompt people to disable the warning, which would
> be a shame).

Yeah :(.  I hate how such obscure stuff (we have *one* occurence in the
whole GCC codebase...) can paralyze the warning as such.

I'm sorry to have to say that I don't quite know what to do, because
these "false positives" aren't easily fixable.  Because for
"m == sizeof (int)" the FE simply sees "m == 4"; ditto for e.g.
#define M 4 and "m == M".

Perhaps move the warning into -Wextra until we have the capacity to
differentiate between those?

> As an aside, I would have expected the change you implemented
> in GCC to get around this to trigger some other warning (such
> as -Wtautological-compare) e.g., if (a > 1 && a == -1), but it
> doesn't seem to, either in GCC or Clang.

This sort of stuff isn't really trivial to do in the front end,
I'm afraid.

> >How does this look like now?
> 
> If no one else is concerned about the above it looks good to
> me. I was hoping to see the warning emitted for conditional
> expressions as well but that can be considered an enhancement.

I think this can surely be done later on.

I realized that current patch has a minor deficiency: it will start
a chain even in case the first condition has a side-effect thus the
chain should be invalid.  I'll fix this problem soon.
 
> FWIW, while testing the patch I noticed the following bug: 67629.
> It seems like the same logic was we discussed in this context is
> needed there as well.

That sounds more like a bug in tree-cfg than in the FE ;-).

Thanks,

	Marek

On Fri, Sep 18, 2015 at 08:16:52PM +0200, Manuel López-Ibáñez wrote:
> On 18/09/15 18:45, Martin Sebor wrote:
> >but it makes me wonder how common this pattern is in portable
> >code and whether adding workarounds for it is the right solution
> >(or if it might prompt people to disable the warning, which would
> >be a shame).
> 
> Perhaps if we are going to warn, we could look for sizeof() and virtual
> locations in the operands, and skip the warning. It would be nice to find a
> heuristic that allows warning in most cases but skip those that appear often
> in common code.

Yeah, but as I mentioned in the previous mail, we don't have the ability to
do that.  For sizeof perhaps we could use SIZEOF_EXPR, but since integer
constants don't have a location we can't use from_macro_expansion_at for them.

I think the warning works right in most cases already, just in some weird
cases it fires.  Perhaps that's acceptable, but I know people aren't keen on
adding kludges to their code to suppress warnings...

So like I said, maybe -Wextra for now.

> Another alternative is to have a more heuristic version of operand_equal(),
> if two operands are equal because of "compilation-dependent" code (macro
> expansion, sizeof, etc), then they are not considered equal. that is
> operand_equal_2(sizeof(long), sizeof(long)) returns true, but
> operand_equal_2(8, sizeof(long)) returns false.
> 
> I have no idea whether it is possible to implement operand_equal_2 in a sane way.

I'm not sanguine about that at all :/.

	Marek

On Mon, Sep 21, 2015 at 07:06:01PM +0200, Marek Polacek wrote:
> I realized that current patch has a minor deficiency: it will start
> a chain even in case the first condition has a side-effect thus the
> chain should be invalid.  I'll fix this problem soon.

I changed my mind, the above mean we'll warn for

int
fn3 (void)
{
  if (bar ()) 
    return 1;
  else if (a) 
    return 2;
  else if (a);
    return 3;
  return 0;
}

But I think that's ok to warn on.

So the v2 patch I posted is still the latest one.

	Marek

On 09/22/2015 06:26 AM, Marek Polacek wrote:
> On Mon, Sep 21, 2015 at 07:06:01PM +0200, Marek Polacek wrote:
>> I realized that current patch has a minor deficiency: it will start
>> a chain even in case the first condition has a side-effect thus the
>> chain should be invalid.  I'll fix this problem soon.
>
> I changed my mind, the above mean we'll warn for
>
> int
> fn3 (void)
> {
>    if (bar ())
>      return 1;
>    else if (a)
>      return 2;
>    else if (a);
>      return 3;
>    return 0;
> }
>
> But I think that's ok to warn on.

I agree.

>
> So the v2 patch I posted is still the latest one.

It's fine by me (for whatever it's worth).

Btw., if you're unhappy about having to wipe out the whole chain
after every side-effect it occurred to me that it might be possible
to do better: instead of deleting the whole chain, only remove from
it the elements that may be affected by the side-effect. This should
make it possible to keep on the chain all conditions involving local
variables whose address hasn't been taken, which I would expect to
be most in most cases.

Martin

On Tue, Sep 22, 2015 at 03:33:34PM -0600, Martin Sebor wrote:
> It's fine by me (for whatever it's worth).

Thanks.  Let's wait if Jason/Joseph or anyone else wants to chime in.
 
> Btw., if you're unhappy about having to wipe out the whole chain
> after every side-effect it occurred to me that it might be possible
> to do better: instead of deleting the whole chain, only remove from
> it the elements that may be affected by the side-effect. This should
> make it possible to keep on the chain all conditions involving local
> variables whose address hasn't been taken, which I would expect to
> be most in most cases.

I'm not unhappy about deleting the chain ;).  I'd rather not do that
because that might get somewhat hairy.  First, I don't think we have
the capability to easily detect variables whose address hasn't been
taken, second, consider e.g.

  if (j == 4) // ...
  else if ((j++, --k, ++l)) // ...
  else if (bar (j, &k)) // ...

we'd probably need some walk_tree, save the variables temporarily somewhere
etc.; that might slow and complicate things for a corner case.  Or am I being
just too lazy? ;)

Thanks,

	Marek

On 09/23/2015 10:32 AM, Marek Polacek wrote:
> On Tue, Sep 22, 2015 at 03:33:34PM -0600, Martin Sebor wrote:
>> It's fine by me (for whatever it's worth).
>
> Thanks.  Let's wait if Jason/Joseph or anyone else wants to chime in.
>
>> Btw., if you're unhappy about having to wipe out the whole chain
>> after every side-effect it occurred to me that it might be possible
>> to do better: instead of deleting the whole chain, only remove from
>> it the elements that may be affected by the side-effect. This should
>> make it possible to keep on the chain all conditions involving local
>> variables whose address hasn't been taken, which I would expect to
>> be most in most cases.
>
> I'm not unhappy about deleting the chain ;).  I'd rather not do that
> because that might get somewhat hairy.  First, I don't think we have
> the capability to easily detect variables whose address hasn't been
> taken, second, consider e.g.
>
>    if (j == 4) // ...
>    else if ((j++, --k, ++l)) // ...
>    else if (bar (j, &k)) // ...
>
> we'd probably need some walk_tree, save the variables temporarily somewhere
> etc.; that might slow and complicate things for a corner case.  Or am I being
> just too lazy? ;)
This is all running on generic, not gimple/ssa, right?  In which case, 
no you don't know what stuff might be aliased.

Jeff

On Wed, Sep 23, 2015 at 12:21:35PM -0600, Jeff Law wrote:
> On 09/23/2015 10:32 AM, Marek Polacek wrote:
> >On Tue, Sep 22, 2015 at 03:33:34PM -0600, Martin Sebor wrote:
> >>It's fine by me (for whatever it's worth).
> >
> >Thanks.  Let's wait if Jason/Joseph or anyone else wants to chime in.
> >
> >>Btw., if you're unhappy about having to wipe out the whole chain
> >>after every side-effect it occurred to me that it might be possible
> >>to do better: instead of deleting the whole chain, only remove from
> >>it the elements that may be affected by the side-effect. This should
> >>make it possible to keep on the chain all conditions involving local
> >>variables whose address hasn't been taken, which I would expect to
> >>be most in most cases.
> >
> >I'm not unhappy about deleting the chain ;).  I'd rather not do that
> >because that might get somewhat hairy.  First, I don't think we have
> >the capability to easily detect variables whose address hasn't been
> >taken, second, consider e.g.
> >
> >   if (j == 4) // ...
> >   else if ((j++, --k, ++l)) // ...
> >   else if (bar (j, &k)) // ...
> >
> >we'd probably need some walk_tree, save the variables temporarily somewhere
> >etc.; that might slow and complicate things for a corner case.  Or am I being
> >just too lazy? ;)
> This is all running on generic, not gimple/ssa, right?  In which case, no
> you don't know what stuff might be aliased.

Right.  Hence this doesn't seem doable, but I don't think that's a big deal
at all.

	Marek

Ping.

On Fri, Sep 18, 2015 at 03:44:34PM +0200, Marek Polacek wrote:
> On Fri, Sep 18, 2015 at 12:06:06PM +0200, Marek Polacek wrote:
> > > Since we don't know bar's side-effects we must assume they change
> > > the value of a and so we must avoid diagnosing the third if.
> > 
> > Ok, I'm convinced now.  We have something similar in the codebase:
> > libsupc++/eh_catch.cc has
> > 
> >   int count = header->handlerCount;
> >   if (count < 0)
> >     {   
> >       // This exception was rethrown.  Decrement the (inverted) catch
> >       // count and remove it from the chain when it reaches zero.
> >       if (++count == 0)
> >         globals->caughtExceptions = header->nextException;
> >     }   
> >   else if (--count == 0)
> >     {   
> >       // Handling for this exception is complete.  Destroy the object.
> >       globals->caughtExceptions = header->nextException;
> >       _Unwind_DeleteException (&header->unwindHeader);
> >       return;
> >     }   
> >   else if (count < 0)
> >     // A bug in the exception handling library or compiler.
> >     std::terminate ();
> > 
> > Here all arms are reachable.  I guess I need to kill the chain of conditions
> > when we find something with side-effects, exactly as you suggested.
> 
> Done in the below.  This version actually bootstraps, because I've added
> -Wno-duplicated-cond for insn-dfatab.o and insn-latencytab.o (don't know
> how to fix these) + I've tweaked a condition in genemit.c.  The problem
> here is that we have
> 
>       if (INTVAL (x) == 0)
>         printf ("const0_rtx");
>       else if (INTVAL (x) == 1)
>         printf ("const1_rtx");
>       else if (INTVAL (x) == -1) 
>         printf ("constm1_rtx");
>       // ...
>       else if (INTVAL (x) == STORE_FLAG_VALUE)
>         printf ("const_true_rtx");
> 
> and STORE_FLAG_VALUE happens to be 1, so we have two same conditions.
> STORE_FLAG_VALUE is 1 or -1, but according to the documentation it can
> also be some other number so we should keep this if statement.  I've
> avoided the warning by adding STORE_FLAG_VALUE > 1 check.
> 
> How does this look like now?
> 
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
> 
> 2015-09-18  Marek Polacek  <polacek@redhat.com>
> 
> 	PR c/64249
> 	* c-common.c (warn_duplicated_cond_add_or_warn): New function.
> 	* c-common.h (warn_duplicated_cond_add_or_warn): Declare.
> 	* c.opt (Wduplicated-cond): New option.
> 
> 	* c-parser.c (c_parser_statement_after_labels): Add CHAIN parameter
> 	and pass it down to c_parser_if_statement.
> 	(c_parser_else_body): Add CHAIN parameter and pass it down to
> 	c_parser_statement_after_labels.
> 	(c_parser_if_statement): Add CHAIN parameter.  Add code to warn about
> 	duplicated if-else-if conditions.
> 
> 	* parser.c (cp_parser_statement): Add CHAIN parameter and pass it
> 	down to cp_parser_selection_statement.
> 	(cp_parser_selection_statement): Add CHAIN parameter.  Add code to
> 	warn about duplicated if-else-if conditions.
> 	(cp_parser_implicitly_scoped_statement): Add CHAIN parameter and pass
> 	it down to cp_parser_statement.
> 
> 	* doc/invoke.texi: Document -Wduplicated-cond.
> 	* Makefile.in (insn-latencytab.o): Use -Wno-duplicated-cond.
> 	(insn-dfatab.o): Likewise.
> 	* genemit.c (gen_exp): Rewrite condition to avoid -Wduplicated-cond
> 	warning.
> 
> 	* c-c++-common/Wduplicated-cond-1.c: New test.
> 	* c-c++-common/Wduplicated-cond-2.c: New test.
> 	* c-c++-common/Wduplicated-cond-3.c: New test.
> 	* c-c++-common/Wduplicated-cond-4.c: New test.
> 	* c-c++-common/Wmisleading-indentation.c (fn_37): Avoid
> 	-Wduplicated-cond warning.
> 
> diff --git gcc/Makefile.in gcc/Makefile.in
> index c2df21d..d7caa76 100644
> --- gcc/Makefile.in
> +++ gcc/Makefile.in
> @@ -217,6 +217,8 @@ libgcov-merge-tool.o-warn = -Wno-error
>  gimple-match.o-warn = -Wno-unused
>  generic-match.o-warn = -Wno-unused
>  dfp.o-warn = -Wno-strict-aliasing
> +insn-latencytab.o-warn = -Wno-duplicated-cond
> +insn-dfatab.o-warn = -Wno-duplicated-cond
>  
>  # All warnings have to be shut off in stage1 if the compiler used then
>  # isn't gcc; configure determines that.  WARN_CFLAGS will be either
> diff --git gcc/c-family/c-common.c gcc/c-family/c-common.c
> index 4b922bf..8991215 100644
> --- gcc/c-family/c-common.c
> +++ gcc/c-family/c-common.c
> @@ -12919,4 +12919,45 @@ reject_gcc_builtin (const_tree expr, location_t loc /* = UNKNOWN_LOCATION */)
>    return false;
>  }
>  
> +/* If we're creating an if-else-if condition chain, first see if we
> +   already have this COND in the CHAIN.  If so, warn and don't add COND
> +   into the vector, otherwise add the COND there.  LOC is the location
> +   of COND.  */
> +
> +void
> +warn_duplicated_cond_add_or_warn (location_t loc, tree cond, vec<tree> **chain)
> +{
> +  /* No chain has been created yet.  Do nothing.  */
> +  if (*chain == NULL)
> +    return;
> +
> +  if (TREE_SIDE_EFFECTS (cond))
> +    {
> +      /* Uh-oh!  This condition has a side-effect, thus invalidates
> +	 the whole chain.  */
> +      delete *chain;
> +      *chain = NULL;
> +      return;
> +    }
> +
> +  unsigned int ix;
> +  tree t;
> +  bool found = false;
> +  FOR_EACH_VEC_ELT (**chain, ix, t)
> +    if (operand_equal_p (cond, t, 0))
> +      {
> +	if (warning_at (loc, OPT_Wduplicated_cond,
> +			"duplicated %<if%> condition"))
> +	  inform (EXPR_LOCATION (t), "previously used here");
> +	found = true;
> +	break;
> +      }
> +
> +  if (!found
> +      && !CONSTANT_CLASS_P (cond)
> +      /* Don't infinitely grow the chain.  */
> +      && (*chain)->length () < 512)
> +    (*chain)->safe_push (cond);
> +}
> +
>  #include "gt-c-family-c-common.h"
> diff --git gcc/c-family/c-common.h gcc/c-family/c-common.h
> index 74d1bc1..7957df5 100644
> --- gcc/c-family/c-common.h
> +++ gcc/c-family/c-common.h
> @@ -1441,5 +1441,6 @@ extern tree cilk_for_number_of_iterations (tree);
>  extern bool check_no_cilk (tree, const char *, const char *,
>  		           location_t loc = UNKNOWN_LOCATION);
>  extern bool reject_gcc_builtin (const_tree, location_t = UNKNOWN_LOCATION);
> +extern void warn_duplicated_cond_add_or_warn (location_t, tree, vec<tree> **);
>  
>  #endif /* ! GCC_C_COMMON_H */
> diff --git gcc/c-family/c.opt gcc/c-family/c.opt
> index 47ba070..f318044 100644
> --- gcc/c-family/c.opt
> +++ gcc/c-family/c.opt
> @@ -406,6 +406,10 @@ Wdiv-by-zero
>  C ObjC C++ ObjC++ Var(warn_div_by_zero) Init(1) Warning
>  Warn about compile-time integer division by zero
>  
> +Wduplicated-cond
> +C ObjC C++ ObjC++ Var(warn_duplicated_cond) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall)
> +Warn about duplicated conditions in an if-else-if chain
> +
>  Weffc++
>  C++ ObjC++ Var(warn_ecpp) Warning
>  Warn about violations of Effective C++ style rules
> diff --git gcc/c/c-parser.c gcc/c/c-parser.c
> index d5de102..9468aff 100644
> --- gcc/c/c-parser.c
> +++ gcc/c/c-parser.c
> @@ -1198,8 +1198,8 @@ static tree c_parser_compound_statement (c_parser *);
>  static void c_parser_compound_statement_nostart (c_parser *);
>  static void c_parser_label (c_parser *);
>  static void c_parser_statement (c_parser *);
> -static void c_parser_statement_after_labels (c_parser *);
> -static void c_parser_if_statement (c_parser *);
> +static void c_parser_statement_after_labels (c_parser *, vec<tree> * = NULL);
> +static void c_parser_if_statement (c_parser *, vec<tree> *);
>  static void c_parser_switch_statement (c_parser *);
>  static void c_parser_while_statement (c_parser *, bool);
>  static void c_parser_do_statement (c_parser *, bool);
> @@ -4961,10 +4961,11 @@ c_parser_statement (c_parser *parser)
>    c_parser_statement_after_labels (parser);
>  }
>  
> -/* Parse a statement, other than a labeled statement.  */
> +/* Parse a statement, other than a labeled statement.  CHAIN is a vector
> +   of if-else-if conditions.  */
>  
>  static void
> -c_parser_statement_after_labels (c_parser *parser)
> +c_parser_statement_after_labels (c_parser *parser, vec<tree> *chain)
>  {
>    location_t loc = c_parser_peek_token (parser)->location;
>    tree stmt = NULL_TREE;
> @@ -4979,7 +4980,7 @@ c_parser_statement_after_labels (c_parser *parser)
>        switch (c_parser_peek_token (parser)->keyword)
>  	{
>  	case RID_IF:
> -	  c_parser_if_statement (parser);
> +	  c_parser_if_statement (parser, chain);
>  	  break;
>  	case RID_SWITCH:
>  	  c_parser_switch_statement (parser);
> @@ -5230,10 +5231,12 @@ c_parser_if_body (c_parser *parser, bool *if_p,
>  
>  /* Parse the else body of an if statement.  This is just parsing a
>     statement but (a) it is a block in C99, (b) we handle an empty body
> -   specially for the sake of -Wempty-body warnings.  */
> +   specially for the sake of -Wempty-body warnings.  CHAIN is a vector
> +   of if-else-if conditions.  */
>  
>  static tree
> -c_parser_else_body (c_parser *parser, const token_indent_info &else_tinfo)
> +c_parser_else_body (c_parser *parser, const token_indent_info &else_tinfo,
> +		    vec<tree> *chain)
>  {
>    location_t body_loc = c_parser_peek_token (parser)->location;
>    tree block = c_begin_compound_stmt (flag_isoc99);
> @@ -5251,7 +5254,7 @@ c_parser_else_body (c_parser *parser, const token_indent_info &else_tinfo)
>        c_parser_consume_token (parser);
>      }
>    else
> -    c_parser_statement_after_labels (parser);
> +    c_parser_statement_after_labels (parser, chain);
>  
>    token_indent_info next_tinfo
>      = get_token_indent_info (c_parser_peek_token (parser));
> @@ -5265,10 +5268,11 @@ c_parser_else_body (c_parser *parser, const token_indent_info &else_tinfo)
>     if-statement:
>       if ( expression ) statement
>       if ( expression ) statement else statement
> -*/
> +
> +  CHAIN is a vector of if-else-if conditions.  */
>  
>  static void
> -c_parser_if_statement (c_parser *parser)
> +c_parser_if_statement (c_parser *parser, vec<tree> *chain)
>  {
>    tree block;
>    location_t loc;
> @@ -5294,15 +5298,47 @@ c_parser_if_statement (c_parser *parser)
>    parser->in_if_block = true;
>    first_body = c_parser_if_body (parser, &first_if, if_tinfo);
>    parser->in_if_block = in_if_block;
> +
> +  if (warn_duplicated_cond)
> +    warn_duplicated_cond_add_or_warn (EXPR_LOCATION (cond), cond, &chain);
> +
>    if (c_parser_next_token_is_keyword (parser, RID_ELSE))
>      {
>        token_indent_info else_tinfo
>  	= get_token_indent_info (c_parser_peek_token (parser));
>        c_parser_consume_token (parser);
> -      second_body = c_parser_else_body (parser, else_tinfo);
> +      if (warn_duplicated_cond)
> +	{
> +	  if (c_parser_next_token_is_keyword (parser, RID_IF)
> +	      && chain == NULL)
> +	    {
> +	      /* We've got "if (COND) else if (COND2)".  Start the
> +		 condition chain and add COND as the first element.  */
> +	      chain = new vec<tree> ();
> +	      if (!CONSTANT_CLASS_P (cond) && !TREE_SIDE_EFFECTS (cond))
> +		chain->safe_push (cond);
> +	    }
> +	  else if (!c_parser_next_token_is_keyword (parser, RID_IF))
> +	    {
> +	      /* This is if-else without subsequent if.  Zap the condition
> +		 chain; we would have already warned at this point.  */
> +	      delete chain;
> +	      chain = NULL;
> +	    }
> +	}
> +      second_body = c_parser_else_body (parser, else_tinfo, chain);
>      }
>    else
> -    second_body = NULL_TREE;
> +    {
> +      second_body = NULL_TREE;
> +      if (warn_duplicated_cond)
> +	{
> +	  /* This if statement does not have an else clause.  We don't
> +	     need the condition chain anymore.  */
> +	  delete chain;
> +	  chain = NULL;
> +	}
> +    }
>    c_finish_if_stmt (loc, cond, first_body, second_body, first_if);
>    if_stmt = c_end_compound_stmt (loc, block, flag_isoc99);
>  
> diff --git gcc/cp/parser.c gcc/cp/parser.c
> index 4f424b6..c82a5c0 100644
> --- gcc/cp/parser.c
> +++ gcc/cp/parser.c
> @@ -2023,7 +2023,7 @@ static void cp_parser_lambda_body
>  /* Statements [gram.stmt.stmt]  */
>  
>  static void cp_parser_statement
> -  (cp_parser *, tree, bool, bool *);
> +  (cp_parser *, tree, bool, bool *, vec<tree> * = NULL);
>  static void cp_parser_label_for_labeled_statement
>  (cp_parser *, tree);
>  static tree cp_parser_expression_statement
> @@ -2033,7 +2033,7 @@ static tree cp_parser_compound_statement
>  static void cp_parser_statement_seq_opt
>    (cp_parser *, tree);
>  static tree cp_parser_selection_statement
> -  (cp_parser *, bool *);
> +  (cp_parser *, bool *, vec<tree> *);
>  static tree cp_parser_condition
>    (cp_parser *);
>  static tree cp_parser_iteration_statement
> @@ -2058,7 +2058,7 @@ static void cp_parser_declaration_statement
>    (cp_parser *);
>  
>  static tree cp_parser_implicitly_scoped_statement
> -  (cp_parser *, bool *, const token_indent_info &);
> +  (cp_parser *, bool *, const token_indent_info &, vec<tree> * = NULL);
>  static void cp_parser_already_scoped_statement
>    (cp_parser *, const token_indent_info &);
>  
> @@ -9923,11 +9923,13 @@ cp_parser_lambda_body (cp_parser* parser, tree lambda_expr)
>  
>    If IF_P is not NULL, *IF_P is set to indicate whether the statement
>    is a (possibly labeled) if statement which is not enclosed in braces
> -  and has an else clause.  This is used to implement -Wparentheses.  */
> +  and has an else clause.  This is used to implement -Wparentheses.
> +
> +  CHAIN is a vector of if-else-if conditions.  */
>  
>  static void
>  cp_parser_statement (cp_parser* parser, tree in_statement_expr,
> -		     bool in_compound, bool *if_p)
> +		     bool in_compound, bool *if_p, vec<tree> *chain)
>  {
>    tree statement, std_attrs = NULL_TREE;
>    cp_token *token;
> @@ -9975,7 +9977,7 @@ cp_parser_statement (cp_parser* parser, tree in_statement_expr,
>  
>  	case RID_IF:
>  	case RID_SWITCH:
> -	  statement = cp_parser_selection_statement (parser, if_p);
> +	  statement = cp_parser_selection_statement (parser, if_p, chain);
>  	  break;
>  
>  	case RID_WHILE:
> @@ -10404,10 +10406,14 @@ cp_parser_statement_seq_opt (cp_parser* parser, tree in_statement_expr)
>     If IF_P is not NULL, *IF_P is set to indicate whether the statement
>     is a (possibly labeled) if statement which is not enclosed in
>     braces and has an else clause.  This is used to implement
> -   -Wparentheses.  */
> +   -Wparentheses.
> +
> +   CHAIN is a vector of if-else-if conditions.  This is used to implement
> +   -Wduplicated-cond.  */
>  
>  static tree
> -cp_parser_selection_statement (cp_parser* parser, bool *if_p)
> +cp_parser_selection_statement (cp_parser* parser, bool *if_p,
> +			       vec<tree> *chain)
>  {
>    cp_token *token;
>    enum rid keyword;
> @@ -10458,6 +10464,10 @@ cp_parser_selection_statement (cp_parser* parser, bool *if_p)
>  	    /* Add the condition.  */
>  	    finish_if_stmt_cond (condition, statement);
>  
> +	    if (warn_duplicated_cond)
> +	      warn_duplicated_cond_add_or_warn (token->location, condition,
> +						&chain);
> +
>  	    /* Parse the then-clause.  */
>  	    in_statement = parser->in_statement;
>  	    parser->in_statement |= IN_IF_STMT;
> @@ -10475,10 +10485,41 @@ cp_parser_selection_statement (cp_parser* parser, bool *if_p)
>  		  = get_token_indent_info (cp_lexer_peek_token (parser->lexer));
>  		/* Consume the `else' keyword.  */
>  		cp_lexer_consume_token (parser->lexer);
> +		if (warn_duplicated_cond)
> +		  {
> +		    if (cp_lexer_next_token_is_keyword (parser->lexer,
> +							RID_IF)
> +			&& chain == NULL)
> +		      {
> +			/* We've got "if (COND) else if (COND2)".  Start
> +			   the condition chain and add COND as the first
> +			   element.  */
> +			chain = new vec<tree> ();
> +			if (!CONSTANT_CLASS_P (condition)
> +			    && !TREE_SIDE_EFFECTS (condition))
> +			{
> +			  /* Wrap it in a NOP_EXPR so that we can set the
> +			     location of the condition.  */
> +			  tree e = build1 (NOP_EXPR, TREE_TYPE (condition),
> +					   condition);
> +			  SET_EXPR_LOCATION (e, token->location);
> +			  chain->safe_push (e);
> +			}
> +		      }
> +		    else if (!cp_lexer_next_token_is_keyword (parser->lexer,
> +							      RID_IF))
> +		      {
> +			/* This is if-else without subsequent if.  Zap the
> +			   condition chain; we would have already warned at
> +			   this point.  */
> +			delete chain;
> +			chain = NULL;
> +		      }
> +		  }
>  		begin_else_clause (statement);
>  		/* Parse the else-clause.  */
>  		cp_parser_implicitly_scoped_statement (parser, NULL,
> -						       guard_tinfo);
> +						       guard_tinfo, chain);
>  
>  		finish_else_clause (statement);
>  
> @@ -10500,6 +10541,12 @@ cp_parser_selection_statement (cp_parser* parser, bool *if_p)
>  		  warning_at (EXPR_LOCATION (statement), OPT_Wparentheses,
>  			      "suggest explicit braces to avoid ambiguous"
>  			      " %<else%>");
> +		if (warn_duplicated_cond)
> +		  {
> +		    /* We don't need the condition chain anymore.  */
> +		    delete chain;
> +		    chain = NULL;
> +		  }
>  	      }
>  
>  	    /* Now we're all done with the if-statement.  */
> @@ -11410,11 +11457,15 @@ cp_parser_declaration_statement (cp_parser* parser)
>     braces and has an else clause.  This is used to implement
>     -Wparentheses.
>  
> +   CHAIN is a vector of if-else-if conditions.  This is used to implement
> +   -Wduplicated-cond.
> +
>     Returns the new statement.  */
>  
>  static tree
>  cp_parser_implicitly_scoped_statement (cp_parser* parser, bool *if_p,
> -				       const token_indent_info &guard_tinfo)
> +				       const token_indent_info &guard_tinfo,
> +				       vec<tree> *chain)
>  {
>    tree statement;
>    location_t body_loc = cp_lexer_peek_token (parser->lexer)->location;
> @@ -11447,7 +11498,7 @@ cp_parser_implicitly_scoped_statement (cp_parser* parser, bool *if_p,
>        /* Create a compound-statement.  */
>        statement = begin_compound_stmt (0);
>        /* Parse the dependent-statement.  */
> -      cp_parser_statement (parser, NULL_TREE, false, if_p);
> +      cp_parser_statement (parser, NULL_TREE, false, if_p, chain);
>        /* Finish the dummy compound-statement.  */
>        finish_compound_stmt (statement);
>      }
> diff --git gcc/doc/invoke.texi gcc/doc/invoke.texi
> index 547ee2d..d6a4973 100644
> --- gcc/doc/invoke.texi
> +++ gcc/doc/invoke.texi
> @@ -241,7 +241,7 @@ Objective-C and Objective-C++ Dialects}.
>  -pedantic-errors @gol
>  -w  -Wextra  -Wall  -Waddress  -Waggregate-return  @gol
>  -Waggressive-loop-optimizations -Warray-bounds -Warray-bounds=@var{n} @gol
> --Wbool-compare -Wframe-address @gol
> +-Wbool-compare -Wduplicated-cond -Wframe-address @gol
>  -Wno-attributes -Wno-builtin-macro-redefined @gol
>  -Wc90-c99-compat -Wc99-c11-compat @gol
>  -Wc++-compat -Wc++11-compat -Wc++14-compat -Wcast-align  -Wcast-qual  @gol
> @@ -3458,6 +3458,7 @@ Options} and @ref{Objective-C and Objective-C++ Dialect Options}.
>  -Wimplicit-int @r{(C and Objective-C only)} @gol
>  -Wimplicit-function-declaration @r{(C and Objective-C only)} @gol
>  -Wbool-compare  @gol
> +-Wduplicated-cond  @gol
>  -Wcomment  @gol
>  -Wformat   @gol
>  -Wmain @r{(only for C/ObjC and unless} @option{-ffreestanding}@r{)}  @gol
> @@ -4489,6 +4490,17 @@ if ((n > 1) == 2) @{ @dots{} @}
>  @end smallexample
>  This warning is enabled by @option{-Wall}.
>  
> +@item -Wduplicated-cond
> +@opindex Wno-duplicated-cond
> +@opindex Wduplicated-cond
> +Warn about duplicated conditions in an if-else-if chain.  For instance,
> +warn for the following code:
> +@smallexample
> +if (p->q != NULL) @{ @dots{} @}
> +else if (p->q != NULL) @{ @dots{} @}
> +@end smallexample
> +This warning is enabled by @option{-Wall}.
> +
>  @item -Wframe-address
>  @opindex Wno-frame-address
>  @opindex Wframe-address
> diff --git gcc/genemit.c gcc/genemit.c
> index a2c474d..13f9119 100644
> --- gcc/genemit.c
> +++ gcc/genemit.c
> @@ -179,10 +179,10 @@ gen_exp (rtx x, enum rtx_code subroutine_type, char *used)
>        else if (INTVAL (x) == -1)
>  	printf ("constm1_rtx");
>        else if (-MAX_SAVED_CONST_INT <= INTVAL (x)
> -	  && INTVAL (x) <= MAX_SAVED_CONST_INT)
> +	       && INTVAL (x) <= MAX_SAVED_CONST_INT)
>  	printf ("const_int_rtx[MAX_SAVED_CONST_INT + (%d)]",
>  		(int) INTVAL (x));
> -      else if (INTVAL (x) == STORE_FLAG_VALUE)
> +      else if (STORE_FLAG_VALUE > 1 && INTVAL (x) == STORE_FLAG_VALUE)
>  	printf ("const_true_rtx");
>        else
>  	{
> diff --git gcc/testsuite/c-c++-common/Wduplicated-cond-1.c gcc/testsuite/c-c++-common/Wduplicated-cond-1.c
> index e69de29..4763a84 100644
> --- gcc/testsuite/c-c++-common/Wduplicated-cond-1.c
> +++ gcc/testsuite/c-c++-common/Wduplicated-cond-1.c
> @@ -0,0 +1,200 @@
> +/* PR c/64249 */
> +/* { dg-do compile } */
> +/* { dg-options "-Wduplicated-cond" } */
> +
> +#ifndef __cplusplus
> +# define bool _Bool
> +# define true 1
> +# define false 0
> +#endif
> +
> +extern int foo (void);
> +
> +int
> +fn1 (int n)
> +{
> +  if (n == 1) /* { dg-message "previously used here" } */
> +    return -1;
> +  else if (n == 2)
> +    return 0;
> +  else if (n == 1) /* { dg-warning "duplicated .if. condition" } */
> +    return 1;
> +  return 0;
> +}
> +
> +int
> +fn2 (void)
> +{
> +  if (4)
> +    return 1;
> +  else if (4)
> +    return 2;
> +
> +#define N 10
> +  if (N)
> +    return 3;
> +  else if (N)
> +    return 4;
> +}
> +
> +int
> +fn3 (int n)
> +{
> +  if (n == 42)
> +    return 1;
> +  if (n == 42)
> +    return 2;
> +
> +  if (n)
> +    if (n)
> +      if (n)
> +	if (n)
> +	  return 42;
> +
> +  if (!n)
> +    return 10;
> +  else
> +    return 11;
> +}
> +
> +int
> +fn4 (int n)
> +{
> +  if (n > 0)
> +    {
> +      if (n == 1) /* { dg-message "previously used here" } */
> +	return 1;
> +      else if (n == 1) /* { dg-warning "duplicated .if. condition" } */
> +	return 2;
> +    }
> +  else if (n < 0)
> +    {
> +      if (n < -1)
> +	return 6;
> +      else if (n < -2)
> +	{
> +	  if (n == -10) /* { dg-message "previously used here" } */
> +	    return 3;
> +	  else if (n == -10) /* { dg-warning "duplicated .if. condition" } */
> +	    return 4;
> +	}
> +    }
> +  else
> +    return 7;
> +  return 0;
> +}
> +
> +struct S { long p, q; };
> +
> +int
> +fn5 (struct S *s)
> +{
> +  if (!s->p) /* { dg-message "previously used here" } */
> +    return 12345;
> +  else if (!s->p) /* { dg-warning "duplicated .if. condition" } */
> +    return 1234;
> +  return 0;
> +}
> +
> +int
> +fn6 (int n)
> +{
> +  if (n) /* { dg-message "previously used here" } */
> +    return n;
> +  else if (n) /* { dg-warning "duplicated .if. condition" } */
> +    return n;
> +  else if (n) /* { dg-warning "duplicated .if. condition" } */
> +    return n;
> +  else if (n) /* { dg-warning "duplicated .if. condition" } */
> +    return n;
> +  else if (n) /* { dg-warning "duplicated .if. condition" } */
> +    return n;
> +  else if (n) /* { dg-warning "duplicated .if. condition" } */
> +    return n;
> +  else if (n) /* { dg-warning "duplicated .if. condition" } */
> +    return n;
> +  else if (n) /* { dg-warning "duplicated .if. condition" } */
> +    return n;
> +  return 0;
> +}
> +
> +int
> +fn7 (int n)
> +{
> +  if (n == 0) /* { dg-message "previously used here" } */
> +    return 10;
> +  else if (n == 1) /* { dg-message "previously used here" } */
> +    return 11;
> +  else if (n == 2) /* { dg-message "previously used here" } */
> +    return 12;
> +  else if (n == 3) /* { dg-message "previously used here" } */
> +    return 13;
> +  else if (n == 4) /* { dg-message "previously used here" } */
> +    return 14;
> +  else if (n == 5) /* { dg-message "previously used here" } */
> +    return 15;
> +  else if (n == 6) /* { dg-message "previously used here" } */
> +    return 16;
> +  else if (n == 7) /* { dg-message "previously used here" } */
> +    return 17;
> +  else if (n == 0) /* { dg-warning "duplicated .if. condition" } */
> +    return 100;
> +  else if (n == 1) /* { dg-warning "duplicated .if. condition" } */
> +    return 101;
> +  else if (n == 2) /* { dg-warning "duplicated .if. condition" } */
> +    return 102;
> +  else if (n == 3) /* { dg-warning "duplicated .if. condition" } */
> +    return 103;
> +  else if (n == 4) /* { dg-warning "duplicated .if. condition" } */
> +    return 104;
> +  else if (n == 5) /* { dg-warning "duplicated .if. condition" } */
> +    return 105;
> +  else if (n == 6) /* { dg-warning "duplicated .if. condition" } */
> +    return 106;
> +  else if (n == 7) /* { dg-warning "duplicated .if. condition" } */
> +    return 107;
> +  return 0;
> +}
> +
> +int
> +fn8 (bool b)
> +{
> +  if (!b) /* { dg-message "previously used here" } */
> +    return 16;
> +  else if (!b) /* { dg-warning "duplicated .if. condition" } */
> +    return 27;
> +  else
> +    return 64;
> +}
> +
> +int
> +fn9 (int i, int j, int k)
> +{
> +  if (i > 0 && j > 0 && k > 0) /* { dg-message "previously used here" } */
> +    return -999;
> +  else
> +  if (i > 0 && j > 0 && k > 0) /* { dg-warning "duplicated .if. condition" } */
> +    return 999;
> +  else
> +    return 0;
> +}
> +
> +int
> +fn10 (void)
> +{
> +  if (foo ())
> +    return 1732984;
> +  else if (foo ())
> +    return 18409;
> +  return 0;
> +}
> +
> +int
> +fn11 (int n)
> +{
> +  if (++n == 10)
> +    return 666;
> +  else if (++n == 10)
> +    return 9;
> +  return 0;
> +}
> diff --git gcc/testsuite/c-c++-common/Wduplicated-cond-2.c gcc/testsuite/c-c++-common/Wduplicated-cond-2.c
> index e69de29..90a8663 100644
> --- gcc/testsuite/c-c++-common/Wduplicated-cond-2.c
> +++ gcc/testsuite/c-c++-common/Wduplicated-cond-2.c
> @@ -0,0 +1,200 @@
> +/* PR c/64249 */
> +/* { dg-do compile } */
> +/* { dg-options "-Wall" } */
> +
> +#ifndef __cplusplus
> +# define bool _Bool
> +# define true 1
> +# define false 0
> +#endif
> +
> +extern int foo (void);
> +
> +int
> +fn1 (int n)
> +{
> +  if (n == 1) /* { dg-message "previously used here" } */
> +    return -1;
> +  else if (n == 2)
> +    return 0;
> +  else if (n == 1) /* { dg-warning "duplicated .if. condition" } */
> +    return 1;
> +  return 0;
> +}
> +
> +int
> +fn2 (void)
> +{
> +  if (4)
> +    return 1;
> +  else if (4)
> +    return 2;
> +
> +#define N 10
> +  if (N)
> +    return 3;
> +  else if (N)
> +    return 4;
> +}
> +
> +int
> +fn3 (int n)
> +{
> +  if (n == 42)
> +    return 1;
> +  if (n == 42)
> +    return 2;
> +
> +  if (n)
> +    if (n)
> +      if (n)
> +	if (n)
> +	  return 42;
> +
> +  if (!n)
> +    return 10;
> +  else
> +    return 11;
> +}
> +
> +int
> +fn4 (int n)
> +{
> +  if (n > 0)
> +    {
> +      if (n == 1) /* { dg-message "previously used here" } */
> +	return 1;
> +      else if (n == 1) /* { dg-warning "duplicated .if. condition" } */
> +	return 2;
> +    }
> +  else if (n < 0)
> +    {
> +      if (n < -1)
> +	return 6;
> +      else if (n < -2)
> +	{
> +	  if (n == -10) /* { dg-message "previously used here" } */
> +	    return 3;
> +	  else if (n == -10) /* { dg-warning "duplicated .if. condition" } */
> +	    return 4;
> +	}
> +    }
> +  else
> +    return 7;
> +  return 0;
> +}
> +
> +struct S { long p, q; };
> +
> +int
> +fn5 (struct S *s)
> +{
> +  if (!s->p) /* { dg-message "previously used here" } */
> +    return 12345;
> +  else if (!s->p) /* { dg-warning "duplicated .if. condition" } */
> +    return 1234;
> +  return 0;
> +}
> +
> +int
> +fn6 (int n)
> +{
> +  if (n) /* { dg-message "previously used here" } */
> +    return n;
> +  else if (n) /* { dg-warning "duplicated .if. condition" } */
> +    return n;
> +  else if (n) /* { dg-warning "duplicated .if. condition" } */
> +    return n;
> +  else if (n) /* { dg-warning "duplicated .if. condition" } */
> +    return n;
> +  else if (n) /* { dg-warning "duplicated .if. condition" } */
> +    return n;
> +  else if (n) /* { dg-warning "duplicated .if. condition" } */
> +    return n;
> +  else if (n) /* { dg-warning "duplicated .if. condition" } */
> +    return n;
> +  else if (n) /* { dg-warning "duplicated .if. condition" } */
> +    return n;
> +  return 0;
> +}
> +
> +int
> +fn7 (int n)
> +{
> +  if (n == 0) /* { dg-message "previously used here" } */
> +    return 10;
> +  else if (n == 1) /* { dg-message "previously used here" } */
> +    return 11;
> +  else if (n == 2) /* { dg-message "previously used here" } */
> +    return 12;
> +  else if (n == 3) /* { dg-message "previously used here" } */
> +    return 13;
> +  else if (n == 4) /* { dg-message "previously used here" } */
> +    return 14;
> +  else if (n == 5) /* { dg-message "previously used here" } */
> +    return 15;
> +  else if (n == 6) /* { dg-message "previously used here" } */
> +    return 16;
> +  else if (n == 7) /* { dg-message "previously used here" } */
> +    return 17;
> +  else if (n == 0) /* { dg-warning "duplicated .if. condition" } */
> +    return 100;
> +  else if (n == 1) /* { dg-warning "duplicated .if. condition" } */
> +    return 101;
> +  else if (n == 2) /* { dg-warning "duplicated .if. condition" } */
> +    return 102;
> +  else if (n == 3) /* { dg-warning "duplicated .if. condition" } */
> +    return 103;
> +  else if (n == 4) /* { dg-warning "duplicated .if. condition" } */
> +    return 104;
> +  else if (n == 5) /* { dg-warning "duplicated .if. condition" } */
> +    return 105;
> +  else if (n == 6) /* { dg-warning "duplicated .if. condition" } */
> +    return 106;
> +  else if (n == 7) /* { dg-warning "duplicated .if. condition" } */
> +    return 107;
> +  return 0;
> +}
> +
> +int
> +fn8 (bool b)
> +{
> +  if (!b) /* { dg-message "previously used here" } */
> +    return 16;
> +  else if (!b) /* { dg-warning "duplicated .if. condition" } */
> +    return 27;
> +  else
> +    return 64;
> +}
> +
> +int
> +fn9 (int i, int j, int k)
> +{
> +  if (i > 0 && j > 0 && k > 0) /* { dg-message "previously used here" } */
> +    return -999;
> +  else
> +  if (i > 0 && j > 0 && k > 0) /* { dg-warning "duplicated .if. condition" } */
> +    return 999;
> +  else
> +    return 0;
> +}
> +
> +int
> +fn10 (void)
> +{
> +  if (foo ())
> +    return 1732984;
> +  else if (foo ())
> +    return 18409;
> +  return 0;
> +}
> +
> +int
> +fn11 (int n)
> +{
> +  if (++n == 10)
> +    return 666;
> +  else if (++n == 10)
> +    return 9;
> +  return 0;
> +}
> diff --git gcc/testsuite/c-c++-common/Wduplicated-cond-3.c gcc/testsuite/c-c++-common/Wduplicated-cond-3.c
> index e69de29..e3b5ac0 100644
> --- gcc/testsuite/c-c++-common/Wduplicated-cond-3.c
> +++ gcc/testsuite/c-c++-common/Wduplicated-cond-3.c
> @@ -0,0 +1,204 @@
> +/* PR c/64249 */
> +/* { dg-do compile } */
> +/* { dg-options "-Wall -Wno-duplicated-cond" } */
> +
> +#ifndef __cplusplus
> +# define bool _Bool
> +# define true 1
> +# define false 0
> +#endif
> +
> +extern int foo (void);
> +
> +int
> +fn1 (int n)
> +{
> +  if (n == 1)
> +    return -1;
> +  else if (n == 2)
> +    return 0;
> +  else if (n == 1)
> +    return 1;
> +  return 0;
> +}
> +
> +int
> +fn2 (void)
> +{
> +  if (4)
> +    return 1;
> +  else if (4)
> +    return 2;
> +
> +#define N 10
> +  if (N)
> +    return 3;
> +  else if (N)
> +    return 4;
> +}
> +
> +int
> +fn3 (int n)
> +{
> +  if (n == 42)
> +    return 1;
> +  if (n == 42)
> +    return 2;
> +
> +  if (n)
> +    if (n)
> +      if (n)
> +	if (n)
> +	  return 42;
> +
> +  if (!n)
> +    return 10;
> +  else
> +    return 11;
> +}
> +
> +int
> +fn4 (int n)
> +{
> +  if (n > 0)
> +    {
> +      if (n == 1)
> +	return 1;
> +      else if (n == 1)
> +	return 2;
> +    }
> +  else if (n < 0)
> +    {
> +      if (n < -1)
> +	return 6;
> +      else if (n < -2)
> +	{
> +	  if (n == -10)
> +	    return 3;
> +	  else if (n == -10)
> +	    return 4;
> +	}
> +    }
> +  else
> +    return 7;
> +  return 0;
> +}
> +
> +struct S { long p, q; };
> +
> +int
> +fn5 (struct S *s)
> +{
> +  if (!s->p)
> +    return 12345;
> +  else if (!s->p)
> +    return 1234;
> +  return 0;
> +}
> +
> +int
> +fn6 (int n)
> +{
> +  if (n)
> +    return n;
> +  else if (n)
> +    return n;
> +  else if (n)
> +    return n;
> +  else if (n)
> +    return n;
> +  else if (n)
> +    return n;
> +  else if (n)
> +    return n;
> +  else if (n)
> +    return n;
> +  else if (n)
> +    return n;
> +  return 0;
> +}
> +
> +int
> +fn7 (int n)
> +{
> +  if (n == 0)
> +    return 10;
> +  else if (n == 1)
> +    return 11;
> +  else if (n == 2)
> +    return 12;
> +  else if (n == 3)
> +    return 13;
> +  else if (n == 4)
> +    return 14;
> +  else if (n == 5)
> +    return 15;
> +  else if (n == 6)
> +    return 16;
> +  else if (n == 7)
> +    return 17;
> +  else if (n == 0)
> +    return 100;
> +  else if (n == 1)
> +    return 101;
> +  else if (n == 2)
> +    return 102;
> +  else if (n == 3)
> +    return 103;
> +  else if (n == 4)
> +    return 104;
> +  else if (n == 5)
> +    return 105;
> +  else if (n == 6)
> +    return 106;
> +  else if (n == 7)
> +    return 107;
> +  return 0;
> +}
> +
> +int
> +fn8 (bool b)
> +{
> +  if (!b)
> +    return 16;
> +  else if (!b)
> +    return 27;
> +  else
> +    return 64;
> +}
> +
> +int
> +fn9 (int i, int j, int k)
> +{
> +  if ((i > 0 && j > 0 && k > 0)
> +      && ((i > 11 && j == 76 && k < 10)
> +	  || (i < 0 && j == 99 && k > 103)))
> +    return -999;
> +  else
> +  if ((i > 0 && j > 0 && k > 0)
> +      && ((i > 11 && j == 76 && k < 10)
> +	  || (i < 0 && j == 99 && k > 103)))
> +    return 999;
> +  else
> +    return 0;
> +}
> +
> +int
> +fn10 (void)
> +{
> +  if (foo ())
> +    return 1732984;
> +  else if (foo ())
> +    return 18409;
> +  return 0;
> +}
> +
> +int
> +fn11 (int n)
> +{
> +  if (++n == 10)
> +    return 666;
> +  else if (++n == 10)
> +    return 9;
> +  return 0;
> +}
> diff --git gcc/testsuite/c-c++-common/Wduplicated-cond-4.c gcc/testsuite/c-c++-common/Wduplicated-cond-4.c
> index e69de29..4fb7e17 100644
> --- gcc/testsuite/c-c++-common/Wduplicated-cond-4.c
> +++ gcc/testsuite/c-c++-common/Wduplicated-cond-4.c
> @@ -0,0 +1,32 @@
> +/* PR c/64249 */
> +/* { dg-do compile } */
> +/* { dg-options "-Wduplicated-cond" } */
> +/* Test we don't warn if a condition in an if-else-if chain
> +   has a side-effect.  E.g. __cxxabiv1::__cxa_end_catch ()
> +   uses such a construction.  */
> +
> +extern int a, bar (void);
> +
> +int
> +fn1 (void)
> +{
> +  if (a)
> +    return 1;
> +  else if (bar ())
> +    return 2;
> +  else if (a)
> +    return 3;
> +  return 0;
> +}
> +
> +int
> +fn2 (int c)
> +{
> +  if (c < 0)
> +    return 1;
> +  else if (--c == 0)
> +    return 2;
> +  else if (c < 0)
> +    return 3;
> +  return 0;
> +}
> diff --git gcc/testsuite/c-c++-common/Wmisleading-indentation.c gcc/testsuite/c-c++-common/Wmisleading-indentation.c
> index 0d6d8d2..ca13141 100644
> --- gcc/testsuite/c-c++-common/Wmisleading-indentation.c
> +++ gcc/testsuite/c-c++-common/Wmisleading-indentation.c
> @@ -708,7 +708,7 @@ fn_37 (void)
>  
>    if (flagA)
>      ;
> -  else if (flagA); /* { dg-message "8: ...this 'if' clause" } */
> +  else if (flagB); /* { dg-message "8: ...this 'if' clause" } */
>      foo (0); /* { dg-warning "statement is indented as if" } */
>    while (flagA) /* { dg-message "3: ...this 'while' clause" } */
>      /* blah */;
> @@ -716,13 +716,13 @@ fn_37 (void)
>  
>    if (flagA)
>      ;
> -  else if (flagA) /* { dg-message "8: ...this 'if' clause" } */
> +  else if (flagB) /* { dg-message "8: ...this 'if' clause" } */
>      foo (1);
>      foo (2); /* { dg-warning "statement is indented as if" } */
>  
>    if (flagA)
>      foo (1);
> -  else if (flagA) /* { dg-message "8: ...this 'if' clause" } */
> +  else if (flagB) /* { dg-message "8: ...this 'if' clause" } */
>      foo (2);
>      foo (3); /* { dg-warning "statement is indented as if" } */
>  
> 
> 	Marek

	Marek

The C front-end changes are OK.

On 09/30/2015 09:47 AM, Joseph Myers wrote:
> The C front-end changes are OK.
The rest are OK as well.

jeff

On Wed, Sep 30, 2015 at 12:45:35PM -0600, Jeff Law wrote:
> On 09/30/2015 09:47 AM, Joseph Myers wrote:
> >The C front-end changes are OK.
> The rest are OK as well.

Thanks Jeff & Joseph.

I'm going to apply the patch soon; should it draw the ire of users, I'll
move the option to -Wextra.

	Marek

On Fri, Oct 2, 2015 at 3:23 AM, Marek Polacek <polacek@redhat.com> wrote:
> On Wed, Sep 30, 2015 at 12:45:35PM -0600, Jeff Law wrote:
>> On 09/30/2015 09:47 AM, Joseph Myers wrote:
>> >The C front-end changes are OK.
>> The rest are OK as well.
>
> Thanks Jeff & Joseph.
>
> I'm going to apply the patch soon; should it draw the ire of users, I'll
> move the option to -Wextra.

It breaks bootstrap:

https://gcc.gnu.org/ml/gcc-regression/2015-10/msg00031.html


../../src-trunk/gcc/fortran/trans-intrinsic.c: In function âvoid
gfc_conv_intrinsic_leadz(gfc_se*, gfc_expr*)â:
../../src-trunk/gcc/fortran/trans-intrinsic.c:4886:8: error:
duplicated âifâ condition [-Werror=duplicated-cond]
   else if (argsize <= LONG_TYPE_SIZE)
        ^
../../src-trunk/gcc/fortran/trans-intrinsic.c:4881:3: note: previously used here
   if (argsize <= INT_TYPE_SIZE)
   ^
../../src-trunk/gcc/fortran/trans-intrinsic.c: In function âvoid
gfc_conv_intrinsic_trailz(gfc_se*, gfc_expr*)â:
../../src-trunk/gcc/fortran/trans-intrinsic.c:5003:8: error:
duplicated âifâ condition [-Werror=duplicated-cond]
   else if (argsize <= LONG_TYPE_SIZE)
        ^
../../src-trunk/gcc/fortran/trans-intrinsic.c:4998:3: note: previously used here
   if (argsize <= INT_TYPE_SIZE)
   ^
../../src-trunk/gcc/fortran/trans-intrinsic.c: In function âvoid
gfc_conv_intrinsic_popcnt_poppar(gfc_se*, gfc_expr*, int)â:
../../src-trunk/gcc/fortran/trans-intrinsic.c:5109:8: error:
duplicated âifâ condition [-Werror=duplicated-cond]
   else if (argsize <= LONG_TYPE_SIZE)
        ^
../../src-trunk/gcc/fortran/trans-intrinsic.c:5102:3: note: previously used here
   if (argsize <= INT_TYPE_SIZE)
   ^

since int may have the same size as long and long may have the
same size as long long.

On Fri, Oct 02, 2015 at 08:43:30AM -0700, H.J. Lu wrote:
> On Fri, Oct 2, 2015 at 3:23 AM, Marek Polacek <polacek@redhat.com> wrote:
> > On Wed, Sep 30, 2015 at 12:45:35PM -0600, Jeff Law wrote:
> >> On 09/30/2015 09:47 AM, Joseph Myers wrote:
> >> >The C front-end changes are OK.
> >> The rest are OK as well.
> >
> > Thanks Jeff & Joseph.
> >
> > I'm going to apply the patch soon; should it draw the ire of users, I'll
> > move the option to -Wextra.
> 
> It breaks bootstrap:
> 
> https://gcc.gnu.org/ml/gcc-regression/2015-10/msg00031.html
> 
> 
> ../../src-trunk/gcc/fortran/trans-intrinsic.c: In function âvoid
> gfc_conv_intrinsic_leadz(gfc_se*, gfc_expr*)â:
> ../../src-trunk/gcc/fortran/trans-intrinsic.c:4886:8: error:
> duplicated âifâ condition [-Werror=duplicated-cond]
>    else if (argsize <= LONG_TYPE_SIZE)
>         ^
> ../../src-trunk/gcc/fortran/trans-intrinsic.c:4881:3: note: previously used here
>    if (argsize <= INT_TYPE_SIZE)
>    ^
> ../../src-trunk/gcc/fortran/trans-intrinsic.c: In function âvoid
> gfc_conv_intrinsic_trailz(gfc_se*, gfc_expr*)â:
> ../../src-trunk/gcc/fortran/trans-intrinsic.c:5003:8: error:
> duplicated âifâ condition [-Werror=duplicated-cond]
>    else if (argsize <= LONG_TYPE_SIZE)
>         ^
> ../../src-trunk/gcc/fortran/trans-intrinsic.c:4998:3: note: previously used here
>    if (argsize <= INT_TYPE_SIZE)
>    ^
> ../../src-trunk/gcc/fortran/trans-intrinsic.c: In function âvoid
> gfc_conv_intrinsic_popcnt_poppar(gfc_se*, gfc_expr*, int)â:
> ../../src-trunk/gcc/fortran/trans-intrinsic.c:5109:8: error:
> duplicated âifâ condition [-Werror=duplicated-cond]
>    else if (argsize <= LONG_TYPE_SIZE)
>         ^
> ../../src-trunk/gcc/fortran/trans-intrinsic.c:5102:3: note: previously used here
>    if (argsize <= INT_TYPE_SIZE)
>    ^
> 
> since int may have the same size as long and long may have the
> same size as long long.

Oh well, sorry about that.  I don't think this easily fixable at present :(.

I opened PR67819 for this problem.  Until that is resolved, I will have to
move -Wduplicated-cond out of -Wall and -Wextra.

	Marek

Marek Polacek <polacek@redhat.com> writes:

> diff --git gcc/Makefile.in gcc/Makefile.in
> index c2df21d..d7caa76 100644
> --- gcc/Makefile.in
> +++ gcc/Makefile.in
> @@ -217,6 +217,8 @@ libgcov-merge-tool.o-warn = -Wno-error
>  gimple-match.o-warn = -Wno-unused
>  generic-match.o-warn = -Wno-unused
>  dfp.o-warn = -Wno-strict-aliasing
> +insn-latencytab.o-warn = -Wno-duplicated-cond
> +insn-dfatab.o-warn = -Wno-duplicated-cond

cc1plus: error: unrecognized command line option "-Wno-duplicated-cond"
make[3]: *** [insn-dfatab.o] Error 1

Andreas.