Fix GetEnv to support environment variables with empty values on windows

[bd1976llvm]

Summary

An environment variable can be in one of three states:

1. undefined.
2. defined with a non-empty value.
3. defined but with an empty value.

The windows implementation did not support case 3 (it was not handling errors). The Linux implementation is already correct.

[bd1976llvm]

fixed indentation

[bd1976llvm]

ping

[ruiu]

lib/Support/Windows/Process.inc

142 ↗

(On Diff #110013)

Do you have to handle this as a special case?

[zturner]

lib/Support/Windows/Process.inc

130–140 ↗

(On Diff #110013)

Can we change this logic a bit? You only really have to call this function twice (not in a while loop), because if the first call fails it will tell you the exact size you need. So you only need to check the error code once. So I propose writing this like this:

SmallVector<wchar_t, MAX_PATH> Buf;
Buf.reserve(MAX_PATH);
size_t Size = GetEnvironmentVariableW(NameUTF16.data(), Buf.data(), Buf.capacity());
if (Size == 0 && GetLastError() == ERROR_ENVVAR_NOT_FOUND)
  return std::string();
if (Size < Buf.capacity()) {
  Buf.reserve(Size);
  Size = GetEnvironmentVariableW(NameUTF16.data(), Buf.data(), Buf.capacity());
  assert(Size >= Buf.capacity());
}

SmallVector<char, MAX_PATH> Res;
if (windows::UTF16ToUTF8(Buf.data(), Size, Res))
  return None;
return std::string(Res.data(), Res.size());

149 ↗

(On Diff #110013)

Incidentally, this code won't correctly handle environment variables with embedded nulls. The above code should work though.

[bd1976llvm]

@zturner , @ruiu - thanks for the feedback. I will rewrite, test and update the review.

[bd1976llvm]

lib/Support/Windows/Process.inc

130–140 ↗

(On Diff #110013)

No. Consider the case of a race between the two calls. A while loop is safer.

149 ↗

(On Diff #110013)

I couldn't find a way to get embedded nulls into an environment variable. I don't think this is possible.

[bd1976llvm]

Removed unnecessary return.

[ruiu]

lib/Support/Windows/Process.inc

130–140 ↗

(On Diff #110013)

If there's a race condition, clang must be setting an environment variable and reading that variable in a different thread concurrently (because no one can set another processes's environment varaibles). But is it even safe? I mean, I wonder if setenv and getenv thread-safe. It looks to me that you don't want to guard against it but just say that GetEnv and SetEnv are not thread-safe.

[bd1976llvm]

lib/Support/Windows/Process.inc

130–140 ↗

(On Diff #110013)

They are thread safe. The GO people seem to agree with my proposed implementation:

https://github.com/golang/go/issues/9753
https://go-review.googlesource.com/c/4940/4/src/syscall/env_windows.go#22

[ruiu]

LGTM

lib/Support/Windows/Process.inc

135 ↗

(On Diff #111157)

I believe we usually prefer GetLastError() == ERROR_ENVVAR_NOT_FOUND instead of ERROR_ENVVAR_NOT_FOUND == GetLastError(), just like we did Size == 0 instead of 0 == Size.

[bd1976llvm]

swapped lhs/rhs args in comparison

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

Optional<std::string> Process::GetEnv(StringRef Name) {
 
   // Environment variable can be encoded in non-UTF8 encoding, and there's no
   // way to know what the encoding is. The only reliable way to look up
   // multibyte environment variable is to use GetEnvironmentVariableW().
   SmallVector<wchar_t, MAX_PATH> Buf;
   size_t Size = MAX_PATH;
   do {
     Buf.reserve(Size);
+    SetLastError(NO_ERROR);
     Size =
-        GetEnvironmentVariableW(NameUTF16.data(), Buf.data(), Buf.capacity());
-    if (Size == 0)
+    if (Size == 0 && GetLastError() == ERROR_ENVVAR_NOT_FOUND)
       return None;
 
     // Try again with larger buffer.
   } while (Size > Buf.capacity());
   Buf.set_size(Size);
 
   // Convert the result from UTF-16 to UTF-8.
   SmallVector<char, MAX_PATH> Res;

llvm/trunk/unittests/Support/ProcessTest.cpp

TEST(ProcessTest, Basic) {
   EXPECT_TRUE(val.hasValue());
   EXPECT_STREQ("abc", val->c_str());
 }
 
 TEST(ProcessTest, None) {
   Optional<std::string> val(
       Process::GetEnv("__LLVM_TEST_ENVIRON_NO_SUCH_VAR__"));
   EXPECT_FALSE(val.hasValue());
-}
+} 
 #endif
 
 #ifdef LLVM_ON_WIN32
+
+TEST(ProcessTest, EmptyVal) {
+  SetEnvironmentVariableA("__LLVM_TEST_ENVIRON_VAR__", "");
+  Optional<std::string> val(Process::GetEnv("__LLVM_TEST_ENVIRON_VAR__"));
+  EXPECT_TRUE(val.hasValue());
+  EXPECT_STREQ("", val->c_str());
+}
+
 TEST(ProcessTest, Wchar) {
   SetEnvironmentVariableW(L"__LLVM_TEST_ENVIRON_VAR__", L"abcdefghijklmnopqrs");
   Optional<std::string> val(Process::GetEnv("__LLVM_TEST_ENVIRON_VAR__"));
   EXPECT_TRUE(val.hasValue());
   EXPECT_STREQ("abcdefghijklmnopqrs", val->c_str());
 }
 #endif
 
 } // end anonymous namespace