[Driver] Fix cross compiling with Visual Studio 2017

[hamzasood]

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.

[zturner]

lib/Driver/ToolChains/MSVC.cpp

48–50

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

474

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.

[hamzasood]

Changed the call to GetEnvironmentStrings to use the unicode version

[zturner]

lib/Driver/Job.cpp

321

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

lib/Driver/ToolChains/MSVC.cpp

478

L'\0'

487–488

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

EnvCount+1, for the extra null terminator.

497

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

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

517

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

[zturner]

lib/Driver/ToolChains/MSVC.cpp

517

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]

lib/Driver/ToolChains/MSVC.cpp

487–488

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

517

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]

lib/Driver/ToolChains/MSVC.cpp

487–488

Sorry, I meant unsigned short.

[hamzasood]

• 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.

[thakis]

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.

[hamzasood]

In D30991#702941, @thakis wrote:

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.

[hamzasood]

*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.

[thakis]

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?

[zturner]

In D30991#702965, @thakis wrote:

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?

[zturner]

In D30991#702966, @zturner wrote:

In D30991#702965, @thakis wrote:

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.

[hamzasood]

In D30991#702965, @thakis wrote:

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.

[thakis]

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

[zturner]

Looks good with one more suggested fix.

include/clang/Driver/Job.h

129

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]

@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.

[zturner]

lgtm, do you have commit access?

[hamzasood]

In D30991#703199, @zturner wrote:

lgtm, do you have commit access?

No, I don't.

include/clang/Driver/Job.h

   /// file
   std::string ResponseFileFlag;
 
+  /// See Command::setEnvironment
+  std::vector<const char *> Environment;
+
   /// When a response file is needed, we try to put most arguments in an
   /// exclusive file, while others remains as regular command line arguments.
   /// This functions fills a vector with the regular command line arguments,

     InputFileList = std::move(List);
   }
 
+  /// \brief Sets the environment to be used by the new process.
+  /// \param NewEnvironment A vector of environment variables to be
+  ///        moved into place. The vector will have a null-terminator
+  ///        appended to it; a potential copy can be avoided by ensuring
+  ///        the vector has space for it before calling this function.
+  /// \remark If the environment remains unset, then the environment
+  ///         from this process will be used.
+  /// \sa Command::setEnvironment(const std::vector<const char *> &)
+  void setEnvironment(std::vector<const char *> &&NewEnvironment);
+
+  /// An overload of the above function. It optimises the case where
+  /// the given vector is to be copied in as opposed to moved. 
+  void setEnvironment(const std::vector<const char *> &NewEnvironment);
+
   const char *getExecutable() const { return Executable; }
 
   const llvm::opt::ArgStringList &getArguments() const { return Arguments; }

lib/Driver/Job.cpp

   ResponseFileFlag += FileName;
 }
 
+void Command::setEnvironment(std::vector<const char *> &&NewEnvironment) {
+  Environment = std::move(NewEnvironment);
+  Environment.push_back(nullptr);
+}
+
+void Command::setEnvironment(const std::vector<const char *> &NewEnvironment) {
+  // Reserve space for the null-terminator before copying the input vector.
+  // This removes the possibility of needing a second copy when appending
+  // the null-terminator.
+  Environment.reserve(NewEnvironment.size() + 1);
+  Environment.assign(NewEnvironment.begin(), NewEnvironment.end());
+  Environment.push_back(nullptr);
+}
+
 int Command::Execute(const StringRef **Redirects, std::string *ErrMsg,
                      bool *ExecutionFailed) const {
   SmallVector<const char*, 128> Argv;
 
+  const char **Envp;
+  if (Environment.empty()) {
+    Envp = nullptr;
+  } else {
+    assert(Environment.back() == nullptr 
+           && "Environment vector should be null-terminated by now");
+    Envp = const_cast<const char **>(Environment.data());
+  }
+
   if (ResponseFile == nullptr) {
     Argv.push_back(Executable);
     Argv.append(Arguments.begin(), Arguments.end());
     Argv.push_back(nullptr);
 
-    return llvm::sys::ExecuteAndWait(Executable, Argv.data(), /*env*/ nullptr,
+    return llvm::sys::ExecuteAndWait(Executable, Argv.data(), Envp,
                                      Redirects, /*secondsToWait*/ 0,
                                      /*memoryLimit*/ 0, ErrMsg,
                                      ExecutionFailed);

     return -1;
   }
 
-  return llvm::sys::ExecuteAndWait(Executable, Argv.data(), /*env*/ nullptr,
+  return llvm::sys::ExecuteAndWait(Executable, Argv.data(), Envp,
                                    Redirects, /*secondsToWait*/ 0,
                                    /*memoryLimit*/ 0, ErrMsg, ExecutionFailed);
 }

lib/Driver/ToolChains/MSVC.cpp

 // Include the necessary headers to interface with the Windows registry and
 // environment.
 #if defined(LLVM_ON_WIN32)
   #define USE_WIN32
 #endif
 
 #ifdef USE_WIN32

 #endif
 
 #ifdef _MSC_VER
   // Don't support SetupApi on MinGW.
   #define USE_MSVC_SETUP_API
 
   // Make sure this comes before MSVCSetupApi.h
   #include <comdef.h>
 
   #include "MSVCSetupApi.h"
   #include "llvm/Support/COM.h"
   _COM_SMARTPTR_TYPEDEF(ISetupConfiguration, __uuidof(ISetupConfiguration));
   _COM_SMARTPTR_TYPEDEF(ISetupConfiguration2, __uuidof(ISetupConfiguration2));
   _COM_SMARTPTR_TYPEDEF(ISetupHelper, __uuidof(ISetupHelper));
   _COM_SMARTPTR_TYPEDEF(IEnumSetupInstances, __uuidof(IEnumSetupInstances));
   _COM_SMARTPTR_TYPEDEF(ISetupInstance, __uuidof(ISetupInstance));
   _COM_SMARTPTR_TYPEDEF(ISetupInstance2, __uuidof(ISetupInstance2));
 #endif
 
 using namespace clang::driver;

 
   TC.addProfileRTLibs(Args, CmdArgs);
 
+  std::vector<const char *> Environment;
+
   // We need to special case some linker paths.  In the case of lld, we need to
   // translate 'lld' into 'lld-link', and in the case of the regular msvc
   // linker, we need to use a special search algorithm.

     // from the program PATH, because other environments like GnuWin32 install
     // their own link.exe which may come first.
     linkPath = FindVisualStudioExecutable(TC, "link.exe");
+
+#ifdef USE_WIN32
+    // When cross-compiling with VS2017 or newer, link.exe expects to have
+    // its containing bin directory at the top of PATH, followed by the
+    // native target bin directory.
+    // e.g. when compiling for x86 on an x64 host, PATH should start with:
+    // /bin/HostX64/x86;/bin/HostX64/x64
+    if (TC.getIsVS2017OrNewer() &&
+        llvm::Triple(llvm::sys::getProcessTriple()).getArch() != TC.getArch()) {
+      auto HostArch = llvm::Triple(llvm::sys::getProcessTriple()).getArch();
+
+      auto EnvBlockWide = std::unique_ptr<wchar_t[],
+                                          decltype(&FreeEnvironmentStringsW)>(
+                            GetEnvironmentStringsW(), FreeEnvironmentStringsW);
+      if (!EnvBlockWide)
+        goto SkipSettingEnvironment;
+
+      size_t EnvCount = 0;
+      size_t EnvBlockLen = 0;
+      while (EnvBlockWide[EnvBlockLen] != L'\0') {
+        ++EnvCount;
+        EnvBlockLen += std::wcslen(&EnvBlockWide[EnvBlockLen])
+                       + 1/*string null-terminator*/;
+      }
+      ++EnvBlockLen;  // add the block null-terminator
+
+      std::string EnvBlock;
+      if (!llvm::convertUTF16ToUTF8String(
+             llvm::ArrayRef<char>(reinterpret_cast<char *>(EnvBlockWide.get()),
+                                  EnvBlockLen * sizeof(EnvBlockWide[0])),
+             EnvBlock))
+        goto SkipSettingEnvironment;
+
+      Environment.reserve(EnvCount + 1);
+
+      // Now loop over each string in the block and copy them into the
+      // environment vector, adjusting the PATH variable as needed when we
+      // find it.
+      for (const char *Cursor = EnvBlock.data(); *Cursor != '\0';) {
+        llvm::StringRef EnvVar(Cursor);
+        if (EnvVar.startswith_lower("path=")) {
+          using SubDirectoryType = toolchains::MSVCToolChain::SubDirectoryType;
+          constexpr size_t PrefixLen = 5;  // strlen("path=")
+          Environment.push_back(Args.MakeArgString(
+              EnvVar.substr(0, PrefixLen)
+            + TC.getSubDirectoryPath(SubDirectoryType::Bin)
+            + llvm::Twine(llvm::sys::EnvPathSeparator)
+            + TC.getSubDirectoryPath(SubDirectoryType::Bin, HostArch)
+            + (EnvVar.size() > PrefixLen
+                  ? llvm::Twine(llvm::sys::EnvPathSeparator)
+                    + EnvVar.substr(PrefixLen)
+                  : "")
+          ));
+        } else {
+          Environment.push_back(Args.MakeArgString(EnvVar));
+        }
+        Cursor += EnvVar.size() + 1/*null-terminator*/;
+      }
+    }
+  SkipSettingEnvironment:
+    ;
+#endif
   } else {
     linkPath = Linker;
     llvm::sys::path::replace_extension(linkPath, "exe");

     linkPath = TC.GetProgramPath(linkPath.c_str());
   }
 
-  const char *Exec = Args.MakeArgString(linkPath);
-  C.addCommand(llvm::make_unique<Command>(JA, *this, Exec, CmdArgs, Inputs));
+  auto LinkCmd = llvm::make_unique<Command>(
+                   JA, *this, Args.MakeArgString(linkPath), CmdArgs, Inputs);
+  if (!Environment.empty())
+    LinkCmd->setEnvironment(std::move(Environment));
+  C.addCommand(std::move(LinkCmd));
 }
 
 void visualstudio::Compiler::ConstructJob(Compilation &C, const JobAction &JA,