Closed
Bug 1412952
Opened 7 years ago
Closed 7 years ago
Use VS2017 for clang-cl builds
Categories
(Firefox Build System :: General, enhancement)
Firefox Build System
General
Tracking
(firefox58 fixed)
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: away, Assigned: away)
References
Details
Attachments
(5 files)
2.98 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
1.84 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
10.39 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
10.10 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
5.49 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
These are still red with VS2017. We kept the clang-cl builders on VS2015 in order to get bug 1408789 landed sooner.
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)
Updated•7 years ago
|
Attachment #8924576 -
Flags: review?(core-build-config-reviews) → review+
Comment 4•7 years ago
|
||
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 5•7 years ago
|
||
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+
(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)
Comment 8•7 years ago
|
||
(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)
Updated•7 years ago
|
Attachment #8924567 -
Flags: review?(core-build-config-reviews) → review?(nfroyd)
Assignee | ||
Comment 10•7 years ago
|
||
Attachment #8924702 -
Flags: review?(core-build-config-reviews)
Assignee | ||
Comment 11•7 years ago
|
||
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 12•7 years ago
|
||
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 13•7 years ago
|
||
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+
Comment 14•7 years ago
|
||
(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 15•7 years ago
|
||
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+
Assignee | ||
Comment 16•7 years ago
|
||
(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-
Assignee | ||
Comment 17•7 years ago
|
||
(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.
Comment 18•7 years ago
|
||
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
Comment 19•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fb2b705b77cb
https://hg.mozilla.org/mozilla-central/rev/ffb00b457b92
https://hg.mozilla.org/mozilla-central/rev/bcb92693eac8
https://hg.mozilla.org/mozilla-central/rev/dbcddce04a80
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Comment 20•7 years ago
|
||
== 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
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•