Closed Bug 1253170 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: cpeterson, Assigned: cpeterson)

References

Details

Attachments

(1 file)

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;
  }
Attachment #8726095 - Flags: review?(mh+mozilla)
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.
Flags: needinfo?(dholbert)
Depends on: 1253194
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.
Flags: needinfo?(dholbert)
Attachment #8726095 - Flags: review?(mh+mozilla) → review+
Thanks, Mike. I am waiting for Daniel (thanks!) to fix the Linux warning in bug 1253194 before I finally enable the warning.
Depends on: 1253753
https://hg.mozilla.org/mozilla-central/rev/4adaa4b1ddb2
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Product: Core → Firefox Build System
Duplicate of this bug: 1288036
You need to log in before you can comment on or make changes to this bug.