[Support] [Windows] Convert paths to the preferred form

[mstorsjo]

Summary

This normalizes most paths (except ones input from the user as command
line arguments) into the preferred form, if real_style() evaluates to
Windows_forward.

[mstorsjo]

Updated after rebasing and reworking the earlier commits.

[mstorsjo]

Make sure to force paths to backslashes in widenPath if they are passed with the absolute long path prefix \\?\ but with forward slashes, i.e. as //?/.

[aaron.ballman]

llvm/lib/Support/Windows/Path.inc

79–80

I'm assuming we purposefully don't care about complete nonsense like \/?\ not getting normalized into \\?\?

We should add tests for both //?/ and the nonsense form.

[mstorsjo]

llvm/lib/Support/Windows/Path.inc

79–80

Thanks for the suggested edit, yeah that's nicer.

I added a test for //?/ in llvm/unittests/Support/Path.cpp, lines 2296-2305, but I guess we could add a test for the nonsense form to clarify that case too.

[aaron.ballman]

llvm/lib/Support/Windows/Path.inc

79–80

I don't insist on the nonsense form, but I figured it would help to demonstrate that we explicitly considered the case and don't care to support it.

[mstorsjo]

Did the suggested code simplification, added a testcase for the degenerate case we don't handle.

[aaron.ballman]

LGTM, thank you!

llvm/lib/Support/Windows/Path.inc

std::error_code widenPath(const Twine &Path8, SmallVectorImpl<wchar_t> &Path16,
                           size_t MaxPathLen) {
   assert(MaxPathLen <= MAX_PATH);
 
   // Several operations would convert Path8 to SmallString; more efficient to do
   // it once up front.
   SmallString<MAX_PATH> Path8Str;
   Path8.toVector(Path8Str);
 
+  // If the path is a long path, mangled into forward slashes, normalize
+  // back to backslashes here.
+  if (Path8Str.startswith("//?/"))
+    llvm::sys::path::native(Path8Str, path::Style::windows_backslash);
+
   if (std::error_code EC = UTF8ToUTF16(Path8Str, Path16))
     return EC;
 
   const bool IsAbsolute = llvm::sys::path::is_absolute(Path8);
   size_t CurPathLen;
   if (IsAbsolute)
     CurPathLen = 0; // No contribution from current_path needed.
   else {
if ((Path16.size() + CurPathLen) < MaxPathLen ||
     return std::error_code();
 
   if (!IsAbsolute) {
     if (std::error_code EC = llvm::sys::fs::make_absolute(Path8Str))
       return EC;
   }
 
   // Remove '.' and '..' because long paths treat these as real path components.
+  // Explicitly use the backslash form here, as we're prepending the \\?\
+  // prefix.
   llvm::sys::path::native(Path8Str, path::Style::windows);
-  llvm::sys::path::remove_dots(Path8Str, true);
+  llvm::sys::path::remove_dots(Path8Str, true, path::Style::windows);
 
   const StringRef RootName = llvm::sys::path::root_name(Path8Str);
   assert(!RootName.empty() &&
          "Root name cannot be empty for an absolute path!");
 
   SmallString<2 * MAX_PATH> FullPath(LongPathPrefix);
   if (RootName[1] != ':') { // Check if UNC.
     FullPath.append("UNC\\");
std::string getMainExecutable(const char *argv0, void *MainExecAddr) {
   // the buffer not including the NULL terminator.
   PathName.set_size(Size);
 
   // Convert the result from UTF-16 to UTF-8.
   SmallVector<char, MAX_PATH> PathNameUTF8;
   if (UTF16ToUTF8(PathName.data(), PathName.size(), PathNameUTF8))
     return "";
 
+  llvm::sys::path::make_preferred(PathNameUTF8);
   return std::string(PathNameUTF8.data());
 }
 
 UniqueID file_status::getUniqueID() const {
   // The file is uniquely identified by the volume serial number along
   // with the 64-bit file identifier.
   uint64_t FileID = (static_cast<uint64_t>(FileIndexHigh) << 32ULL) |
                     static_cast<uint64_t>(FileIndexLow);
do {
 
     // If there's insufficient space, the len returned is larger than the len
     // given.
   } while (len > cur_path.capacity());
 
   // On success, GetCurrentDirectoryW returns the number of characters not
   // including the null-terminator.
   cur_path.set_size(len);
-  return UTF16ToUTF8(cur_path.begin(), cur_path.size(), result);
+
+  if (std::error_code EC =
+          UTF16ToUTF8(cur_path.begin(), cur_path.size(), result))
+    return EC;
+
+  llvm::sys::path::make_preferred(result);
+  return std::error_code();
 }
 
 std::error_code set_current_path(const Twine &path) {
   // Convert to utf-16.
   SmallVector<wchar_t, 128> wide_path;
   if (std::error_code ec = widenPath(path, wide_path))
     return ec;
 
if (CountChars >= 8 && ::memcmp(Data, L"\\\\?\\UNC\\", 16) == 0) {
     Data[0] = '\\';
   } else if (CountChars >= 4 && ::memcmp(Data, L"\\\\?\\", 8) == 0) {
     // Convert \\?\c:\foo to c:\foo
     CountChars -= 4;
     Data += 4;
   }
 
   // Convert the result from UTF-16 to UTF-8.
-  return UTF16ToUTF8(Data, CountChars, RealPath);
+  if (std::error_code EC = UTF16ToUTF8(Data, CountChars, RealPath))
+    return EC;
+
+  llvm::sys::path::make_preferred(RealPath);
+  return std::error_code();
 }
 
 std::error_code is_local(int FD, bool &Result) {
   SmallVector<wchar_t, 128> FinalPath;
   HANDLE Handle = reinterpret_cast<HANDLE>(_get_osfhandle(FD));
 
   if (std::error_code EC = realPathFromHandle(Handle, FinalPath))
     return EC;

 static bool getKnownFolderPath(KNOWNFOLDERID folderId,
                                SmallVectorImpl<char> &result) {
   wchar_t *path = nullptr;
   if (::SHGetKnownFolderPath(folderId, KF_FLAG_CREATE, nullptr, &path) != S_OK)
     return false;
 
   bool ok = !UTF16ToUTF8(path, ::wcslen(path), result);
   ::CoTaskMemFree(path);
+  if (ok)
+    llvm::sys::path::make_preferred(result);
   return ok;
 }
 
 bool home_directory(SmallVectorImpl<char> &result) {
   return getKnownFolderPath(FOLDERID_Profile, result);
 }
 
 bool user_config_directory(SmallVectorImpl<char> &result) {
if (getTempDirEnvVar(Result)) {
     native(Result); // Some Unix-like shells use Unix path separator in $TMP.
     fs::make_absolute(Result); // Make it absolute if not already.
     return;
   }
 
   // Fall back to a system default.
   const char *DefaultResult = "C:\\Temp";
   Result.append(DefaultResult, DefaultResult + strlen(DefaultResult));
+  llvm::sys::path::make_preferred(Result);
 }
 } // end namespace path
 
 namespace windows {
 std::error_code CodePageToUTF16(unsigned codepage,
                                 llvm::StringRef original,
                                 llvm::SmallVectorImpl<wchar_t> &utf16) {
   if (!original.empty()) {

llvm/lib/Support/Windows/Process.inc

windows::GetCommandLineArguments(SmallVectorImpl<const char *> &Args,
   }
 
   SmallVector<char, MAX_PATH> Arg0(Args[0], Args[0] + strlen(Args[0]));
   SmallVector<char, MAX_PATH> Filename;
   sys::path::remove_filename(Arg0);
   EC = GetExecutableName(Filename);
   if (EC)
     return EC;
+  sys::path::make_preferred(Arg0);
   sys::path::append(Arg0, Filename);
   Args[0] = Saver.save(Arg0).data();
   return std::error_code();
 }
 
 std::error_code Process::FixupStandardFileDescriptors() {
   return std::error_code();
 }

llvm/lib/Support/Windows/Program.inc

if (sys::fs::can_execute(U8Result))
       break; // Found it.
 
     U8Result.clear();
   }
 
   if (U8Result.empty())
     return mapWindowsError(::GetLastError());
 
+  llvm::sys::path::make_preferred(U8Result);
   return std::string(U8Result.begin(), U8Result.end());
 }
 
 bool MakeErrMsg(std::string *ErrMsg, const std::string &prefix) {
   if (!ErrMsg)
     return true;
   char *buffer = NULL;
   DWORD LastError = GetLastError();

llvm/unittests/Support/CommandLineTest.cpp

TEST(CommandLineTest, PositionalEatArgsError) {
   cl::ResetAllOptionOccurrences();
   EXPECT_TRUE(cl::ParseCommandLineOptions(6, args4, StringRef(), &OS)); OS.flush();
   EXPECT_TRUE(PosEatArgs.size() == 1);
   EXPECT_TRUE(PosEatArgs2.size() == 2);
   EXPECT_TRUE(Errs.empty());
 }
 
 #ifdef _WIN32
+void checkSeparators(StringRef Path) {
+  char UndesiredSeparator = sys::path::get_separator()[0] == '/' ? '\\' : '/';
+  ASSERT_EQ(Path.find(UndesiredSeparator), StringRef::npos);
+}
+
 TEST(CommandLineTest, GetCommandLineArguments) {
   int argc = __argc;
   char **argv = __argv;
 
   // GetCommandLineArguments is called in InitLLVM.
   llvm::InitLLVM X(argc, argv);
 
   EXPECT_EQ(llvm::sys::path::is_absolute(argv[0]),
             llvm::sys::path::is_absolute(__argv[0]));
+  checkSeparators(argv[0]);
 
   EXPECT_TRUE(
       llvm::sys::path::filename(argv[0]).equals_insensitive("supporttests.exe"))
       << "Filename of test executable is "
       << llvm::sys::path::filename(argv[0]);
 }
 #endif
 

llvm/unittests/Support/Path.cpp

if (!x) {                                                                    \
     SmallString<128> MessageStorage;                                           \
     raw_svector_ostream Message(MessageStorage);                               \
     Message << #x ": did not return a failure error code.\n";                  \
     GTEST_FATAL_FAILURE_(MessageStorage.c_str());                              \
   }
 
 namespace {
 
+void checkSeparators(StringRef Path) {
+#ifdef _WIN32
+  char UndesiredSeparator = sys::path::get_separator()[0] == '/' ? '\\' : '/';
+  ASSERT_EQ(Path.find(UndesiredSeparator), StringRef::npos);
+#endif
+}
+
 struct FileDescriptorCloser {
   explicit FileDescriptorCloser(int FD) : FD(FD) {}
   ~FileDescriptorCloser() { ::close(FD); }
   int FD;
 };
 
 TEST(is_style_Style, Works) {
   using namespace llvm::sys::path;

 #ifdef _WIN32
 std::string getEnvWin(const wchar_t *Var) {
   std::string expected;
   if (wchar_t const *path = ::_wgetenv(Var)) {
     auto pathLen = ::wcslen(path);
     ArrayRef<char> ref{reinterpret_cast<char const *>(path),
                        pathLen * sizeof(wchar_t)};
     convertUTF16ToUTF8String(ref, expected);
+    SmallString<32> Buf(expected);
+    path::make_preferred(Buf);
+    expected.assign(Buf.begin(), Buf.end());
   }
   return expected;
 }
 #else
 // RAII helper to set and restore an environment variable.
 class WithEnv {
   const char *Var;
   llvm::Optional<std::string> OriginalValue;
TEST(Support, TempDirectory) {
   TempDir.clear();
   path::system_temp_directory(true, TempDir);
   EXPECT_TRUE(!TempDir.empty());
 }
 
 #ifdef _WIN32
 static std::string path2regex(std::string Path) {
   size_t Pos = 0;
+  bool Forward = path::get_separator()[0] == '/';
   while ((Pos = Path.find('\\', Pos)) != std::string::npos) {
+    if (Forward) {
+      Path.replace(Pos, 1, "/");
+      Pos += 1;
+    } else {
       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;                                                               \
TEST_F(FileSystemTest, RealPath) {
   SmallString<64> Actual;
 
   // TestDirectory itself might be under a symlink or have been specified with
   // a different case than the existing temp directory.  In such cases real_path
   // on the concatenated path will differ in the TestDirectory portion from
   // how we specified it.  Make sure to compare against the real_path of the
   // TestDirectory, and not just the value of TestDirectory.
   ASSERT_NO_ERROR(fs::real_path(TestDirectory, RealBase));
+  checkSeparators(RealBase);
   path::native(Twine(RealBase) + "/test1/test2", Expected);
 
   ASSERT_NO_ERROR(fs::real_path(
       Twine(TestDirectory) + "/././test1/../test1/test2/./test3/..", Actual));
+  checkSeparators(Actual);
 
   EXPECT_EQ(Expected, Actual);
 
   SmallString<64> HomeDir;
 
   // This can fail if $HOME is not set and getpwuid fails.
   bool Result = llvm::sys::path::home_directory(HomeDir);
   if (Result) {
+    checkSeparators(HomeDir);
     ASSERT_NO_ERROR(fs::real_path(HomeDir, Expected));
+    checkSeparators(Expected);
     ASSERT_NO_ERROR(fs::real_path("~", Actual, true));
     EXPECT_EQ(Expected, Actual);
     ASSERT_NO_ERROR(fs::real_path("~/", Actual, true));
     EXPECT_EQ(Expected, Actual);
   }
 
   ASSERT_NO_ERROR(fs::remove_directories(Twine(TestDirectory) + "/test1"));
 }
TEST_F(FileSystemTest, widenPath) {
   // Construct the expected result.
   SmallVector<wchar_t, MAX_PATH + 16> Expected;
   ASSERT_NO_ERROR(windows::UTF8ToUTF16(Input, Expected));
   Expected.insert(Expected.begin(), LongPathPrefix.begin(),
                   LongPathPrefix.end());
 
   ASSERT_NO_ERROR(windows::widenPath(Input, Result));
   EXPECT_EQ(Result, Expected);
+  // Pass a path with forward slashes, check that it ends up with
+  // backslashes when widened with the long path prefix.
+  SmallString<MAX_PATH + 16> InputForward(Input);
+  path::make_preferred(InputForward, path::Style::windows_slash);
+  ASSERT_NO_ERROR(windows::widenPath(InputForward, Result));
+  EXPECT_EQ(Result, Expected);
+
+  // Pass a path which has the long path prefix prepended originally, but
+  // which is short enough to not require the long path prefix. If such a
+  // path is passed with forward slashes, make sure it gets normalized to
+  // backslashes.
+  SmallString<MAX_PATH + 16> PrefixedPath("\\\\?\\C:\\foldername");
+  ASSERT_NO_ERROR(windows::UTF8ToUTF16(PrefixedPath, Expected));
+  // Mangle the input to forward slashes.
+  path::make_preferred(PrefixedPath, path::Style::windows_slash);
+  ASSERT_NO_ERROR(windows::widenPath(PrefixedPath, Result));
+  EXPECT_EQ(Result, Expected);
+
+  // A short path with an inconsistent prefix is passed through as-is; this
+  // is a degenerate case that we currently don't care about handling.
+  PrefixedPath.assign("/\\?/C:/foldername");
+  ASSERT_NO_ERROR(windows::UTF8ToUTF16(PrefixedPath, Expected));
+  ASSERT_NO_ERROR(windows::widenPath(PrefixedPath, Result));
+  EXPECT_EQ(Result, Expected);
 
   // Test that UNC paths are handled correctly.
   const std::string ShareName("\\\\sharename\\");
   const std::string FileName("\\filename");
   // Initialize directory name so that the input is within the MAX_PATH limit.
   const char DirChar = 'x';
   std::string DirName(MAX_PATH - ShareName.size() - FileName.size() - 1,
                       DirChar);

llvm/unittests/Support/ProgramTest.cpp

#endif
   }
 
   void addEnvVar(StringRef Var) { EnvTable.emplace_back(Var); }
 
   ArrayRef<StringRef> getEnviron() const { return EnvTable; }
 };
 
 #ifdef _WIN32
+void checkSeparators(StringRef Path) {
+  char UndesiredSeparator = sys::path::get_separator()[0] == '/' ? '\\' : '/';
+  ASSERT_EQ(Path.find(UndesiredSeparator), StringRef::npos);
+}
+
 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);
+  SmallString<128> MyAbsExe(
+      sys::fs::getMainExecutable(TestMainArgv0, &ProgramTestStringArg1));
+  checkSeparators(MyAbsExe);
+  // Force a path with backslashes, when we are going to prepend the \\?\
+  // prefix.
+  sys::path::native(MyAbsExe, sys::path::Style::windows_backslash);
   std::string MyExe;
   if (!StringRef(MyAbsExe).startswith("\\\\?\\"))
     MyExe.append("\\\\?\\");
-  MyExe.append(MyAbsExe);
+  MyExe.append(std::string(MyAbsExe.begin(), MyAbsExe.end()));
 
   StringRef ArgV[] = {MyExe,
                       "--gtest_filter=ProgramEnvTest.CreateProcessLongPath"};
 
   // Add LLVM_PROGRAM_TEST_LONG_PATH to the environment of the child.
   addEnvVar("LLVM_PROGRAM_TEST_LONG_PATH=1");
 
   // Redirect stdout to a long path.