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

2022-03-29 Thread Jakub Jelinek via Gcc-patches
On Tue, Mar 29, 2022 at 12:13:51PM -0400, Marek Polacek wrote:
> 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
> 
> : // case 0
> if (1 != 0) goto ; else goto ;
> :
> D.1987 = 0;
> // predicted unlikely by early return (on trees) predictor.
> return D.1987;
> :  // dead
> : // case 1
> 
> and the warning thinks that : falls through to :.  It
> does not know that  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 
> ;"
> at the end of the then branch and so when the warning reaches
> 
> ...
> : // from if-then
> : // 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 
> 
> 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.

LGTM.

Jakub



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

2022-03-29 Thread Marek Polacek via Gcc-patches
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

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

and the warning thinks that : falls through to :.  It
does not know that  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 ;"
at the end of the then branch and so when the warning reaches

...
: // from if-then
: // 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 

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 ; else goto ;
+ :
+ n = n + 1; // #1
+ :  // #2
+ :  // #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 (, gimple_build_label (label_true));
  have_then_clause_p = gimplify_stmt (_OPERAND (expr, 1), );
  /* For if (...) { code; } else {} or
@@ -4477,6 +4502,16 @@