[llvm][Support][Windows] Fix slash in path for remove_directories

[jsji]

Before 925471e remove_directories
supports path with slash (instead of backslash).
The ILCreateFromPathW in new implementation requires backslash path,
so the call to remove_directories will fail if the path contains slash.

This is to normalize the path to make sure remove_directories still
support path with slash as well.

[llvmbot]

@llvm/pr-subscribers-llvm-support

Author: Jinsong Ji (jsji)

Changes

Before 925471e remove_directories
supports path with slash (instead of backslash).
The ILCreateFromPathW in new implementation requirs backslash path,
so the call to remove_directories will fail if the path contains slash.

This is to normalize the path to make sure remove_directories still
support path with slash as well.

---

Full diff: https://github.com/llvm/llvm-project/pull/121448.diff

1 Files Affected:

• (modified) llvm/lib/Support/Windows/Path.inc (+3-1)

diff --git a/llvm/lib/Support/Windows/Path.inc b/llvm/lib/Support/Windows/Path.inc
index 17db114caeb1ec..5b311e7c475c56 100644
--- a/llvm/lib/Support/Windows/Path.inc
+++ b/llvm/lib/Support/Windows/Path.inc
@@ -1373,9 +1373,11 @@ std::error_code closeFile(file_t &F) {
 }
 
 std::error_code remove_directories(const Twine &path, bool IgnoreErrors) {
+  SmallString<128> NativePath;
+  llvm::sys::path::native(path, NativePath, path::Style::windows_backslash);
   // Convert to utf-16.
   SmallVector<wchar_t, 128> Path16;
-  std::error_code EC = widenPath(path, Path16);
+  std::error_code EC = widenPath(NativePath, Path16);
   if (EC && !IgnoreErrors)
     return EC;
 

[llvmbot]

@llvm/pr-subscribers-platform-windows

Author: Jinsong Ji (jsji)

Changes

Before 925471e remove_directories
supports path with slash (instead of backslash).
The ILCreateFromPathW in new implementation requirs backslash path,
so the call to remove_directories will fail if the path contains slash.

This is to normalize the path to make sure remove_directories still
support path with slash as well.

---

Full diff: https://github.com/llvm/llvm-project/pull/121448.diff

1 Files Affected:

• (modified) llvm/lib/Support/Windows/Path.inc (+3-1)

diff --git a/llvm/lib/Support/Windows/Path.inc b/llvm/lib/Support/Windows/Path.inc
index 17db114caeb1ec..5b311e7c475c56 100644
--- a/llvm/lib/Support/Windows/Path.inc
+++ b/llvm/lib/Support/Windows/Path.inc
@@ -1373,9 +1373,11 @@ std::error_code closeFile(file_t &F) {
 }
 
 std::error_code remove_directories(const Twine &path, bool IgnoreErrors) {
+  SmallString<128> NativePath;
+  llvm::sys::path::native(path, NativePath, path::Style::windows_backslash);
   // Convert to utf-16.
   SmallVector<wchar_t, 128> Path16;
-  std::error_code EC = widenPath(path, Path16);
+  std::error_code EC = widenPath(NativePath, Path16);
   if (EC && !IgnoreErrors)
     return EC;
 

[mstorsjo]

LGTM, thanks.

Do you think it would be feasible to hit this case in one of the unit tests? IIRC the existing unit test already tests a number of tricky corner cases relating to this implementation.

[mstorsjo]

Thanks; does this particular case fail before this change? (Or would we need to create this particular path first to check that it really is removed?)

[jsji]

yes, it was failing without the fix .

[jsji]

bash-3.2$ ./unittests/Support/SupportTests.exe --gtest_filter=FileSystemTest.Remove
Note: Google Test filter = FileSystemTest.Remove
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from FileSystemTest
[ RUN      ] FileSystemTest.Remove
Test Directory: C:\Users\jinsongj\AppData\Local\Temp\3\file-system-test-09f535
D:/llvm/llvm/unittests/Support/Path.cpp(1339): error: Value of: fs::exists(Twine(BaseDir) + "/foo/bar/baz")
  Actual: true
Expected: false

[  FAILED  ] FileSystemTest.Remove (54 ms)
[----------] 1 test from FileSystemTest (57 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (63 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] FileSystemTest.Remove

[jsji]

The path was created above in line 1306, we just did not test the removal of it.