1253170 - Enable clang's -Wimplicit-fallthrough warning to catch unintentional switch fallthroughs

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Chris Peterson [:cpeterson]]

Attachment: enable-Wimplicit-fallthrough.patch

clang's -Wimplicit-fallthrough warning reports switch cases that fall through without a break or return statement. MOZ_FALLTHROUGH (bug 1215411) is an annotation to suppress -Wimplicit-fallthrough warnings where the fallthrough is intentional. Before this patch lands, I can send an email about using MOZ_FALLTHROUGH to dev-platform.

I have fixed all -Wimplicit-fallthrough warnings in mozilla-central. I found 6 real bugs. :-) Today we have 286 intentional fallthroughs with MOZ_FALLTHROUGH annotations.

MOZ_FALLTHROUGH is only needed on cases that have code:

  switch (foo) {
    case 1: // These cases have no code. No fallthrough annotations are needed.
    case 2:
    case 3:
      foo = 4; // This case has code, so a fallthrough annotation is needed:
      MOZ_FALLTHROUGH;
    default:
      return foo;
  }

For clang, MOZ_FALLTHROUGH is #defined as `[[clang::fallthrough]]`, which is only available in C++11 code. For MSVC, MOZ_FALLTHROUGH is #defined as `__fallthrough`, an annotation checked by MSVC /analyze ("Code Analysis"), though I have not tested it. gcc does not have a similar warning or annotation, so MOZ_FALLTHROUGH is a no-op.

Unfortunately, a second annotation macro, MOZ_FALLTHROUGH_ASSERT, is needed for switch cases that previously called MOZ_ASSERT(false) (or its alias MOZ_ASSERT_UNREACHABLE) to crash in debug builds, but intentionally fall through in release builds. The problem is that, in release builds, MOZ_ASSERT(false) expands to `do { } while (0)`, requiring a MOZ_FALLTHROUGH annotation to suppress a -Wimplicit-fallthrough warning because the case has code. But in debug builds, the MOZ_ASSERT(false) will expand to something like `if (true) { MOZ_CRASH(); }` and the MOZ_FALLTHROUGH annotation will cause a -Wunreachable-code warning because the program dies before the fallthrough is reached. The MOZ_FALLTHROUGH_ASSERT macro breaks this warning stalemate.

BROKEN EXAMPLE WITHOUT MOZ_FALLTHROUGH_ASSERT:

  switch (foo) {
    default:
      // This case wants to assert in debug builds, fall through in release.
      MOZ_ASSERT(false); // -Wimplicit-fallthrough warning in release builds!
      MOZ_FALLTHROUGH;   // but -Wunreachable-code warning in debug builds!
    case 5:
      return 5;
  }

WORKING EXAMPLE WITH MOZ_FALLTHROUGH_ASSERT:

  switch (foo) {
    default:
      // This case asserts in debug builds, falls through in release.
      MOZ_FALLTHROUGH_ASSERT("Unexpected foo value?!");
    case 5:
      return 5;
  }

Comment 1

[Chris Peterson [:cpeterson]]

Daniel, if you have a chance, can you please test this patch with clang on Linux? AFAIK, the try servers only use clang on OS X.

Comment 2

[Daniel Holbert [:dholbert]]

I can almost build successfully on linux, with clang 3.8, with this patch applied & --enable-warnings-as-errors.  Bug 1253194 (just filed) is the only problem that I run into.

If I hack around that bug locally, then ./mach build finishes successfully.

Comment 3

[Chris Peterson [:cpeterson]]

Thanks, Mike. I am waiting for Daniel (thanks!) to fix the Linux warning in bug 1253194 before I finally enable the warning.

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4adaa4b1ddb2

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4adaa4b1ddb2