Merge pull request #3341 from kinke/msvcEnv

Windows: Do not leak MSVC-environment-setup into `-run` child processes

.azure-pipelines/windows.yml

@@ -134,7 +134,7 @@ steps:
     cd ..
     set PATH=%CD%\ninja;%PATH%
     cd build
-    ninja install > nul
+    ninja install > nul || exit /b
     cd ..
     powershell -c "(cat install/etc/ldc2.conf).replace('%CD:\=/%/install/', '%%%%ldcbinarypath%%%%/../') | Set-Content install/etc/ldc2.conf"
     cat install/etc/ldc2.conf
@@ -178,7 +178,7 @@ steps:
     git clone --recursive https://github.com/dlang/dub.git 2>&1
     cd dub
     powershell -c "git checkout \"$(cat %BUILD_SOURCESDIRECTORY%/packaging/dub_version -Raw)\"" 2>&1
-    call build.cmd 2>&1
+    call build.cmd 2>&1 || exit /b
     cp bin/dub.exe ../installed/bin
     ..\installed\bin\dub --version
   displayName: Build & copy dub
@@ -187,9 +187,9 @@ steps:
     call "%VSINSTALLDIR%Common7\Tools\VsDevCmd.bat" -arch=%ARCH%
     git clone --recursive https://github.com/dlang/tools.git dlang-tools 2>&1
     cd dlang-tools
-    ..\installed\bin\ldmd2 -w -de -dip1000 rdmd.d
-    ..\installed\bin\ldmd2 -w -de -dip1000 ddemangle.d
-    ..\installed\bin\ldmd2 -w -de -dip1000 DustMite\dustmite.d DustMite\splitter.d
+    ..\installed\bin\ldmd2 -w -de -dip1000 rdmd.d || exit /b
+    ..\installed\bin\ldmd2 -w -de -dip1000 ddemangle.d || exit /b
+    ..\installed\bin\ldmd2 -w -de -dip1000 DustMite\dustmite.d DustMite\splitter.d || exit /b
     cp *.exe ../installed/bin
   displayName: Build & copy dlang tools
 - script: |

azure-pipelines.yml

@@ -42,9 +42,6 @@ jobs:
     vmImage: 'windows-2019'
   dependsOn:
     - Windows
-  variables:
-    # Let LDC auto-detect the Visual C++ installation for the smoke tests.
-    LDC_VSDIR: C:\non-existing_dummy
   steps:
     - checkout: none
     - task: DownloadPipelineArtifact@2
@@ -85,14 +82,24 @@ jobs:
         cat etc\ldc2.conf
       displayName: Merge ldc2.conf
     - powershell: |
-        echo 'void main() { import std.stdio; writefln("Hello world, %d bits", size_t.sizeof * 8); }' > hello.d
+        $hello  = "void main() {`n"
+        $hello += "  import std.process, std.stdio;`n"
+        $hello += "  writefln(""Hello world, %d bits"", size_t.sizeof * 8);`n"
+        $hello += "  assert(!environment.get(""VSINSTALLDIR""));`n"
+        $hello += "}`n"
+        echo "$hello" > hello.d
       displayName: Generate hello.d
     - script: |
-        %ARTIFACT_NAME%\bin\ldc2 -run hello.d
-      displayName: Run 64-bit hello-world smoke test
+        echo on
+        %ARTIFACT_NAME%\bin\ldc2 -v -run hello.d || exit /b
+        %ARTIFACT_NAME%\bin\ldc2 -v -m32 -run hello.d
+      displayName: Run 32/64-bit hello-world smoke test with internal toolchain
     - script: |
-        %ARTIFACT_NAME%\bin\ldc2 -m32 -run hello.d
-      displayName: Run 32-bit hello-world smoke test
+        echo on
+        set LDC_VSDIR_FORCE=1
+        %ARTIFACT_NAME%\bin\ldc2 -v -run hello.d || exit /b
+        %ARTIFACT_NAME%\bin\ldc2 -v -m32 -run hello.d
+      displayName: Run 32/64-bit hello-world smoke test with MSVC auto-detection
     - script: |
         mkdir newArtifacts
         7z a -mx=9 newArtifacts/%ARTIFACT_NAME%.7z %ARTIFACT_NAME% > nul

driver/archiver.cpp

@@ -266,14 +266,18 @@ int createStaticLibrary() {
 
   const bool useInternalArchiver = ar.empty();
 
+#ifdef _WIN32
+  windows::MsvcEnvironmentScope msvcEnv;
+#endif
+
   // find archiver
   std::string tool;
   if (useInternalArchiver) {
     tool = isTargetMSVC ? "llvm-lib" : "llvm-ar";
   } else {
 #ifdef _WIN32
     if (isTargetMSVC)
-      windows::setupMsvcEnvironment();
+      msvcEnv.setup();
 #endif
 
     tool = getProgram(isTargetMSVC ? "lib.exe" : "ar", &ar);

driver/linker-msvc.cpp

@@ -88,8 +88,9 @@ int linkObjToBinaryMSVC(llvm::StringRef outputPath,
   const bool useInternalToolchain = useInternalToolchainForMSVC();
 
 #ifdef _WIN32
+  windows::MsvcEnvironmentScope msvcEnv;
   if (!useInternalToolchain)
-    windows::setupMsvcEnvironment();
+    msvcEnv.setup();
 #endif
 
   // build arguments

driver/linker.cpp

@@ -188,7 +188,10 @@ bool useInternalToolchainForMSVC() {
 #ifndef _WIN32
   return true;
 #else
-  return !env::has(L"VSINSTALLDIR") && !env::has(L"LDC_VSDIR");
+  return !env::has(L"VSINSTALLDIR") && !env::has(L"LDC_VSDIR") &&
+         // LDC_VSDIR_FORCE alone can be used to prefer MSVC toolchain
+         // auto-detection over the internal toolchain.
+         !env::has(L"LDC_VSDIR_FORCE");
 #endif
 }
 

driver/tool.cpp

@@ -77,9 +77,7 @@ std::string getProgram(const char *name, const llvm::cl::opt<std::string> *opt,
 
 ////////////////////////////////////////////////////////////////////////////////
 
-std::string getGcc() {
-  return getProgram("cc", &gcc, "CC");
-}
+std::string getGcc() { return getProgram("cc", &gcc, "CC"); }
 
 ////////////////////////////////////////////////////////////////////////////////
 
@@ -265,7 +263,7 @@ std::string quoteArg(llvm::StringRef arg) {
   return quotedArg;
 }
 
-int executeAndWait(const char *commandLine) {
+int executeAndWait(const char *commandLine, DWORD creationFlags = 0) {
   STARTUPINFO si;
   ZeroMemory(&si, sizeof(si));
   si.cb = sizeof(si);
@@ -279,8 +277,8 @@ int executeAndWait(const char *commandLine) {
   if (llvm::sys::windows::UTF8ToUTF16(commandLine, wcommandLine))
     return -3;
   wcommandLine.push_back(0);
-  if (!CreateProcessW(nullptr, wcommandLine.data(), nullptr, nullptr, TRUE, 0,
-                      nullptr, nullptr, &si, &pi)) {
+  if (!CreateProcessW(nullptr, wcommandLine.data(), nullptr, nullptr, TRUE,
+                      creationFlags, nullptr, nullptr, &si, &pi)) {
     exitCode = -1;
   } else {
     if (WaitForSingleObject(pi.hProcess, INFINITE) != 0 ||
@@ -294,8 +292,9 @@ int executeAndWait(const char *commandLine) {
   return exitCode;
 }
 
-bool setupMsvcEnvironmentImpl() {
-  if (env::has(L"VSINSTALLDIR"))
+bool setupMsvcEnvironmentImpl(
+    std::vector<std::pair<std::wstring, wchar_t *>> &rollback) {
+  if (env::has(L"VSINSTALLDIR") && !env::has(L"LDC_VSDIR_FORCE"))
     return true;
 
   llvm::SmallString<128> tmpFilePath;
@@ -325,14 +324,17 @@ bool setupMsvcEnvironmentImpl() {
   llvm::SmallString<512> commandLine;
   commandLine += quoteArg(cmdExecutable);
   commandLine += " /s /c \"";
+  commandLine += "chcp 65001 && "; // => UTF-8 output encoding
   commandLine += quoteArg(batchFile);
   commandLine += ' ';
   commandLine += arch;
   commandLine += " > ";
   commandLine += quoteArg(tmpFilePath);
   commandLine += '"';
 
-  const int exitCode = executeAndWait(commandLine.c_str());
+  // do NOT pass down our console
+  const DWORD procCreationFlags = CREATE_NO_WINDOW;
+  const int exitCode = executeAndWait(commandLine.c_str(), procCreationFlags);
   if (exitCode != 0) {
     error(Loc(), "'%s' failed with status: %d", commandLine.c_str(), exitCode);
     llvm::sys::fs::remove(tmpFilePath);
@@ -377,16 +379,15 @@ bool setupMsvcEnvironmentImpl() {
   if (global.params.verbose)
     message("Applying environment variables:");
 
+  rollback.reserve(env.size());
   bool haveVsInstallDir = false;
 
   for (const auto &pair : env) {
     const llvm::StringRef key = pair.first;
     const llvm::StringRef value = pair.second;
 
-    if (global.params.verbose) {
-      message("  %.*s=%.*s", (int)key.size(), key.data(), (int)value.size(),
-              value.data());
-    }
+    if (key == "VSINSTALLDIR")
+      haveVsInstallDir = true;
 
     llvm::SmallVector<wchar_t, 32> wkey;
     llvm::SmallVector<wchar_t, 512> wvalue;
@@ -395,22 +396,41 @@ bool setupMsvcEnvironmentImpl() {
     wkey.push_back(0);
     wvalue.push_back(0);
 
-    SetEnvironmentVariableW(wkey.data(), wvalue.data());
+    wchar_t *originalValue = _wgetenv(wkey.data());
+    if (originalValue && wcscmp(originalValue, wvalue.data()) == 0)
+      continue; // no change
 
-    if (key == "VSINSTALLDIR")
-      haveVsInstallDir = true;
+    if (global.params.verbose) {
+      message("  %.*s=%.*s", (int)key.size(), key.data(), (int)value.size(),
+              value.data());
+    }
+
+    // copy the original value, if set
+    if (originalValue)
+      originalValue = wcsdup(originalValue);
+    rollback.emplace_back(wkey.data(), originalValue);
+
+    SetEnvironmentVariableW(wkey.data(), wvalue.data());
   }
 
   return haveVsInstallDir;
 }
 
-bool setupMsvcEnvironment() {
-  const bool success = setupMsvcEnvironmentImpl();
+bool MsvcEnvironmentScope::setup() {
+  rollback.clear();
+  const bool success = setupMsvcEnvironmentImpl(rollback);
   if (!success)
     warning(Loc(), "no Visual C++ installation detected");
   return success;
 }
 
+MsvcEnvironmentScope::~MsvcEnvironmentScope() {
+  for (const auto &pair : rollback) {
+    SetEnvironmentVariableW(pair.first.c_str(), pair.second);
+    free(pair.second);
+  }
+}
+
 } // namespace windows
 
 #endif // _WIN32

driver/tool.h

@@ -44,9 +44,18 @@ int executeToolAndWait(const std::string &tool,
 #ifdef _WIN32
 
 namespace windows {
-// Tries to set up the MSVC environment variables and returns true if
-// successful.
-bool setupMsvcEnvironment();
+struct MsvcEnvironmentScope {
+  // Tries to set up the MSVC environment variables for the current process and
+  // returns true if successful. The original environment is restored on
+  // destruction.
+  bool setup();
+
+  ~MsvcEnvironmentScope();
+
+private:
+  // for each changed env var: name & original value
+  std::vector<std::pair<std::wstring, wchar_t *>> rollback;
+};
 }
 
 #endif

packaging/README.txt

@@ -20,15 +20,16 @@ following options:
 * Run LDC in a 'VS Native/Cross Tools Command Prompt' (LDC checks whether the
   VSINSTALLDIR environment variable is set).
   LDC assumes the environment variables are all set up appropriately.
-* Set the LDC_VSDIR environment variable to some Visual Studio/Visual C++ Build
-  Tools installation directory, e.g.,
+* Or set the LDC_VSDIR environment variable to some Visual Studio/Visual C++
+  Build Tools installation directory, e.g.,
   'C:\Program Files (x86)\Microsoft Visual Studio\2017\Community'.
   LDC will invoke a batch file provided by VS to set up the environment
   variables for the selected 32/64-bit target platform, which adds an overhead
   of about 1 second for each linking operation.
-  You can also set LDC_VSDIR to some non-existing dummy path; LDC will try to
-  auto-detect your latest Visual C++ installation in that case.
-* Set up the etc\ldc2.conf config file and specify the directories containing
+  You can also set LDC_VSDIR_FORCE (to some non-empty value); LDC will then try
+  to auto-detect your latest Visual C++ installation if you haven't set
+  LDC_VSDIR, and won't skip the environment setup if VSINSTALLDIR is pre-set.
+* Or set up the etc\ldc2.conf config file and specify the directories containing
   the MS libs (appending them to the 'lib-dirs' array; check out the LIB
   environment variable in a VS tools command prompt) as well as the C runtime
   flavor (e.g., appending '-mscrtlib=libcmt' to the 'switches' array).

vcbuild/msvcEnv.bat

@@ -1,8 +1,5 @@
 @echo off
 
-:: Environment already set up?
-if not "%VSINSTALLDIR%"=="" goto :eof
-
 :: Skip detection if an existing LDC_VSDIR environment variable points to an existing folder
 if "%LDC_VSDIR%"=="" goto detect
 if not "%LDC_VSDIR:~-1%"=="\" set LDC_VSDIR=%LDC_VSDIR%\