[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 ↗

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

[hamzasood]

Changed the call to GetEnvironmentStrings to use the unicode version

[zturner]

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]

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]

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]

lib/Driver/ToolChains/MSVC.cpp

487–488 ↗

(On Diff #91962)

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 ↗

(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]

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

cfe/trunk/include/clang/Driver/Job.h

 //===--- Job.h - Commands to Execute ----------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_DRIVER_JOB_H
 #define LLVM_CLANG_DRIVER_JOB_H
 
 #include "clang/Basic/LLVM.h"
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/iterator.h"
 #include "llvm/Option/Option.h"
 #include <memory>
 
 namespace llvm {
   class raw_ostream;
 }
class Command {
   /// The input file list in case we need to emit a file list instead of a
   /// proper response file
   llvm::opt::ArgStringList InputFileList;
 
   /// String storage if we need to create a new argument to specify a response
   /// 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,
   /// argv, excluding the ones passed in a response file.
   void buildArgvForResponseFile(llvm::SmallVectorImpl<const char *> &Out) const;
 
   /// Encodes an array of C strings into a single string separated by whitespace.
   /// This function will also put in quotes arguments that have whitespaces and
public:
   void setResponseFile(const char *FileName);
 
   /// Set an input file list, necessary if we need to use a response file but
   /// the tool being called only supports input files lists.
   void setInputFileList(llvm::opt::ArgStringList List) {
     InputFileList = std::move(List);
   }
 
+  /// \brief Sets the environment to be used by the new process.
+  /// \param NewEnvironment An array of environment variables.
+  /// \remark If the environment remains unset, then the environment
+  ///         from the parent process will be used.
+  void setEnvironment(llvm::ArrayRef<const char *> NewEnvironment);
+
   const char *getExecutable() const { return Executable; }
 
   const llvm::opt::ArgStringList &getArguments() const { return Arguments; }
 
   /// Print a command argument, and optionally quote it.
   static void printArg(llvm::raw_ostream &OS, StringRef Arg, bool Quote);
 };
 

cfe/trunk/lib/Driver/Job.cpp

 }
 
 void Command::setResponseFile(const char *FileName) {
   ResponseFile = FileName;
   ResponseFileFlag = Creator.getResponseFileFlag();
   ResponseFileFlag += FileName;
 }
 
+void Command::setEnvironment(llvm::ArrayRef<const char *> NewEnvironment) {
+  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,
-                                     Redirects, /*secondsToWait*/ 0,
-                                     /*memoryLimit*/ 0, ErrMsg,
-                                     ExecutionFailed);
+    return llvm::sys::ExecuteAndWait(
+        Executable, Argv.data(), Envp, Redirects, /*secondsToWait*/ 0,
+        /*memoryLimit*/ 0, ErrMsg, ExecutionFailed);
   }
 
   // We need to put arguments in a response file (command is too large)
   // Open stream to store the response file contents
   std::string RespContents;
   llvm::raw_string_ostream SS(RespContents);
 
   // Write file contents and build the Argv vector
   writeResponseFile(SS);
   buildArgvForResponseFile(Argv);
   Argv.push_back(nullptr);
   SS.flush();
 
   // Save the response file in the appropriate encoding
   if (std::error_code EC = writeFileWithEncoding(
           ResponseFile, RespContents, Creator.getResponseFileEncoding())) {
     if (ErrMsg)
       *ErrMsg = EC.message();
     if (ExecutionFailed)
       *ExecutionFailed = true;
     return -1;
   }
 
-  return llvm::sys::ExecuteAndWait(Executable, Argv.data(), /*env*/ nullptr,
-                                   Redirects, /*secondsToWait*/ 0,
+  return llvm::sys::ExecuteAndWait(Executable, Argv.data(), Envp, Redirects,
+                                   /*secondsToWait*/ 0,
                                    /*memoryLimit*/ 0, ErrMsg, ExecutionFailed);
 }
 
 FallbackCommand::FallbackCommand(const Action &Source_, const Tool &Creator_,
                                  const char *Executable_,
                                  const ArgStringList &Arguments_,
                                  ArrayRef<InputInfo> Inputs,
                                  std::unique_ptr<Command> Fallback_)

cfe/trunk/lib/Driver/ToolChains/MSVC.cpp

for (const auto &Input : Inputs) {
 
     // Otherwise, this is some other kind of linker input option like -Wl, -z,
     // or -L. Render it, even if MSVC doesn't understand it.
     A.renderAsInput(Args, CmdArgs);
   }
 
   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.
   llvm::SmallString<128> linkPath;
   StringRef Linker = Args.getLastArgValue(options::OPT_fuse_ld_EQ, "link");
   if (Linker.equals_lower("lld"))
     Linker = "lld-link";
 
   if (Linker.equals_lower("link")) {
     // If we're using the MSVC linker, it's not sufficient to just use link
     // 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);
+
+      // 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(Environment);
+  C.addCommand(std::move(LinkCmd));
 }
 
 void visualstudio::Compiler::ConstructJob(Compilation &C, const JobAction &JA,
                                           const InputInfo &Output,
                                           const InputInfoList &Inputs,
                                           const ArgList &Args,
                                           const char *LinkingOutput) const {
   C.addCommand(GetCommand(C, JA, Output, Inputs, Args, LinkingOutput));