Closed Bug 1412952 Opened 7 years ago Closed 7 years ago

Use VS2017 for clang-cl builds

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set
normal

Tracking

(firefox58 fixed)

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: away, Assigned: away)

References

Details

Attachments

(5 files)

These are still red with VS2017. We kept the clang-cl builders on VS2015 in order to get bug 1408789 landed sooner.
Depends on: 1413675
In a proper VS install, the path to cl.exe looks like: ...\VC\Tools\MSVC\14.11.25503\bin\HostX64\x64\cl.exe In our automation, the path is just: ...\VC\bin\HostX64\x64\cl.exe Clang tries to do some sanity-checking to make sure that the cl.exe it finds is the Microsoft compiler and not some other program. But the checks are a little too strict for us, so just look for "bin\Host*\*\cl.exe".
Attachment #8924566 - Flags: review?(core-build-config-reviews)
32-bit clang-cl.exe was looking specifically for HostX86\x86\link.exe, which doesn't exist in our automation package. Make it look in HostX64\x86 instead.
Attachment #8924567 - Flags: review?(core-build-config-reviews)
Attachment #8924576 - Flags: review?(core-build-config-reviews)
Attachment #8924576 - Flags: review?(core-build-config-reviews) → review+
Comment on attachment 8924567 [details] [diff] [review] We want a HostX64 linker even with 32-bit clang-cl.exe Review of attachment 8924567 [details] [diff] [review]: ----------------------------------------------------------------- ::: build/build-clang/msvc-host-x64.patch @@ +6,5 @@ > + case SubDirectoryType::Bin: > + if (VSLayout == ToolsetLayout::VS2017OrNewer) { > +- const bool HostIsX64 = > +- llvm::Triple(llvm::sys::getProcessTriple()).isArch64Bit(); > ++ const bool HostIsX64 = true; Why doesn't the original code work in the first place? That seems like a pretty serious bug.
Comment on attachment 8924566 [details] [diff] [review] Loosen clang-cl's MSVC detection to accept our automation's fake paths Review of attachment 8924566 [details] [diff] [review]: ----------------------------------------------------------------- This seems pretty unfortunate, but such is life. ::: build/build-clang/loosen-msvc-detection.patch @@ +7,5 @@ > + // the path. > + // Note: empty strings match anything. > +- llvm::StringRef ExpectedPrefixes[] = {"", "Host", "bin", "", > +- "MSVC", "Tools", "VC"}; > ++ llvm::StringRef ExpectedPrefixes[] = {"", "Host", "bin"}; It might be worth a comment here to indicate the expected vs. actual paths we're matching, and why the original code doesn't work for us?
Attachment #8924566 - Flags: review?(core-build-config-reviews) → review+
ni? for comment 4.
Flags: needinfo?(dmajor)
(In reply to Nathan Froyd [:froydnj] from comment #4) > Why doesn't the original code work in the first place? That seems like a > pretty serious bug. It works with a proper VS install that has the full matrix of (host)x(target) linkers installed. But our automation package contains only the 64-bit tools.
Flags: needinfo?(dmajor)
(In reply to David Major [:dmajor] from comment #7) > (In reply to Nathan Froyd [:froydnj] from comment #4) > > Why doesn't the original code work in the first place? That seems like a > > pretty serious bug. > > It works with a proper VS install that has the full matrix of > (host)x(target) linkers installed. But our automation package contains only > the 64-bit tools. Right, but all we're doing here is: 1) Ask for what we think is the host we're running on; 2) Turn that into an llvm::Triple; 3) Ask if that's 64-bit or not. Which part of that is buggy? It doesn't seem like any of those steps should depend on which host tools we have installed.
Flags: needinfo?(dmajor)
(In reply to Nathan Froyd [:froydnj] from comment #8) > 1) Ask for what we think is the host we're running on; It's not looking at the true host, it's checking the current process type, and this compiler is a 32-bit process. To be honest that bothers me a bit, I think a better approach (for which I am not signing up :)) would be to use a 64-bit clang-cl.exe for everything, and pass -m32 as necessary.
Flags: needinfo?(dmajor)
Attachment #8924567 - Flags: review?(core-build-config-reviews) → review?(nfroyd)
Attachment #8924702 - Flags: review?(core-build-config-reviews)
I had been leaning towards leaving these in just in case we need to revert compilers, but hey, the beauty of source history is we can always bring them back.
Attachment #8924974 - Flags: review?(core-build-config-reviews)
Comment on attachment 8924702 [details] [diff] [review] Build clang-cl itself with VS2017 Review of attachment 8924702 [details] [diff] [review]: ----------------------------------------------------------------- r=me with moving the test_toolchain_configure.py changes to the correct commit. ::: python/mozbuild/mozbuild/test/configure/test_toolchain_configure.py @@ +251,5 @@ > '*.cpp': { > '__STDC_VERSION__': False, > '__cplusplus': '201103L', > }, > + '-fms-compatibility-version=19.11.25547': VS('19.11.25547')[None], All of these test changes should really go in whatever commit changed toolchain.configure, right?
Attachment #8924702 - Flags: review?(core-build-config-reviews) → review+
Comment on attachment 8924974 [details] [diff] [review] Remove the now-unused VS2015 manifests Review of attachment 8924974 [details] [diff] [review]: ----------------------------------------------------------------- I feel like it might be better to leave them in for a release cycle (6-8 weeks) so somebody doesn't have to trawl through `hg log` to figure out what the proper values are supposed to be in case we do find ourselves needing to change, but I will leave that decision up to you. Maybe at least leave them in until mozilla-central is officially 59, so they are at least present on beta in case we find ourselves having to scramble there?
Attachment #8924974 - Flags: review?(core-build-config-reviews) → review+
(In reply to David Major [:dmajor] from comment #9) > (In reply to Nathan Froyd [:froydnj] from comment #8) > > 1) Ask for what we think is the host we're running on; > > It's not looking at the true host, it's checking the current process type, > and this compiler is a 32-bit process. Aha, that was the context I was missing. Maybe we should summarize this in a comment in the patch file itself? > To be honest that bothers me a bit, I think a better approach (for which I > am not signing up :)) would be to use a 64-bit clang-cl.exe for everything, > and pass -m32 as necessary. Yes, this would be a better solution, and should not really be that hard; we already have code in toolchain.configure to add -m32 where necessary. We just don't have a case for clang-cl. Would you please file a followup bug to a) add that support and b) remove this patch?
Comment on attachment 8924567 [details] [diff] [review] We want a HostX64 linker even with 32-bit clang-cl.exe Review of attachment 8924567 [details] [diff] [review]: ----------------------------------------------------------------- r=me, but let's get that followup bug in.
Attachment #8924567 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] from comment #13) > Comment on attachment 8924974 [details] [diff] [review] > Remove the now-unused VS2015 manifests > > Review of attachment 8924974 [details] [diff] [review]: > ----------------------------------------------------------------- > > I feel like it might be better to leave them in for a release cycle (6-8 > weeks) so somebody doesn't have to trawl through `hg log` to figure out what > the proper values are supposed to be in case we do find ourselves needing to > change, but I will leave that decision up to you. Maybe at least leave them > in until mozilla-central is officially 59, so they are at least present on > beta in case we find ourselves having to scramble there? Leaving them in for 58 beta makes sense. But I probably won't remember to revisit this bug, so I'll just leave them in altogether.
Attachment #8924974 - Flags: checkin-
(In reply to Nathan Froyd [:froydnj] from comment #12) > All of these test changes should really go in whatever commit changed > toolchain.configure, right? Hmm, probably. I'll move it.
Blocks: 1414287
Pushed by dmajor@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/fb2b705b77cb Loosen clang's MSVC detection to accept our automation's fake paths. r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/ffb00b457b92 We want a HostX64 linker even with 32-bit clang-cl.exe. r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/bcb92693eac8 Use VS2017 with clang-cl builds. r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/dbcddce04a80 Build clang-cl itself with VS2017. r=froydnj
== Change summary for alert #10448 (as of Thu, 09 Nov 2017 08:02:53 GMT) == Improvements: 2% build times summary windows2012-32 debug static-analysis taskcluster-c4.4xlarge 1,886.39 -> 1,844.34 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=10448
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: