1389851 - (Wunreachable-code-return) Fix and enable clang's -Wunreachable-code-return warnings

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Chris Peterson [:cpeterson]]

clang's -Wunreachable-code-return warns about unreachable return statements.

Comment 1

[Chris Peterson [:cpeterson]]

Attachment: Bug 1389851 - sandbox: Suppress -Wunreachable-code-return warning in third-party Chromium sandbox code.

security/sandbox/chromium/sandbox/linux/seccomp-bpf/trap.cc:277:12: warning: 'return' will never be executed [-Wunreachable-code-return]

This return statement is unreachable because SANDBOX_DIE() is a noreturn function:

https://searchfox.org/mozilla-central/rev/a887f0edbd9f6b176b64111455ba62bb0cf356a6/security/sandbox/chromium/sandbox/linux/seccomp-bpf/trap.cc#270-274

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

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: Bug 1389851 - netwerk: Fix -Wunreachable-code-return warning in nsGIOProtocolHandler.cpp.

netwerk/protocol/gio/nsGIOProtocolHandler.cpp:89:10: warning: 'return' will never be executed [-Wunreachable-code-return]

This `return NS_ERROR_FAILURE` statement at the end of this function is unreachable because all of the preceding switch statement's cases return.

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

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: Bug 1389851 - js: Fix -Wunreachable-code-return warning in StoreBuffer.cpp.

js/src/gc/StoreBuffer.cpp:138:8: warning: 'return' will never be executed [-Wunreachable-code-return]

This `return nullptr` statement is unreachable because oomUnsafe.crash() is a noreturn function.

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

Comment 4

[Chris Peterson [:cpeterson]]

Attachment: Bug 1389851 - build: Enable clang's -Wunreachable-code-return warnings.

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

Comment 5

[Karl Tomlinson (:karlt)]

Comment on attachment 8896653 [details]
Bug 1389851 - netwerk: Fix -Wunreachable-code-return warning in nsGIOProtocolHandler.cpp.

https://reviewboard.mozilla.org/r/167940/#review173216

Comment 6

[Jon Coppeard (:jonco)]

Comment on attachment 8896654 [details]
Bug 1389851 - js: Fix -Wunreachable-code-return warning in StoreBuffer.cpp.

https://reviewboard.mozilla.org/r/167942/#review173334

Thanks.

Comment 7

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Comment on attachment 8896652 [details]
Bug 1389851 - sandbox: Suppress -Wunreachable-code-return warning in third-party Chromium sandbox code.

https://reviewboard.mozilla.org/r/167938/#review173514

::: security/sandbox/linux/moz.build:85
(Diff revision 1)
>  # This copy of SafeSPrintf doesn't need to avoid the Chromium logging
>  # dependency like the one in libxul does, but this way the behavior is
>  # consistent.  See also the comment in SandboxLogging.h.
>  SOURCES['../chromium/base/strings/safe_sprintf.cc'].flags += ['-DNDEBUG']
>  
>  # Keep clang from warning about intentional 'switch' fallthrough in icu_utf.cc:

Nit: this comment should move to the line adjusting ICU (or be made more general).

::: security/sandbox/linux/moz.build:87
(Diff revision 1)
>  # consistent.  See also the comment in SandboxLogging.h.
>  SOURCES['../chromium/base/strings/safe_sprintf.cc'].flags += ['-DNDEBUG']
>  
>  # Keep clang from warning about intentional 'switch' fallthrough in icu_utf.cc:
>  if CONFIG['CLANG_CXX']:
> +    CXXFLAGS += ['-Wno-unreachable-code-return']

Can this be restricted to the specific source file, like the next line does for `icu_utf.cc`?  There might be some value in having this warning for the non-imported code in this directory.

Comment 8

[Chris Peterson [:cpeterson]]

(In reply to Jed Davis [:jld] (⏰UTC-6) from comment #7)
> >  # Keep clang from warning about intentional 'switch' fallthrough in icu_utf.cc:
> 
> Nit: this comment should move to the line adjusting ICU (or be made more
> general).

OK.

> ::: security/sandbox/linux/moz.build:87
> Can this be restricted to the specific source file, like the next line does
> for `icu_utf.cc`?  There might be some value in having this warning for the
> non-imported code in this directory.

Sure. I'll restrict the warning to just that one Chromium file.

Comment 9

[Chris Peterson [:cpeterson]]

Attachment: Bug 1389851 - libfuzzer: Suppress -Wunreachable-code-return warnings in FuzzerDriver.cpp.

Suppress these warnings intead of fixing them because libfuzzer is a third-party llvm library.

tools/fuzzing/libfuzzer/FuzzerDriver.cpp:450:10: warning: 'return' will never be executed [-Wunreachable-code-return]
tools/fuzzing/libfuzzer/FuzzerDriver.cpp:663:12: warning: 'return' will never be executed [-Wunreachable-code-return]

And while I'm poking around in this moz.build, also build libfuzzer as UNIFIED_SOURCES.

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

Comment 10

[Chris Peterson [:cpeterson]]

Comment on attachment 8896652 [details]
Bug 1389851 - sandbox: Suppress -Wunreachable-code-return warning in third-party Chromium sandbox code.

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

Comment 11

[Chris Peterson [:cpeterson]]

Comment on attachment 8896653 [details]
Bug 1389851 - netwerk: Fix -Wunreachable-code-return warning in nsGIOProtocolHandler.cpp.

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

Comment 12

[Chris Peterson [:cpeterson]]

Comment on attachment 8896654 [details]
Bug 1389851 - js: Fix -Wunreachable-code-return warning in StoreBuffer.cpp.

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

Comment 13

[Chris Peterson [:cpeterson]]

Comment on attachment 8896655 [details]
Bug 1389851 - build: Enable clang's -Wunreachable-code-return warnings.

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

Comment 14

[Christian Holler (:decoder)]

Comment on attachment 8897768 [details]
Bug 1389851 - libfuzzer: Suppress -Wunreachable-code-return warnings in FuzzerDriver.cpp.

https://reviewboard.mozilla.org/r/169072/#review174426

I'm fine with the warning but uncertain about the UNIFIED change. I see a certain risk associated with building this unified because it was not meant to be built that way and has some weak symbols magic. Not sure if this is of any relevance, so I'm willing to try it for now.

Comment 15

[Chris Peterson [:cpeterson]]

(In reply to Christian Holler (:decoder) from comment #14)
> I'm fine with the warning but uncertain about the UNIFIED change. I see a
> certain risk associated with building this unified because it was not meant
> to be built that way and has some weak symbols magic. Not sure if this is of
> any relevance, so I'm willing to try it for now.

I will revert the UNIFIED_SOURCES change. Adding unknown risk to our fuzzing automation outweighs any hypothetical improvements of a few milliseconds in build times. :)

Comment 16

[Mike Hommey [:glandium]]

Comment on attachment 8896655 [details]
Bug 1389851 - build: Enable clang's -Wunreachable-code-return warnings.

https://reviewboard.mozilla.org/r/167944/#review175244

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a484948fb8e
sandbox: Suppress -Wunreachable-code-return warning in third-party Chromium sandbox code. r=jld
https://hg.mozilla.org/integration/mozilla-inbound/rev/74d4d1d6155d
netwerk: Fix -Wunreachable-code-return warning in nsGIOProtocolHandler.cpp. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea883b4c00cd
libfuzzer: Suppress -Wunreachable-code-return warnings in FuzzerDriver.cpp. r=decoder
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d3165f35b0c
js: Fix -Wunreachable-code-return warning in StoreBuffer.cpp. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b998ade630a
build: Enable clang's -Wunreachable-code-return warnings. r=glandium

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3a484948fb8e
https://hg.mozilla.org/mozilla-central/rev/74d4d1d6155d
https://hg.mozilla.org/mozilla-central/rev/ea883b4c00cd
https://hg.mozilla.org/mozilla-central/rev/4d3165f35b0c
https://hg.mozilla.org/mozilla-central/rev/8b998ade630a

Comment 19

[Ionuț Goldan [:igoldan]]

These alerts started to show up. Chris, am I correct to say these were expected or at least this bug isn't responsible for fixing them?

== Change summary for alert #8888 (as of August 19 2017 22:35 UTC) ==

Regressions:

  6%  compiler warnings summary linux64 asan asan debug     326.00 -> 344.00
  5%  compiler warnings summary linux64 debug static-analysis328.00 -> 346.00
  5%  compiler warnings summary linux64 asan asan opt       295.00 -> 310.00
  5%  compiler warnings summary linux64 asan asan-fuzzing   297.00 -> 312.00
  5%  compiler warnings summary linux64 opt static-analysis 297.00 -> 312.00
  2%  compiler warnings summary osx-cross debug             330.00 -> 338.00
  2%  compiler warnings summary osx-cross-noopt debug       330.00 -> 338.00
  2%  compiler warnings summary osx-cross opt               296.00 -> 301.00

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=8888

Comment 20

[Chris Peterson [:cpeterson]]

(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #19)
> These alerts started to show up. Chris, am I correct to say these were
> expected or at least this bug isn't responsible for fixing them?

A few new warnings are expected. They are from third-party code. I can submit a patch to suppress these warnings.