Closed Bug 1455504 Opened 2 years ago Closed 2 years ago

MSVC is always performing full links

Categories

(Firefox Build System :: General, defect)

Unspecified
Windows
defect
Not set

Tracking

(firefox61 fixed)

VERIFIED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: aklotz, Assigned: chmanchester)

References

Details

Attachments

(1 file)

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
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.
Bas was complaining about this in #developers yesterday, I think?
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.
dmajor: I remember you talking about incremental linking vs. lld recently, have you seen anything like this?
(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?
Flags: needinfo?(cmanchester)
Another way to test this hypothesis would be: do you see the same results as bug 1417958 comment 10?
Flags: needinfo?(aklotz)
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
Flags: needinfo?(aklotz)
And then later you got "Added" with forward slashes, right?
(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
(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.
Assignee: nobody → cmanchester
Blocks: 1429875
Flags: needinfo?(cmanchester)
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!
Attachment #8970759 - Flags: review+
Attachment #8970759 - Flags: review?(core-build-config-reviews)
Pushed by aklotz@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8645b620dad4
Normalize library paths to work around windows incremental linking bug. r=ted
Sorry Chris, I didn't realize you were updating the patch and already pushed it to autoland. :-(
(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...
Pushed by cmanchester@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b0e196352e17
Normalize library paths to work around windows incremental linking bug. r=ted
Backout by aiakab@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/674b3e7dd3dd
Backed out changeset 8645b620dad4 on request of aklotz.
https://hg.mozilla.org/mozilla-central/rev/b0e196352e17
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
It looks like this worked. I've now got xul.dll linking incrementally in ~16 seconds! \o/
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.