On Fri, 25 Aug 2023, Jakub Jelinek wrote:
> Hi!
>
> The following patch implements
> CWG 2406 - [[fallthrough]] attribute and iteration statements
> The genericization of some loops leaves nothing at all or just a label
> after a body of a loop, so if the loop is later followed by
> case or default label in a switch, the fallthrough statement isn't
> diagnosed.
>
> The following patch implements it by marking the IFN_FALLTHROUGH call
> in such a case, such that during gimplification it can be pedantically
> diagnosed even if it is followed by case or default label or some normal
> labels followed by case/default labels.
>
> While looking into this, I've discovered other problems.
> expand_FALLTHROUGH_r is removing the IFN_FALLTHROUGH calls from the IL,
> but wasn't telling that to walk_gimple_stmt/walk_gimple_seq_mod, so
> the callers would then skip the next statement after it, and it would
> return non-NULL if the removed stmt was last in the sequence. This could
> lead to wi->callback_result being set even if it didn't appear at the very
> end of switch sequence.
> The patch makes use of wi->removed_stmt such that the callers properly
> know what happened, and use different way to handle the end of switch
> sequence case.
>
> That change discovered a bug in the gimple-walk handling of
> wi->removed_stmt. If that flag is set, the callback is telling the callers
> that the current statement has been removed and so the innermost
> walk_gimple_seq_mod shouldn't gsi_next. The problem is that
> wi->removed_stmt is only reset at the start of a walk_gimple_stmt, but that
> can be too late for some cases. If we have two nested gimple sequences,
> say GIMPLE_BIND as the last stmt of some gimple seq, we remove the last
> statement inside of that GIMPLE_BIND, set wi->removed_stmt there, don't
> do gsi_next correctly because already gsi_remove moved us to the next stmt,
> there is no next stmt, so we return back to the caller, but wi->removed_stmt
> is still set and so we don't do gsi_next even in the outer sequence, despite
> the GIMPLE_BIND (etc.) not being removed. That means we walk the
> GIMPLE_BIND with its whole sequence again.
> The patch fixes that by resetting wi->removed_stmt after we've used that
> flag in walk_gimple_seq_mod. Nothing really uses that flag after the
> outermost walk_gimple_seq_mod, it is just a private notification that
> the stmt callback has removed a stmt.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
The gimple-walk.cc/gimplify.cc changes are OK, I don't understand
the c-gimplify.cc one.
Thanks,
Richard.
> 2023-08-25 Jakub Jelinek <ja...@redhat.com>
>
> gcc/
> * gimplify.cc (expand_FALLTHROUGH_r): Use wi->removed_stmt after
> gsi_remove, change the way of passing fallthrough stmt at the end
> of sequence to expand_FALLTHROUGH. Diagnose IFN_FALLTHROUGH
> with GF_CALL_NOTHROW flag.
> (expand_FALLTHROUGH): Change loc into array of 2 location_t elts,
> don't test wi.callback_result, instead check whether first
> elt is not UNKNOWN_LOCATION and in that case pedwarn with the
> second location.
> * gimple-walk.cc (walk_gimple_seq_mod): Clear wi->removed_stmt
> after the flag has been used.
> gcc/c-family/
> * c-gimplify.cc (genericize_c_loop): For C++ mark IFN_FALLTHROUGH
> call at the end of loop body as TREE_NOTHROW.
> gcc/testsuite/
> * g++.dg/DRs/dr2406.C: New test.
>
> --- gcc/gimplify.cc.jj 2023-08-23 11:22:28.115592483 +0200
> +++ gcc/gimplify.cc 2023-08-25 13:43:58.711847414 +0200
> @@ -2588,17 +2588,33 @@ expand_FALLTHROUGH_r (gimple_stmt_iterat
> *handled_ops_p = false;
> break;
> case GIMPLE_CALL:
> + static_cast<location_t *>(wi->info)[0] = UNKNOWN_LOCATION;
> if (gimple_call_internal_p (stmt, IFN_FALLTHROUGH))
> {
> + location_t loc = gimple_location (stmt);
> gsi_remove (gsi_p, true);
> + wi->removed_stmt = true;
> +
> + /* nothrow flag is added by genericize_c_loop to mark fallthrough
> + statement at the end of some loop's body. Those should be
> + always diagnosed, either because they indeed don't precede
> + a case label or default label, or because the next statement
> + is not within the same iteration statement. */
> + if ((stmt->subcode & GF_CALL_NOTHROW) != 0)
> + {
> + pedwarn (loc, 0, "attribute %<fallthrough%> not preceding "
> + "a case label or default label");
> + break;
> + }
> +
> if (gsi_end_p (*gsi_p))
> {
> - *static_cast<location_t *>(wi->info) = gimple_location (stmt);
> - return integer_zero_node;
> + static_cast<location_t *>(wi->info)[0] = BUILTINS_LOCATION;
> + static_cast<location_t *>(wi->info)[1] = loc;
> + break;
> }
>
> bool found = false;
> - location_t loc = gimple_location (stmt);
>
> gimple_stmt_iterator gsi2 = *gsi_p;
> stmt = gsi_stmt (gsi2);
> @@ -2648,6 +2664,7 @@ expand_FALLTHROUGH_r (gimple_stmt_iterat
> }
> break;
> default:
> + static_cast<location_t *>(wi->info)[0] = UNKNOWN_LOCATION;
> break;
> }
> return NULL_TREE;
> @@ -2659,14 +2676,16 @@ static void
> expand_FALLTHROUGH (gimple_seq *seq_p)
> {
> struct walk_stmt_info wi;
> - location_t loc;
> + location_t loc[2];
> memset (&wi, 0, sizeof (wi));
> - wi.info = (void *) &loc;
> + loc[0] = UNKNOWN_LOCATION;
> + loc[1] = UNKNOWN_LOCATION;
> + wi.info = (void *) &loc[0];
> walk_gimple_seq_mod (seq_p, expand_FALLTHROUGH_r, NULL, &wi);
> - if (wi.callback_result == integer_zero_node)
> + if (loc[0] != UNKNOWN_LOCATION)
> /* We've found [[fallthrough]]; at the end of a switch, which the C++
> standard says is ill-formed; see [dcl.attr.fallthrough]. */
> - pedwarn (loc, 0, "attribute %<fallthrough%> not preceding "
> + pedwarn (loc[1], 0, "attribute %<fallthrough%> not preceding "
> "a case label or default label");
> }
>
> --- gcc/gimple-walk.cc.jj 2023-01-02 09:32:28.298199849 +0100
> +++ gcc/gimple-walk.cc 2023-08-25 14:21:10.264376130 +0200
> @@ -56,11 +56,21 @@ walk_gimple_seq_mod (gimple_seq *pseq, w
> gcc_assert (wi);
> wi->callback_result = ret;
>
> - return wi->removed_stmt ? NULL : gsi_stmt (gsi);
> + gimple *g;
> + if (!wi->removed_stmt)
> + g = gsi_stmt (gsi);
> + else
> + {
> + g = NULL;
> + wi->removed_stmt = false;
> + }
> + return g;
> }
>
> if (!wi->removed_stmt)
> gsi_next (&gsi);
> + else
> + wi->removed_stmt = false;
> }
>
> if (wi)
> --- gcc/c-family/c-gimplify.cc.jj 2023-07-11 13:40:37.594467535 +0200
> +++ gcc/c-family/c-gimplify.cc 2023-08-25 12:38:02.406574469 +0200
> @@ -307,6 +307,27 @@ genericize_c_loop (tree *stmt_p, locatio
> }
>
> append_to_statement_list (body, &stmt_list);
> + if (c_dialect_cxx ()
> + && stmt_list
> + && TREE_CODE (stmt_list) == STATEMENT_LIST)
> + {
> + tree_stmt_iterator tsi = tsi_last (stmt_list);
> + if (!tsi_end_p (tsi))
> + {
> + tree t = *tsi;
> + while (TREE_CODE (t) == CLEANUP_POINT_EXPR
> + || TREE_CODE (t) == EXPR_STMT
> + || CONVERT_EXPR_CODE_P (TREE_CODE (t)))
> + t = TREE_OPERAND (t, 0);
> + /* For C++, if iteration statement body ends with fallthrough
> + statement, mark it such that we diagnose it even if next
> + statement would be labeled statement with case/default label. */
> + if (TREE_CODE (t) == CALL_EXPR
> + && !CALL_EXPR_FN (t)
> + && CALL_EXPR_IFN (t) == IFN_FALLTHROUGH)
> + TREE_NOTHROW (t) = 1;
> + }
> + }
> finish_bc_block (&stmt_list, bc_continue, clab);
> if (incr)
> {
> --- gcc/testsuite/g++.dg/DRs/dr2406.C.jj 2023-08-25 14:16:53.095670934
> +0200
> +++ gcc/testsuite/g++.dg/DRs/dr2406.C 2023-08-25 14:16:04.732290555 +0200
> @@ -0,0 +1,81 @@
> +// DR 2406 - [[fallthrough]] attribute and iteration statements
> +// { dg-do compile { target c++11 } }
> +// { dg-options "-pedantic-errors -Wimplicit-fallthrough" }
> +
> +void bar ();
> +void baz ();
> +void qux ();
> +
> +void
> +foo (int n)
> +{
> + switch (n)
> + {
> + case 1:
> + case 2:
> + bar ();
> + [[fallthrough]];
> + case 3:
> + do
> + {
> + [[fallthrough]]; // { dg-error "attribute 'fallthrough' not
> preceding a case label or default label" }
> + }
> + while (false);
> + case 6:
> + do
> + {
> + [[fallthrough]]; // { dg-error "attribute 'fallthrough' not
> preceding a case label or default label" }
> + }
> + while (n--);
> + case 7:
> + while (false)
> + {
> + [[fallthrough]]; // { dg-error "attribute 'fallthrough' not
> preceding a case label or default label" }
> + }
> + case 5:
> + baz (); // { dg-warning "this statement may
> fall through" }
> + case 4: // { dg-message "here" }
> + qux ();
> + [[fallthrough]]; // { dg-error "attribute 'fallthrough'
> not preceding a case label or default label" }
> + }
> +}
> +
> +void
> +corge (int n)
> +{
> + switch (n)
> + {
> + case 1:
> + {
> + int i = 0;
> + do
> + {
> + [[fallthrough]]; // { dg-error "attribute 'fallthrough' not
> preceding a case label or default label" }
> + }
> + while (false);
> + }
> + case 2:
> + bar ();
> + break;
> + default:
> + break;
> + }
> +}
> +
> +void
> +fred (int n)
> +{
> + switch (n)
> + {
> + case 1:
> + {
> + int i = 0;
> + [[fallthrough]];
> + }
> + case 2:
> + bar ();
> + break;
> + default:
> + break;
> + }
> +}
>
> Jakub
>
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
