[PATCH] gimple: Wrong -Wimplicit-fallthrough with if(1) [PR103597]

[Marek Polacek <polacek@redhat.com>]

This patch fixes a wrong -Wimplicit-fallthrough warning for

    case 0:
      if (1)  // wrong may fallthrough
	return 0;
    case 1:

which in .gimple looks like

    <D.1981>: // case 0
    if (1 != 0) goto <D.1985>; else goto <D.1986>;
    <D.1985>:
    D.1987 = 0;
    // predicted unlikely by early return (on trees) predictor.
    return D.1987;
    <D.1986>:  // dead
    <D.1982>: // case 1

and the warning thinks that <D.1986>: falls through to <D.1982>:.  It
does not know that <D.1986> is effectively a dead label, only reachable
through fallthrough from previous instructions, never jumped to.  To
that effect, Jakub introduced UNUSED_LABEL_P, which is set on such dead
labels.

collect_fallthrough_labels has code to deal with cases like

    case 2:
      if (e != 10)
	i++; // this may fallthru, warn
      else
	return 44;
    case 3:

which collects labels that may fall through.  Here it sees the "goto <D.1990>;"
at the end of the then branch and so when the warning reaches

    ...
    <D.1990>: // from if-then
    <D.1984>: // case 3

it knows it should warn about the possible fallthrough.  But an UNUSED_LABEL_P
is not a label that can fallthrough like that, so it should ignore those.

However, we still want to warn about this:

    case 0:
      if (1)
	n++; // falls through
    case 1:

so collect_fallthrough_labels needs to return the "n = n + 1;" statement, rather
than the dead label.

Co-authored-by: Jakub Jelinek <jakub@redhat.com>

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

	PR middle-end/103597

gcc/ChangeLog:

	* gimplify.cc (collect_fallthrough_labels): Don't push UNUSED_LABEL_Ps
	into labels.  Maybe set prev to the statement preceding UNUSED_LABEL_P.
	(gimplify_cond_expr): Set UNUSED_LABEL_P.
	* tree.h (UNUSED_LABEL_P): New.

gcc/testsuite/ChangeLog:

	* c-c++-common/Wimplicit-fallthrough-39.c: New test.
---
 gcc/gimplify.cc                               |  54 ++++++-
 .../c-c++-common/Wimplicit-fallthrough-39.c   | 140 ++++++++++++++++++
 gcc/tree.h                                    |   6 +
 3 files changed, 194 insertions(+), 6 deletions(-)
 create mode 100644 gcc/testsuite/c-c++-common/Wimplicit-fallthrough-39.c

diff --git a/gcc/gimplify.cc b/gcc/gimplify.cc
index f62f150fc08..2588824dce2 100644
--- a/gcc/gimplify.cc
+++ b/gcc/gimplify.cc
@@ -2250,9 +2250,9 @@ last_stmt_in_scope (gimple *stmt)
     }
 }
 
-/* Collect interesting labels in LABELS and return the statement preceding
-   another case label, or a user-defined label.  Store a location useful
-   to give warnings at *PREVLOC (usually the location of the returned
+/* Collect labels that may fall through into LABELS and return the statement
+   preceding another case label, or a user-defined label.  Store a location
+   useful to give warnings at *PREVLOC (usually the location of the returned
    statement or of its surrounding scope).  */
 
 static gimple *
@@ -2331,8 +2331,12 @@ collect_fallthrough_labels (gimple_stmt_iterator *gsi_p,
 	  if (gsi_end_p (*gsi_p))
 	    break;
 
-	  struct label_entry l = { false_lab, if_loc };
-	  labels->safe_push (l);
+	  /* A dead label can't fall through.  */
+	  if (!UNUSED_LABEL_P (false_lab))
+	    {
+	      struct label_entry l = { false_lab, if_loc };
+	      labels->safe_push (l);
+	    }
 
 	  /* Go to the last statement of the then branch.  */
 	  gsi_prev (gsi_p);
@@ -2359,6 +2363,17 @@ collect_fallthrough_labels (gimple_stmt_iterator *gsi_p,
 		  labels->safe_push (l);
 		}
 	    }
+	  /* This case is about
+	      if (1 != 0) goto <D.2022>; else goto <D.2023>;
+	      <D.2022>:
+	      n = n + 1; // #1
+	      <D.2023>:  // #2
+	      <D.1988>:  // #3
+	     where #2 is UNUSED_LABEL_P and we want to warn about #1 falling
+	     through to #3.  So set PREV to #1.  */
+	  else if (UNUSED_LABEL_P (false_lab))
+	    prev = gsi_stmt (*gsi_p);
+
 	  /* And move back.  */
 	  gsi_next (gsi_p);
 	}
@@ -4461,9 +4476,19 @@ gimplify_cond_expr (tree *expr_p, gimple_seq *pre_p, fallback_t fallback)
       if (TREE_OPERAND (expr, 1) == NULL_TREE
 	  && !have_else_clause_p
 	  && TREE_OPERAND (expr, 2) != NULL_TREE)
-	label_cont = label_true;
+	{
+	  /* For if (0) {} else { code; } tell -Wimplicit-fallthrough
+	     handling that label_cont == label_true can be only reached
+	     through fallthrough from { code; }.  */
+	  if (integer_zerop (COND_EXPR_COND (expr)))
+	    UNUSED_LABEL_P (label_true) = 1;
+	  label_cont = label_true;
+	}
       else
 	{
+	  bool then_side_effects
+	    = (TREE_OPERAND (expr, 1)
+	       && TREE_SIDE_EFFECTS (TREE_OPERAND (expr, 1)));
 	  gimplify_seq_add_stmt (&seq, gimple_build_label (label_true));
 	  have_then_clause_p = gimplify_stmt (&TREE_OPERAND (expr, 1), &seq);
 	  /* For if (...) { code; } else {} or
@@ -4477,6 +4502,16 @@ gimplify_cond_expr (tree *expr_p, gimple_seq *pre_p, fallback_t fallback)
 	      gimple *g;
 	      label_cont = create_artificial_label (UNKNOWN_LOCATION);
 
+	      /* For if (0) { non-side-effect-code } else { code }
+		 tell -Wimplicit-fallthrough handling that label_cont can
+		 be only reached through fallthrough from { code }.  */
+	      if (integer_zerop (COND_EXPR_COND (expr)))
+		{
+		  UNUSED_LABEL_P (label_true) = 1;
+		  if (!then_side_effects)
+		    UNUSED_LABEL_P (label_cont) = 1;
+		}
+
 	      g = gimple_build_goto (label_cont);
 
 	      /* GIMPLE_COND's are very low level; they have embedded
@@ -4493,6 +4528,13 @@ gimplify_cond_expr (tree *expr_p, gimple_seq *pre_p, fallback_t fallback)
     }
   if (!have_else_clause_p)
     {
+      /* For if (1) { code } or if (1) { code } else { non-side-effect-code }
+	 tell -Wimplicit-fallthrough handling that label_false can be only
+	 reached through fallthrough from { code }.  */
+      if (integer_nonzerop (COND_EXPR_COND (expr))
+	  && (TREE_OPERAND (expr, 2) == NULL_TREE
+	      || !TREE_SIDE_EFFECTS (TREE_OPERAND (expr, 2))))
+	UNUSED_LABEL_P (label_false) = 1;
       gimplify_seq_add_stmt (&seq, gimple_build_label (label_false));
       have_else_clause_p = gimplify_stmt (&TREE_OPERAND (expr, 2), &seq);
     }
diff --git a/gcc/testsuite/c-c++-common/Wimplicit-fallthrough-39.c b/gcc/testsuite/c-c++-common/Wimplicit-fallthrough-39.c
new file mode 100644
index 00000000000..da4aef3a318
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/Wimplicit-fallthrough-39.c
@@ -0,0 +1,140 @@
+/* PR middle-end/103597 */
+/* { dg-do compile } */
+/* { dg-options "-Wimplicit-fallthrough" } */
+
+#define E(c, e) if (c) e
+
+int
+fn0 (int n)
+{
+  switch (n)
+    {
+    case 0:
+      E (1, return 0);
+    case 1:
+      return -1;
+    }
+  return 0;
+}
+
+int
+fn1 (int n)
+{
+  switch (n)
+    {
+    case 0:
+      E (1, goto out);
+    case 1:
+      return -1;
+    }
+out:
+  return 0;
+}
+
+int
+fn2 (int n)
+{
+  switch (n)
+    {
+    case 0:
+      if (1)	  /* { dg-warning "statement may fall through" "" { target c++ } } */
+       	n++;	  /* { dg-warning "statement may fall through" "" { target c } } */
+    case 1:	  /* { dg-message "here" } */
+      return -1;
+    }
+  return 0;
+}
+
+int
+fn3 (int n)
+{
+  switch (n)
+    {
+    case 0:
+      if (0)		/* { dg-warning "statement may fall through" } */
+       	return 0;
+    case 1:		/* { dg-message "here" } */
+      return -1;
+    }
+  return 0;
+}
+
+int
+fn4 (int n)
+{
+  switch (n)
+    {
+    case 0:
+      E (0, n++);
+      --n;	  /* { dg-warning "statement may fall through" } */
+    case 1:	  /* { dg-message "here" } */
+      return -1;
+    }
+  return 0;
+}
+
+int
+fn5 (int n)
+{
+  switch (n)
+    {
+    case 0:
+      if (1)
+	return 0;
+      else
+	return -1;
+    case 1:
+      return -1;
+    }
+  return 0;
+}
+
+int
+fn6 (int n)
+{
+  switch (n)
+    {
+    case 0:
+      if (1)
+	return 0;
+      else
+	{
+meow:
+	  n--;  /* { dg-warning "statement may fall through" } */
+	}
+    case 1:   /* { dg-message "here" } */
+      return -1;
+    case 2:
+      goto meow;
+    }
+  return 0;
+}
+
+int
+fn7 (int n)
+{
+  switch (n)
+    {
+    case 0:
+      if (1)
+       	return 0;
+woof:
+    case 1:
+      return -1;
+    }
+  return 0;
+}
+
+int
+fn8 (int n)
+{
+  switch (n)
+    {
+    case 0:
+      if (1) n++; /* { dg-warning "statement may fall through" } */
+woof:		  /* { dg-message "here" } */
+    case 1:
+      return -1;
+    }
+  return 0;
+}
diff --git a/gcc/tree.h b/gcc/tree.h
index 2fedcf08b3d..cea49a500f2 100644
--- a/gcc/tree.h
+++ b/gcc/tree.h
@@ -787,6 +787,12 @@ extern void omp_clause_range_check_failed (const_tree, const char *, int,
 #define SWITCH_BREAK_LABEL_P(NODE) \
   (LABEL_DECL_CHECK (NODE)->base.protected_flag)
 
+/* Set on label that is known not to be jumped to, it can be only
+   reached by falling through from previous statements.
+   This is used to implement -Wimplicit-fallthrough.  */
+#define UNUSED_LABEL_P(NODE) \
+  (LABEL_DECL_CHECK (NODE)->base.default_def_flag)
+
 /* Nonzero means this expression is volatile in the C sense:
    its address should be of type `volatile WHATEVER *'.
    In other words, the declared item is volatile qualified.

base-commit: 1dca4ca1bf2f1b05537a1052e373d8b0ff11e53c
-- 
2.35.1