procyberian/gcc
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

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

Browse source 
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>

	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.
This commit is contained in:
Marek Polacek 2022-03-28 18:19:20 -04:00
parent 89976d0824
commit d886a5248e
3 changed files with 194 additions and 6 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

54
gcc/gimplify.cc
View file

@ -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);
}

140
gcc/testsuite/c-c++-common/Wimplicit-fallthrough-39.c Normal file
View file

@ -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;
}

6
gcc/tree.h
View file

@ -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.