This is an archive of the discontinued LLVM Phabricator instance.

[Driver] Fix cross compiling with Visual Studio 2017
ClosedPublic

Authored by hamzasood on Mar 15 2017, 11:26 AM.

Details

Summary
  • Adds a settable environment to clang::driver::Command.
  • Fixes cross compiling with Visual Studio 2017 by ensuring the linker has the correct environment.

This was originally part of https://reviews.llvm.org/D28365, however it was removed for separate review as requested.
Now that the other patch has landed (in https://reviews.llvm.org/D30758), this can now be submitted.

Diff Detail

Repository
rL LLVM

Event Timeline

hamzasood created this revision.Mar 15 2017, 11:26 AM
zturner requested changes to this revision.Mar 15 2017, 11:41 AM
zturner added inline comments.
lib/Driver/ToolChains/MSVC.cpp
48–50 ↗(On Diff #91905)

I think you will need to delete this part (see comment below).

474 ↗(On Diff #91905)

This is all wrong. In the implementation of ExecuteAndWait, we construct a wide environment by calling UTF8toUTF16 on each string. GetEnvironmentStringsA not only doesn't return UTF8, it doesn't even return ANSI characters.

I think you need to do the work to call GetEnvironmentStringsW, then do the reverse of the operation performed in llvm/lib/Support/Windows/Program.inc in the static bool Execute function. It's unfortunate that there's not a version that already takes a wide character array, though.

This revision now requires changes to proceed.Mar 15 2017, 11:41 AM
hamzasood updated this revision to Diff 91962.Mar 15 2017, 5:20 PM
hamzasood edited edge metadata.

Changed the call to GetEnvironmentStrings to use the unicode version

zturner added inline comments.Mar 15 2017, 5:52 PM
lib/Driver/Job.cpp
312 ↗(On Diff #91962)

Can you add assert(Environment.back() == nullptr);?

lib/Driver/ToolChains/MSVC.cpp
478 ↗(On Diff #91962)

L'\0'

487–488 ↗(On Diff #91962)

There's an overload of convertUTF16ToUTF8String that takes an ArrayRef<UTF16>. So I think you can just write this:

if (!llvm::convertUTF16ToUTF8String(makeArrayRef(EnvBlockWide.get(), EnvBlockLen)))
492 ↗(On Diff #91962)

EnvCount+1, for the extra null terminator.

497 ↗(On Diff #91962)

We could avoid the manual index and loop counters if we do something like this:

StringRef Cursor(EnvBlock);
while (!Cursor.empty()) {
  StringRef ThisVar;
  std::tie(ThisVar, Cursor) = Cursor.split('\0');
}
499 ↗(On Diff #91962)

Too bad StringRef doesn't have consume_front_lower(). That would have been perfect here.

517 ↗(On Diff #91962)

I think you need to push 1 more null terminator onto the end here to terminate the block.

zturner added inline comments.Mar 15 2017, 5:54 PM
lib/Driver/ToolChains/MSVC.cpp
517 ↗(On Diff #91962)

Actually if you use the std::tie() algorithm I proposed, then it will enter the body of the loop on the terminating null and push it back (as long as MakeArgString returns nullptr when its argument is empty, which I haven't checked)

hamzasood marked an inline comment as done.Mar 15 2017, 6:14 PM
hamzasood added inline comments.
lib/Driver/ToolChains/MSVC.cpp
487–488 ↗(On Diff #91962)

Using that overload would involve casting wchar_t* to UTF16* (i.e. unsigned char*), which I think breaks aliasing rules.

517 ↗(On Diff #91962)

Command::setEnvironment adds a nullptr onto the end of any vector it's given. I figured it's simpler to do it there than to rely on the caller. Do you think that should be changed?

hamzasood added inline comments.Mar 15 2017, 6:17 PM
lib/Driver/ToolChains/MSVC.cpp
487–488 ↗(On Diff #91962)

Sorry, I meant unsigned short.

hamzasood updated this revision to Diff 92019.Mar 16 2017, 10:46 AM
  • Added an assertion in Command::Execute to ensure the environment vector is null-terminated before using it.
  • Split Command::setEnvironment so that there's an overload to specifically handle the case where the input vector is to be copied in. While this adds a small amount of extra code complexity, it allows us to reduce the worst case from two copies (copy into the function, re-allocate/copy when appending the null-terminator) to just one. Considering the non-trivial size of environment blocks (and that we're already quite inefficient by going from UTF16 -> UTF8 -> UTF16), I think it's worth the few extra lines of code. However it's simple to revert if anyone disagrees.
hamzasood marked 4 inline comments as done.Mar 16 2017, 10:48 AM
thakis added a subscriber: thakis.Mar 16 2017, 10:55 AM

What env vars are needed here? Reading an env file seems a bit inelegant, could we pass the values of these env vars as flags instead?

For example, MSVC2015 needs %INCLUDE%, but for cross-compiling (and for building on Windows without requiring env vars to be set we added the -imsvc flag instead of doing something like this.

What env vars are needed here? Reading an env file seems a bit inelegant, could we pass the values of these env vars as flags instead?

For example, MSVC2015 needs %INCLUDE%, but for cross-compiling (and for building on Windows without requiring env vars to be set we added the -imsvc flag instead of doing something like this.

The windows dynamic linker uses %PATH% to determine the location of dlls to load. link.exe is linked against dlls in a directory that usually wouldn't be in %PATH%, so launching link.exe will fail if the environment isn't modified.

*dynamic loader
And that's the cross-compiling link.exes that have those requirements. The native ones have the required dlls in their containing directories.

When you say "cross-compiling", you mean targeting Windows while running on non-Windows, right? How do dlls get loaded there at all?

Also, when does clang invoke link.exe? Normally on Windows the linker is invoked directly, no through the compiler driver. Are you using clang.exe instead of clang-cl.exe?

When you say "cross-compiling", you mean targeting Windows while running on non-Windows, right? How do dlls get loaded there at all?

Also, when does clang invoke link.exe? Normally on Windows the linker is invoked directly, no through the compiler driver. Are you using clang.exe instead of clang-cl.exe?

I think he means "targeting x86 using an x64 toolchain", but yes, some clarification would be helpful. Also, which DLLs (specifically) are we talking about?

When you say "cross-compiling", you mean targeting Windows while running on non-Windows, right? How do dlls get loaded there at all?

Also, when does clang invoke link.exe? Normally on Windows the linker is invoked directly, no through the compiler driver. Are you using clang.exe instead of clang-cl.exe?

I think he means "targeting x86 using an x64 toolchain", but yes, some clarification would be helpful. Also, which DLLs (specifically) are we talking about?

A quick side-by-side comparison of two directories shows that the following files are missing from the x64_x86 cross compiler folder but are present in the x64_x64 folder:

atlprov.dll
CppCoreCheck.dll
d3dcompiler_47.dll
EspXEngine.dll
ExperimentalCppCoreCheck.dll
LocalESPC.dll
msobj140.dll
mspdb140.dll
mspdbcore.dll
mspdbst.dll
mspft140.dll
msvcdis140.dll
pgort140.dll

I then set up a cross compilation build environment using vcvarsall and the first two entries of my path are (in order):

C:\Program Files (x86)\Microsoft Visual Studio\2017\Professional\VC\Tools\MSVC\14.10.25017\bin\HostX64\x86
C:\Program Files (x86)\Microsoft Visual Studio\2017\Professional\VC\Tools\MSVC\14.10.25017\bin\HostX64\x64

So it looks like this matches up with what he's saying.

When you say "cross-compiling", you mean targeting Windows while running on non-Windows, right? How do dlls get loaded there at all?

Also, when does clang invoke link.exe? Normally on Windows the linker is invoked directly, no through the compiler driver. Are you using clang.exe instead of clang-cl.exe?

Sorry, I should have been more specific. @zturner is correct, I meant using an x64 link.exe to build for x86 (or vice-versa).

The new 2017 toolchains are split up by host architecture and target architecture:
HostX64/x64. Native compilation. Compile for x64 on an x64 host.
HostX64/x86. Cross-compilation. Compile for x86 on an x64 host.
HostX86/x64. Cross-compilation. Compile for x64 on an x86 host
HostX86/x86. Native compilation. Compile for x86 on an x86 host.

The cross compiling toolchains (HostX64/x86, HostX86/x64) are incomplete as they rely on components containing in the corresponding native toolchains (e.g. HostX64/x86 uses components from HostX64/x64).
You can see this in the vcvars bat scripts supplied with Visual Studio. They setup %PATH% in the same way when cross-compiling.
mspdbcore.dll is an example of something missing from the cross-compiling toolchains. There are probably others but I didn't check.

Ah ok, thanks for explaining. In that case, this sounds fine and I'll leave the review to zturner.

Looks good with one more suggested fix.

include/clang/Driver/Job.h
129 ↗(On Diff #92019)

Since it's just a vector of pointers, I don't think this is a very valuable optimization, especially at the risk of complicating the API (I had to think again to recall how overload resolution works with rvalue references and const char*.

Personally I would just have one function, void setEnvironment(ArrayRef<const char*> NewEnvironment); (for example, this allows someone to pass in a SmallVector as well), and not worry about the optimization. More flexibility in the API is preferable to optimizations unless this is a specific bottleneck, which seems unlikely.

hamzasood updated this revision to Diff 92050.Mar 16 2017, 2:04 PM

@zturner: "it's just a vector of pointers"
Oh...... I was so focussed on reducing copies that I completely forgot it's just an array of pointers being copied, not the entire environment block contents. In that case, yeah. It's a dumb optimisation.

  • setEnvironment is now just one function that accepts an ArrayRef.
hamzasood marked an inline comment as done.Mar 16 2017, 2:07 PM
zturner accepted this revision.Mar 16 2017, 2:23 PM

lgtm, do you have commit access?

This revision is now accepted and ready to land.Mar 16 2017, 2:23 PM

lgtm, do you have commit access?

No, I don't.

This revision was automatically updated to reflect the committed changes.