1455504 - MSVC is always performing full links

Status: VERIFIED FIXED

(Firefox Build System :: General, defect)

Description

[(No longer employed by Mozilla) Aaron Klotz]

STR:

1. Add the following to a debug mozconfig:
export LDFLAGS='-verbose -time+'

2. Do a fresh build of gecko;
3. Touch a file that will trigger a relink of xul.dll;
4. ./mach build -v binaries
and log all output to a file.

Expected:
The linker's diagnostic information shows that it is performing an incremental link.

Actual:
The linker outputs the following message:
LINK : object file removed; performing full link

and then the option /fullbuild is shown as having been injected into the linker command line, which presumably overrides the request to link incrementally.

This might be a MSVC linker bug relating to forward slashes in paths passed into the linker. See https://developercommunity.visualstudio.com/content/problem/142544/incremental-linking-regresion-in-154.html

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

Another possibility could be embedding of the manifest in a subsequent step using mt.exe instead of doing it directly using the /MANIFESTINPUT linker switch.

Comment 2

[Nathan Froyd [:froydnj]]

Bas was complaining about this in #developers yesterday, I think?

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

I don't think it can be the manifest (at least in the xul.dll case) because we embed that directly into the resource script, instead of using mt.exe.

Comment 4

[(not currently active) Ted Mielczarek]

dmajor: I remember you talking about incremental linking vs. lld recently, have you seen anything like this?

Comment 5

[(Away)]

(In reply to Aaron Klotz [:aklotz] from comment #0)
> This might be a MSVC linker bug relating to forward slashes in paths passed
> into the linker. See
> https://developercommunity.visualstudio.com/content/problem/142544/
> incremental-linking-regresion-in-154.html

We hacked around that in https://hg.mozilla.org/integration/mozilla-inbound/rev/a1b0ad890d98 But I see that that commit includes a change in expandlibs_exec.py, which has since been reworked.

chmanchester, do you know if the new libs code preserves the `os.path.normpath` workaround?

Comment 6

[(Away)]

Another way to test this hypothesis would be: do you see the same results as bug 1417958 comment 10?

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

Slightly different, but similar:

 0:33.69 LINK :Removed file: ..\..\security\nss\lib\crmf\crmf_crmf\crmf.lib
 0:33.69 LINK :Removed file: ..\..\js\src\build\js_static.lib
 0:33.69 LINK :Removed file: ..\..\toolkit\library\rust\..\i686-pc-windows-msvc\debug\gkrust.lib
 0:33.69 LINK :Removed file: ..\..\mozglue\build\mozglue.lib
 0:33.69 LINK :Removed file: ..\..\security\nss3.lib
 0:33.69 LINK :Removed file: ..\..\config\external\lgpllibs\lgpllibs.lib

Comment 8

[(Away)]

And then later you got "Added" with forward slashes, right?

Comment 9

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to David Major [:dmajor] from comment #8)
> And then later you got "Added" with forward slashes, right?

Interestingly enough, those lines are not present:

 0:28.78 LINK :Removed file: ..\..\security\nss\lib\crmf\crmf_crmf\crmf.lib
 0:28.78 LINK :Removed file: ..\..\js\src\build\js_static.lib
 0:28.78 LINK :Removed file: ..\..\toolkit\library\rust\..\i686-pc-windows-msvc\debug\gkrust.lib
 0:28.78 LINK :Removed file: ..\..\mozglue\build\mozglue.lib
 0:28.78 LINK :Removed file: ..\..\security\nss3.lib
 0:28.78 LINK :Removed file: ..\..\config\external\lgpllibs\lgpllibs.lib
 0:28.80 LINK : 0 new modules and 1 (out of 2544) modules have changed since prior linking
 0:28.80 LINK : object file removed; performing full link
 0:28.87 Invoking LINK.EXE:
 0:28.87  -NOLOGO -DLL -OUT:xul.dll -PDB:xul.pdb -SUBSYSTEM:WINDOWS,6.01 -MACHINE:X86 @c:/Users/dblohm7/src/mc2/obj-ff-prof-dbg/toolkit/library/xul_dll.list ./module.res -verbose -time+ -LARGEADDRESSAWARE -NXCOMPAT -DYNAMICBASE -SAFESEH -DEBUG -DEBUGTYPE:CV -DELAYLOAD:comdlg32.dll -DELAYLOAD:netapi32.dll -DELAYLOAD:secur32.dll -DELAYLOAD:wininet.dll -DELAYLOAD:winspool.drv -DELAYLOAD:oleacc.dll -DELAYLOAD:msdmo.dll -DELAYLOAD:api-ms-win-core-winrt-l1-1-0.dll -DELAYLOAD:api-ms-win-core-winrt-string-l1-1-0.dll ../../security/nss/lib/crmf/crmf_crmf/crmf.lib ../../js/src/build/js_static.lib ../../toolkit/library/rust/../i686-pc-windows-msvc/debug/gkrust.lib ../../mozglue/build/mozglue.lib ../../security/nss3.lib ../../config/external/lgpllibs/lgpllibs.lib kernel32.lib user32.lib gdi32.lib winmm.lib wsock32.lib advapi32.lib secur32.lib wscapi.lib shell32.lib dbghelp.lib hid.lib rpcrt4.lib avrt.lib usp10.lib ole32.lib msimg32.lib winmm.lib Strmiids.lib d3d11.lib dxgi.lib ntdll.lib secur32.lib iphlpapi.lib strmiids.lib dmoguids.lib wmcodecdspuuid.lib amstrmid.lib msdmo.lib wininet.lib mfuuid.lib crypt32.lib version.lib winspool.lib comdlg32.lib imm32.lib netapi32.lib shlwapi.lib ws2_32.lib dwmapi.lib uxtheme.lib setupapi.lib sensorsapi.lib portabledeviceguids.lib wbemuuid.lib wintrust.lib wtsapi32.lib locationapi.lib sapi.lib dxguid.lib oleacc.lib oleaut32.lib delayimp.lib
 0:28.87  /incremental
 0:28.87  /nologo
 0:28.87  /fullbuild

Comment 10

[Chris Manchester (limited bugmail, email directly)]

(In reply to David Major [:dmajor] from comment #5)
> (In reply to Aaron Klotz [:aklotz] from comment #0)
> > This might be a MSVC linker bug relating to forward slashes in paths passed
> > into the linker. See
> > https://developercommunity.visualstudio.com/content/problem/142544/
> > incremental-linking-regresion-in-154.html
> 
> We hacked around that in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a1b0ad890d98 But I
> see that that commit includes a change in expandlibs_exec.py, which has
> since been reworked.
> 
> chmanchester, do you know if the new libs code preserves the
> `os.path.normpath` workaround?

It looks like it didn't for library paths, I can look into this.

Comment 11

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 1455504 - Normalize library paths to work around windows incremental linking bug.

Review commit: https://reviewboard.mozilla.org/r/239510/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/239510/

Comment 12

[(not currently active) Ted Mielczarek]

Comment on attachment 8970759 [details]
Bug 1455504 - Normalize library paths to work around windows incremental linking bug.

https://reviewboard.mozilla.org/r/239510/#review245294

What a pain!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8645b620dad4
Normalize library paths to work around windows incremental linking bug. r=ted

Comment 14

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8970759 [details]
Bug 1455504 - Normalize library paths to work around windows incremental linking bug.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/239510/diff/1-2/

Comment 15

[(No longer employed by Mozilla) Aaron Klotz]

Sorry Chris, I didn't realize you were updating the patch and already pushed it to autoland. :-(

Comment 16

[Chris Manchester (limited bugmail, email directly)]

(In reply to Aaron Klotz [:aklotz] from comment #15)
> Sorry Chris, I didn't realize you were updating the patch and already pushed
> it to autoland. :-(

No worries, I found a silly check test issue on try that took a while to test a fix for, I'll reland shortly...

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b0e196352e17
Normalize library paths to work around windows incremental linking bug. r=ted

Comment 18

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/674b3e7dd3dd
Backed out changeset 8645b620dad4 on request of aklotz.

Comment 19

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/b0e196352e17

Comment 20

[(No longer employed by Mozilla) Aaron Klotz]

It looks like this worked. I've now got xul.dll linking incrementally in ~16 seconds! \o/