* [PATCH] gimple: Wrong -Wimplicit-fallthrough with if(1) [PR103597]
@ 2022-03-29 16:13 Marek Polacek
2022-03-29 16:39 ` Jakub Jelinek
0 siblings, 1 reply; 2+ messages in thread
From: Marek Polacek @ 2022-03-29 16:13 UTC (permalink / raw)
To: GCC Patches; +Cc: Jakub Jelinek
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
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH] gimple: Wrong -Wimplicit-fallthrough with if(1) [PR103597]
2022-03-29 16:13 [PATCH] gimple: Wrong -Wimplicit-fallthrough with if(1) [PR103597] Marek Polacek
@ 2022-03-29 16:39 ` Jakub Jelinek
0 siblings, 0 replies; 2+ messages in thread
From: Jakub Jelinek @ 2022-03-29 16:39 UTC (permalink / raw)
To: Marek Polacek; +Cc: 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
>
> <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.
LGTM.
Jakub
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-03-29 16:40 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-29 16:13 [PATCH] gimple: Wrong -Wimplicit-fallthrough with if(1) [PR103597] Marek Polacek
2022-03-29 16:39 ` Jakub Jelinek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).