[Bug c++/103597] New: False -Wimplicit-fallthrough= involving macro

[Bug c++/103597] New: False -Wimplicit-fallthrough= involving macro

[sbergman at redhat dot com]

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

            Bug ID: 103597
           Summary: False -Wimplicit-fallthrough= involving macro
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: sbergman at redhat dot com
  Target Milestone: ---

I think this is a recent regression on GCC 12 trunk (I'm at
basepoints/gcc-12-5818-g30a08286e67; it doesn't happen with e.g.
gcc-c++-11.2.1-1.fc35.x86_64):

> $ cat test.cc
> #define E(c, e) if (c) e
> int f(int n) {
>     switch (n) {
>     case 0:
>         E(true, return 0);
>     case 1:
>         return 1;
>     }
>     return 2;
> }

> $ g++ -c -Wimplicit-fallthrough test.cc
> test.cc: In function ‘int f(int)’:
> test.cc:1:17: warning: this statement may fall through [-Wimplicit-fallthrough=]
>     1 | #define E(c, e) if (c) e
>       |                 ^~
> test.cc:5:9: note: in expansion of macro ‘E’
>     5 |         E(true, return 0);
>       |         ^
> test.cc:6:5: note: here
>     6 |     case 1:
>       |     ^~~~

[Bug c++/103597] [12 Regression] False -Wimplicit-fallthrough= involving macro since r12-5638-ga3e75c1491cd2d50

[marxin at gcc dot gnu.org]

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

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |12.0
   Last reconfirmed|                            |2021-12-07
             Status|UNCONFIRMED                 |NEW
      Known to fail|                            |12.0
     Ever confirmed|0                           |1
                 CC|                            |jason at gcc dot gnu.org,
                   |                            |marxin at gcc dot gnu.org
            Summary|False                       |[12 Regression] False
                   |-Wimplicit-fallthrough=     |-Wimplicit-fallthrough=
                   |involving macro             |involving macro since
                   |                            |r12-5638-ga3e75c1491cd2d50
      Known to work|                            |11.2.0

--- Comment #1 from Martin Liška <marxin at gcc dot gnu.org> ---
Started with r12-5638-ga3e75c1491cd2d50.

[Bug c++/103597] [12 Regression] False -Wimplicit-fallthrough= involving macro since r12-5638-ga3e75c1491cd2d50

[jakub at gcc dot gnu.org]

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

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Testcase without the >s
#define E(c, e) if (c) e

int
foo (int n)
{
  switch (n)
    {
    case 0:
      E (1, return 0);
    case 1:
      return 1;
    }
  return 2;
}

In C we actually warn about this since the warning has been introduced.

[Bug c++/103597] [12 Regression] False -Wimplicit-fallthrough= involving macro since r12-5638-ga3e75c1491cd2d50

[pinskia at gcc dot gnu.org]

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

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
For the C++ front-end, we have IF_STMT.

Which is handled in cxx_block_may_fallthru:
    case IF_STMT:
      if (block_may_fallthru (THEN_CLAUSE (stmt)))
        return true;
      return block_may_fallthru (ELSE_CLAUSE (stmt));

-    stmt = else_;
-  else if (integer_nonzerop (cond) && !TREE_SIDE_EFFECTS (else_))
-    stmt = then_;
-  else if (integer_zerop (cond) && !TREE_SIDE_EFFECTS (then_))
-    stmt = else_;

I don't know if this runs counter to again to unreachable code warning
experiment though ...

[Bug c++/103597] [12 Regression] False -Wimplicit-fallthrough= involving macro since r12-5638-ga3e75c1491cd2d50

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org,
                   |                            |mpolacek at gcc dot gnu.org

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
-Wimplicit-fallthrough is performed on the freshly gimplified IL, so
cxx_block_may_fallthru is irrelevant to it.
And so it sees
  switch (n) <default: <D.1987>, case 0: <D.1980>, case 1: <D.1981>>
  <D.1980>:
  if (1 != 0) goto <D.1984>; else goto <D.1985>;
  <D.1984>:
  D.1986 = 0;
  // predicted unlikely by early return (on trees) predictor.
  return D.1986;
  <D.1985>:
  <D.1981>:
  D.1986 = 1;
  return D.1986;
  <D.1987>:
  D.1986 = 2;
  return D.1986;
where it sees that <D.1985>: falls through to <D.1981>: aka case 1:, and
because this is done before cfg is created, we can't cheaply go back and see
that D.1985 is only reachable from else of if (1).

What we could if we have a spare bit on LABEL_DECL or GIMPLE_LABEL is do
roughly what the C++ FE did previously in gimplify_cond_expr, but not through
actually not emitting that cond, but by setting a flag on the LABEL_DECL or
GIMPLE_LABEL that for the purposes of -Wimplicit-fallthrough warning we would
consider unreachable and set it on labels for COND_EXPR with integer_nonzerop
condition if !TREE_OPERANDS (expr, 2) || !TREE_SIDE_EFFECTS (TREE_OPERANDS
(expr, 2))
on the label_false label and if integer_zerop condition and
if !TREE_OPERANDS (expr, 1) || !TREE_SIDE_EFFECTS (TREE_OPERANDS (expr, 1))
on the label_true label.  Perhaps only do that for the freshly created
artificial labels though?
After gimplification of the COND_EXPR this is harder because we could have if
(1) { whatever; } else { ...; lab: ... } and goto lab;
from somewhere etc.
Unfortunately I'm not familiar enough with the -Wimplicit-fallthrough code, so
I can help with setting a flag in gimplify_cond_expr for labels that should be
ignored (i.e. we should act as if they weren't there, so when walking back look
at what is before them etc.).  Marek, could you please have a look?

[Bug c++/103597] [12 Regression] False -Wimplicit-fallthrough= involving macro since r12-5638-ga3e75c1491cd2d50

[jakub at gcc dot gnu.org]

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

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Completely untested patch to set the flag when a label is only known reachable
through fallthrough from previous instructions, never jumped to, as the only
goto to it is on a non-executable branch of GIMPLE_COND with constant
condition.
--- gcc/tree.h.jj       2021-11-29 14:24:14.121633747 +0100
+++ gcc/tree.h  2021-12-07 19:26:06.645324657 +0100
@@ -787,6 +787,12 @@ extern void omp_clause_range_check_faile
 #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.
--- gcc/gimplify.c.jj   2021-12-02 19:41:52.620552908 +0100
+++ gcc/gimplify.c      2021-12-07 19:49:43.964134745 +0100
@@ -4387,9 +4387,19 @@ gimplify_cond_expr (tree *expr_p, gimple
       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
@@ -4403,6 +4413,16 @@ gimplify_cond_expr (tree *expr_p, gimple
              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
@@ -4419,6 +4439,13 @@ gimplify_cond_expr (tree *expr_p, gimple
     }
   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);
     }

[Bug c++/103597] [12 Regression] False -Wimplicit-fallthrough= involving macro since r12-5638-ga3e75c1491cd2d50

[mpolacek at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |mpolacek at gcc dot gnu.org
             Status|NEW                         |ASSIGNED

[Bug debug/103597] [12 Regression] False -Wimplicit-fallthrough= involving macro since r12-5638-ga3e75c1491cd2d50

[jason at gcc dot gnu.org]

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

Jason Merrill <jason at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|c++                         |debug

--- Comment #6 from Jason Merrill <jason at gcc dot gnu.org> ---
I'm marking this as middle-end; as mentioned in the commit message, I removed
the folding from the front-end because it was interfering with a middle-end
experiment richi was doing.  I'm also fine with reverting the change.

[Bug middle-end/103597] [12 Regression] False -Wimplicit-fallthrough= involving macro since r12-5638-ga3e75c1491cd2d50

[mpolacek at gcc dot gnu.org]

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

--- Comment #7 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
(In reply to Jason Merrill from comment #6)
> I'm marking this as middle-end; as mentioned in the commit message, I
> removed the folding from the front-end because it was interfering with a
> middle-end experiment richi was doing.  I'm also fine with reverting the
> change.

No worries, I have a middle-end patch now.

[Bug middle-end/103597] [12 Regression] False -Wimplicit-fallthrough= involving macro since r12-5638-ga3e75c1491cd2d50

[cvs-commit at gcc dot gnu.org]

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

--- Comment #8 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The trunk branch has been updated by Marek Polacek <mpolacek@gcc.gnu.org>:

https://gcc.gnu.org/g:d886a5248e66ab911391af18bf955beb87ee8461

commit r12-7899-gd886a5248e66ab911391af18bf955beb87ee8461
Author: Marek Polacek <polacek@redhat.com>
Date:   Mon Mar 28 18:19:20 2022 -0400

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

    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.

[Bug middle-end/103597] [12 Regression] False -Wimplicit-fallthrough= involving macro since r12-5638-ga3e75c1491cd2d50

[mpolacek at gcc dot gnu.org]

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

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

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

--- Comment #9 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
Fixed.