New issue
Advanced search Search tips
Starred by 11 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug


Sign in to add a comment
link

Issue 346399: Enable unreachable code warning

Reported by pkasting@chromium.org, Feb 24 2014 Project Member

Issue description

At least on MSVC, the "unreachable code warning" is 4702 and is enabled by default, but we disable it.

We should enable this warning after we fix the places that violate it.  For an added bonus, turn on any such warnings in gcc/clang (-Wunreachable-code?).  Note that according to external people, for these compilers, -Wall won't turn the warning on by default.
 

Comment 1 by scottmg@chromium.org, Feb 24 2014

Full build logs here: https://codereview.chromium.org/176973004

As those disappear eventually, attached are just the warning lines from those runs.
release-unreachable.txt
39.3 KB View Download
debug-unreachable.txt
34.0 KB View Download

Comment 2 by jia...@opera.com, Feb 25 2014

Blockedon: chromium:346681

Comment 3 by jia...@opera.com, Feb 25 2014

Blockedon: chromium:346697

Comment 4 by thakis@chromium.org, Mar 3 2014

(One of the reasons we don't build with -Wunreachable on clang is because it makes local development harder – if you make some code unreachable locally your file won't compile because we build with warnings-as-errors. And having some amounts of code unreachable during development is fairly common.)

Comment 5 by thakis@chromium.org, Mar 3 2014

(Also, I think turning on -Wunreachable-code requires building CFGs all over the place, which IIRC slows down compiles considerably.)

Comment 6 by pkasting@chromium.org, Mar 4 2014

I'm not terribly sympathetic to comment 4 -- use #if 0 or comment blocks -- but I am sympathetic to comment 5.  If we measured and saw this was a nontrivial speed hit, I'd suggest turning it on only for some subset of builds.

Comment 7 by pkasting@chromium.org, Mar 15 2014

Cc: thakis@chromium.org
Oof, I just hit a big problem while trying to turn this warning on locally.

When building the release mode non-component build (I don't know if this repros in other configurations, but I wasn't seeing it through most of a successful release-mode component build without [p]nacl), I got the following warning building simple_command_line_parser.cc (but I assume it would happen many other places too):

1>c:\program files (x86)\microsoft visual studio 12.0\vc\include\xtree(1826): error C2220: warning treated as error - no 'object' file generated
1>c:\program files (x86)\microsoft visual studio 12.0\vc\include\xtree(1826): warning C4702: unreachable code

This in turn led me here:

http://connect.microsoft.com/VisualStudio/feedback/details/809962/has-exceptions-0-triggers-unreachable-code-warning-in-xtree

This basically says that _HAS_EXCEPTIONS=0 is something Microsoft doesn't want to exist and won't support.  Other posts elsewhere from Dinkumware basically say "ignore the warning", which is... hard.

The only way around this I can see is if we can somehow disable exceptions without using _HAS_EXCEPTIONS=0.  I don't know if such a way exists.  Or maybe we can find that this doesn't happen in some configurations, like Debug or component build or no [p]nacl, and just enable warning 4702 there.

Scott or Nico, thoughts?

(Incidentally, while fixing the warnings elsewhere in the codebase, I've caught a number of real bugs, so I'm convinced there's value in trying to turn this warning on at least sometimes.)

Comment 8 by pkasting@chromium.org, Mar 15 2014

Oops, I missed a negative in the post on the MSDN page.  I guess Microsoft doesn't hate this mode as much as I thought -- but they still don't support it.

Comment 9 by thakis@chromium.org, Mar 15 2014

Non-msvc compilers can disable warnings in system headers (cl.exe can't). So if it doesn't slow down builds too much, maybe turn this on for clang instead?

Comment 10 by pkasting@chromium.org, Mar 15 2014

Well, for clang, presumably the libraries don't have this issue -- unless you're talking about a Windows clang build with the MSVC libraries.  Is such a thing in the cards?

Sadly, if this becomes a non-Windows-only thing, I won't be able to do the work (though I will submit my fixes for the existing issues I've uncovered first).

Comment 11 by thakis@chromium.org, Mar 15 2014

Yes, here: http://build.chromium.org/p/chromium.fyi/builders/Chromium%20Win%20Clang But it'll be a while until that's off FYI.

Comment 12 by pkasting@chromium.org, Mar 15 2014

Are there instructions on how I could build such a thing locally?

Comment 13 by scottmg@chromium.org, Mar 15 2014

If that's the only problem, we could #pragma push/pop in xtree on the next toolchain update. I'm not sure if it's important enough to do that though.

Comment 14 by pkasting@chromium.org, Mar 15 2014

I thought about that, but does everyone always build with the packaged toolchain, or only Googlers?  I wouldn't want to leave exterior people in the lurch.

Comment 15 by pkasting@chromium.org, Mar 15 2014

Hmm, locally, it looks like if I build with ninja (even via MSVC-Ninja projects) I get the packaged toolchain, but if I e.g. ctrl-F7 on a file, I get Visual Studio's internal headers.

Comment 16 by scottmg@chromium.org, Mar 15 2014

Yes, everyone gets a packaged toolchain, just the acquisition method differs.

The other one sounds like a bug: https://code.google.com/p/chromium/issues/detail?id=352876

Comment 17 by thakis@chromium.org, Mar 15 2014

Re comment 12:

  python tools/clang/scripts/update.py

to check out and build clang (this requires that you have a recent cmake installed), and then add clang=1 to your GYP_DEFINES. (This isn't really intended for general consumption yet, so it's a bit annoying to set up atm.)

update.py will fetch current clang trunk, so it's possible it fetches a broken revision. (But the fyi bot uses this script, and it looks relatively happy atm)

Comment 18 by pkasting@chromium.org, Mar 18 2014

@17: I was unable to get this to work.  The script initially refused to run any commands successfully until I changed RunCommand to not set Shell=True.  That got me to attempting to run the vcvarsXXX.bat files and CMake.  Since this was cryptically not working inside the script, I tried running things by hand, but got CMake fatal errors as follows.  At this point I gave up.

-- The C compiler identification is GNU 4.8.2
-- Check for working C compiler using: Ninja
-- Check for working C compiler using: Ninja -- broken
CMake Error at /usr/share/cmake-2.8.9/Modules/CMakeTestCCompiler.cmake:52 (MESSAGE):
  The C compiler "/usr/bin/gcc.exe" is not able to compile a simple test
  program.

  It fails with the following output:

   Change Dir: /d/chrome/CMakeFiles/CMakeTmp



  Run Build Command:/cygdrive/d/src/depot_tools/ninja.exe
  cmTryCompileExec2935833029

  [1/1] Re-running CMake...

  FAILED: /usr/bin/cmake.exe -H/d/chrome/CMakeFiles/CMakeTmp
  -B/d/chrome/CMakeFiles/CMakeTmp

  CreateProcess failed: The system cannot find the file specified.

  ninja: error: rebuilding 'build.ninja': subcommand failed





  CMake will not be able to correctly generate this project.
Call Stack (most recent call first):
  CMakeLists.txt:19 (project)


CMake Error: Error required internal CMake variable not set, cmake may be not be built correctly.
Missing variable is:
CMAKE_CXX_COMPILER_ENV_VAR
CMake Error: Error required internal CMake variable not set, cmake may be not be built correctly.
Missing variable is:
CMAKE_CXX_COMPILER
CMake Error: Could not find cmake module file:/d/chrome/CMakeFiles/CMakeCXXCompiler.cmake
CMake Error: CMAKE_CXX_COMPILER not set, after EnableLanguage
-- Configuring incomplete, errors occurred!

Comment 19 by bugdroid1@chromium.org, Mar 18 2014

Project Member
------------------------------------------------------------------
r257705 | pkasting@chromium.org | 2014-03-18T19:04:21.821855Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/tools/gn/gn_main.cc?r1=257705&r2=257704&pathrev=257705
   M http://src.chromium.org/viewvc/chrome/trunk/src/cc/output/output_surface.cc?r1=257705&r2=257704&pathrev=257705
   M http://src.chromium.org/viewvc/chrome/trunk/src/cloud_print/service/win/chrome_launcher.cc?r1=257705&r2=257704&pathrev=257705
   M http://src.chromium.org/viewvc/chrome/trunk/src/sync/syncable/model_type.cc?r1=257705&r2=257704&pathrev=257705
   M http://src.chromium.org/viewvc/chrome/trunk/src/device/bluetooth/bluetooth_adapter_factory.cc?r1=257705&r2=257704&pathrev=257705
   M http://src.chromium.org/viewvc/chrome/trunk/src/components/translate/language_detection/language_detection_util.cc?r1=257705&r2=257704&pathrev=257705
   M http://src.chromium.org/viewvc/chrome/trunk/src/extensions/common/extension_paths.cc?r1=257705&r2=257704&pathrev=257705
   M http://src.chromium.org/viewvc/chrome/trunk/src/media/filters/ffmpeg_demuxer_unittest.cc?r1=257705&r2=257704&pathrev=257705
   M http://src.chromium.org/viewvc/chrome/trunk/src/media/filters/ffmpeg_demuxer.cc?r1=257705&r2=257704&pathrev=257705
   M http://src.chromium.org/viewvc/chrome/trunk/src/courgette/disassembler_elf_32_arm.cc?r1=257705&r2=257704&pathrev=257705
   M http://src.chromium.org/viewvc/chrome/trunk/src/gpu/config/gpu_info_collector_win.cc?r1=257705&r2=257704&pathrev=257705
   M http://src.chromium.org/viewvc/chrome/trunk/src/webkit/browser/fileapi/isolated_context.cc?r1=257705&r2=257704&pathrev=257705

Fix "unreachable code" warnings (MSVC warning 4702), misc. edition.

This CL covers top-level directories that only had one or two modified files.

BUG=346399
TEST=none
R=darin@chromium.org

Review URL: https://codereview.chromium.org/203043002
-----------------------------------------------------------------

Comment 21 by bugdroid1@chromium.org, Mar 18 2014

Project Member

Comment 23 by bugdroid1@chromium.org, Mar 18 2014

Project Member
------------------------------------------------------------------
r257797 | pkasting@chromium.org | 2014-03-18T22:53:40.353555Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc?r1=257797&r2=257796&pathrev=257797
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/renderer_context_menu/context_menu_content_type_panel.cc?r1=257797&r2=257796&pathrev=257797
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/views/frame/browser_view.cc?r1=257797&r2=257796&pathrev=257797
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/app_list/app_list_service.cc?r1=257797&r2=257796&pathrev=257797
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/fullscreen/fullscreen_controller.cc?r1=257797&r2=257796&pathrev=257797
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/devtools/devtools_sanity_browsertest.cc?r1=257797&r2=257796&pathrev=257797
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/api/extension_action/browser_action_apitest.cc?r1=257797&r2=257796&pathrev=257797
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/views/tabs/tab_strip.cc?r1=257797&r2=257796&pathrev=257797
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/views/hung_renderer_view.cc?r1=257797&r2=257796&pathrev=257797
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/lifetime/application_lifetime_win.cc?r1=257797&r2=257796&pathrev=257797
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/chrome_plugin_browsertest.cc?r1=257797&r2=257796&pathrev=257797
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/upgrade_detector_impl.cc?r1=257797&r2=257796&pathrev=257797
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/renderer_context_menu/context_menu_content_type.cc?r1=257797&r2=257796&pathrev=257797
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc?r1=257797&r2=257796&pathrev=257797
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/renderer_context_menu/context_menu_content_type.h?r1=257797&r2=257796&pathrev=257797
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/views/find_bar_host_interactive_uitest.cc?r1=257797&r2=257796&pathrev=257797
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/api/input/input.cc?r1=257797&r2=257796&pathrev=257797
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/renderer_context_menu/spelling_menu_observer.cc?r1=257797&r2=257796&pathrev=257797
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc?r1=257797&r2=257796&pathrev=257797
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/views/tabs/tab_drag_controller.cc?r1=257797&r2=257796&pathrev=257797
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/notifications/sync_notifier/synced_notification_app_info_service.cc?r1=257797&r2=257796&pathrev=257797
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/prefs/incognito_mode_prefs.cc?r1=257797&r2=257796&pathrev=257797
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/search/search_tab_helper.cc?r1=257797&r2=257796&pathrev=257797
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/process_singleton_win.cc?r1=257797&r2=257796&pathrev=257797

Fix "unreachable code" warnings (MSVC warning 4702) in chrome/browser/.

BUG=346399
TEST=none
R=sky@chromium.org

Review URL: https://codereview.chromium.org/202993002
-----------------------------------------------------------------

Comment 24 by bugdroid1@chromium.org, Mar 19 2014

Project Member
------------------------------------------------------------------
r257818 | pkasting@chromium.org | 2014-03-18T23:59:00.528467Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/api/input/input.cc?r1=257818&r2=257817&pathrev=257818

Compile fix for Clang

BUG=346399
TEST=none

-----------------------------------------------------------------

Comment 25 by bugdroid1@chromium.org, Mar 19 2014

Project Member
------------------------------------------------------------------
r258096 | pkasting@chromium.org | 2014-03-19T21:15:58.779978Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/test/mock_webclipboard_impl.cc?r1=258096&r2=258095&pathrev=258096
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/common/content_switches_internal.cc?r1=258096&r2=258095&pathrev=258096
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/renderer_host/render_widget_host_view_aura.cc?r1=258096&r2=258095&pathrev=258096
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/common/indexed_db/indexed_db_param_traits.cc?r1=258096&r2=258095&pathrev=258096
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/renderer_host/input/gesture_event_queue.cc?r1=258096&r2=258095&pathrev=258096
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/gpu/compositor_util.cc?r1=258096&r2=258095&pathrev=258096
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/common/indexed_db/indexed_db_key.cc?r1=258096&r2=258095&pathrev=258096
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/dom_storage/dom_storage_database.cc?r1=258096&r2=258095&pathrev=258096
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/indexed_db/indexed_db_leveldb_coding.cc?r1=258096&r2=258095&pathrev=258096
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/compositor/gpu_process_transport_factory.cc?r1=258096&r2=258095&pathrev=258096
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/shell/renderer/test_runner/TestPlugin.cpp?r1=258096&r2=258095&pathrev=258096
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/renderer_host/render_process_host_impl.cc?r1=258096&r2=258095&pathrev=258096
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/renderer/media/webrtc_audio_device_impl.cc?r1=258096&r2=258095&pathrev=258096
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/child/webcrypto/jwk.cc?r1=258096&r2=258095&pathrev=258096
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/child/child_thread.cc?r1=258096&r2=258095&pathrev=258096
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/child/indexed_db/indexed_db_key_builders.cc?r1=258096&r2=258095&pathrev=258096
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/common/page_state_serialization_unittest.cc?r1=258096&r2=258095&pathrev=258096
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/common/content_paths.cc?r1=258096&r2=258095&pathrev=258096
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/child/npapi/webplugin_delegate_impl.cc?r1=258096&r2=258095&pathrev=258096

Fix "unreachable code" warnings (MSVC warning 4702) in content/.

BUG=346399
TEST=none
R=jam@chromium.org

Review URL: https://codereview.chromium.org/202863004
-----------------------------------------------------------------

Comment 26 by bugdroid1@chromium.org, Mar 19 2014

Project Member
------------------------------------------------------------------
r258105 | pkasting@chromium.org | 2014-03-19T21:38:44.494804Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/common/content_paths.cc?r1=258105&r2=258104&pathrev=258105

Attempt to fix compile failure.

BUG=346399
TEST=none

-----------------------------------------------------------------

Comment 29 by bugdroid1@chromium.org, Mar 20 2014

Project Member
------------------------------------------------------------------
r258398 | pkasting@chromium.org | 2014-03-20T20:37:45.690695Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/base/win/event_trace_consumer_unittest.cc?r1=258398&r2=258397&pathrev=258398
   M http://src.chromium.org/viewvc/chrome/trunk/src/base/test/expectations/expectation.cc?r1=258398&r2=258397&pathrev=258398

Fix "unreachable code" warnings (MSVC warning 4702) in base/.

This also does a little bit of code formatting in the hopes of shorter, more readable code.

BUG=346399
TEST=none
R=thakis@chromium.org

Review URL: https://codereview.chromium.org/202993003
-----------------------------------------------------------------

Comment 30 by bugdroid1@chromium.org, Mar 24 2014

Project Member
------------------------------------------------------------------
r258971 | pkasting@chromium.org | 2014-03-24T18:12:27.647324Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/ppapi/ppapi_test.cc?r1=258971&r2=258970&pathrev=258971

Fix "unreachable code" warnings (MSVC warning 4702) in a PPAPI test.

We can't convert preprocessor defines into unconditional early-returns.  Instead
make these functions simply do nothing when DISABLE_NACL is on.

BUG=346399
TEST=none
R=dmichael@chromium.org

Review URL: https://codereview.chromium.org/209183003
-----------------------------------------------------------------

Comment 31 by pkasting@chromium.org, Mar 25 2014

The current status of this bug, for anyone wondering, is that I've checked in all the fixes I'm aware of MSVC needing, safe for a warning disable for xtree in the MSVC STL headers which Scott is going to include with the next toolchain update.  Once that toolchain update ships, I can start trying the actual warning-re-enable change.

Nico, did you want to try to look at whether we should do this on Clang and what additional fixes might be needed to do so?

Comment 32 by thakis@chromium.org, Mar 25 2014

Yes, we should look at that. But -Wunreachable-code has seen a lot of attention on clang trunk and "our" clang currently trails trunk quite a bit (I'm actively working on fixing this), so I'd wait for a clang roll or two to land before looking at -Wunreachable-code.

Comment 33 by bugdroid1@chromium.org, Apr 4 2014

Project Member
------------------------------------------------------------------
r261868 | pkasting@chromium.org | 2014-04-04T21:04:05.699532Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/ui/views/controls/textfield/textfield.cc?r1=261868&r2=261867&pathrev=261868

Remove unreachable code in textfield.cc.

BUG=346399
TEST=none
TBR=msw

Review URL: https://codereview.chromium.org/226473004
-----------------------------------------------------------------

Comment 34 by bugdroid1@chromium.org, Apr 18 2014

Project Member
------------------------------------------------------------------
r264838 | pkasting@chromium.org | 2014-04-18T20:12:13.463239Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/utility/cloud_print/pwg_encoder.cc?r1=264838&r2=264837&pathrev=264838

Fix "unreachable code" warning (MSVC warning 4702) in
chrome/utility/cloud_print/.

BUG=346399
TEST=none

Review URL: https://codereview.chromium.org/241943003
-----------------------------------------------------------------

Comment 35 by bugdroid1@chromium.org, Apr 18 2014

Project Member
------------------------------------------------------------------
r264848 | pkasting@chromium.org | 2014-04-18T20:45:32.493914Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/ui/wm/core/compound_event_filter.cc?r1=264848&r2=264847&pathrev=264848

Fix "unreachable code" warning (MSVC warning 4702) in ui/wm/core/.

BUG=346399
TEST=none
R=sky@chromium.org

Review URL: https://codereview.chromium.org/242263002
-----------------------------------------------------------------

Comment 36 by pkasting@chromium.org, Apr 30 2014

Status: Started

Comment 37 by bugdroid1@chromium.org, May 5 2014

Project Member
The following revision refers to this bug:
  http://src.chromium.org/viewvc/blink?view=rev&rev=173228

------------------------------------------------------------------
r173228 | pkasting@chromium.org | 2014-05-02T21:11:28.904531Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/MediaValues.cpp?r1=173228&r2=173227&pathrev=173228

Remove unreachable code.

BUG=346399
TEST=none

Review URL: https://codereview.chromium.org/268553003
-----------------------------------------------------------------

Comment 38 by bugdroid1@chromium.org, May 20 2014

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9e3223381bfc0f198beb5e4057adac1e79ba3284

commit 9e3223381bfc0f198beb5e4057adac1e79ba3284
Author: scottmg@chromium.org <scottmg@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>
Date: Tue May 20 19:15:33 2014

VS2013 Update 2

As discussed in the linked bug, the toolchain2013.py script in
depot_tools is no longer workable (because the update for Update 2 is
supplied as a .msp that must be applied against a system-installed
VS2013).

As such, the Express hash here is not updated.

The hash zip referenced here was built as follows:
- Install VS2013 Update 2 on a clean VM
- Copy DIA SDK\, VC\
- Copy DLLs from VC\redist to sys32\ and sys64\
- Copy win8sdk unchanged from previous .zip.
- Delete various unused arm\ subdirectories
- Delete the IDE-only Snippets, etc. subdirectories in VC\
- Patch VC\include\xtree to disable warning 4702 per request in
  http://crbug.com/346399 .

A followup change will be to write a script that does these steps
semi-automatically, hopefully for Express too, though there is the
added complication of the WDK/ATL/MFC hacking required there. In
particular, this script will not be useful for a dev to actually run
as part of runhooks, but will be a bit useful for deployment/
documentation of the above process.

In the interim there are no extremely-pressing reasons for Express
users to update to Update2 that I'm aware of, so they get non-Update2
for now.

R=iannucci@chromium.org
BUG= 372451 ,346399, 371847 , 339215 , 350639 

Review URL: https://codereview.chromium.org/284663003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@271721 0039d316-1c4b-4281-b951-d872f2087c98

Comment 39 by bugdroid1@chromium.org, May 20 2014

Project Member
------------------------------------------------------------------
r271721 | scottmg@chromium.org | 2014-05-20T19:15:33.824791Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/apps/shell/app_shell.gyp?r1=271721&r2=271720&pathrev=271721
   M http://src.chromium.org/viewvc/chrome/trunk/src/build/get_landmines.py?r1=271721&r2=271720&pathrev=271721
   M http://src.chromium.org/viewvc/chrome/trunk/src/build/toolchain_vs2013.hash?r1=271721&r2=271720&pathrev=271721
   D http://src.chromium.org/viewvc/chrome/trunk/src/chrome/empty_pdb_workaround.cc?r1=271721&r2=271720&pathrev=271721
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/chrome_tests_unit.gypi?r1=271721&r2=271720&pathrev=271721
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/content_shell.gypi?r1=271721&r2=271720&pathrev=271721
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/chrome_tests.gypi?r1=271721&r2=271720&pathrev=271721
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/chrome_dll.gypi?r1=271721&r2=271720&pathrev=271721

VS2013 Update 2

As discussed in the linked bug, the toolchain2013.py script in
depot_tools is no longer workable (because the update for Update 2 is
supplied as a .msp that must be applied against a system-installed
VS2013).

As such, the Express hash here is not updated.

The hash zip referenced here was built as follows:
- Install VS2013 Update 2 on a clean VM
- Copy DIA SDK\, VC\
- Copy DLLs from VC\redist to sys32\ and sys64\
- Copy win8sdk unchanged from previous .zip.
- Delete various unused arm\ subdirectories
- Delete the IDE-only Snippets, etc. subdirectories in VC\
- Patch VC\include\xtree to disable warning 4702 per request in
  http://crbug.com/346399 .

A followup change will be to write a script that does these steps
semi-automatically, hopefully for Express too, though there is the
added complication of the WDK/ATL/MFC hacking required there. In
particular, this script will not be useful for a dev to actually run
as part of runhooks, but will be a bit useful for deployment/
documentation of the above process.

In the interim there are no extremely-pressing reasons for Express
users to update to Update2 that I'm aware of, so they get non-Update2
for now.

R=iannucci@chromium.org
BUG= 372451 ,346399, 371847 , 339215 , 350639 

Review URL: https://codereview.chromium.org/284663003
-----------------------------------------------------------------

Comment 40 by pkasting@chromium.org, May 20 2014

Scott, is it safe for me to start trying to push through this warning change now?  Or do I need to wait until e.g. "Express users get Update2"?

Comment 41 by scottmg@chromium.org, May 20 2014

As we don't have any bot infra for Express, my recommendation would be to keep 4702 disabled in an msvs_express block in build/common.gypi and push this through. I'm not sure how long it'll be before we can update Express users.

Comment 42 by pkasting@chromium.org, May 20 2014

OK.  Is there another bug that will track the VC Express work, so I can put a note on there to remove this block once it lands?

Comment 44 by bugdroid1@chromium.org, May 20 2014

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a08ecaa04b1435b076d7980cf1e62dccda9a19b2

commit a08ecaa04b1435b076d7980cf1e62dccda9a19b2
Author: scottmg@chromium.org <scottmg@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>
Date: Tue May 20 20:13:56 2014

Revert 271721 "VS2013 Update 2"

static_library build ICEing on some builders on mini_installer.

> VS2013 Update 2
> 
> As discussed in the linked bug, the toolchain2013.py script in
> depot_tools is no longer workable (because the update for Update 2 is
> supplied as a .msp that must be applied against a system-installed
> VS2013).
> 
> As such, the Express hash here is not updated.
> 
> The hash zip referenced here was built as follows:
> - Install VS2013 Update 2 on a clean VM
> - Copy DIA SDK\, VC\
> - Copy DLLs from VC\redist to sys32\ and sys64\
> - Copy win8sdk unchanged from previous .zip.
> - Delete various unused arm\ subdirectories
> - Delete the IDE-only Snippets, etc. subdirectories in VC\
> - Patch VC\include\xtree to disable warning 4702 per request in
>   http://crbug.com/346399 .
> 
> A followup change will be to write a script that does these steps
> semi-automatically, hopefully for Express too, though there is the
> added complication of the WDK/ATL/MFC hacking required there. In
> particular, this script will not be useful for a dev to actually run
> as part of runhooks, but will be a bit useful for deployment/
> documentation of the above process.
> 
> In the interim there are no extremely-pressing reasons for Express
> users to update to Update2 that I'm aware of, so they get non-Update2
> for now.
> 
> R=iannucci@chromium.org
> BUG= 372451 ,346399, 371847 , 339215 , 350639 
> 
> Review URL: https://codereview.chromium.org/284663003

TBR=scottmg@chromium.org

Review URL: https://codereview.chromium.org/297753002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@271737 0039d316-1c4b-4281-b951-d872f2087c98

Comment 45 by bugdroid1@chromium.org, May 20 2014

Project Member
------------------------------------------------------------------
r271737 | scottmg@chromium.org | 2014-05-20T20:13:56.227839Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/chrome_dll.gypi?r1=271737&r2=271736&pathrev=271737
   M http://src.chromium.org/viewvc/chrome/trunk/src/apps/shell/app_shell.gyp?r1=271737&r2=271736&pathrev=271737
   M http://src.chromium.org/viewvc/chrome/trunk/src/build/get_landmines.py?r1=271737&r2=271736&pathrev=271737
   M http://src.chromium.org/viewvc/chrome/trunk/src/build/toolchain_vs2013.hash?r1=271737&r2=271736&pathrev=271737
   A http://src.chromium.org/viewvc/chrome/trunk/src/chrome/empty_pdb_workaround.cc?r1=271737&r2=271736&pathrev=271737
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/chrome_tests_unit.gypi?r1=271737&r2=271736&pathrev=271737
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/content_shell.gypi?r1=271737&r2=271736&pathrev=271737
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/chrome_tests.gypi?r1=271737&r2=271736&pathrev=271737

Revert 271721 "VS2013 Update 2"

static_library build ICEing on some builders on mini_installer.

> VS2013 Update 2
> 
> As discussed in the linked bug, the toolchain2013.py script in
> depot_tools is no longer workable (because the update for Update 2 is
> supplied as a .msp that must be applied against a system-installed
> VS2013).
> 
> As such, the Express hash here is not updated.
> 
> The hash zip referenced here was built as follows:
> - Install VS2013 Update 2 on a clean VM
> - Copy DIA SDK\, VC\
> - Copy DLLs from VC\redist to sys32\ and sys64\
> - Copy win8sdk unchanged from previous .zip.
> - Delete various unused arm\ subdirectories
> - Delete the IDE-only Snippets, etc. subdirectories in VC\
> - Patch VC\include\xtree to disable warning 4702 per request in
>   http://crbug.com/346399 .
> 
> A followup change will be to write a script that does these steps
> semi-automatically, hopefully for Express too, though there is the
> added complication of the WDK/ATL/MFC hacking required there. In
> particular, this script will not be useful for a dev to actually run
> as part of runhooks, but will be a bit useful for deployment/
> documentation of the above process.
> 
> In the interim there are no extremely-pressing reasons for Express
> users to update to Update2 that I'm aware of, so they get non-Update2
> for now.
> 
> R=iannucci@chromium.org
> BUG= 372451 ,346399, 371847 , 339215 , 350639 
> 
> Review URL: https://codereview.chromium.org/284663003

TBR=scottmg@chromium.org

Review URL: https://codereview.chromium.org/297753002
-----------------------------------------------------------------

Comment 46 by bugdroid1@chromium.org, May 21 2014

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d60734768b6044451278ea61045d4843694d9562

commit d60734768b6044451278ea61045d4843694d9562
Author: scottmg@chromium.org <scottmg@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>
Date: Wed May 21 17:10:43 2014

Revert 271737 "Revert 271721 "VS2013 Update 2""

https://src.chromium.org/viewvc/chrome?view=rev&revision=271919 fixes the
compiler crash, http://src.chromium.org/viewvc/chrome?view=rev&revision=271780
fixes the bug in landmines that didn't allow the initial land/revert to
go smoothly.

Should be A-OK this time.

> Revert 271721 "VS2013 Update 2"
> 
> static_library build ICEing on some builders on mini_installer.
> 
> > VS2013 Update 2
> > 
> > As discussed in the linked bug, the toolchain2013.py script in
> > depot_tools is no longer workable (because the update for Update 2 is
> > supplied as a .msp that must be applied against a system-installed
> > VS2013).
> > 
> > As such, the Express hash here is not updated.
> > 
> > The hash zip referenced here was built as follows:
> > - Install VS2013 Update 2 on a clean VM
> > - Copy DIA SDK\, VC\
> > - Copy DLLs from VC\redist to sys32\ and sys64\
> > - Copy win8sdk unchanged from previous .zip.
> > - Delete various unused arm\ subdirectories
> > - Delete the IDE-only Snippets, etc. subdirectories in VC\
> > - Patch VC\include\xtree to disable warning 4702 per request in
> >   http://crbug.com/346399 .
> > 
> > A followup change will be to write a script that does these steps
> > semi-automatically, hopefully for Express too, though there is the
> > added complication of the WDK/ATL/MFC hacking required there. In
> > particular, this script will not be useful for a dev to actually run
> > as part of runhooks, but will be a bit useful for deployment/
> > documentation of the above process.
> > 
> > In the interim there are no extremely-pressing reasons for Express
> > users to update to Update2 that I'm aware of, so they get non-Update2
> > for now.
> > 
> > R=iannucci@chromium.org
> > BUG= 372451 ,346399, 371847 , 339215 , 350639 
> > 
> > Review URL: https://codereview.chromium.org/284663003
> 
> TBR=scottmg@chromium.org
> 
> Review URL: https://codereview.chromium.org/297753002

TBR=scottmg@chromium.org

Review URL: https://codereview.chromium.org/295093004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@271921 0039d316-1c4b-4281-b951-d872f2087c98

Comment 47 by bugdroid1@chromium.org, May 21 2014

Project Member
------------------------------------------------------------------
r271921 | scottmg@chromium.org | 2014-05-21T17:10:43.365759Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/apps/shell/app_shell.gyp?r1=271921&r2=271920&pathrev=271921
   M http://src.chromium.org/viewvc/chrome/trunk/src/build/get_landmines.py?r1=271921&r2=271920&pathrev=271921
   M http://src.chromium.org/viewvc/chrome/trunk/src/build/toolchain_vs2013.hash?r1=271921&r2=271920&pathrev=271921
   D http://src.chromium.org/viewvc/chrome/trunk/src/chrome/empty_pdb_workaround.cc?r1=271921&r2=271920&pathrev=271921
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/chrome_tests_unit.gypi?r1=271921&r2=271920&pathrev=271921
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/content_shell.gypi?r1=271921&r2=271920&pathrev=271921
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/chrome_tests.gypi?r1=271921&r2=271920&pathrev=271921
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/chrome_dll.gypi?r1=271921&r2=271920&pathrev=271921

Revert 271737 "Revert 271721 "VS2013 Update 2""

https://src.chromium.org/viewvc/chrome?view=rev&revision=271919 fixes the
compiler crash, http://src.chromium.org/viewvc/chrome?view=rev&revision=271780
fixes the bug in landmines that didn't allow the initial land/revert to
go smoothly.

Should be A-OK this time.

> Revert 271721 "VS2013 Update 2"
> 
> static_library build ICEing on some builders on mini_installer.
> 
> > VS2013 Update 2
> > 
> > As discussed in the linked bug, the toolchain2013.py script in
> > depot_tools is no longer workable (because the update for Update 2 is
> > supplied as a .msp that must be applied against a system-installed
> > VS2013).
> > 
> > As such, the Express hash here is not updated.
> > 
> > The hash zip referenced here was built as follows:
> > - Install VS2013 Update 2 on a clean VM
> > - Copy DIA SDK\, VC\
> > - Copy DLLs from VC\redist to sys32\ and sys64\
> > - Copy win8sdk unchanged from previous .zip.
> > - Delete various unused arm\ subdirectories
> > - Delete the IDE-only Snippets, etc. subdirectories in VC\
> > - Patch VC\include\xtree to disable warning 4702 per request in
> >   http://crbug.com/346399 .
> > 
> > A followup change will be to write a script that does these steps
> > semi-automatically, hopefully for Express too, though there is the
> > added complication of the WDK/ATL/MFC hacking required there. In
> > particular, this script will not be useful for a dev to actually run
> > as part of runhooks, but will be a bit useful for deployment/
> > documentation of the above process.
> > 
> > In the interim there are no extremely-pressing reasons for Express
> > users to update to Update2 that I'm aware of, so they get non-Update2
> > for now.
> > 
> > R=iannucci@chromium.org
> > BUG= 372451 ,346399, 371847 , 339215 , 350639 
> > 
> > Review URL: https://codereview.chromium.org/284663003
> 
> TBR=scottmg@chromium.org
> 
> Review URL: https://codereview.chromium.org/297753002

TBR=scottmg@chromium.org

Review URL: https://codereview.chromium.org/295093004
-----------------------------------------------------------------

Comment 48 by bugdroid1@chromium.org, May 22 2014

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/887dc5d85a487e0a071099b58f1ea2235602a0ab

commit 887dc5d85a487e0a071099b58f1ea2235602a0ab
Author: pkasting@chromium.org <pkasting@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>
Date: Thu May 22 03:21:41 2014

Fix more cases of unreachable code on Windows, mostly added recently.

BUG=346399
TEST=none

Review URL: https://codereview.chromium.org/296053005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@272069 0039d316-1c4b-4281-b951-d872f2087c98

Comment 50 by bugdroid1@chromium.org, May 24 2014

Project Member
------------------------------------------------------------------
r272687 | pkasting@chromium.org | 2014-05-24T02:35:26.673831Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/build/common.gypi?r1=272687&r2=272686&pathrev=272687

Enable "unreachable code" warning for MSVC.

BUG=346399
TEST=none

Review URL: https://codereview.chromium.org/260903003
-----------------------------------------------------------------

Comment 51 by pkasting@chromium.org, May 24 2014

Cc: -thakis@chromium.org
Owner: thakis@chromium.org
Status: Assigned
As of r272687, this warning is now enabled for Windows.

Over to Nico to handle Clang/gcc as desired.

Comment 52 by thakis@google.com, May 24 2014

Did you measure compile time impact?

Our clang is now new enough to have all recent improvements for this warning, I'll give it a try.

Comment 53 by pkasting@chromium.org, May 24 2014

I did not check the MSVC compile time impact.

Comment 54 by pkasting@chromium.org, May 24 2014

That said, I think this would be worth doing even with a compile time hit.  In fixing the places that triggered this warning, I've seen people slowly but steadily introducing new cases of this, about a quarter of which are real bugs.  So IMO this catches real issues in an ongoing way.

Comment 55 by marshall@chromium.org, Jun 11 2014

How should we handle warnings in third_party code that are now errors due to this change? For example, building with VS2013 Pro Update 2 on Windows 8.1 64-bit at Chromium revision 275973 I now get the following error:

[38/11056] CXX obj\third_party\webrtc\modules\remote_bitra...\rbe_components.remote_bitrate_estimator_single_stream.obj
FAILED: ninja -t msvc -e environment.x86 -- "C:\Program Files (x86)\Microsoft Visual Studio 12.0\VC\bin\amd64_x86\cl.exe" /nologo /showIncludes /FC @obj\third_party\webrtc\modules\remote_bitrate_estimator\rbe_components.remote_bitrate_estimator_single_stream.obj.rsp /c ..\..\third_party\webrtc\modules\remote_bitrate_estimator\remote_bitrate_estimator_single_stream.cc /Foobj\third_party\webrtc\modules\remote_bitrate_estimator\rbe_components.remote_bitrate_estimator_single_stream.obj /Fdobj\third_party\webrtc\modules\remote_bitrate_estimator\rbe_components.cc.pdb
c:\program files (x86)\microsoft visual studio 12.0\vc\include\xtree(1826) : error C2220: warning treated as error - no
'object' file generated
c:\program files (x86)\microsoft visual studio 12.0\vc\include\xtree(1826) : warning C4702: unreachable code

Comment 56 by marshall@chromium.org, Jun 11 2014

The error in comment#55 is for a Debug non-component build, so sounds like a similar problem to comment #7.

Comment 57 by scottmg@chromium.org, Jun 11 2014

Given the drawbacks here of breaking various configs, and requiring a manual patch to xtree, I think we should disable warning 4702, and perhaps add an FYI bot that has it on, with warnings-not-as-errors so we can evaluate/avoid/fix as it seems beneficial.

The variation between when it's reported and not based on compiler settings make it less useful than it could otherwise be.

Comment 58 by pkasting@chromium.org, Jun 11 2014

I am strongly opposed to comment 57.  Fixing these warnings caught a _ton_ of real errors in our code, and continued to catch real errors being introduced at a rapid rate as I kept the tree clean before enabling this.  There is no way an FYI bot will have the same effect.

I think "breaking various configs" is overstating things.  We've seen exactly two breaks: One with PGO builds which is now permanently addressed, and the one in comment 55.

@55: How is it that you're not using the toolchain that gclient distributes in order to build?  While you can easily fix this warning with a small change to xtree, it shouldn't be necessary if you're using the gclient toolchain.

Comment 59 by scottmg@chromium.org, Jun 11 2014

The header is not patched for Express users, only Googlers.

Could you quantify the style of bugs that were caught? Anything that was actually causing user-visible bugs?

Comment 60 by marshall@chromium.org, Jun 11 2014

@comment#58: Non-Googlers currently get VS2013 Express Update 1 via the bundled gclient toolchain, and that currently fails to build for other reasons (see https://code.google.com/p/chromium/issues/detail?id=380332#c7). Requiring all non-Googlers modify their local copy of xtree isn't very scalable (or easy to communicate).

Comment 61 by pkasting@chromium.org, Jun 11 2014

@59: Yes, we had memory issues, as well as subtle behavior changes.  For example, I believe  bug 346382  reduced our sandbox effectiveness on Vista.

@59, 60: Our build files turn off C4702 for Visual Studio Express users, so this isn't a problem for them.  This is only a problem if you're using your own, non-bundled version of Visual Studio Pro to build.  That seems like a much smaller group of affected people, which we _could_ potentially direct to apply a patch, or at least to set some gyp flag that we can add to cause us to disable C4702.

Comment 62 by marshall@chromium.org, Jun 11 2014

@comment#61: Maybe we can detect if the user is building with a non-bundled VS version and turn off C4702 automatically? If not, I would be satisfied with a gyp variable that turns off C4702.

Comment 63 by pkasting@chromium.org, Jun 11 2014

FWIW the patch to make xtree work is to add the following line near the top:

  #undef new

  #pragma warning(disable: 4127)
+ #pragma warning(disable: 4702)
 _STD_BEGIN
                 // TEMPLATE CLASS _Tree_unchecked_const_iterator
 template<class _Mytree,

I posted on the Microsoft bug about this asking if they would be willing to make this change themselves (fingers crossed).

As for detecting a non-bundled VS version, I'd have to defer to Scott about whether such a thing is possible.

Comment 64 by mmenke@chromium.org, Jul 25 2014

And for anyone running into this issue, you may need to remove the read-only property from xtree, if you're using the version from a normal MSVC install (I thought I was running into a permissions issue, and wasted some time on it).

Comment 65 by thakis@chromium.org, Aug 3 2014

Cc: pkasting@chromium.org
I just saw https://code.google.com/p/pdfium/issues/detail?id=25#c13 (and comment 64 here, I guess). Requiring folks to patch their system headers isn't acceptable.

Is there some way to only enable this warning with msvc if we know that it's going to work (i.e. when the automatic toolchain is used)?

Comment 66 by pkasting@chromium.org, Aug 4 2014

We've set things up so that everyone who builds the standard Chromium config works -- if you have google.com credentials, you get a correctly-patched toolchain, and if you don't, you get VC express and the .gypi configuration disables the warning.  The only problematic case is for people who don't build using either of these options.

I would be happy to modify the build files so that we disable the warning for DEPOT_TOOLS_WIN_TOOLCHAIN=0, if someone knows how to do that.  I think that ought to cover the rest of the cases.

Comment 67 by thakis@chromium.org, Aug 4 2014

Maybe a gyp conditional with

  '<!(python -c 'import os; print os.environ.get("DEPOT_TOOLS_WIN_TOOLCHAIN", 1)')==0

to disable it would do the trick?

Comment 68 by pkasting@chromium.org, Aug 4 2014

I don't know; I don't speak gyp or python :(

It seems like there ought to be a more direct way of doing this somehow, but maybe I'm smoking crack.

Comment 69 by bugdroid1@chromium.org, Jan 7 2015

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/04dd0973fcc16a1839bf1a57ad9392857844ad52

commit 04dd0973fcc16a1839bf1a57ad9392857844ad52
Author: scottmg <scottmg@chromium.org>
Date: Wed Jan 07 23:41:48 2015

Fix xtree patch check, and in turn C4702 disabling

Two bugs:
- IsPatched was backwards
- The output was "True"/"False", but .gyp was expecting "1"/"0".

R=cpu@chromium.org
BUG=346399

Review URL: https://codereview.chromium.org/836393002

Cr-Commit-Position: refs/heads/master@{#310409}

[modify] http://crrev.com/04dd0973fcc16a1839bf1a57ad9392857844ad52/build/win_is_xtree_patched.py
[modify] http://crrev.com/04dd0973fcc16a1839bf1a57ad9392857844ad52/chrome/browser/extensions/api/networking_private/networking_private_verify_delegate_factory_impl.cc

Comment 70 by sheriffbot@chromium.org, Jul 6 2016

Project Member
Labels: Hotlist-OpenBugWithCL
A change has landed for this issue, but it's been open for over 6 months. Please review and close it if applicable. If this issue should remain open, remove the "Hotlist-OpenBugWithCL" label. If no action is taken, it will be archived in 30 days.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 71 by pkasting@chromium.org, Jul 6 2016

Labels: -Hotlist-OpenBugWithCL
4702 is mostly enabled; it's still disabled for PGO builds (which is probably OK), Blink (which may or may not be fixable), and some third-party code, at least some of which (ANGLE) says it can probably be removed.

I don't know where the gcc/clang builds stand on their version of this warning.

Comment 72 by jmad...@chromium.org, Jul 15 2016

pkasting: I was looking at fixing this warning in ANGLE, and ran into an issue with the header patch workaround on the try bots.

See: https://chromium-review.googlesource.com/#/c/360940/
eg: https://build.chromium.org/p/tryserver.chromium.angle/builders/win_angle_x64_rel_ng/builds/1684/steps/compile%20%28with%20patch%29/logs/stdio

I'm getting a bunch of errors in the xtree header with 'recursive call has no side effects, deleting'.

Any ideas?

Comment 73 by thakis@chromium.org, Jul 15 2016

(Re #72: We looked at it some, and that's about a different warning and unrelated to this bug.)

Comment 74 by thakis@chromium.org, Aug 15 2017

We (I) really should do this now that we're moving towards clang everywhere. We don't want to lose all the progress pkasting made here.

Comment 75 by thakis@chromium.org, Sep 15 2017

I started looking at this a bit. I applied https://chromium-review.googlesource.com/c/chromium/src/+/615523 on top of https://chromium-review.googlesource.com/c/chromium/src/+/584810 and build all targets with `-k 0` targeting windows (since that's where pkasting cleaned up the build) on my linux box (since I'm at home and can only ssh to that).

Some of the attached output is due to the cross build not being completely functional in all cases (e.g. nacl) yet, but overall this gives a good idea of the kinds of issues we'll have to fix. It looks like it found at least 2 bugs, too:

https://codereview.chromium.org/2573573004/#msg27
https://codereview.chromium.org/2943983002/#msg23

(Getting to this point was dependent on http://llvm.org/viewvc/llvm-project?view=revision&revision=311561)
wunreach.txt
67.3 KB View Download

Comment 76 by bugdroid1@chromium.org, Sep 16 2017

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/616100e9bd4284dfb3fcc62c66ae966be1e042aa

commit 616100e9bd4284dfb3fcc62c66ae966be1e042aa
Author: Koji Ishii <kojii@chromium.org>
Date: Sat Sep 16 12:36:06 2017

[LayoutNG] Fix ComputedStyle::ShouldUseTextIndent()

This patch fixes a typo that calls the default constructor instead of
the getter when the logic was copied from BreakingContextInlineHeaders.

This code path is used only in LayoutNG. Tests in fast/css3-text/css3-
text-indent/ still fail for other reasons when LayoutNG is enabled.

Bug: 636993, 346399
Change-Id: I9beea46bd11d3486bd9047ad98d14d960df8bad3
Reviewed-on: https://chromium-review.googlesource.com/670199
Commit-Queue: Koji Ishii <kojii@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#502501}
[modify] https://crrev.com/616100e9bd4284dfb3fcc62c66ae966be1e042aa/third_party/WebKit/Source/core/style/ComputedStyle.cpp

Comment 77 by bugdroid1@chromium.org, Sep 20 2017

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/255bf2d65ada1a77bc23a111267bef0a94e8c47f

commit 255bf2d65ada1a77bc23a111267bef0a94e8c47f
Author: Nico Weber <thakis@chromium.org>
Date: Wed Sep 20 11:25:51 2017

v8: Fix most -Wunreachable-code warnings.

Do this by deleting code after calls to V8_Fatal() (either through
UNREACHABLE() or FATAL()). Comments suggest that the returns there
were needed to make a compiler happy, but all compilers seem to be
happy with this change too. My guess is that either
https://codereview.chromium.org/1393023003 which marked V8_Fatal()
as noreturn, or https://chromium-review.googlesource.com/#/c/544845/
which switched to the C++11 spelling of noreturn, fixed the warnings
that the explicit code after V8_Fatal() was supposed to silence.

There's one more warning in src/compiler/machine-graph-verifier.cc,
but fixing that changes behavior.  I asked about that one in
https://codereview.chromium.org/2573573004/#msg27 instead.

Bug: chromium:346399
Change-Id: Ie9519d5432bdeaaf382e8390d8254d3b79e622e4
Reviewed-on: https://chromium-review.googlesource.com/669803
Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Commit-Queue: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#48092}
[modify] https://crrev.com/255bf2d65ada1a77bc23a111267bef0a94e8c47f/src/api-natives.cc
[modify] https://crrev.com/255bf2d65ada1a77bc23a111267bef0a94e8c47f/src/compiler/access-builder.cc
[modify] https://crrev.com/255bf2d65ada1a77bc23a111267bef0a94e8c47f/src/compiler/bytecode-graph-builder.cc
[modify] https://crrev.com/255bf2d65ada1a77bc23a111267bef0a94e8c47f/src/deoptimizer.cc
[modify] https://crrev.com/255bf2d65ada1a77bc23a111267bef0a94e8c47f/src/dtoa.cc
[modify] https://crrev.com/255bf2d65ada1a77bc23a111267bef0a94e8c47f/src/feedback-vector.cc
[modify] https://crrev.com/255bf2d65ada1a77bc23a111267bef0a94e8c47f/src/objects/module.cc
[modify] https://crrev.com/255bf2d65ada1a77bc23a111267bef0a94e8c47f/src/runtime/runtime-strings.cc

Comment 78 by bugdroid1@chromium.org, Sep 21 2017

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/2c22b8ecb7248f3fa17f4092b62c974f9a2b12d8

commit 2c22b8ecb7248f3fa17f4092b62c974f9a2b12d8
Author: Igor Sheludko <ishell@chromium.org>
Date: Thu Sep 21 08:45:09 2017

[csa] Fix typo in machine graph verifier.

Bug: chromium:346399
Change-Id: I4d93dbef6deb0fee477f88e20c40106868e99dee
Reviewed-on: https://chromium-review.googlesource.com/674940
Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Commit-Queue: Igor Sheludko <ishell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#48101}
[modify] https://crrev.com/2c22b8ecb7248f3fa17f4092b62c974f9a2b12d8/src/compiler/machine-graph-verifier.cc

Comment 79 by bugdroid1@chromium.org, Sep 27 2017

Project Member
The following revision refers to this bug:
  https://pdfium.googlesource.com/pdfium/+/fe9ea0eff300458314d8a422d064597eb588e25d

commit fe9ea0eff300458314d8a422d064597eb588e25d
Author: Nicolás Peña <npm@chromium.org>
Date: Wed Sep 27 12:59:38 2017

Remove unreachable code in fx_codec_icc

This CL removes unreachable code and also removes the flag
Icc_Format_DEFAULT which becomes unused.

Bug: chromium:346399
Change-Id: I1cdd0f70ffec2abcd20ddf5b181273971b92ecaa
Reviewed-on: https://pdfium-review.googlesource.com/14850
Commit-Queue: Nico Weber <thakis@chromium.org>
Reviewed-by: dsinclair <dsinclair@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>

[modify] https://crrev.com/fe9ea0eff300458314d8a422d064597eb588e25d/core/fxcodec/fx_codec_def.h
[modify] https://crrev.com/fe9ea0eff300458314d8a422d064597eb588e25d/core/fxcodec/codec/fx_codec_icc.cpp

Comment 80 by mseaborn@chromium.org, Sep 28 2017

What happened with the issue that thakis@ mentioned in comment 4?  i.e. "One of the reasons we don't build with -Wunreachable on clang is because it makes local development harder – if you make some code unreachable locally your file won't compile because we build with warnings-as-errors."

It's common to have code like this, and to want to disable the "Bar(x)" call during development:

  auto x = Foo();
  ...
  Bar(x);

Commenting out Bar(x) or disabling it with #if gives the warning/error that x is unused.  One can write "if (0) Bar(x)" instead to disable the function call, but that also stops working with -Wunreachable.  I suppose one can write "if ((0)) Bar(x)" instead.

Comment 81 by pkasting@chromium.org, Sep 28 2017

I think the best solution there would be to make it easy to disable warnings-as-errors for local development.  This is not the only error that's caused me problems in that case, and it seems like we should try to both make local development easy and make what's landed in the tree maximally safe; simply leaving this disabled wouldn't do the latter.

Comment 82 by bugdroid1@chromium.org, Sep 29 2017

Project Member

Comment 83 by robliao@chromium.org, Jun 4 2018

Blocking: 848979

Sign in to add a comment