Closed
Bug 1389851
(Wunreachable-code-return)
Opened 7 years ago
Closed 7 years ago
Fix and enable clang's -Wunreachable-code-return warnings
Categories
(Firefox Build System :: General, enhancement)
Tracking
(firefox-esr52 wontfix, firefox55 wontfix, firefox56 wontfix, firefox57 fixed)
RESOLVED
FIXED
mozilla57
People
(Reporter: cpeterson, Assigned: cpeterson)
References
(Blocks 1 open bug)
Details
Attachments
(5 files)
|
59 bytes,
text/x-review-board-request
|
jld
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
karlt
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
jonco
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
glandium
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
decoder
:
review+
|
Details |
clang's -Wunreachable-code-return warns about unreachable return statements.
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
||
Comment 5•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8896653 [details] Bug 1389851 - netwerk: Fix -Wunreachable-code-return warning in nsGIOProtocolHandler.cpp. https://reviewboard.mozilla.org/r/167940/#review173216
Attachment #8896653 -
Flags: review?(karlt) → review+
|
||
Comment 6•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8896654 [details] Bug 1389851 - js: Fix -Wunreachable-code-return warning in StoreBuffer.cpp. https://reviewboard.mozilla.org/r/167942/#review173334 Thanks.
Attachment #8896654 -
Flags: review?(jcoppeard) → review+
|
||
Comment 7•7 years ago
|
||
| mozreview-review | ||
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.
Attachment #8896652 -
Flags: review?(jld) → review+
|
Assignee | |
Comment 8•7 years ago
|
||
(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 hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
||
Comment 14•7 years ago
|
||
| mozreview-review | ||
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.
Attachment #8897768 -
Flags: review?(choller) → review+
|
|
Assignee | |
Comment 15•7 years ago
|
||
(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•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8896655 [details] Bug 1389851 - build: Enable clang's -Wunreachable-code-return warnings. https://reviewboard.mozilla.org/r/167944/#review175244
Attachment #8896655 -
Flags: review?(mh+mozilla) → review+
|
||
Comment 17•7 years ago
|
||
Pushed by cpeterson@mozilla.com: 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•7 years ago
|
||
| bugherder | ||
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
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
|
||
Comment 19•7 years ago
|
||
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
Flags: needinfo?(cpeterson)
|
|
Assignee | |
Comment 20•7 years ago
|
||
(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.
Flags: needinfo?(cpeterson)
|
||
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•