Incremental linking doesn't work for browser_tests due to slash direction mismatch |
|||
Issue descriptionThese were the gn args I used: is_debug = true symbol_level = 2 win_linker_timing = true is_component_build = true is_win_fastlink = true use_lld = false enable_google_now = false enable_nacl = false is_clang = false use_goma = true goma_dir = "C:\goma\goma-win64" target_cpu = "x86" I noticed this because I enabled win_linker_timing. Here are the important bits of my log: $ ninja -j500 browser_tests [4/4] LINK browser_tests.exe browser_tests.exe.pdb LINK :Removed file: obj\chrome\test\browser_tests_runner.lib LINK :Removed file: obj\chrome\test\test_support.lib ... many more removed files LINK :New file: obj/chrome/test/browser_tests_runner.lib LINK :New file: obj/chrome/test/test_support.lib ... many more added files LINK : object file added; performing full link LibDef: Total time = 0.015s Pass 1: Interval #1, time = 57.344s Wait PDB close: Total time = 0.047s Pass 2: Interval #2, time = 133.125s Final: Total time = 190.469s Final: Total time = 194.641s Note that I only changed two .cc files to add and remove __debugbreak() instructions. My understanding is that these days we pass forward slashes to the Microsoft tools, but it looks like the linker is canonicalizing the strings some of the time but not all of the time. Thoughts? Maybe recent fallout of the new VS toolchain?
,
Nov 1 2017
Thanks for the report! I've confirmed this bug, and this is indeed caused by the recent VS toolchain. This is consistent with another slash/back-slash bug that we hit where the /libpath command no longer supports slashes: https://developercommunity.visualstudio.com/content/problem/139061/libpath-no-longer-supports-slashes.html This caused /wholearchive commands to fail (the fact that /wholearchive fails silently is yet another bug, but that is why we didn't notice this problem immediately). The libpath slash/backslash bug has reportedly been fixed: "We have fixed the problem in an upcoming release.", which probably means 15.5. I'll file another bug to make sure. I tested with the 15.3 toolchain (git revert 70155edf7d3f4ffa931adc3077968ee99b9462ac, gclient runhooks, rebuild) to confirm that the bug did not exist then. I also tested by manually doing the link after replacing the / characters with \ characters. On the first link it complained about removing items with '/' in the path and adding in items with '\' in the path, but on the second link it worked. I guess they are storing the path names twice, once canonicalized and once not. Awesome. It looks like our options are: 1) We could go back to the 15.3 toolchain. We would lose a code-gen bug fix but we had a workaround for it 2) We could modify gn to always use back slashes 3) We could sit tight and suffer until 15.5 comes out I don't like any of the options, not really, but I think #1 might be the best? My test command line was: >touch c:\src\chromium3\src\chrome\browser\accessibility\browser_accessibility_state_browsertest.cc & ninja -v -d keeprsp -C out\debug_component browser_tests My gn args are: is_clang = false use_lld = false win_linker_timing = true is_component_build = true is_debug = true target_cpu = "x86" enable_nacl = false use_goma = true is_win_fastlink = true symbol_level = 1 remove_webcore_debug_symbols = true use_jumbo_build = true
,
Nov 1 2017
I came up with a minimal repro and filed a bug. I also verified that the bug still exists in 15.5.0 Preview 2.0. https://developercommunity.visualstudio.com/content/problem/142544/incremental-linking-regresion-in-154.html
,
Nov 3 2017
@brucedawson: my vote is for whatever fixes this regression asap. This is hurting my productivity a lot.
,
Nov 3 2017
Revert created here: https://chromium-review.googlesource.com/c/chromium/src/+/753422 It does seem that the benefits of 15.4 do not justify the costs. In fact, the main benefits of switching to 15.4 are probably that it let us identify this problem so that it will get fixed, so having done that we should revert it.
,
Nov 3 2017
The VS bug has been updated with this comment: Fixed - Pending Release I suspect that this means that it will be fixed in VS 15.5 which is currently available in preview form. I will test with subsequent previews.
,
Nov 13 2017
The revert of the 15.4 toolchain has resolved this bug. I debated lowering the priority but decided that closing the bug made the most sense. I will test incremental linking before we upgrade the toolchain again. Here is the CL that resolved this bug: The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/db45606398cf4389bf332b0cdcffd04e7de4a4f6 commit db45606398cf4389bf332b0cdcffd04e7de4a4f6 Author: Bruce Dawson <brucedawson@chromium.org> Date: Fri Nov 03 23:48:27 2017 Revert "VS 2017 15.4 with 10.0.15063.468 SDK" This reverts commit 70155edf7d3f4ffa931adc3077968ee99b9462ac. Reason for revert: the 15.4 toolchain breaks incremental linking for Chrome ( crbug.com/780161 ) Original change's description: > VS 2017 15.4 with 10.0.15063.468 SDK > > This change switches the VS 2017 package to use VS 2017 Update 4 while > still using the 10.0.15063.468 SDK. Update 4 fixes at least one code-gen > bug ( crbug.com/759402 ) but the 10.0.16299.0 SDK is still incompatible > with Chrome ( crbug.com/773476 ). > > Packaging was done on a Windows Server 2016 VM, cleanly created for this > purpose. > > Compiler was packaged up by downloading VS 2017 Update 4, from > https://www.visualstudio.com/vs/, and then passing these parameters to > the installer: > > --add Microsoft.VisualStudio.Workload.NativeDesktop > --add Microsoft.VisualStudio.Component.VC.ATLMFC --includeRecommended > --passive > > Then the Windows 10.0.15063.468 SDK installer was run and everything was > installed except for the Windows Performance Toolkit, .Net Framework, > and the arm SDKs. > > Then the packaging script was run like this: > > python depot_tools\win_toolchain\package_from_installed.py 2017 -w 10.0.15063.0 > > Bug: 773476 , 759402 > Change-Id: Ie2176b5ff765d9e5497f51a7b00c02fad04fb973 > Reviewed-on: https://chromium-review.googlesource.com/727523 > Reviewed-by: Scott Graham <scottmg@chromium.org> > Commit-Queue: Bruce Dawson <brucedawson@chromium.org> > Cr-Commit-Position: refs/heads/master@{#510207} TBR=thakis@chromium.org,brucedawson@chromium.org,scottmg@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 773476 , 759402 Change-Id: Ica63e9740c954499b85ee891fb3bec0d48dd70fb Reviewed-on: https://chromium-review.googlesource.com/753422 Reviewed-by: Bruce Dawson <brucedawson@chromium.org> Reviewed-by: John Abd-El-Malek <jam@chromium.org> Commit-Queue: Bruce Dawson <brucedawson@chromium.org> Cr-Commit-Position: refs/heads/master@{#513965} [modify] https://crrev.com/db45606398cf4389bf332b0cdcffd04e7de4a4f6/build/vs_toolchain.py |
|||
►
Sign in to add a comment |
|||
Comment 1 by jam@chromium.org
, Oct 31 2017