Use VS2017 for clang-cl builds

RESOLVED FIXED in Firefox 58

Status

enhancement
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: dmajor, Assigned: dmajor)

Tracking

unspecified
mozilla58
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(5 attachments)

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.