This is an archive of the discontinued LLVM Phabricator instance.

clang-cl: Invent a /winsysroot concept
ClosedPublic

Authored by thakis on Jan 27 2021, 8:15 AM.

Details

Summary

On non-Windows platforms, --sysroot can be used to make the compiler use
a single, hermetic directory for all header and library files.

This is useful, but difficult to do on Windows. After D95472 it's
possible to achieve this with two flags:

out/gn/bin/clang-cl win.c -fuse-ld=lld \
    /vctoolsdir path/to/VC/Tools/MSVC/14.26.28801 \
    /winsdkdir path/to/win_sdk

But that's still cumbersome: It requires two flags instead of one, and
it requires writing down the (changing) VC/Tools/MSVC version.

This adds a new /winsysroot <dir> flag that's effectively an alias to
these two flags. With this, building against a hermetic Windows
toolchain only needs:

out/gn/bin/clang-cl win.c -fuse-ld=lld /winsysroot path

/winsysroot <dir> is the same as adding

/vctoolsdir <dir>/VC/Tools/MSVC/<vctoolsver>
/winsdkdir <dir>/Windows Kits/<winsdkmajorversion>

<vctoolsver> is taken from /vctoolsversion if passed, or else it's the
name of the directory in <dir>/VC/Tools/MSVC that's the highest numeric
tuple.

<winsdkmajorversion> is the major version in /winsdkversion if passed,
else it's the name of the directory in <dir>/Windows Kits that's
the highest number.

So /winsysroot <path> requires this subfolder structure:

path/
  VC/
    Tools/
      MSVC/
        14.26.28801  (or another number)
          include/
          ...
  Windows Kits/
    10/
      Include/
        10.0.19041.0/ (or another number)
          um/
          ...
      Lib/
        10.0.19041.0/ (or another number)
          um/
            x64/
            ...
          ...

Diff Detail

Event Timeline

thakis created this revision.Jan 27 2021, 8:15 AM
thakis requested review of this revision.Jan 27 2021, 8:15 AM

Maybe we should just go with `Windows Kits/10" -- the space is only seen at the driver->cc1 boundary and it works fine there, and the 10 can be inferred from /winsdkversion.

On the other hand, thinking about how to create such a sysroot directory, saying "just copy %vctoolsinstalldir% to somedir/vctools and %windowssdkdir% to somedir/win_sdk" would be nice too. On the other hand, it'd break relative paths from vc\tools\msvc\XXX to (say) vc\tools\llvm. I don't know if anything needs such relative paths.

"Windows Kits/10" has the drawback that it contains a space and yet another number. Ideally the contents of that directory would be available under some short name, so I went with /winsdkdir <dir>/win_sdk. I'm not married to this though. Maybe <dir>/WinSdk looks move like VC/Tools/MSVC?

FWIW, for my own cross-MSVC setups, I use <dir>/vc/tools/msvc and <dir>/kits/10 - kinda preserving the "Windows Kits" directory, but in a friendlier form - but this proposal is clearly workable too.

thakis updated this revision to Diff 319936.Jan 28 2021, 12:16 PM
thakis edited the summary of this revision. (Show Details)

Seems like there are no strong opinions, so now with actual code. I went with "VC/Tools/MSVC/XXX" and "Windows Kits/10" since that seems like the obvious thing.

thakis edited the summary of this revision. (Show Details)Jan 28 2021, 12:37 PM
hans added inline comments.Jan 28 2021, 12:54 PM
clang/lib/Driver/ToolChains/MSVC.cpp
75

for being defensive, would it be safer to check !EC before comparing the iterators, in case DirIt is in a bad state because of an error?

88

Since the success of the function is directly tied to whether the string is empty, maybe just return the string directly to simplify the API?

Oh I see, this comes from getWindows10SDKVersionFromPath(). Either way is fine I suppose.

1111

EC is unused now

clang/test/Driver/cl-sysroot.cpp
2 ↗(On Diff #319938)

fancy :)

4 ↗(On Diff #319938)

Driver/ tests don't usually run the actual compiler. Any reason not to just check the -### output?

thakis updated this revision to Diff 319949.Jan 28 2021, 1:23 PM
thakis marked an inline comment as done.

address comments

thakis edited the summary of this revision. (Show Details)Jan 28 2021, 1:23 PM
thakis added inline comments.
clang/lib/Driver/ToolChains/MSVC.cpp
75

Done (but this code is just moved up from below)

88

Thanks, that's much nicer.

clang/test/Driver/cl-sysroot.cpp
4 ↗(On Diff #319938)

I had that at first, but it felt a bit brittle to me. But sure, moved back to that.

hans accepted this revision.Jan 29 2021, 12:35 AM

lgtm (with test nit)

clang/test/Driver/cl-sysroot.cpp
10 ↗(On Diff #319949)

could the slashes be backslashes when this test runs on a windows machine?

4 ↗(On Diff #319938)

Thanks! Running the compiler here could be brittle too, since it will run for all backends, and some could get confused by seeing windows as part of the target triple.

This revision is now accepted and ready to land.Jan 29 2021, 12:35 AM
thakis added inline comments.Jan 29 2021, 6:46 AM
clang/test/Driver/cl-sysroot.cpp
10 ↗(On Diff #319949)

Yes, thanks. Even worse, the whole -DFOO=%t approach doesn't work because clang escapes %t for the driver, which double all the slashes etc. Instead gave the toolsdir a number that no real msvc install should use and then captured the root dir via filecheck from there.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJan 29 2021, 6:47 AM

+People who have spent time on cross-compilation Windows sysroots in the past: @smeenai @compnerd

I saw this on LLVM Weekly, and I like it a lot :)

Now if only Windows could case-correct their SDKs so we didn't need VFS overlays and symlinks for the linker...

aganea added a subscriber: aganea.Feb 8 2021, 3:21 PM

Now if only Windows could case-correct their SDKs so we didn't need VFS overlays and symlinks for the linker...

I opened a ticket but it was closed shortly after. Microsoft could easily fix the #includes but the #pragma comment(lib, ..) would require an additionnal layer of OLDNAMES.lib aka /ALTERNATENAME directives when dealing with external libraries, since they already have the (old) #pragma comment(lib, ..) directives already stamped in. Even after making these changes manually for a PoC, we still needed clang-cl ... -Xclang -fixit -Wnonportable-include-path -Wnonportable-system-include-path to fix some paths in third-party libraries - and discovered along the way that -fixit didn't work with PCH files in this specific case. All just a massive pain.

We ended up mounting a 'casefold' filesystem and now it cross-compiles like a charm! :-) (and incidentally it is a bit faster to build than on Windows, on the same hardware)

thakis added a comment.Feb 9 2021, 5:00 AM

I saw this on LLVM Weekly, and I like it a lot :)

Now if only Windows could case-correct their SDKs so we didn't need VFS overlays and symlinks for the linker...

We use ciopfs for this, https://source.chromium.org/chromium/chromium/src/+/master:build/vs_toolchain.py;l=485?q=ciopfs That code has worked without changes for the last 2.5 years.

thakis added a comment.Feb 9 2021, 5:01 AM

We use ciopfs for this, https://source.chromium.org/chromium/chromium/src/+/master:build/vs_toolchain.py;l=485?q=ciopfs That code has worked without changes for the last 2.5 years.

(ciopfs is user-space, so it can be set up transparently by a normal post-pull hook and this doesn't need any manual setup by devs or bots.)

aganea added a comment.EditedFeb 15 2021, 5:42 AM

I saw this on LLVM Weekly, and I like it a lot :)

Now if only Windows could case-correct their SDKs so we didn't need VFS overlays and symlinks for the linker...

We use ciopfs for this, https://source.chromium.org/chromium/chromium/src/+/master:build/vs_toolchain.py;l=485?q=ciopfs That code has worked without changes for the last 2.5 years.

(sorry a bit off topic for LLVM)
Hello @thakis ! What is the best way to report Chrome dev issues? I wanted to try ciopfs, so I followed the guide here, but clang-cl then fails with "Too many files open", pointing to files in chromium/src/third_party/depot_tools/win_toolchain/vs_files/. I've removed the ciopfs mount point and instead created a 'casefold' folder at the same location (ie. chattr +F vs_files), unzipped the SDKs again, and now the build compiles fine. Raising ulimit -n 250000 and fs.file-max = 1000000 didn't help the ciopfs issue in the first place. I'm running on a 48C/96HT machine on Ubuntu 20.04. There seems to be either a leak of FDs or caching inside ciopfs? Any suggestions?