[Support] Use std::filesystem::remove_all() in remove_directories() #119146
Conversation
|
@llvm/pr-subscribers-platform-windows @llvm/pr-subscribers-llvm-support Author: Dmitry Vasilyev (slydiman) ChangesSee #118677 for details. Full diff: https://github.com/llvm/llvm-project/pull/119146.diff 3 Files Affected:
diff --git a/llvm/lib/Support/Path.cpp b/llvm/lib/Support/Path.cpp
index d7752851971030..4c0804f2dd494d 100644
--- a/llvm/lib/Support/Path.cpp
+++ b/llvm/lib/Support/Path.cpp
@@ -22,6 +22,7 @@
#include "llvm/Support/Process.h"
#include "llvm/Support/Signals.h"
#include <cctype>
+#include <filesystem>
#if !defined(_MSC_VER) && !defined(__MINGW32__)
#include <unistd.h>
@@ -1206,6 +1207,13 @@ namespace llvm {
namespace sys {
namespace fs {
+std::error_code remove_directories(const Twine &path, bool IgnoreErrors) {
+ const std::filesystem::path Path(path.str());
+ std::error_code EC;
+ std::filesystem::remove_all(Path, EC);
+ return IgnoreErrors ? std::error_code() : EC;
+}
+
TempFile::TempFile(StringRef Name, int FD)
: TmpName(std::string(Name)), FD(FD) {}
TempFile::TempFile(TempFile &&Other) { *this = std::move(Other); }
diff --git a/llvm/lib/Support/Unix/Path.inc b/llvm/lib/Support/Unix/Path.inc
index 44097bad7b46ed..b8cc6b8bdb464a 100644
--- a/llvm/lib/Support/Unix/Path.inc
+++ b/llvm/lib/Support/Unix/Path.inc
@@ -1272,46 +1272,6 @@ std::error_code closeFile(file_t &F) {
return Process::SafelyCloseFileDescriptor(TmpF);
}
-template <typename T>
-static std::error_code remove_directories_impl(const T &Entry,
- bool IgnoreErrors) {
- std::error_code EC;
- directory_iterator Begin(Entry, EC, false);
- directory_iterator End;
- while (Begin != End) {
- auto &Item = *Begin;
- ErrorOr<basic_file_status> st = Item.status();
- if (st) {
- if (is_directory(*st)) {
- EC = remove_directories_impl(Item, IgnoreErrors);
- if (EC && !IgnoreErrors)
- return EC;
- }
-
- EC = fs::remove(Item.path(), true);
- if (EC && !IgnoreErrors)
- return EC;
- } else if (!IgnoreErrors) {
- return st.getError();
- }
-
- Begin.increment(EC);
- if (EC && !IgnoreErrors)
- return EC;
- }
- return std::error_code();
-}
-
-std::error_code remove_directories(const Twine &path, bool IgnoreErrors) {
- auto EC = remove_directories_impl(path, IgnoreErrors);
- if (EC && !IgnoreErrors)
- return EC;
- EC = fs::remove(path, true);
- if (EC && !IgnoreErrors)
- return EC;
- return std::error_code();
-}
-
std::error_code real_path(const Twine &path, SmallVectorImpl<char> &dest,
bool expand_tilde) {
dest.clear();
diff --git a/llvm/lib/Support/Windows/Path.inc b/llvm/lib/Support/Windows/Path.inc
index c4bd5e24723517..e70576540edaa1 100644
--- a/llvm/lib/Support/Windows/Path.inc
+++ b/llvm/lib/Support/Windows/Path.inc
@@ -1372,32 +1372,6 @@ std::error_code closeFile(file_t &F) {
return std::error_code();
}
-std::error_code remove_directories(const Twine &path, bool IgnoreErrors) {
- // Convert to utf-16.
- SmallVector<wchar_t, 128> Path16;
- std::error_code EC = widenPath(path, Path16);
- if (EC && !IgnoreErrors)
- return EC;
-
- // SHFileOperation() accepts a list of paths, and so must be double null-
- // terminated to indicate the end of the list. The buffer is already null
- // terminated, but since that null character is not considered part of the
- // vector's size, pushing another one will just consume that byte. So we
- // need to push 2 null terminators.
- Path16.push_back(0);
- Path16.push_back(0);
-
- SHFILEOPSTRUCTW shfos = {};
- shfos.wFunc = FO_DELETE;
- shfos.pFrom = Path16.data();
- shfos.fFlags = FOF_NO_UI;
-
- int result = ::SHFileOperationW(&shfos);
- if (result != 0 && !IgnoreErrors)
- return mapWindowsError(result);
- return std::error_code();
-}
-
static void expandTildeExpr(SmallVectorImpl<char> &Path) {
// Path does not begin with a tilde expression.
if (Path.empty() || Path[0] != '~')
|
llvm/lib/Support/Windows/Path.inc
Outdated
| if (result != 0 && !IgnoreErrors) | ||
| return mapWindowsError(result); | ||
| return std::error_code(); | ||
| const std::filesystem::path Path(path.str()); |
There was a problem hiding this comment.
Unfortunately, passing paths like this won't do the right thing.
Within LLVM, paths in the usual form with narrow chars is assumed to be UTF-8; for the existing Windows implementations here, it is converted to UTF-16 into wchar_t form with widenPath above.
However, in std::filesystem, a narrow char path is assumed to be in the system active code page (ACP). Unless the ACP is set to UTF-8, only plain ASCII path names would work correctly.
There is a std::filesystem::u8path() function which takes char input and interprets it as UTF-8, which we could use instead. That one is deprecated since C++20 though (because in C++20, there's a char8_t data type to natively signal UTF8 strings).
I guess it's simplest to just keep the existing widenPath call first and then pass the path from that to std::filesystem though, but I don't have a very strong opinion on it - u8path() might be ok.
(At least this is how MS STL and libc++ implements std::filesystem. I haven't observed libstdc++'s implementation much at all; I'm not sure how mature it is on Windows.)
There was a problem hiding this comment.
I have updated this patch to use widenPath() and tested it with non-ASCII symbols locally.
|
I did some empirical testing with this patch in mingw environments; with libc++ it seems fine, all tests in However with libstdc++ (tested with the latest version in msys2, based on GCC 14), calling I tried running it in a debugger, and it does seem to do ... something, with backtraces like this: |
|
@mstorsjo Does the log show what the dir paths looked like in the handing test? |
The log didn't show anything about it, but I tried in a local build and added a printout before calling it, and the path is |
|
The location and reason for the hang are not clear from this stack trace. It is clear that we cannot rely on libstdc++. |
Forward slash - if you add an explicit path.make_preferred() - does it still hang? |
That doesn't make any difference, it still hangs. |
It seems __cxx11 means c++11, not c++17. |
I think that's unrelated - I would guess that it's some libstdc++ internal ABI grouping, where some classes had to break ABI betwen C++03 and C++11, and this belongs to the post-C++11 category. |
|
I think the reason it hangs is because this test intentionally tries to create a deep nested structure of paths, like So that makes the issue a bit easier to understand; libstdc++'s implementation of |
|
Should we close this PR in favor of #118677 ? |
Yes, I think this one can be closed. |
See #118677 for details.