Bug 77817 - -Wimplicit-fallthrough: cpp directive renders FALLTHRU comment ineffective
Summary: -Wimplicit-fallthrough: cpp directive renders FALLTHRU comment ineffective
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: c (show other bugs)
Version: 7.0
: P3 minor
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: diagnostic
: 77955 79750 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-10-02 14:33 UTC by jim
Modified: 2017-09-29 00:06 UTC (History)
6 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2016-10-04 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description jim 2016-10-02 14:33:38 UTC
Normally, adding a comment like /* FALLTHRU */ works fine to mark a switch case that is intended to fall through. But if a cpp directive appears in the vicinity of that comment, the comment becomes ineffective and we get the warning anyway.

Here is a minimal example (without the "#undef", that comment *does* suppress the warning):

int
foo (int x)
{
  switch (x)
    {
    case 1:
      x = 3;
#undef X
      /* fallthrough */
    case 2:
      x = 4;
    }
  return x;
}
$ gcc -c -O -Wimplicit-fallthrough /t/ft.c
/t/ft.c: In function ‘foo’:
/t/ft.c:20:9: warning: this statement may fall through [-Wimplicit-fallthrough]
       x = 3;
       ~~^~~
/t/ft.c:23:5: note: here
     case 2:
     ^~~~

The above was compiled with yesterday's GCC 7:
  gcc version 7.0.0 20161001 (experimental) (GCC)

I encountered an actual instance of this in gnulib's vasnprintf.c: http://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/vasnprintf.c#n4823
Comment 1 Marek Polacek 2016-10-03 08:38:37 UTC
Interesting, I can't reproduce:

$ ./cc1 -quiet -Wimplicit-fallthrough ft.c
$
Comment 2 jim 2016-10-03 15:16:18 UTC
Oops. That must have been the "worked-around" version.
Swap the #undef and fallthrough comment to repro:

int
foo (int x)
{
  switch (x)
    {
    case 1:
      x = 3;
      /* fallthrough */
#undef X
    case 2:
      x = 4;
    }
  return x;
}
--------------------
$ /p/p/gcc-2016-10-02.12h48/libexec/gcc/x86_64-pc-linux-gnu/7.0.0/cc1 -Wimplicit-fallthrough /t/ft.c
 foo
Analyzing compilation unit

/t/ft.c: In function ‘foo’:
/t/ft.c:20:9: warning: this statement may fall through [-Wimplicit-fallthroug ]
       x = 3;
       ~~^~~
/t/ft.c:23:5: note: here
     case 2:
     ^~~~
Comment 3 Alan Modra 2016-10-04 23:27:54 UTC
Confirmed.  See also pr77853
Comment 4 Jakub Jelinek 2016-10-05 07:54:31 UTC
While the comment after /* FALLTHRU */ comment is easily solvable, this one is I'm afraid going to be too difficult, there are many tokens in between and it is hard to propagate the info across the preprocessing directives.  Just use attributes?
Comment 5 Marek Polacek 2016-10-05 10:35:24 UTC
Yeah, I think this one's WONTFIX.
Comment 6 Jakub Jelinek 2016-10-05 10:38:56 UTC
(In reply to Marek Polacek from comment #5)
> Yeah, I think this one's WONTFIX.

Well, in theory we could for the comments that satisfy fallthrough_comment_p create CPP_COMMENT token and insert it, even without -C or -CC (perhaps only if -Wimplicit-fallthrough).  But, when without those options, we'd have to cope with the behavior changes where it makes a difference.  E.g. I believe
/* FALLTHROUGH */ #ifdef SOMETHING
etc. would change behavior.
As a workaround, with the patches I've posted -Wimplicit-fallthrough -C (or -CC) would work in this case.
Comment 7 jim 2016-10-05 15:26:40 UTC
Thanks for investigating.
I already pushed a workaround for gnulib's vasnprintf problem (http://git.savannah.gnu.org/cgit/gnulib.git/commit/?id=e63f5eb55570a1ea3e51ce47df33689785e085c1).
I may just leave that there.

The alternative of using an empty "__attribute__((__fallthrough__));" statement would require a macro expanding to that or to nothing.
Comment 8 Marc Mutz 2016-10-09 07:51:39 UTC
I see the same in conditional code. Example fix that I applied to Qt locally, but won't push, because the next compiler will warn that the *comment is bogus*: 

       case QVariant::Bool:
           return qulonglong(d->data.b);
   #ifndef QT_BOOTSTRAPPED
       case QMetaType::QJsonValue:
           if (!v_cast<QJsonValue>(d)->isDouble())
               break;
  -        // fall through
   #endif
  +        // fall through
       case QVariant::Double:

We have about a dozen of those in QtBase alone, and while Qt 5.8 has a macro for the attribute, the 5.6 LTS version does not, and still has >2 years of support ahead of it, so a comment solution would be strongly preferred, and it would *really* help if this was fixed before this version of the compiler went into production.
Comment 9 Marek Polacek 2016-10-12 16:17:40 UTC
*** Bug 77955 has been marked as a duplicate of this bug. ***
Comment 10 Markus Trippelsdorf 2016-10-12 16:21:39 UTC
The testcase from PR77955:

markus@x4 /tmp % cat fall.c
void bar(int);

void foo(int i) {
  switch (i) {
  case 1: {
    bar(1);
    // fall-through
  }
  case 2:
    bar(2);
  default:
    break;
  }
}
markus@x4 /tmp % gcc -Wimplicit-fallthrough=1 -c fall.c
fall.c: In function ‘foo’:
fall.c:6:5: warning: this statement may fall through [-Wimplicit-fallthrough=]
     bar(1);
     ^~~~~~
fall.c:9:3: note: here
   case 2:
   ^~~~

Two issues; the first is the bogus warning, the second that we don't
warn for the case 2 fall-through.
Comment 11 Jakub Jelinek 2016-10-12 16:28:26 UTC
(In reply to Markus Trippelsdorf from comment #10)
> The testcase from PR77955:
> 
> markus@x4 /tmp % cat fall.c
> void bar(int);
> 
> void foo(int i) {
>   switch (i) {
>   case 1: {
>     bar(1);
>     // fall-through
>   }
>   case 2:

Well, this one is even much harder than the preprocessor issue, it isn't solvable by preserving CPP_COMMENT tokens and ignoring them, there are tokens in between the fallthru comment and case keyword even after preprocessing here.
I'm afraid this is a clear WONTFIX, move the comment or better use __attribute__((fallthrough))/[[fallthrough]] instead.

>     bar(2);
>   default:
>     break;

Not warning here is completely intentional, it is a fallthru into break;
Comment 12 Marc Mutz 2016-10-12 21:06:25 UTC
Is replacing a matching comment with __attribute__(fallthrough)) so complicated as to make this a wontfix?
Comment 13 Jakub Jelinek 2016-10-12 21:10:53 UTC
(In reply to Marc Mutz from comment #12)
> Is replacing a matching comment with __attribute__(fallthrough)) so
> complicated as to make this a wontfix?

It is not really possible.
__attribute__((fallthrough)) has precise rules on where it can appear, while /* FALLTHRU */ comments, being comments, can appear anywhere.  Especially with -Wimplicit-fallthrough=1 when all comments are considered fallthru comments...
Comment 14 Eric Gallager 2017-09-28 23:57:27 UTC
*** Bug 79750 has been marked as a duplicate of this bug. ***
Comment 15 Marc Mutz 2017-09-29 00:06:31 UTC
(In reply to Jakub Jelinek from comment #13)
> (In reply to Marc Mutz from comment #12)
> > Is replacing a matching comment with __attribute__(fallthrough)) so
> > complicated as to make this a wontfix?
> 
> It is not really possible.
> __attribute__((fallthrough)) has precise rules on where it can appear, while
> /* FALLTHRU */ comments, being comments, can appear anywhere.  Especially
> with -Wimplicit-fallthrough=1 when all comments are considered fallthru
> comments...

I don't buy this. You don't need to use the 'official' attribute. You can invent an internal one, say __attribute__((__replaced_fallthrough_comment)), which does not have the limitations of [[fallthrough]]. It's just a way to preserve a comment over the preprocessor stage.