Fix unit tests on Windows: handle env vars with non-ASCII chars.

[chfast]

Summary

On Windows we have to take UTF16 encoded env vars and convert them to UTF8. This patch fixes CopyEnvironment helper function used by process unit tests.

[yaron.keren]

unittests/Support/ProgramTest.cpp

64

Will this unit test never run multithreaded?

[chfast]

unittests/Support/ProgramTest.cpp

64

That is the weak point. What do you think about extracting storage as a function argument?

[yaron.keren]

I see the usage pattern is CopyEnvironment, push_back something, use std::vector<const char *> with ExecuteAndWait. For this you must have a consecutive vector of char * in memory, so someone will have to manage their lifetime. It's better not to leave this to the caller or split responsibility so you could objectize CopyEnvironment to do everything required:

class CopyEnvironment {
  std::vector<const char *> env;
public:
  CopyEnvironment() { copy environment into newly allocated copies, even on unix  }
  push_back() { env.push_back an allocated copy }
  const char *get() { env.push_back(nullptr); return env[0]; }
  ~CopyEnvironment() { release memory }
};

so the usage pattern is

CopyEnvironment CE;
CE.push_back("LLVM_PROGRAM_TEST_LONG_PATH=1");
ExecuteAndWait(... CE.get()...);

this is not very efficient especially on Unix but for a unittest where CopyEnvironment runs just once per test it should not matter at all.

[chfast]

Use local temporary storage for processing env vars on Windows.

[chfast]

Sorry, I missed your comment. I have another revision on the way.

[chfast]

Env vars processing is now implemented using GTest Fixture.

[yaron.keren]

Neat and efficient, LGTM

unittests/Support/ProgramTest.cpp

 //===- unittest/Support/ProgramTest.cpp -----------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
+#include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Program.h"
 #include "gtest/gtest.h"
 #include <stdlib.h>
 #if defined(__APPLE__)
 # include <crt_externs.h>

 using namespace llvm;
 using namespace sys;
 
 static cl::opt<std::string>
 ProgramTestStringArg1("program-test-string-arg1");
 static cl::opt<std::string>
 ProgramTestStringArg2("program-test-string-arg2");
 
-static void CopyEnvironment(std::vector<const char *> &out) {
+static std::vector<const char *>
+CopyEnvironment(std::vector<std::string> &Storage) {
+  std::vector<const char *> out;
+#ifdef LLVM_ON_WIN32
+  // On Windows we have to take UTF16 encoded env vars and convert them to UTF8.
+  _wgetenv(L"TMP"); // Populate _wenviron, initially is null
+  wchar_t **envp = _wenviron;
+  assert(envp);
+  while (*envp != nullptr) {
+    auto pathLen = wcslen(*envp);
+    ArrayRef<char> ref{reinterpret_cast<char const *>(*envp),
+      pathLen * sizeof(wchar_t)};
+    Storage.emplace_back();
+    convertUTF16ToUTF8String(ref, Storage.back());
+    out.emplace_back(Storage.back().c_str());
+    ++envp;
+  }
+#else
+  (void)Storage;
 #ifdef __APPLE__
   char **envp = *_NSGetEnviron();
 #else
-  // environ seems to work for Windows and most other Unices.
   char **envp = environ;
 #endif
   while (*envp != nullptr) {
     out.push_back(*envp);
     ++envp;
   }
+#endif
+  return out;
 }
 
 #ifdef LLVM_ON_WIN32
 TEST(ProgramTest, CreateProcessLongPath) {
   if (getenv("LLVM_PROGRAM_TEST_LONG_PATH"))
     exit(0);
 
   // getMainExecutable returns an absolute path; prepend the long-path prefix.
   std::string MyAbsExe =
       sys::fs::getMainExecutable(TestMainArgv0, &ProgramTestStringArg1);
   std::string MyExe;
   if (!StringRef(MyAbsExe).startswith("\\\\?\\"))
     MyExe.append("\\\\?\\");
   MyExe.append(MyAbsExe);
 
   const char *ArgV[] = {
     MyExe.c_str(),
     "--gtest_filter=ProgramTest.CreateProcessLongPath",
     nullptr
   };
 
   // Add LLVM_PROGRAM_TEST_LONG_PATH to the environment of the child.
-  std::vector<const char *> EnvP;
-  CopyEnvironment(EnvP);
+  std::vector<std::string> EnvStorage;
+  auto EnvP = CopyEnvironment(EnvStorage);
   EnvP.push_back("LLVM_PROGRAM_TEST_LONG_PATH=1");
   EnvP.push_back(nullptr);
 
   // Redirect stdout to a long path.
   SmallString<128> TestDirectory;
   ASSERT_NO_ERROR(
     fs::createUniqueDirectory("program-redirect-test", TestDirectory));
   SmallString<256> LongPath(TestDirectory);
const char *argv[] = {
     my_exe.c_str(),
     "--gtest_filter=ProgramTest.CreateProcessTrailingSlash",
     "-program-test-string-arg1", "has\\\\ trailing\\",
     "-program-test-string-arg2", "has\\\\ trailing\\",
     nullptr
   };
 
   // Add LLVM_PROGRAM_TEST_CHILD to the environment of the child.
-  std::vector<const char *> envp;
-  CopyEnvironment(envp);
+  std::vector<std::string> EnvStorage;
+  auto envp = CopyEnvironment(EnvStorage);
   envp.push_back("LLVM_PROGRAM_TEST_CHILD=1");
   envp.push_back(nullptr);
 
   std::string error;
   bool ExecutionFailed;
   // Redirect stdout and stdin to NUL, but let stderr through.
 #ifdef LLVM_ON_WIN32
   StringRef nul("NUL");
std::string Executable =
       sys::fs::getMainExecutable(TestMainArgv0, &ProgramTestStringArg1);
   const char *argv[] = {
     Executable.c_str(),
     "--gtest_filter=ProgramTest.TestExecuteNoWait",
     nullptr
   };
 
   // Add LLVM_PROGRAM_TEST_EXECUTE_NO_WAIT to the environment of the child.
-  std::vector<const char *> envp;
-  CopyEnvironment(envp);
+  std::vector<std::string> EnvStorage;
+  auto envp = CopyEnvironment(EnvStorage);
   envp.push_back("LLVM_PROGRAM_TEST_EXECUTE_NO_WAIT=1");
   envp.push_back(nullptr);
 
   std::string Error;
   bool ExecutionFailed;
   ProcessInfo PI1 = ExecuteNoWait(Executable, argv, &envp[0], nullptr, 0,
                                   &Error, &ExecutionFailed);
   ASSERT_FALSE(ExecutionFailed) << Error;
std::string Executable =
       sys::fs::getMainExecutable(TestMainArgv0, &ProgramTestStringArg1);
   const char *argv[] = {
     Executable.c_str(),
     "--gtest_filter=ProgramTest.TestExecuteAndWaitTimeout",
     nullptr
   };
 
   // Add LLVM_PROGRAM_TEST_TIMEOUT to the environment of the child.
-  std::vector<const char *> envp;
-  CopyEnvironment(envp);
+  std::vector<std::string> EnvStorage;
+  auto envp = CopyEnvironment(EnvStorage);
   envp.push_back("LLVM_PROGRAM_TEST_TIMEOUT=1");
   envp.push_back(nullptr);
 
   std::string Error;
   bool ExecutionFailed;
   int RetCode =
       ExecuteAndWait(Executable, argv, &envp[0], nullptr, /*secondsToWait=*/1, 0,
                      &Error, &ExecutionFailed);