[Bug c++/107571] New: Missing fallthrough attribute diagnostics

[Bug c++/107571] New: Missing fallthrough attribute diagnostics

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107571

            Bug ID: 107571
           Summary: Missing fallthrough attribute diagnostics
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: jakub at gcc dot gnu.org
  Target Milestone: ---

On:

void
foo (int n)
{
  void g(), h(), i();
  switch (n)
    {
    case 1:
    case 2:
      g();
      [[fallthrough]];
    case 3: // warning on fallthrough discouraged
      do {
        [[fallthrough]]; // error: next statement is not part of the same
substatement execution
      } while (false);
    case 6:
      do {
        [[fallthrough]]; // error: next statement is not part of the same
substatement execution
      } while (n--);
    case 7:
      while (false) {
        [[fallthrough]]; // error: next statement is not part of the same
substatement execution
      }
    case 5:
      h();
    case 4: // implementation may warn on fallthrough
      i();
      [[fallthrough]]; // error
    }
}

mentioned in https://isocpp.org/files/papers/D2552R1.pdf we don't diagnose
misplaced [[fallthrough]] in 2 spots.
The original dump shows:
  switch (n)
    {
      case 1:;
      case 2:;
      <<cleanup_point <<< Unknown tree: expr_stmt
        g () >>>>>;
      <<cleanup_point <<< Unknown tree: expr_stmt
        .FALLTHROUGH () >>>>>;
      case 3:;
      <<cleanup_point <<< Unknown tree: expr_stmt
        .FALLTHROUGH () >>>>>;
      case 6:;
      <D.2778>:;
      <<cleanup_point <<< Unknown tree: expr_stmt
        .FALLTHROUGH () >>>>>;
      if (<<cleanup_point n--  != 0>>) goto <D.2778>; else goto <D.2776>;
      <D.2776>:;
      case 7:;
      goto <D.2779>;
      <<cleanup_point <<< Unknown tree: expr_stmt
        .FALLTHROUGH () >>>>>;
      <D.2779>:;
      case 5:;
      <<cleanup_point <<< Unknown tree: expr_stmt
        h () >>>>>;
      case 4:;
      <<cleanup_point <<< Unknown tree: expr_stmt
        i () >>>>>;
      <<cleanup_point <<< Unknown tree: expr_stmt
        .FALLTHROUGH () >>>>>;
    }
so the reason we don't warn in the do { ... } while (false); case is that it
disappears probably during
genericize_c_loop and the while (false) case because the genericization in that
case makes the loop body followed by artificial label.

[Bug c++/107571] Missing fallthrough attribute diagnostics

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107571

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Apparently this is
https://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#2406
which we probably never implemented.

[Bug c++/107571] Missing fallthrough attribute diagnostics

[mpolacek at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107571

Marek Polacek <mpolacek at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2022-11-08
             Status|UNCONFIRMED                 |NEW

[Bug c++/107571] Missing fallthrough attribute diagnostics

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107571

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jsm28 at gcc dot gnu.org

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
And looking at the C wording in n2596.pdf, seems it is different again:
"The next block item(6.8.2) that would be encountered after a fallthrough
declaration shall be a case label or default label associated with the smallest
enclosing switch statement."
So, if my understanding is well,
  int j = 0;
  switch (n)
    {
    case 1:
      for (int i = 0; i < 1; ++i)
        {
          [[fallthrough]]; // Invalid in both C and C++
        }
    case 2:
      while (++j < 2)
        {
          [[fallthrough]]; // Invalid in both C and C++
        }
    case 3:
      do
        {
          [[fallthrough]]; // Invalid in both C and C++
        }
      while (0);
    case 4:
      if (1)
        {
          [[fallthrough]]; // Invalid in C, valid in C++?
        }
    case 5:
      for (int i = 0; i < 1; ++i)
        [[fallthrough]]; // Invalid in C++, dunno about C
    case 6:
      while (++j < 2)
        [[fallthrough]]; // Invalid in C++, dunno about C
    case 7:
      do
        [[fallthrough]]; // Invalid in C++, dunno about C
      while (0);
    case 8:
      if (1)
        [[fallthrough]]; // Dunno about either C or C++
    case 9:
      {
        [[fallthrough]]; // Invalid in C, valid in C++?
      }
    default:
      break;
    }

[Bug c++/107571] Missing fallthrough attribute diagnostics

[joseph at codesourcery dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107571

--- Comment #3 from joseph at codesourcery dot com <joseph at codesourcery dot com> ---
On Tue, 8 Nov 2022, jakub at gcc dot gnu.org via Gcc-bugs wrote:

> And looking at the C wording in n2596.pdf, seems it is different again:

That's a very old version.  N3054 is the most recent public draft (SC22 
N5777 is more recent than that and is the actual CD ballot text).

> "The next block item(6.8.2) that would be encountered after a fallthrough
> declaration shall be a case label or default label associated with the smallest
> enclosing switch statement."

It's not exactly clear what "next block item" is for any of the examples 
you give - next lexically (OK once the current one is exited) or in 
execution (no good for a Constraint)?  And thus not clear that any of 
these are invalid.  I've noted that the inconsistency with C++ should be 
raised in an NB comment.

[Bug c++/107571] Missing fallthrough attribute diagnostics

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107571

--- Comment #4 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:52eedfa00960f2d255ec542626e3531a65aa8bb8

commit r14-5561-g52eedfa00960f2d255ec542626e3531a65aa8bb8
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Fri Nov 17 15:43:31 2023 +0100

    c++: Implement C++ DR 2406 - [[fallthrough]] attribute and iteration
statements

    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.

    2023-11-17  Jakub Jelinek  <jakub@redhat.com>

            PR c++/107571
    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.
            * internal-fn.def (FALLTHROUGH): Mention in comment the special
            meaning of the TREE_NOTHROW/GF_CALL_NOTHROW flag on the calls.
    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.

[Bug c++/107571] Missing fallthrough attribute diagnostics

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107571

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Should be implemented now.