1355692 - Add MOZ_FALLTHROUGH macro definition for gcc 7 to suppress -Wimplicit-fallthrough warnings

Status: RESOLVED FIXED

(Core :: MFBT, enhancement)

Description

[Chris Peterson [:cpeterson]]

gcc 7 added support for clang's -Wimplicit-fallthrough warning:

https://gcc.gnu.org/gcc-7/changes.html

Bug 1215411 defined the MOZ_FALLTHROUGH annotation macro as `[[clang::fallthrough]]` for clang. We should add a gcc-compatible definition like `[[gnu::fallthrough]]` for gcc >= 7.

Comment 1

[:Cykesiopka]

Attachment: Bug 1355692 - Add MOZ_FALLTHROUGH macro definition for gcc 7 to suppress -Wimplicit-fallthrough warnings.

The generic fallback MOZ_FALLTHROUGH definition is insufficient for GCC 7 and
above, resulting in --enable-warnings-as-errors builds failing.

Review commit: https://reviewboard.mozilla.org/r/157776/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/157776/

Comment 2

[:Cykesiopka]

Attachment: Bug 1355692 - Add MOZ_FALLTHROUGH macro definition for gcc 7 to suppress -Wimplicit-fallthrough warnings.

The generic fallback MOZ_FALLTHROUGH definition is insufficient for GCC 7 and
above, resulting in --enable-warnings-as-errors builds failing.

Review commit: https://reviewboard.mozilla.org/r/157776/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/157776/

Comment 3

[:Cykesiopka]

Comment on attachment 8887036 [details]
Bug 1355692 - Add MOZ_FALLTHROUGH macro definition for gcc 7 to suppress -Wimplicit-fallthrough warnings.

(MozReview and/or Bugzilla ran into some DB issue and created two review requests somehow.)

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 8887036 [details]
Bug 1355692 - Add MOZ_FALLTHROUGH macro definition for gcc 7 to suppress -Wimplicit-fallthrough warnings.

https://reviewboard.mozilla.org/r/157776/#review163238

::: mfbt/Attributes.h:363
(Diff revision 1)
>   *     MOZ_FALLTHROUGH_ASSERT("Unexpected foo value?!");
>   *   case 5:
>   *     return 5;
>   * }
>   */
>  #if defined(__clang__) && __cplusplus >= 201103L

while here, would you mind changing that to use __has_cpp_attribute(clang:fallthrough)?

::: mfbt/Attributes.h:373
(Diff revision 1)
> +#elif defined(__GNUC__) && __GNUC__ >= 7 && __cplusplus > 201402L
> +   /* GCC 7 and above in C++17 mode support standard fallthrough
> +    * annotations. */
> +#  define MOZ_FALLTHROUGH [[fallthrough]]

Let's leave this out for the moment. Eventually, we'll justs be able to add it under #if __cplusplus > 2017something (and that would go first)

Comment 5

[:Cykesiopka]

Comment on attachment 8887036 [details]
Bug 1355692 - Add MOZ_FALLTHROUGH macro definition for gcc 7 to suppress -Wimplicit-fallthrough warnings.

https://reviewboard.mozilla.org/r/157776/#review163238

> while here, would you mind changing that to use __has_cpp_attribute(clang:fallthrough)?

That would indeed be nicer, but it looks like the GCC we use in CI doesn't support `__has_cpp_attribute` and therefore barfs once it sees the macro in the same line as the `#if defined(__clang__)` check.

We'd have to choose from:
 1. Move the `__has_cpp_attribute` check inside the `#if defined(__clang__)` body and duplicate a bit of logic.
 2. Define `__has_cpp_attribute` as 0 if it's not already defined.
 3. Keep using the existing check.

Thoughts?

Comment 6

[Mike Hommey [:glandium]]

2 sounds reasonable.

Comment 7

[:Cykesiopka]

Comment on attachment 8887036 [details]
Bug 1355692 - Add MOZ_FALLTHROUGH macro definition for gcc 7 to suppress -Wimplicit-fallthrough warnings.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157776/diff/1-2/

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 8887036 [details]
Bug 1355692 - Add MOZ_FALLTHROUGH macro definition for gcc 7 to suppress -Wimplicit-fallthrough warnings.

https://reviewboard.mozilla.org/r/157776/#review165376

::: mfbt/Attributes.h:365
(Diff revision 2)
> + * The __has_cpp_attribute macro is not directly usable because:
> + *   1. When this header is included as part of a C file, Clang does not define
> + *      the macro. Also, GCC will treat passing a scoped attribute to the macro
> + *      as a syntax error.
> + *   2. We still support versions of GCC where the macro isn't supported. If
> + *      use of the macro isn't wrapped by a compiler check, GCC will consider
> + *      the use of the macro a syntax error.

it sounds like:
#ifndef __has_cpp_attribute
#define __has_cpp_attribute(x) 0
#endif

would work just as much, and would be simpler.

And then you don't even need to check __clang__ and __GNUC__.

Comment 10

[:Cykesiopka]

Comment on attachment 8887036 [details]
Bug 1355692 - Add MOZ_FALLTHROUGH macro definition for gcc 7 to suppress -Wimplicit-fallthrough warnings.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157776/diff/2-3/

Comment 11

[Mike Hommey [:glandium]]

Comment on attachment 8887036 [details]
Bug 1355692 - Add MOZ_FALLTHROUGH macro definition for gcc 7 to suppress -Wimplicit-fallthrough warnings.

https://reviewboard.mozilla.org/r/157776/#review169476

Comment 12

[:Cykesiopka]

Thanks for the review.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=857a72eef992a76ad004cffbf2f21cf240ccb74f
(Only red is from Buildbot builds, which AIUI can be ignored.)

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3403a367f1a7
Add MOZ_FALLTHROUGH macro definition for gcc 7 to suppress -Wimplicit-fallthrough warnings. r=glandium

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/3403a367f1a7