Use Windows Vista API to get the user's home directory

[chfast]

Summary

This patch replaces usage of deprecated SHGetFolderPathW with SHGetKnownFolderPath. The usage of SHGetKnownFolderPath is wrapped to allow queries for other "known" folders in the near future.

[gbedwell]

lib/Support/Windows/WindowsSupport.h

29–30 ↗

(On Diff #37412)

Please can you split this part off into its own change?

I've had something similar on my TODO for far too long (sorry!), so I'm very happy to see this being done. Given the previous discussion on Windows versions, I think we should jump straight to Windows 7 API here (we've already said we're doing that in the release notes), and ideally we might accompany this by a check/warning in CMake as a courtesy although I don't feel too strongly about it. I think a check based on ${CMAKE_SYSTEM_VERSION} should work.

[chfast]

Sure, I will do it.

CMake warning should be about "you're building LLVM on a version of Windows that not meet minimum requirements"?
I think the main problem is *running* binaries (built on supported Windows) on not-supported versions of Windows, isn't it?

[gbedwell]

In D13753#267864, @chfast wrote:

CMake warning should be about "you're building LLVM on a version of Windows that not meet minimum requirements"?
I think the main problem is *running* binaries (built on supported Windows) on not-supported versions of Windows, isn't it?

This should be the distinction between CMAKE_HOST_SYSTEM_VERSION (OS version CMake is running on) and CMAKE_SYSTEM_VERSION (The OS version CMake is building for) (according to https://cmake.org/cmake/help/v2.8.12/cmake.html#section_VariablesThatDescribetheSystem ). My fear that it was still possible to build with the CMake option "-T vs120_xp" to specify an XP targeting platform toolset, but having just tried it to see whether it affects CMAKE_SYSTEM_VERSION or not, I've not actually been able to get that far without running into weird CMake errors so I'm not convinced that anyone would be able to use it anyway. As I said, I don't feel too strongly about this. It's been documented for a while that XP will no longer be supported and nobody leapt into the discussion to state that they still cared, so I don't think it's important to present a user friendly CMake-time error. It would just be nice if it turned out to be trivial.

[aaron.ballman]

LGTM, once you split out the switch to Win 7 support.

lib/Support/Windows/WindowsSupport.h

29–30 ↗

(On Diff #37412)

I agree that this should be changd to require at least Windows 7, and as a separate commit. You may also want to see whether we need to change _WIN32_IE as well while we're at it.

[chfast]

In D13753#267883, @gbedwell wrote:

My fear that it was still possible to build with the CMake option "-T vs120_xp" to specify an XP targeting platform toolset, but having just tried it to see whether it affects CMAKE_SYSTEM_VERSION or not, I've not actually been able to get that far without running into weird CMake errors so I'm not convinced that anyone would be able to use it anyway. As I said, I don't feel too strongly about this. It's been documented for a while that XP will no longer be supported and nobody leapt into the discussion to state that they still cared, so I don't think it's important to present a user friendly CMake-time error. It would just be nice if it turned out to be trivial.

I've checked that. -T "v120_xp" option do not modify CMAKE_SYSTEM variables. The toolset option itself can by queried with CMAKE_GENERATOR_TOOLSET variable but only if was explicitly set. Considering all possible combination of options (MSVC, toolset, mingw32, mingw-w64) I would avoid detecting unsupported targeted Windows version in cmake script.

[gbedwell]

In D13753#267904, @chfast wrote:

I've checked that. -T "v120_xp" option do not modify CMAKE_SYSTEM variables. The toolset option itself can by queried with CMAKE_GENERATOR_TOOLSET variable but only if was explicitly set. Considering all possible combination of options (MSVC, toolset, mingw32, mingw-w64) I would avoid detecting unsupported targeted Windows version in cmake script.

Great. Thanks for checking! I absolutely agree with your reasoning, so please do not worry about the CMake check suggestion.

[chfast]

lib/Support/Windows/WindowsSupport.h

29–30 ↗

(On Diff #37412)

_WIN32_IE looks to be needed because mingw fails to define it basic on _WIN32_WINNT? I'm not able to verify that (happily haven't been using mingw for years). I will update it to the value matching Windows 7.

[chfast]

Required Windows API bump committed in as separated change http://reviews.llvm.org/rL250413.

This update also modifies the unit test to handle path containing non-ASCII characters.

[chapuni]

Looks good.

lib/Support/Windows/Path.inc

 
 namespace path {
 
-bool home_directory(SmallVectorImpl<char> &result) {
-  wchar_t Path[MAX_PATH];
-  if (::SHGetFolderPathW(0, CSIDL_APPDATA | CSIDL_FLAG_CREATE, 0,
-                         /*SHGFP_TYPE_CURRENT*/0, Path) != S_OK)
+namespace {
+bool getKnownFolderPath(KNOWNFOLDERID folderId, SmallVectorImpl<char> &result) {
+  wchar_t *path = nullptr;
+  if (::SHGetKnownFolderPath(folderId, KF_FLAG_CREATE, nullptr, &path) != S_OK)
     return false;
 
-  if (UTF16ToUTF8(Path, ::wcslen(Path), result))
-    return false;
+  bool ok = !UTF16ToUTF8(path, ::wcslen(path), result);
+  ::CoTaskMemFree(path);
+  return ok;
+}
+}
 
-  return true;
+bool home_directory(SmallVectorImpl<char> &result) {
+  return getKnownFolderPath(FOLDERID_Profile, result);
 }
 
 static bool getTempDirEnvVar(const char *Var, SmallVectorImpl<char> &Res) {

unittests/Support/Path.cpp

 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/Path.h"
+#include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FileSystem.h"

 }
 
 TEST(Support, HomeDirectory) {
-#ifdef LLVM_ON_UNIX
-  // This test only makes sense on Unix if $HOME is set.
-  if (::getenv("HOME")) {
-#endif
-    SmallString<128> HomeDir;
-    EXPECT_TRUE(path::home_directory(HomeDir));
-    EXPECT_FALSE(HomeDir.empty());
-#ifdef LLVM_ON_UNIX
-  }
+#ifdef LLVM_ON_WIN32
+  wchar_t *path = ::_wgetenv(L"USERPROFILE");
+  auto pathLen = ::wcslen(path);
+  ArrayRef<char> ref{reinterpret_cast<char*>(path), pathLen * sizeof(wchar_t)};
+  std::string expected;
+  convertUTF16ToUTF8String(ref, expected);
+#else
+  std::string expected{::getenv("HOME")};
 #endif
+  SmallString<128> HomeDir;
+  auto status = path::home_directory(HomeDir);
+  EXPECT_TRUE(status ^ HomeDir.empty());
+  EXPECT_EQ(expected, HomeDir);
 }
 
 class FileSystemTest : public testing::Test {