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 ↗

(On Diff #39041)

Will this unit test never run multithreaded?

[chfast]

unittests/Support/ProgramTest.cpp

64 ↗

(On Diff #39041)

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

llvm/trunk/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) {
-#ifdef __APPLE__
-  char **envp = *_NSGetEnviron();
+class ProgramEnvTest : public testing::Test {
+  std::vector<const char *> EnvTable;
+  std::vector<std::string> EnvStorage;
+
+protected:
+  void SetUp() override {
+    auto EnvP = [] {
+#if defined(LLVM_ON_WIN32)
+      _wgetenv(L"TMP"); // Populate _wenviron, initially is null
+      return _wenviron;
+#elif defined(__APPLE__)
+      return *_NSGetEnviron();
 #else
-  // environ seems to work for Windows and most other Unices.
-  char **envp = environ;
+      return environ;
 #endif
-  while (*envp != nullptr) {
-    out.push_back(*envp);
-    ++envp;
+    }();
+    ASSERT_TRUE(EnvP);
+
+    auto prepareEnvVar = [this](decltype(*EnvP) Var) {
+#if defined(LLVM_ON_WIN32)
+      // On Windows convert UTF16 encoded variable to UTF8
+      auto Len = wcslen(Var);
+      ArrayRef<char> Ref{reinterpret_cast<char const *>(Var),
+                         Len * sizeof(*Var)};
+      EnvStorage.emplace_back();
+      auto convStatus = convertUTF16ToUTF8String(Ref, EnvStorage.back());
+      EXPECT_TRUE(convStatus);
+      return EnvStorage.back().c_str();
+#else
+      return Var;
+#endif
+    };
+
+    while (*EnvP != nullptr) {
+      EnvTable.emplace_back(prepareEnvVar(*EnvP));
+      ++EnvP;
     }
   }
 
+  void TearDown() override {
+    EnvTable.clear();
+    EnvStorage.clear();
+  }
+
+  void addEnvVar(const char *Var) {
+    ASSERT_TRUE(EnvTable.empty() || EnvTable.back()) << "Env table sealed";
+    EnvTable.emplace_back(Var);
+  }
+
+  const char **getEnviron() {
+    if (EnvTable.back() != nullptr)
+      EnvTable.emplace_back(nullptr); // Seal table.
+    return &EnvTable[0];
+  }
+};
+
 #ifdef LLVM_ON_WIN32
-TEST(ProgramTest, CreateProcessLongPath) {
+TEST_F(ProgramEnvTest, 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",
+    "--gtest_filter=ProgramEnvTest.CreateProcessLongPath",
     nullptr
   };
 
   // Add LLVM_PROGRAM_TEST_LONG_PATH to the environment of the child.
-  std::vector<const char *> EnvP;
-  CopyEnvironment(EnvP);
-  EnvP.push_back("LLVM_PROGRAM_TEST_LONG_PATH=1");
-  EnvP.push_back(nullptr);
+  addEnvVar("LLVM_PROGRAM_TEST_LONG_PATH=1");
 
   // Redirect stdout to a long path.
   SmallString<128> TestDirectory;
   ASSERT_NO_ERROR(
     fs::createUniqueDirectory("program-redirect-test", TestDirectory));
   SmallString<256> LongPath(TestDirectory);
   LongPath.push_back('\\');
   // MAX_PATH = 260
   LongPath.append(260 - TestDirectory.size(), 'a');
   StringRef LongPathRef(LongPath);
 
   std::string Error;
   bool ExecutionFailed;
   const StringRef *Redirects[] = { nullptr, &LongPathRef, nullptr };
-  int RC = ExecuteAndWait(MyExe, ArgV, &EnvP[0], Redirects,
+  int RC = ExecuteAndWait(MyExe, ArgV, getEnviron(), Redirects,
     /*secondsToWait=*/ 10, /*memoryLimit=*/ 0, &Error,
     &ExecutionFailed);
   EXPECT_FALSE(ExecutionFailed) << Error;
   EXPECT_EQ(0, RC);
 
   // Remove the long stdout.
   ASSERT_NO_ERROR(fs::remove(Twine(LongPath)));
   ASSERT_NO_ERROR(fs::remove(Twine(TestDirectory)));
 }
 #endif
 
-TEST(ProgramTest, CreateProcessTrailingSlash) {
+TEST_F(ProgramEnvTest, CreateProcessTrailingSlash) {
   if (getenv("LLVM_PROGRAM_TEST_CHILD")) {
     if (ProgramTestStringArg1 == "has\\\\ trailing\\" &&
         ProgramTestStringArg2 == "has\\\\ trailing\\") {
       exit(0);  // Success!  The arguments were passed and parsed.
     }
     exit(1);
   }
 
   std::string my_exe =
       sys::fs::getMainExecutable(TestMainArgv0, &ProgramTestStringArg1);
   const char *argv[] = {
     my_exe.c_str(),
-    "--gtest_filter=ProgramTest.CreateProcessTrailingSlash",
+    "--gtest_filter=ProgramEnvTest.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);
-  envp.push_back("LLVM_PROGRAM_TEST_CHILD=1");
-  envp.push_back(nullptr);
+  addEnvVar("LLVM_PROGRAM_TEST_CHILD=1");
 
   std::string error;
   bool ExecutionFailed;
   // Redirect stdout and stdin to NUL, but let stderr through.
 #ifdef LLVM_ON_WIN32
   StringRef nul("NUL");
 #else
   StringRef nul("/dev/null");
 #endif
   const StringRef *redirects[] = { &nul, &nul, nullptr };
-  int rc = ExecuteAndWait(my_exe, argv, &envp[0], redirects,
+  int rc = ExecuteAndWait(my_exe, argv, getEnviron(), redirects,
                           /*secondsToWait=*/ 10, /*memoryLimit=*/ 0, &error,
                           &ExecutionFailed);
   EXPECT_FALSE(ExecutionFailed) << error;
   EXPECT_EQ(0, rc);
 }
 
-TEST(ProgramTest, TestExecuteNoWait) {
+TEST_F(ProgramEnvTest, TestExecuteNoWait) {
   using namespace llvm::sys;
 
   if (getenv("LLVM_PROGRAM_TEST_EXECUTE_NO_WAIT")) {
     sleep_for(/*seconds*/ 1);
     exit(0);
   }
 
   std::string Executable =
       sys::fs::getMainExecutable(TestMainArgv0, &ProgramTestStringArg1);
   const char *argv[] = {
     Executable.c_str(),
-    "--gtest_filter=ProgramTest.TestExecuteNoWait",
+    "--gtest_filter=ProgramEnvTest.TestExecuteNoWait",
     nullptr
   };
 
   // Add LLVM_PROGRAM_TEST_EXECUTE_NO_WAIT to the environment of the child.
-  std::vector<const char *> envp;
-  CopyEnvironment(envp);
-  envp.push_back("LLVM_PROGRAM_TEST_EXECUTE_NO_WAIT=1");
-  envp.push_back(nullptr);
+  addEnvVar("LLVM_PROGRAM_TEST_EXECUTE_NO_WAIT=1");
 
   std::string Error;
   bool ExecutionFailed;
-  ProcessInfo PI1 = ExecuteNoWait(Executable, argv, &envp[0], nullptr, 0,
+  ProcessInfo PI1 = ExecuteNoWait(Executable, argv, getEnviron(), nullptr, 0,
                                   &Error, &ExecutionFailed);
   ASSERT_FALSE(ExecutionFailed) << Error;
   ASSERT_NE(PI1.Pid, 0) << "Invalid process id";
 
   unsigned LoopCount = 0;
 
   // Test that Wait() with WaitUntilTerminates=true works. In this case,
   // LoopCount should only be incremented once.
   while (true) {
     ++LoopCount;
     ProcessInfo WaitResult = Wait(PI1, 0, true, &Error);
     ASSERT_TRUE(Error.empty());
     if (WaitResult.Pid == PI1.Pid)
       break;
   }
 
   EXPECT_EQ(LoopCount, 1u) << "LoopCount should be 1";
 
-  ProcessInfo PI2 = ExecuteNoWait(Executable, argv, &envp[0], nullptr, 0,
+  ProcessInfo PI2 = ExecuteNoWait(Executable, argv, getEnviron(), nullptr, 0,
                                   &Error, &ExecutionFailed);
   ASSERT_FALSE(ExecutionFailed) << Error;
   ASSERT_NE(PI2.Pid, 0) << "Invalid process id";
 
   // Test that Wait() with SecondsToWait=0 performs a non-blocking wait. In this
   // cse, LoopCount should be greater than 1 (more than one increment occurs).
   while (true) {
     ++LoopCount;
     ProcessInfo WaitResult = Wait(PI2, 0, false, &Error);
     ASSERT_TRUE(Error.empty());
     if (WaitResult.Pid == PI2.Pid)
       break;
   }
 
   ASSERT_GT(LoopCount, 1u) << "LoopCount should be >1";
 }
 
-TEST(ProgramTest, TestExecuteAndWaitTimeout) {
+TEST_F(ProgramEnvTest, TestExecuteAndWaitTimeout) {
   using namespace llvm::sys;
 
   if (getenv("LLVM_PROGRAM_TEST_TIMEOUT")) {
     sleep_for(/*seconds*/ 10);
     exit(0);
   }
 
   std::string Executable =
       sys::fs::getMainExecutable(TestMainArgv0, &ProgramTestStringArg1);
   const char *argv[] = {
     Executable.c_str(),
-    "--gtest_filter=ProgramTest.TestExecuteAndWaitTimeout",
+    "--gtest_filter=ProgramEnvTest.TestExecuteAndWaitTimeout",
     nullptr
   };
 
   // Add LLVM_PROGRAM_TEST_TIMEOUT to the environment of the child.
-  std::vector<const char *> envp;
-  CopyEnvironment(envp);
-  envp.push_back("LLVM_PROGRAM_TEST_TIMEOUT=1");
-  envp.push_back(nullptr);
+ addEnvVar("LLVM_PROGRAM_TEST_TIMEOUT=1");
 
   std::string Error;
   bool ExecutionFailed;
   int RetCode =
-      ExecuteAndWait(Executable, argv, &envp[0], nullptr, /*secondsToWait=*/1, 0,
+      ExecuteAndWait(Executable, argv, getEnviron(), nullptr, /*secondsToWait=*/1, 0,
                      &Error, &ExecutionFailed);
   ASSERT_EQ(-2, RetCode);
 }
 
 TEST(ProgramTest, TestExecuteNegative) {
   std::string Executable = "i_dont_exist";
   const char *argv[] = { Executable.c_str(), nullptr };