diff mbox

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

Message ID 20150916155915.GA27588@redhat.com
State New
Headers show

Commit Message

Marek Polacek Sept. 16, 2015, 3:59 p.m. UTC 
This patch implements a new warning, -Wduplicated-cond.  It warns for
code such as

  if (x)
    // ...
  else if (x)
    // ...

This happened in the GCC codebase as well:
<https://gcc.gnu.org/ml/gcc-patches/2014-12/msg00875.html>.

The approach I took for this was to create vectors of conditions in an
if-else-if chain.  I.e.,

  if (A1)
    {
      if (B1) // ...
      else if (B2) // ...
      if (C1) // ...
    }
  else if (A2)
    // ...
  else
    // ...
    
contains two chains: {A1, A2} and {B1, B2}.  We don't create a chain for
C1 because that if doesn't have else-if clause.  Now, every time we're
trying to add a new condition to the chain, we first check whether the
condition is already there using operand_equal_p.  If it is, we warn.

So that's the general overview.  Since I think this oughtn't to generate
many false positives and almost always will point to a real bug (famous
last words), I've made it a part of -Wall.

I couldn't bootstrap this yet because I'm tripping over

insn-dfatab.c: In function ‘int internal_dfa_insn_code_pentiumpro(rtx_insn*)’:
insn-dfatab.c:5392:12: error: duplicated ‘if’ condition [-Werror=duplicated-cond]
       else if ((cached_memory == MEMORY_NONE) && ((cached_mode == MODE_SF) && (cached_type == TYPE_SSEDIV)))
            ^
insn-dfatab.c:5388:47: note: previously used here
       else if ((cached_memory == MEMORY_NONE) && ((cached_mode == MODE_SF) && (cached_type == TYPE_SSEDIV)))
                                               ^
insn-latencytab.c: In function ‘int insn_default_latency_pentiumpro(rtx_insn*)’:
insn-latencytab.c:5164:12: error: duplicated ‘if’ condition [-Werror=duplicated-cond]
       else if ((cached_memory == MEMORY_NONE) && ((cached_mode == MODE_SF) && (cached_type == TYPE_SSEDIV)))
            ^
insn-latencytab.c:5160:47: note: previously used here
       else if ((cached_memory == MEMORY_NONE) && ((cached_mode == MODE_SF) && (cached_type == TYPE_SSEDIV)))

Unless I'm blind those conditions are indeed the same so the warning is
right and this might be a bug either in some .md file or in genattrtab,
but I don't know how to fix this.  There might be other bootstrap problems
as well.  But I thought I would post this for comments anyway.

Regtested on x86_64-linux.

2015-09-16  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.

	* 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/Wmisleading-indentation.c (fn_37): Avoid
	-Wduplicated-cond warning.


	Marek

Comments

Martin Sebor Sept. 16, 2015, 8:59 p.m. UTC | #1 
On 09/16/2015 09:59 AM, Marek Polacek wrote:
> This patch implements a new warning, -Wduplicated-cond.  It warns for
> code such as
>
>    if (x)
>      // ...
>    else if (x)
>      // ...

As usual, I like this improvement. I think it's worth extending
to conditional expressions (e.g., x ? f() : x ? g() : h() should
be diagnosed as well).

The patch currently issues a false positive for the test case
below. I suspect the chain might need to be cleared after each
condition that involves a side-effect.

   int foo (int a)
   {
     if (a) return 1; else if (++a) return 2; else if (a) return 3;
     return 0;
   }

The patch doesn't diagnose some more involved cases like the one
below:

   if (i < 0) return 1; else if (!(i > 0 || i == 0)) return 2;

even though it does diagnose some other such cases, including:

   if (i < 0) return 1; else if (!(i >= 0)) return 2;

and even:

   int foo (int a, int b, int c) {
     if (a + c == b) return 1; else if (a + c - b == 0) return 2;
     return 0;
   }

I'm guessing this is due to operand_equal_p returning true for
some pairs of equivalent expressions and false for others. Would
making this work be feasible?

You probably didn't intend to diagnose the final else clause in
the following case but I wonder if it should be (as an extension
of the original idea):

     if (i) return 1; else if (!i) return 2; else return 0;

(In fact, I wonder if it might be worth diagnosing even the
'if (!i)' part since the condition is the inverse of the one
in the if and thus redundant).

Is diagnosing things like 'if (a) if (a);' or 'if (a); else {
if (a); }' not feasible or too involved, or did you choose not
to because you expect it might generate too many warnings? (If
the latter, perhaps it could be disabled by default and enabled
by -Wduplicated-cond=2).

Martin
Marek Polacek Sept. 17, 2015, 4:05 p.m. UTC | #2 
On Wed, Sep 16, 2015 at 02:59:12PM -0600, Martin Sebor wrote:
> On 09/16/2015 09:59 AM, Marek Polacek wrote:
> >This patch implements a new warning, -Wduplicated-cond.  It warns for
> >code such as
> >
> >   if (x)
> >     // ...
> >   else if (x)
> >     // ...
> 
> As usual, I like this improvement. I think it's worth extending
> to conditional expressions (e.g., x ? f() : x ? g() : h() should
> be diagnosed as well).
 
Maybe, probably.  I admit I wasn't thinking of conditional expressions
at all.

> The patch currently issues a false positive for the test case
> below. I suspect the chain might need to be cleared after each
> condition that involves a side-effect.
> 
>   int foo (int a)
>   {
>     if (a) return 1; else if (++a) return 2; else if (a) return 3;
>     return 0;
>   }

But the last branch here can never be reached, right?  If a == 0, foo
returns 2, otherwise it just returns 1.  So I think we should diagnose
this.

> The patch doesn't diagnose some more involved cases like the one
> below:
> 
>   if (i < 0) return 1; else if (!(i > 0 || i == 0)) return 2;
> 
> even though it does diagnose some other such cases, including:
> 
>   if (i < 0) return 1; else if (!(i >= 0)) return 2;
> 
> and even:
> 
>   int foo (int a, int b, int c) {
>     if (a + c == b) return 1; else if (a + c - b == 0) return 2;
>     return 0;
>   }
> 
> I'm guessing this is due to operand_equal_p returning true for
> some pairs of equivalent expressions and false for others. Would
> making this work be feasible?

You're right that this is limited by what operand_equal_p considers equal and
what not.  I'm not sure if there is something more convoluted else I could use 
in the FE, but I think not.  It certainly doesn't look terrible important to
me.

> You probably didn't intend to diagnose the final else clause in
> the following case but I wonder if it should be (as an extension
> of the original idea):
> 
>     if (i) return 1; else if (!i) return 2; else return 0;

Diagnosing this wasn't my intention.  I'm afraid it would be kinda hard
to do that.

> (In fact, I wonder if it might be worth diagnosing even the
> 'if (!i)' part since the condition is the inverse of the one
> in the if and thus redundant).

This, on the other hand, seems doable provided we have some predicate that
would tell us whether an expression is a logical inverse of another expression.
E.g. "a > 3" and "a <= 3".  Something akin to pred_equal_p -- invert one expr
and check whether they're equal.

But that sounds like another warning ;).  And it smells of doing some kind of
VRP in the FE - ick.

> Is diagnosing things like 'if (a) if (a);' or 'if (a); else {
> if (a); }' not feasible or too involved, or did you choose not
> to because you expect it might generate too many warnings? (If
> the latter, perhaps it could be disabled by default and enabled
> by -Wduplicated-cond=2).

I intentionally avoided that.  It would certainly be harder and I'm
not sure it's worth it.  We'd need to be careful to not warn on e.g.

  if (a > 5)
    {
      --a;
      if (a < 5) // ...
    }
etc.

Thanks a lot for looking into this.

	Marek
Jeff Law Sept. 17, 2015, 4:36 p.m. UTC | #3 
On 09/17/2015 10:05 AM, Marek Polacek wrote:
>
>> The patch doesn't diagnose some more involved cases like the one
>> below:
>>
>>    if (i < 0) return 1; else if (!(i > 0 || i == 0)) return 2;
>>
>> even though it does diagnose some other such cases, including:
>>
>>    if (i < 0) return 1; else if (!(i >= 0)) return 2;
>>
>> and even:
>>
>>    int foo (int a, int b, int c) {
>>      if (a + c == b) return 1; else if (a + c - b == 0) return 2;
>>      return 0;
>>    }
>>
>> I'm guessing this is due to operand_equal_p returning true for
>> some pairs of equivalent expressions and false for others. Would
>> making this work be feasible?
>
> You're right that this is limited by what operand_equal_p considers equal and
> what not.  I'm not sure if there is something more convoluted else I could use
> in the FE, but I think not.  It certainly doesn't look terrible important to
> me.
And you'll run into a point of diminishing returns quickly.  There's 
many ways to write this stuff in ways that are equivalent, but a pain to 
uncover.  I think relying on operand_equal_p is probably sufficient at 
this stage.

>
>> You probably didn't intend to diagnose the final else clause in
>> the following case but I wonder if it should be (as an extension
>> of the original idea):
>>
>>      if (i) return 1; else if (!i) return 2; else return 0;
>
> Diagnosing this wasn't my intention.  I'm afraid it would be kinda hard
> to do that.
This is the "unreachable code" problem.  We used to have a warning for 
that, but in practice it was so unstable it was removed.

>
>> (In fact, I wonder if it might be worth diagnosing even the
>> 'if (!i)' part since the condition is the inverse of the one
>> in the if and thus redundant).
>
> This, on the other hand, seems doable provided we have some predicate that
> would tell us whether an expression is a logical inverse of another expression.
> E.g. "a > 3" and "a <= 3".  Something akin to pred_equal_p -- invert one expr
> and check whether they're equal.
I think you just want to select a canonical form.  So you canonicalize, 
then compare.

jeff
Martin Sebor Sept. 17, 2015, 4:37 p.m. UTC | #4 
>> The patch currently issues a false positive for the test case
>> below. I suspect the chain might need to be cleared after each
>> condition that involves a side-effect.
>>
>>    int foo (int a)
>>    {
>>      if (a) return 1; else if (++a) return 2; else if (a) return 3;
>>      return 0;
>>    }
>
> But the last branch here can never be reached, right?  If a == 0, foo
> returns 2, otherwise it just returns 1.  So I think we should diagnose
> this.

It probably wasn't the best example. The general issue here is
that the second condition has a side-effect that can change (in
this case clearly does) the value of the expression.

Here's a better example:

     int a;

     int bar (void) { a = 1; return 0; }

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

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.

Martin
Marek Polacek Sept. 18, 2015, 10:06 a.m. UTC | #5 
On Thu, Sep 17, 2015 at 10:37:40AM -0600, Martin Sebor wrote:
> >>The patch currently issues a false positive for the test case
> >>below. I suspect the chain might need to be cleared after each
> >>condition that involves a side-effect.
> >>
> >>   int foo (int a)
> >>   {
> >>     if (a) return 1; else if (++a) return 2; else if (a) return 3;
> >>     return 0;
> >>   }
> >
> >But the last branch here can never be reached, right?  If a == 0, foo
> >returns 2, otherwise it just returns 1.  So I think we should diagnose
> >this.
> 
> It probably wasn't the best example. The general issue here is
> that the second condition has a side-effect that can change (in
> this case clearly does) the value of the expression.
> 
> Here's a better example:
> 
>     int a;
> 
>     int bar (void) { a = 1; return 0; }
> 
>     int foo (void) {
>         if (a) return 1;
>         else if (foo ()) return 2;
>         else if (a) return 3;
>         return 0;
>     }
> 
> 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.

Again, thanks for your comments.

	Marek
diff mbox

Patch

diff --git gcc/c-family/c-common.c gcc/c-family/c-common.c
index 4b922bf..ee15abc 100644
--- gcc/c-family/c-common.c
+++ gcc/c-family/c-common.c
@@ -12919,4 +12919,37 @@  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;
+
+  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)
+      && !TREE_SIDE_EFFECTS (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..6d91a4b 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 4d9cbe0..f475f5b 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);
@@ -4953,10 +4953,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;
@@ -4971,7 +4972,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);
@@ -5222,10 +5223,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);
@@ -5243,7 +5246,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));
@@ -5257,10 +5260,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;
@@ -5286,15 +5290,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 3a68dd7..c27c145 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 &);
 
@@ -9720,11 +9720,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;
@@ -9772,7 +9774,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:
@@ -10201,10 +10203,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;
@@ -10255,6 +10261,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;
@@ -10272,10 +10282,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);
 
@@ -10297,6 +10338,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.  */
@@ -11207,11 +11254,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;
@@ -11244,7 +11295,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 99c9685..caae737 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/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/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" } */