[PATCH] D30991: [Driver] Fix cross compiling with Visual Studio 2017

2017-03-17 Thread Zachary Turner via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rL298098: [clang-cl] Fix cross-compilation with MSVC 2017. 
(authored by zturner).

Changed prior to commit:
  https://reviews.llvm.org/D30991?vs=92050=92154#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D30991

Files:
  cfe/trunk/include/clang/Driver/Job.h
  cfe/trunk/lib/Driver/Job.cpp
  cfe/trunk/lib/Driver/ToolChains/MSVC.cpp

Index: cfe/trunk/lib/Driver/ToolChains/MSVC.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/MSVC.cpp
+++ cfe/trunk/lib/Driver/ToolChains/MSVC.cpp
@@ -446,6 +446,8 @@
 
   TC.addProfileRTLibs(Args, CmdArgs);
 
+  std::vector 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.
@@ -459,14 +461,77 @@
 // 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(
+  GetEnvironmentStringsW(), FreeEnvironmentStringsW);
+  if (!EnvBlockWide)
+goto SkipSettingEnvironment;
+
+  size_t EnvCount = 0;
+  size_t EnvBlockLen = 0;
+  while (EnvBlockWide[EnvBlockLen] != L'\0') {
+++EnvCount;
+EnvBlockLen += std::wcslen([EnvBlockLen]) +
+   1 /*string null-terminator*/;
+  }
+  ++EnvBlockLen; // add the block null-terminator
+
+  std::string EnvBlock;
+  if (!llvm::convertUTF16ToUTF8String(
+  llvm::ArrayRef(reinterpret_cast(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(JA, *this, Exec, CmdArgs, Inputs));
+  auto LinkCmd = llvm::make_unique(
+  JA, *this, Args.MakeArgString(linkPath), CmdArgs, Inputs);
+  if (!Environment.empty())
+LinkCmd->setEnvironment(Environment);
+  C.addCommand(std::move(LinkCmd));
 }
 
 void visualstudio::Compiler::ConstructJob(Compilation , const JobAction ,
Index: cfe/trunk/lib/Driver/Job.cpp
===
--- cfe/trunk/lib/Driver/Job.cpp
+++ cfe/trunk/lib/Driver/Job.cpp
@@ -301,19 +301,33 @@
   ResponseFileFlag += FileName;
 }
 
+void Command::setEnvironment(llvm::ArrayRef 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 Argv;
 
+  const char **Envp;
+  if (Environment.empty()) {
+Envp = nullptr;
+  } else {
+assert(Environment.back() == nullptr &&
+   "Environment vector

[PATCH] D30991: [Driver] Fix cross compiling with Visual Studio 2017

2017-03-16 Thread Hamza Sood via Phabricator via cfe-commits 
hamzasood added a comment.

In https://reviews.llvm.org/D30991#703199, @zturner wrote:

> lgtm, do you have commit access?


No, I don't.


https://reviews.llvm.org/D30991



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30991: [Driver] Fix cross compiling with Visual Studio 2017

2017-03-16 Thread Zachary Turner via Phabricator via cfe-commits 
zturner accepted this revision.
zturner added a comment.
This revision is now accepted and ready to land.

lgtm, do you have commit access?


https://reviews.llvm.org/D30991



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30991: [Driver] Fix cross compiling with Visual Studio 2017

2017-03-16 Thread Hamza Sood via Phabricator via cfe-commits 
hamzasood updated this revision to Diff 92050.
hamzasood added a comment.

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


https://reviews.llvm.org/D30991

Files:
  include/clang/Driver/Job.h
  lib/Driver/Job.cpp
  lib/Driver/ToolChains/MSVC.cpp

Index: lib/Driver/ToolChains/MSVC.cpp
===
--- lib/Driver/ToolChains/MSVC.cpp
+++ lib/Driver/ToolChains/MSVC.cpp
@@ -34,7 +34,7 @@
 // Include the necessary headers to interface with the Windows registry and
 // environment.
 #if defined(LLVM_ON_WIN32)
-#define USE_WIN32
+  #define USE_WIN32
 #endif
 
 #ifdef USE_WIN32
@@ -47,20 +47,20 @@
 #endif
 
 #ifdef _MSC_VER
-// Don't support SetupApi on MinGW.
-#define USE_MSVC_SETUP_API
+  // Don't support SetupApi on MinGW.
+  #define USE_MSVC_SETUP_API
 
-// Make sure this comes before MSVCSetupApi.h
-#include 
+  // Make sure this comes before MSVCSetupApi.h
+  #include 
 
-#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));
+  #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;
@@ -446,6 +446,8 @@
 
   TC.addProfileRTLibs(Args, CmdArgs);
 
+  std::vector 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.
@@ -459,6 +461,68 @@
 // 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(
+GetEnvironmentStringsW(), FreeEnvironmentStringsW);
+  if (!EnvBlockWide)
+goto SkipSettingEnvironment;
+
+  size_t EnvCount = 0;
+  size_t EnvBlockLen = 0;
+  while (EnvBlockWide[EnvBlockLen] != L'\0') {
+++EnvCount;
+EnvBlockLen += std::wcslen([EnvBlockLen])
+   + 1/*string null-terminator*/;
+  }
+  ++EnvBlockLen;  // add the block null-terminator
+
+  std::string EnvBlock;
+  if (!llvm::convertUTF16ToUTF8String(
+ llvm::ArrayRef(reinterpret_cast(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
+  ?

[PATCH] D30991: [Driver] Fix cross compiling with Visual Studio 2017

2017-03-16 Thread Zachary Turner via Phabricator via cfe-commits 
zturner added a comment.

Looks good with one more suggested fix.




Comment at: include/clang/Driver/Job.h:129
+  /// the given vector is to be copied in as opposed to moved. 
+  void setEnvironment(const std::vector );
+

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


https://reviews.llvm.org/D30991



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30991: [Driver] Fix cross compiling with Visual Studio 2017

2017-03-16 Thread Nico Weber via Phabricator via cfe-commits 
thakis added a comment.

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


https://reviews.llvm.org/D30991



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30991: [Driver] Fix cross compiling with Visual Studio 2017

2017-03-16 Thread Hamza Sood via Phabricator via cfe-commits 
hamzasood added a comment.

In https://reviews.llvm.org/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.


https://reviews.llvm.org/D30991



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30991: [Driver] Fix cross compiling with Visual Studio 2017

2017-03-16 Thread Zachary Turner via Phabricator via cfe-commits 
zturner added a comment.

In https://reviews.llvm.org/D30991#702966, @zturner wrote:

> In https://reviews.llvm.org/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.


https://reviews.llvm.org/D30991



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30991: [Driver] Fix cross compiling with Visual Studio 2017

2017-03-16 Thread Zachary Turner via Phabricator via cfe-commits 
zturner added a comment.

In https://reviews.llvm.org/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?


https://reviews.llvm.org/D30991



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30991: [Driver] Fix cross compiling with Visual Studio 2017

2017-03-16 Thread Nico Weber via Phabricator via cfe-commits 
thakis added a comment.

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?


https://reviews.llvm.org/D30991



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30991: [Driver] Fix cross compiling with Visual Studio 2017

2017-03-16 Thread Hamza Sood via Phabricator via cfe-commits 
hamzasood added a comment.

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


https://reviews.llvm.org/D30991



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30991: [Driver] Fix cross compiling with Visual Studio 2017

2017-03-16 Thread Hamza Sood via Phabricator via cfe-commits 
hamzasood added a comment.

In https://reviews.llvm.org/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.


https://reviews.llvm.org/D30991



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30991: [Driver] Fix cross compiling with Visual Studio 2017

2017-03-16 Thread Nico Weber via Phabricator via cfe-commits 
thakis added a comment.

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.


https://reviews.llvm.org/D30991



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30991: [Driver] Fix cross compiling with Visual Studio 2017

2017-03-16 Thread Hamza Sood via Phabricator via cfe-commits 
hamzasood updated this revision to Diff 92019.
hamzasood added a comment.

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


https://reviews.llvm.org/D30991

Files:
  include/clang/Driver/Job.h
  lib/Driver/Job.cpp
  lib/Driver/ToolChains/MSVC.cpp

Index: lib/Driver/ToolChains/MSVC.cpp
===
--- lib/Driver/ToolChains/MSVC.cpp
+++ lib/Driver/ToolChains/MSVC.cpp
@@ -34,7 +34,7 @@
 // Include the necessary headers to interface with the Windows registry and
 // environment.
 #if defined(LLVM_ON_WIN32)
-#define USE_WIN32
+  #define USE_WIN32
 #endif
 
 #ifdef USE_WIN32
@@ -47,20 +47,20 @@
 #endif
 
 #ifdef _MSC_VER
-// Don't support SetupApi on MinGW.
-#define USE_MSVC_SETUP_API
+  // Don't support SetupApi on MinGW.
+  #define USE_MSVC_SETUP_API
 
-// Make sure this comes before MSVCSetupApi.h
-#include 
+  // Make sure this comes before MSVCSetupApi.h
+  #include 
 
-#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));
+  #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;
@@ -446,6 +446,8 @@
 
   TC.addProfileRTLibs(Args, CmdArgs);
 
+  std::vector 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.
@@ -459,6 +461,68 @@
 // 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(
+GetEnvironmentStringsW(), FreeEnvironmentStringsW);
+  if (!EnvBlockWide)
+goto SkipSettingEnvironment;
+
+  size_t EnvCount = 0;
+  size_t EnvBlockLen = 0;
+  while (EnvBlockWide[EnvBlockLen] != L'\0') {
+++EnvCount;
+EnvBlockLen += std::wcslen([EnvBlockLen])
+   + 1/*string null-terminator*/;
+  }
+  ++EnvBlockLen;  // add the block null-terminator
+
+  std::string EnvBlock;
+  if (!llvm::convertUTF16ToUTF8String(
+ llvm::ArrayRef(reinterpret_cast(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;  //

[PATCH] D30991: [Driver] Fix cross compiling with Visual Studio 2017

2017-03-15 Thread Hamza Sood via Phabricator via cfe-commits 
hamzasood added inline comments.



Comment at: lib/Driver/ToolChains/MSVC.cpp:487-488
+  if (!llvm::convertUTF16ToUTF8String(
+ llvm::ArrayRef(reinterpret_cast(EnvBlockWide.get()),
+  EnvBlockLen * sizeof(EnvBlockWide[0])),
+ EnvBlock))

hamzasood wrote:
> zturner wrote:
> > There's an overload of `convertUTF16ToUTF8String` that takes an 
> > `ArrayRef`.  So I think you can just write this:
> > 
> > ```
> > if (!llvm::convertUTF16ToUTF8String(makeArrayRef(EnvBlockWide.get(), 
> > EnvBlockLen)))
> > ```
> Using that overload would involve casting wchar_t* to UTF16* (i.e. unsigned 
> char*), which I think breaks aliasing rules.
Sorry, I meant unsigned short.


https://reviews.llvm.org/D30991



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30991: [Driver] Fix cross compiling with Visual Studio 2017

2017-03-15 Thread Hamza Sood via Phabricator via cfe-commits 
hamzasood marked an inline comment as done.
hamzasood added inline comments.



Comment at: lib/Driver/ToolChains/MSVC.cpp:487-488
+  if (!llvm::convertUTF16ToUTF8String(
+ llvm::ArrayRef(reinterpret_cast(EnvBlockWide.get()),
+  EnvBlockLen * sizeof(EnvBlockWide[0])),
+ EnvBlock))

zturner wrote:
> There's an overload of `convertUTF16ToUTF8String` that takes an 
> `ArrayRef`.  So I think you can just write this:
> 
> ```
> if (!llvm::convertUTF16ToUTF8String(makeArrayRef(EnvBlockWide.get(), 
> EnvBlockLen)))
> ```
Using that overload would involve casting wchar_t* to UTF16* (i.e. unsigned 
char*), which I think breaks aliasing rules.



Comment at: lib/Driver/ToolChains/MSVC.cpp:517
+  }
+}
+  SkipSettingEnvironment:

zturner wrote:
> zturner wrote:
> > I think you need to push 1 more null terminator onto the end here to 
> > terminate the block.
> 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)
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?


https://reviews.llvm.org/D30991



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30991: [Driver] Fix cross compiling with Visual Studio 2017

2017-03-15 Thread Zachary Turner via Phabricator via cfe-commits 
zturner added inline comments.



Comment at: lib/Driver/ToolChains/MSVC.cpp:517
+  }
+}
+  SkipSettingEnvironment:

zturner wrote:
> I think you need to push 1 more null terminator onto the end here to 
> terminate the block.
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)


https://reviews.llvm.org/D30991



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30991: [Driver] Fix cross compiling with Visual Studio 2017

2017-03-15 Thread Zachary Turner via Phabricator via cfe-commits 
zturner added inline comments.



Comment at: lib/Driver/Job.cpp:312
   SmallVector Argv;
+  auto Envp = Environment.size() > 1
+? const_cast(Environment.data())

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



Comment at: lib/Driver/ToolChains/MSVC.cpp:478
+  size_t EnvBlockLen = 0;
+  while (EnvBlockWide[EnvBlockLen] != '\0') {
+++EnvCount;

`L'\0'`



Comment at: lib/Driver/ToolChains/MSVC.cpp:487-488
+  if (!llvm::convertUTF16ToUTF8String(
+ llvm::ArrayRef(reinterpret_cast(EnvBlockWide.get()),
+  EnvBlockLen * sizeof(EnvBlockWide[0])),
+ EnvBlock))

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

```
if (!llvm::convertUTF16ToUTF8String(makeArrayRef(EnvBlockWide.get(), 
EnvBlockLen)))
```



Comment at: lib/Driver/ToolChains/MSVC.cpp:492
+
+  Environment.reserve(EnvCount);
+

`EnvCount+1`, for the extra null terminator.



Comment at: lib/Driver/ToolChains/MSVC.cpp:497
+  // find it.
+  for (const char *Cursor = EnvBlock.data(); *Cursor != '\0';) {
+llvm::StringRef EnvVar(Cursor);

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');
}
```





Comment at: lib/Driver/ToolChains/MSVC.cpp:499
+llvm::StringRef EnvVar(Cursor);
+if (EnvVar.startswith_lower("path=")) {
+  using SubDirectoryType = toolchains::MSVCToolChain::SubDirectoryType;

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



Comment at: lib/Driver/ToolChains/MSVC.cpp:517
+  }
+}
+  SkipSettingEnvironment:

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


https://reviews.llvm.org/D30991



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30991: [Driver] Fix cross compiling with Visual Studio 2017

2017-03-15 Thread Hamza Sood via Phabricator via cfe-commits 
hamzasood updated this revision to Diff 91962.
hamzasood added a comment.

Changed the call to GetEnvironmentStrings to use the unicode version


https://reviews.llvm.org/D30991

Files:
  include/clang/Driver/Job.h
  lib/Driver/Job.cpp
  lib/Driver/ToolChains/MSVC.cpp

Index: lib/Driver/ToolChains/MSVC.cpp
===
--- lib/Driver/ToolChains/MSVC.cpp
+++ lib/Driver/ToolChains/MSVC.cpp
@@ -45,17 +45,17 @@
   #endif
   #include 
 
-// Make sure this comes before MSVCSetupApi.h
-#include 
+  // Make sure this comes before MSVCSetupApi.h
+  #include 
 
-#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));
+  #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;
@@ -441,6 +441,8 @@
 
   TC.addProfileRTLibs(Args, CmdArgs);
 
+  std::vector 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.
@@ -454,6 +456,68 @@
 // 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(
+GetEnvironmentStringsW(), FreeEnvironmentStringsW);
+  if (!EnvBlockWide)
+goto SkipSettingEnvironment;
+
+  size_t EnvCount = 0;
+  size_t EnvBlockLen = 0;
+  while (EnvBlockWide[EnvBlockLen] != '\0') {
+++EnvCount;
+EnvBlockLen += std::wcslen([EnvBlockLen])
+   + 1/*string null-terminator*/;
+  }
+  ++EnvBlockLen;  // add the block null-terminator
+
+  std::string EnvBlock;
+  if (!llvm::convertUTF16ToUTF8String(
+ llvm::ArrayRef(reinterpret_cast(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");
@@ -460,8 +524,11 @@
 linkPath = TC.GetProgramPath(linkPath.c_str());
   }
 
-  const char *Exec = Args.MakeArgString(linkPath);
-  C.addCommand(llvm::make_unique(JA, *this, Exec, CmdArgs,

[PATCH] D30991: [Driver] Fix cross compiling with Visual Studio 2017

2017-03-15 Thread Zachary Turner via Phabricator via cfe-commits 
zturner requested changes to this revision.
zturner added inline comments.
This revision now requires changes to proceed.



Comment at: lib/Driver/ToolChains/MSVC.cpp:48-50
+  // Undefine this macro so we can call the ANSI version of the function.
+  #undef GetEnvironmentStrings
+  #define GetEnvironmentStringsA GetEnvironmentStrings

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



Comment at: lib/Driver/ToolChains/MSVC.cpp:474
+
+  char *EnvBlock = GetEnvironmentStringsA();
+  if (EnvBlock == nullptr) goto SkipSettingEnvironment;

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.


https://reviews.llvm.org/D30991



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30991: [Driver] Fix cross compiling with Visual Studio 2017

2017-03-15 Thread Hamza Sood via Phabricator via cfe-commits 
hamzasood created this revision.

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


https://reviews.llvm.org/D30991

Files:
  include/clang/Driver/Job.h
  lib/Driver/Job.cpp
  lib/Driver/ToolChains/MSVC.cpp

Index: lib/Driver/ToolChains/MSVC.cpp
===
--- lib/Driver/ToolChains/MSVC.cpp
+++ lib/Driver/ToolChains/MSVC.cpp
@@ -45,17 +45,21 @@
   #endif
   #include 
 
-// Make sure this comes before MSVCSetupApi.h
-#include 
+  // Undefine this macro so we can call the ANSI version of the function.
+  #undef GetEnvironmentStrings
+  #define GetEnvironmentStringsA GetEnvironmentStrings
 
-#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));
+  // Make sure this comes before MSVCSetupApi.h
+  #include 
+
+  #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;
@@ -441,6 +445,8 @@
 
   TC.addProfileRTLibs(Args, CmdArgs);
 
+  std::vector 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.
@@ -454,6 +460,59 @@
 // 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
+if (TC.getIsVS2017OrNewer()) {
+  // 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
+  llvm::Triple Host(llvm::sys::getProcessTriple());
+  if (Host.getArch() == TC.getArch()) goto SkipSettingEnvironment;
+
+  char *EnvBlock = GetEnvironmentStringsA();
+  if (EnvBlock == nullptr) goto SkipSettingEnvironment;
+
+  size_t EnvCount = 0;
+  for (const char *Cursor = EnvBlock;
+   *Cursor != '\0';
+   Cursor += strlen(Cursor) + 1/*null-terminator*/)
+++EnvCount;
+
+  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; *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, Host.getArch())
++ (EnvVar.size() > PrefixLen
+ ? llvm::Twine(llvm::sys::EnvPathSeparator)
+   + EnvVar.substr(PrefixLen)
+ : "")
+  ));
+}
+else {
+  Environment.push_back(Args.MakeArgString(EnvVar));
+}
+Cursor += EnvVar.size() + 1/*null-terminator*/;
+  }
+
+  FreeEnvironmentStringsA(EnvBlock);
+
+SkipSettingEnvironment:
+  ;
+}
+#endif
   } else {
 linkPath = Linker;
 llvm::sys::path::replace_extension(linkPath, "exe");
@@ -460,8 +519,11 @@
 linkPath = TC.GetProgramPath(linkPath.c_str());
   }
 
-  const char *Exec = Args.MakeArgString(linkPath);
-  C.addCommand(llvm::make_unique(JA, *this, Exec, CmdArgs, Inputs));
+  auto LinkCmd = llvm::make_unique(
+   JA,