[Support] Use GetTempDir to get the temporary dir path on Windows.

[chfast]

Summary

In general GetTempDir follows the same logic as the replaced code: checks env variables TMP, TEMP, USERPROFILE in order. However, it also perform other checks like making separators native (\), making the path absolute, etc.

This change fixes FileSystemTest.CreateDir unittest that had been failing when run from Unix-like shell on Windows (Unix-like path separator (/) used in env variables).

[aaron.ballman]

GetTempPath is unfortunately a broken API.

http://blogs.msdn.com/b/larryosterman/archive/2010/10/19/because-if-you-do_2c00_-stuff-doesn_2700_t-work-the-way-you-intended_2e00_.aspx

Unless this has been fixed in Windows 7 (which I don't know the answer to), I would be hesitant to switch to that API.

[• rafael]

LGTM.

[• rafael]

oops.

Sorry about that.

We should probably add a test with long env var.

[aaron.ballman]

In D14231#282312, @rafael wrote:

oops.

Sorry about that.

We should probably add a test with long env var.

And make sure we test it on the lowest common denominator platform we support (which is Win 7, currently).

[chfast]

Thanks for quick checking.

There is a unit test that checks path with 260 char in it. 260 is the Windows path limit.

[aaron.ballman]

unittests/Support/Path.cpp

366 ↗

(On Diff #38899)

I am slightly worried about this for systems that don't mount a C drive, but I suspect those are passingly rare. ;-)

What has me way more worried is that failing these tests will not restore the TMP (and TEMP, below) environment variables. So if any of these tests fail, we leave the user's machine in a broken state. This can be solved with some RAII wrappers.

Further, changing the temp directory out from under the user affects *every* application on their machine. This means there's a non-zero chance that temp files start going to incorrect locations for other applications. I don't think there's a way to solve this, and I'm really not comfortable with that because it can lead to hard-to-trace problems for developers. Ideas most certainly welcome.

[chfast]

unittests/Support/Path.cpp

366 ↗

(On Diff #38899)

https://msdn.microsoft.com/en-us/library/83zh4e6k.aspx

_putenv and _wputenv affect only the environment that is local to the current process; you cannot use them to modify the command-level environment

So, changing env vars can affect unit tests running in other threads. RAII for restoring original values is worth considering.

386 ↗

(On Diff #38899)

Here is the test that checks system_temp_directory() in case $TMP is 260 chars long.

[aaron.ballman]

unittests/Support/Path.cpp

366 ↗

(On Diff #38899)

That is awesome, and also something I totally did not know. Thank you for pointing that out!

However, a simple test shows there's still a problem -- we have ExecuteAndWait (and other functions) that create subprocesses (possibly in parallel to these tests), almost all of which pass nullptr as the environment variable. When CreateProcess is eventually called with no environment, it inherits the changes made by _wputenv_s(). So this has the possibility to change the environment out from under our own tests, and those tests may in turn try to create temporary files in the wrong place. :-(

My very simple test code was:

#include <iostream>
#include <Windows.h>

void f(const wchar_t *M) {
  wchar_t B[MAX_PATH + 2];
  ::GetTempPathW(MAX_PATH + 2, B);
  std::wcout << M << B << std::endl;
}

int main(int argc, const char *argv[]) {
  if (argc == 1) {
    f(L"Original: ");
    _wputenv_s(L"TMP", L"C:\\OtherFolder");
    STARTUPINFOA si = { 0 };
    si.cb = sizeof(si);
    PROCESS_INFORMATION pi = { 0 };
    ::CreateProcessA(argv[0], "test test test", nullptr, nullptr, TRUE,
                     CREATE_UNICODE_ENVIRONMENT, nullptr, nullptr, &si, &pi);
  } else {
    f(L"Subprocess: ");
  }
}

While this is hopefully not really an issue in reality, I still worry about giving ourselves head-scratching debugging problems that are nondeterministic, as well as cluttering up someone's hard drive with temp files in the wrong locations.

Do these tests currently run serially with regards to other tests (even from lit)? If so, then I think it's a risk we should document (probably around these new tests as well as Execute() in Program.inc for Win32) and just move on despite my discomfort. This seems about as good as we're going to be able to get for testing. If we run in parallel, then I'm a bit more worried.

386 ↗

(On Diff #38899)

Does this test pass on Windows 7 (if possible, with no service packs installed)?

[chfast]

unittests/Support/Path.cpp

366 ↗

(On Diff #38899)

I have an idea how to better solve the env modification issue. If the process can only modify env of itself and its child processes, we should run this test in a separated process.

I've just checked that GTest supports something called Death Tests, that spans a new process to run the test. It is not ideal as combining parallel testing with Death Tests has some issues, but at least GTest will try to inform about such case if detected.
https://code.google.com/p/googletest/wiki/AdvancedGuide#Death_Tests

386 ↗

(On Diff #38899)

I'm not 100% sure, but I think I tested that on Windows 8. However, I have Windows 7 at home so I will test that there to be sure.

If I get https://msdn.microsoft.com/pl-pl/library/windows/desktop/aa383745(v=vs.85).aspx right, there was no API change from Windows 7 to Windows 7 SP1.

[aaron.ballman]

unittests/Support/Path.cpp

366 ↗

(On Diff #38899)

That sounds like a great approach to me!

386 ↗

(On Diff #38899)

If you only have access to SP1, I'm not too worried. I only ask about the base version in case there were changes to the API behavior introduced in a service pack or KB update. If SP1 works, I'm totally fine saying we support Windows 7 with the latest service packs installed for this functionality.

[chfast]

Unit tests updated. They now run in separated process using GTest Death Tests.

[aaron.ballman]

LGTM with one small nit about comments. Thank you for working on this!

lib/Support/Windows/Path.inc

781 ↗

(On Diff #39527)

Good catch.

unittests/Support/Path.cpp

372 ↗

(On Diff #39527)

It would be good to put some comments here as to why this is useful.

[chfast]

It turned out Windows 7 is affected by 130 bytes env var issue. Windows 8 is fine. Sorry for not checking that correctly in the first place as I was supposed to do.

In other words Windows 7 fails unit tests that check 260 char length paths. It there any way we can still safe this patch?

[aaron.ballman]

In D14231#290061, @chfast wrote:

It turned out Windows 7 is affected by 130 bytes env var issue. Windows 8 is fine. Sorry for not checking that correctly in the first place as I was supposed to do.

In other words Windows 7 fails unit tests that check 260 char length paths. It there any way we can still safe this patch?

I'm not certain that we should. When we drop support for Windows 7, then this would obviously be a good patch to bring back. However, in the interim, it may make sense to use the existing code perform the operations we were getting for free with GetTempPath (making separators native (\), making the path absolute, etc.). All of your unit tests are still great, of course.

llvm/trunk/lib/Support/Windows/Path.inc

 bool getUserCacheDir(SmallVectorImpl<char> &Result) {
   return getKnownFolderPath(FOLDERID_LocalAppData, Result);
 }
 
 bool home_directory(SmallVectorImpl<char> &result) {
   return getKnownFolderPath(FOLDERID_Profile, result);
 }
 
-static bool getTempDirEnvVar(const char *Var, SmallVectorImpl<char> &Res) {
-  SmallVector<wchar_t, 128> NameUTF16;
-  if (windows::UTF8ToUTF16(Var, NameUTF16))
-    return false;
-
-  SmallVector<wchar_t, 1024> Buf;
-  size_t Size = 1024;
-  do {
-    Buf.reserve(Size);
-    Size =
-        GetEnvironmentVariableW(NameUTF16.data(), Buf.data(), Buf.capacity());
-    if (Size == 0)
-      return false;
-
-    // Try again with larger buffer.
-  } while (Size > Buf.capacity());
-  Buf.set_size(Size);
-
-  if (windows::UTF16ToUTF8(Buf.data(), Size, Res))
-    return false;
-  return true;
-}
-
-static bool getTempDirEnvVar(SmallVectorImpl<char> &Res) {
-  const char *EnvironmentVariables[] = {"TMP", "TEMP", "USERPROFILE"};
-  for (const char *Env : EnvironmentVariables) {
-    if (getTempDirEnvVar(Env, Res))
-      return true;
-  }
-  return false;
-}
-
 void system_temp_directory(bool ErasedOnReboot, SmallVectorImpl<char> &Result) {
   (void)ErasedOnReboot;
-  Result.clear();
 
-  // Check whether the temporary directory is specified by an environment
-  // variable.
-  if (getTempDirEnvVar(Result))
+  wchar_t Path[MAX_PATH + 2]; // GetTempPath can return MAX_PATH + 1 + null
+  if (auto PathLength = ::GetTempPathW(sizeof(Path) / sizeof(wchar_t), Path)) {
+    assert(PathLength > 0 && PathLength <= (MAX_PATH + 1) &&
+           "GetTempPath returned undocumented result");
+    if (Path[PathLength - 1] == L'\\')
+      --PathLength; // skip trailing "\" added by GetTempPath
+    if (!UTF16ToUTF8(Path, PathLength, Result))
       return;
+  }
 
   // Fall back to a system default.
   const char *DefaultResult = "C:\\TEMP";
+  Result.clear();
   Result.append(DefaultResult, DefaultResult + strlen(DefaultResult));
 }
 } // end namespace path
 
 namespace windows {
 std::error_code UTF8ToUTF16(llvm::StringRef utf8,
                             llvm::SmallVectorImpl<wchar_t> &utf16) {
   if (!utf8.empty()) {

llvm/trunk/unittests/Support/Path.cpp

if (Status) {
     EXPECT_EQ("\xE2\x84\xB5.0", *It2);
     EXPECT_EQ("\xCF\x80r\xC2\xB2", *++It2);
     auto ParentDir2 = *++It2;
 
     EXPECT_EQ(ParentDir, ParentDir2);
   }
 }
 
+TEST(Support, TempDirectory) {
+  SmallString<32> TempDir;
+  path::system_temp_directory(false, TempDir);
+  EXPECT_TRUE(!TempDir.empty());
+  TempDir.clear();
+  path::system_temp_directory(true, TempDir);
+  EXPECT_TRUE(!TempDir.empty());
+}
+
+static std::string path2regex(std::string Path) {
+  size_t Pos = 0;
+  while ((Pos = Path.find('\\', Pos)) != std::string::npos) {
+    Path.replace(Pos, 1, "\\\\");
+    Pos += 2;
+  }
+  return Path;
+}
+
+/// Helper for running temp dir test in separated process. See below.
+#define EXPECT_TEMP_DIR(prepare, expected)                                     \
+  EXPECT_EXIT(                                                                 \
+      {                                                                        \
+        prepare;                                                               \
+        SmallString<300> TempDir;                                              \
+        path::system_temp_directory(true, TempDir);                            \
+        raw_os_ostream(std::cerr) << TempDir;                                  \
+        std::exit(0);                                                          \
+      },                                                                       \
+      ::testing::ExitedWithCode(0), path2regex(expected))
+
+#ifdef LLVM_ON_WIN32
+TEST(SupportDeathTest, TempDirectoryOnWindows) {
+  // In this test we want to check how system_temp_directory responds to
+  // different values of specific env vars. To prevent corrupting env vars of
+  // the current process all checks are done in separated processes.
+  EXPECT_TEMP_DIR(_wputenv_s(L"TMP", L"C:\\OtherFolder"), "C:\\OtherFolder");
+  EXPECT_TEMP_DIR(_wputenv_s(L"TMP", L"C:/Unix/Path/Seperators"),
+                  "C:\\Unix\\Path\\Seperators");
+  EXPECT_TEMP_DIR(_wputenv_s(L"TMP", L"Local Path"), ".+\\Local Path$");
+  EXPECT_TEMP_DIR(
+      _wputenv_s(L"TMP", L"C:\\2\x03C0r-\x00B5\x00B3\\\x2135\x2080"),
+      "C:\\2\xCF\x80r-\xC2\xB5\xC2\xB3\\\xE2\x84\xB5\xE2\x82\x80");
+
+  // Test $TMP empty, $TEMP set.
+  EXPECT_TEMP_DIR(
+      {
+        _wputenv_s(L"TMP", L"");
+        _wputenv_s(L"TEMP", L"C:\\Valid\\Path");
+      },
+      "C:\\Valid\\Path");
+
+  // Test evn var / path with 260 chars.
+  SmallString<270> Expected{"C:\\Temp\\AB\\123456789"};
+  while (Expected.size() < 260)
+    Expected.append("\\DirNameWith19Charss");
+  ASSERT_EQ(260, Expected.size());
+  EXPECT_TEMP_DIR(_putenv_s("TMP", Expected.c_str()), Expected.c_str());
+
+  // Test evn var 261 chars.
+  Expected.append("X");
+  ASSERT_EQ(261, Expected.size());
+  EXPECT_TEMP_DIR(
+      {
+        _putenv_s("TMP", Expected.c_str());
+        _wputenv_s(L"TEMP", L"C:\\Short\\Path");
+      },
+      "C:\\Short\\Path");
+}
+#endif
+
 class FileSystemTest : public testing::Test {
 protected:
   /// Unique temporary directory in which all created filesystem entities must
   /// be placed. It is removed at the end of each test (must be empty).
   SmallString<128> TestDirectory;
 
   void SetUp() override {
     ASSERT_NO_ERROR(