1236321 - Annotate intentional switch fallthroughs to suppress -Wimplicit-fallthrough warnings in js/

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Chris Peterson [:cpeterson]]

Attachment: js_MOZ_FALLTHROUGH.patch

clang's -Wimplicit-fallthrough warnings (not yet enabled in mozilla-central) warn about switch cases that fall through without a break or return statement. MOZ_FALLTHROUGH (bug 1215411) is an annotation to suppress -Wimplicit-fallthrough warnings about switch cases that intentionally fall through without a break or return statement.  MOZ_FALLTHROUGH is only needed on cases that have code.

MOZ_FALLTHROUGH_ASSERT (bug 1235277) will suppress -Wimplicit-fallthrough warnings about switch cases that MOZ_ASSERT(false) (or its alias MOZ_ASSERT_UNREACHABLE) in debug builds, but intentionally fall through in release builds.

Why do we need MOZ_FALLTHROUGH_ASSERT in addition to MOZ_FALLTHROUGH? In release builds, the MOZ_ASSERT(false) will expand to `do { } while (0)`, requiring a MOZ_FALLTHROUGH annotation to suppress a -Wimplicit-fallthrough warning. 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. The MOZ_FALLTHROUGH_ASSERT macro breaks this warning stalemate.

Comment 1

[Chris Peterson [:cpeterson]]

js/src/asmjs/AsmJS.cpp:2636:11 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/asmjs/AsmJS.cpp:3209:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/builtin/ReflectParse.cpp:2583:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/ctypes/CTypes.cpp:4032:3 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/dtoa.c:1514:3 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/dtoa.c:1518:3 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/dtoa.c:1606:4 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/dtoa.c:2792:3 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/dtoa.c:2800:3 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/frontend/BytecodeEmitter.cpp:2072:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/frontend/BytecodeEmitter.cpp:8335:7: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
js/src/frontend/FoldConstants.cpp:245:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/frontend/NameFunctions.cpp:161:15 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/frontend/NameFunctions.cpp:731:11 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/frontend/Parser.cpp:6023:11 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/frontend/Parser.cpp:6056:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/frontend/Parser.cpp:6326:15 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/frontend/Parser.cpp:9325:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/irregexp/RegExpEngine.cpp:3626:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/irregexp/RegExpParser.cpp:871:15 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/irregexp/RegExpParser.cpp:950:11 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/jit/BacktrackingAllocator.cpp:2493:15 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/jit/BaselineIC.cpp:1906:11 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/jit/IonBuilder.cpp:717:15 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/jit/IonBuilder.cpp:1538:15 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/jit/IonBuilder.cpp:1893:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/jit/JitcodeMap.h:926:11 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/jit/JitcodeMap.h:944:11 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/jit/Lowering.cpp:1750:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/jit/Lowering.cpp:1806:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/jit/MIR.cpp:2184:9 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/jit/MIR.cpp:2264:11 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/jit/MIR.cpp:3042:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/jit/MIR.cpp:3329:11 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/jit/MacroAssembler.cpp:552:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/jit/RangeAnalysis.cpp:265:11 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/jit/RangeAnalysis.cpp:273:11 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/jit/RangeAnalysis.cpp:3009:15 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/jit/shared/CodeGenerator-shared-inl.h:299:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/jsdtoa.cpp:140:13 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/jsgc.cpp:5937:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/jsgc.cpp:5953:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/jsgc.cpp:5999:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/jsgc.cpp:6012:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/jsiter.cpp:456:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/jsiter.cpp:457:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/jsiter.cpp:458:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/jsiter.cpp:459:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/jsiter.cpp:460:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/jsiter.cpp:461:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/jsiter.cpp:462:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/jsstr.cpp:1133:9 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/jsstr.cpp:1134:9 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/jsstr.cpp:1135:9 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/jsstr.cpp:1136:9 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/jsstr.cpp:1137:9 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/jsstr.cpp:1138:9 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/jsstr.cpp:1139:9 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/vm/Runtime.cpp:857:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/src/vm/Runtime.cpp:859:7 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/xpconnect/src/XPCConvert.cpp:497:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
js/xpconnect/src/XPCShellImpl.cpp:966:9 [-Wimplicit-fallthrough] unannotated fall-through between switch labels

Comment 2

[Chris Peterson [:cpeterson]]

Comment on attachment 8703395 [details] [diff] [review]
js_MOZ_FALLTHROUGH.patch

Review of attachment 8703395 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/NameFunctions.cpp
@@ -730,2 @@
>            case PNK_IMPORT_SPEC_LIST: {
> -          case PNK_EXPORT_SPEC_LIST:

btw, clang issues a -Wimplicit-fallthrough warning here, apparently interpreting the left brace in `case PNK_IMPORT_SPEC_LIST: {` as code needing a fallthrough annotation. I moved both case labels outside the brace.

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 8703395 [details] [diff] [review]
js_MOZ_FALLTHROUGH.patch

Review of attachment 8703395 [details] [diff] [review]:
-----------------------------------------------------------------

Great job!

::: js/src/asmjs/WasmStubs.cpp
@@ +262,5 @@
>          MOZ_CRASH("no int64 in asm.js");
>        case ExprType::F32:
>          masm.convertFloat32ToDouble(ReturnFloat32Reg, ReturnDoubleReg);
>          // Fall through as ReturnDoubleReg now contains a Double
> +        MOZ_FALLTHROUGH;

Can you delete the preceding line and append to the MOZ_FALLTHROUGH; line "// because ReturnDoubleReg now contains a Double"?

::: js/xpconnect/src/XPCConvert.cpp
@@ +492,5 @@
>              (**((nsAString**)d)).SetIsVoid(true);
>              return true;
>          }
>          // Fall through to T_DOMSTRING case.
> +        MOZ_FALLTHROUGH;

I bet you can remove the comment now, since it's redundant.

Comment 4

[Chris Peterson [:cpeterson]]

Thanks. I'll update those comments before landing.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f998906d53bc

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f998906d53bc