Bug 1389851 (Wunreachable-code-return)

Fix and enable clang's -Wunreachable-code-return warnings

RESOLVED FIXED in Firefox 57

Status

RESOLVED FIXED
a year ago
7 months ago

People

(Reporter: cpeterson, Assigned: cpeterson)

Tracking

(Blocks: 1 bug)

51 Branch
mozilla57
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox55 wontfix, firefox56 wontfix, firefox57 fixed)

Details

Attachments

(5 attachments)

(Assignee)

Description

a year ago
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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

Updated

a year ago
Depends on: 1391993
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

a year 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)
(Assignee)

Updated

a year ago
Depends on: 1392512

Updated

7 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.