New issue
Advanced search Search tips

Issue 780161 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 1
Type: Bug



Sign in to add a comment

Incremental linking doesn't work for browser_tests due to slash direction mismatch

Project Member Reported by r...@chromium.org, Oct 31 2017

Issue description

These 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?
 
incremental-link-fail.txt
73.0 KB View Download

Comment 1 by jam@chromium.org, Oct 31 2017

Labels: -Pri-3 Pri-1
Thanks for filing. Raising the priority as a full link of browsr_tests on debug is very painful
Owner: brucedaw...@chromium.org
Status: Started (was: Untriaged)
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

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

Comment 4 by jam@chromium.org, Nov 3 2017

@brucedawson: my vote is for whatever fixes this regression asap. This is hurting my productivity a lot.
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.
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.

Status: Fixed (was: Started)
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