Add llvm::sys::fs::remove_directories()

[zturner]

Summary

We have create_directories() which can create an entire tree,
and remove(), which can remove an empty directory, but not
a remove_directories() which can remove an entire tree.

This patch adds such a function.

[mehdi_amini]

LGTM

llvm/lib/Support/Path.cpp

937

I assume removing entries in a directory does not invalidate the iterator?

[amccarth]

llvm/lib/Support/Path.cpp

933

This is the usual Posix approach, so I'm concerned it won't be reliable on Windows.

When you delete files from a directory on Windows, they are marked for deletion and actually deleted later, typically milliseconds later, but it can actually be even longer. So when you try to delete the directory, it may not actually be empty yet.

The correct pattern on Windows is to rename the files out of directory (to some temp location) and then delete them. The rename, if on the same volume, is synchronous, so you know that the directory is actually empty, even if the delete hasn't happened yet. Is this what the implementation of remove does on Windows?

[mehdi_amini]

llvm/lib/Support/Path.cpp

933

Windows implementation of remove() here: https://github.com/llvm-project/llvm-project/blob/master/llvm/lib/Support/Windows/Path.inc#L251

[zturner]

Updated to use a more robust method on Windows. Also, on non-Windows platforms unfortunately directory_iterator will follow symlinks when statting, so directory symlinks will look like ordinary directories. This is a serious problem for a recursive delete function, because we will walk into the child and delete everything in it.

To err on the side of caution, I've updated directory_iterator and recursive_directory_iterator to make following symlinks opt-out (this way all existing code is unchanged). In the deletion function specifically, I use this opt-out flag so that we do not follow symlinks, and we just delete the link itself.

Although it's not explicitly documented what the behavior of SHFileOperation is with respect to this, and despite the fact that there is a mysterious flag called SH_NORECURSEREPARSE which it claims is "unsupported", I've confirmed that with the code as written, the API does *not* recurse into reparse points (either junctions or symlinks).

[amccarth]

LGTM.

llvm/include/llvm/Support/FileSystem.h

 /// @brief Remove path. Equivalent to POSIX remove().
 ///
 /// @param path Input path.
 /// @returns errc::success if path has been removed or didn't exist, otherwise a
 ///          platform-specific error code. If IgnoreNonExisting is false, also
 ///          returns error if the file didn't exist.
 std::error_code remove(const Twine &path, bool IgnoreNonExisting = true);
 
+/// @brief Recursively delete a directory.
+///
+/// @param path Input path.
+/// @returns errc::success if path has been removed or didn't exist, otherwise a
+///          platform-specific error code.
+std::error_code remove_directories(const Twine &path, bool IgnoreErrors = true);
+
 /// @brief Rename \a from to \a to. Files are renamed as if by POSIX rename().
 ///
 /// @param from The path to rename from.
 /// @param to The path to rename to. This is created.
 std::error_code rename(const Twine &from, const Twine &to);
 
 /// @brief Copy the contents of \a From to \a To.
 ///

 /// @name Iterators
 /// @{
 
 /// directory_entry - A single entry in a directory. Caches the status either
 /// from the result of the iteration syscall, or the first time status is
 /// called.
 class directory_entry {
   std::string Path;
+  bool FollowSymlinks;
   mutable file_status Status;
 
 public:
-  explicit directory_entry(const Twine &path, file_status st = file_status())
-    : Path(path.str())
-    , Status(st) {}
+  explicit directory_entry(const Twine &path, bool follow_symlinks = true,
+                           file_status st = file_status())
+      : Path(path.str()), FollowSymlinks(follow_symlinks), Status(st) {}
 
   directory_entry() = default;
 
   void assign(const Twine &path, file_status st = file_status()) {
     Path = path.str();
     Status = st;
   }
 
public:
   bool operator> (const directory_entry& rhs) const;
   bool operator>=(const directory_entry& rhs) const;
 };
 
 namespace detail {
 
   struct DirIterState;
 
-  std::error_code directory_iterator_construct(DirIterState &, StringRef);
+  std::error_code directory_iterator_construct(DirIterState &, StringRef, bool);
   std::error_code directory_iterator_increment(DirIterState &);
   std::error_code directory_iterator_destruct(DirIterState &);
 
   /// Keeps state for the directory_iterator.
   struct DirIterState {
     ~DirIterState() {
       directory_iterator_destruct(*this);
     }
 
     intptr_t IterationHandle = 0;
     directory_entry CurrentEntry;
   };
 
 } // end namespace detail
 
 /// directory_iterator - Iterates through the entries in path. There is no
 /// operator++ because we need an error_code. If it's really needed we can make
 /// it call report_fatal_error on error.
 class directory_iterator {
   std::shared_ptr<detail::DirIterState> State;
+  bool FollowSymlinks = true;
 
 public:
-  explicit directory_iterator(const Twine &path, std::error_code &ec) {
+  explicit directory_iterator(const Twine &path, std::error_code &ec,
+                              bool follow_symlinks = true)
+      : FollowSymlinks(follow_symlinks) {
     State = std::make_shared<detail::DirIterState>();
     SmallString<128> path_storage;
-    ec = detail::directory_iterator_construct(*State,
-            path.toStringRef(path_storage));
+    ec = detail::directory_iterator_construct(
+        *State, path.toStringRef(path_storage), FollowSymlinks);
   }
 
-  explicit directory_iterator(const directory_entry &de, std::error_code &ec) {
+  explicit directory_iterator(const directory_entry &de, std::error_code &ec,
+                              bool follow_symlinks = true)
+      : FollowSymlinks(follow_symlinks) {
     State = std::make_shared<detail::DirIterState>();
-    ec = detail::directory_iterator_construct(*State, de.path());
+    ec =
+        detail::directory_iterator_construct(*State, de.path(), FollowSymlinks);
   }
 
   /// Construct end iterator.
   directory_iterator() = default;
 
   // No operator++ because we need error_code.
   directory_iterator &increment(std::error_code &ec) {
     ec = directory_iterator_increment(*State);
namespace detail {
   };
 
 } // end namespace detail
 
 /// recursive_directory_iterator - Same as directory_iterator except for it
 /// recurses down into child directories.
 class recursive_directory_iterator {
   std::shared_ptr<detail::RecDirIterState> State;
+  bool Follow;
 
 public:
   recursive_directory_iterator() = default;
-  explicit recursive_directory_iterator(const Twine &path, std::error_code &ec)
-      : State(std::make_shared<detail::RecDirIterState>()) {
-    State->Stack.push(directory_iterator(path, ec));
+  explicit recursive_directory_iterator(const Twine &path, std::error_code &ec,
+                                        bool follow_symlinks = true)
+      : State(std::make_shared<detail::RecDirIterState>()),
+        Follow(follow_symlinks) {
+    State->Stack.push(directory_iterator(path, ec, Follow));
     if (State->Stack.top() == directory_iterator())
       State.reset();
   }
 
   // No operator++ because we need error_code.
   recursive_directory_iterator &increment(std::error_code &ec) {
     const directory_iterator end_itr = {};
 
     if (State->HasNoPushRequest)
       State->HasNoPushRequest = false;
     else {
       file_status st;
       if ((ec = State->Stack.top()->status(st))) return *this;
       if (is_directory(st)) {
-        State->Stack.push(directory_iterator(*State->Stack.top(), ec));
+        State->Stack.push(directory_iterator(*State->Stack.top(), ec, Follow));
         if (ec) return *this;
         if (State->Stack.top() != end_itr) {
           ++State->Level;
           return *this;
         }
         State->Stack.pop();
       }
     }

llvm/lib/Support/Path.cpp

std::error_code copy_file(const Twine &From, const Twine &To) {
   int BytesRead = 0, BytesWritten = 0;
   for (;;) {
     BytesRead = read(ReadFD, Buf, BufSize);
     if (BytesRead <= 0)
       break;
     while (BytesRead) {
       BytesWritten = write(WriteFD, Buf, BytesRead);
       if (BytesWritten < 0)
         break;
       BytesRead -= BytesWritten;
     }
     if (BytesWritten < 0)
       break;
   }
   close(ReadFD);
   close(WriteFD);
   delete[] Buf;
 
   if (BytesRead < 0 || BytesWritten < 0)
     return std::error_code(errno, std::generic_category());
   return std::error_code();
std::error_code identify_magic(const Twine &Path, file_magic &Result) {
   if (close(FD) != 0 || Length < 0)
     return std::error_code(errno, std::generic_category());
 
   Result = identify_magic(StringRef(Buffer, Length));
   return std::error_code();
 }
 
 std::error_code directory_entry::status(file_status &result) const {
-  return fs::status(Path, result);
+  return fs::status(Path, result, FollowSymlinks);
 }
 
 } // end namespace fs
 } // end namespace sys
 } // end namespace llvm
 
 // Include the truly platform-specific parts.
 #if defined(LLVM_ON_UNIX)

llvm/lib/Support/Unix/Path.inc

const char *mapped_file_region::const_data() const {
   return reinterpret_cast<const char*>(Mapping);
 }
 
 int mapped_file_region::alignment() {
   return Process::getPageSize();
 }
 
 std::error_code detail::directory_iterator_construct(detail::DirIterState &it,
-                                                StringRef path){
+                                                     StringRef path,
+                                                     bool follow_symlinks) {
   SmallString<128> path_null(path);
   DIR *directory = ::opendir(path_null.c_str());
   if (!directory)
     return std::error_code(errno, std::generic_category());
 
   it.IterationHandle = reinterpret_cast<intptr_t>(directory);
   // Add something for replace_filename to replace.
   path::append(path_null, ".");
-  it.CurrentEntry = directory_entry(path_null.str());
+  it.CurrentEntry = directory_entry(path_null.str(), follow_symlinks);
   return directory_iterator_increment(it);
 }
 
 std::error_code detail::directory_iterator_destruct(detail::DirIterState &it) {
   if (it.IterationHandle)
     ::closedir(reinterpret_cast<DIR *>(it.IterationHandle));
   it.IterationHandle = 0;
   it.CurrentEntry = directory_entry();
if (static_cast<size_t>(CharCount) == ResultPath.capacity()) {
     if (CharCount > sb.st_size)
       return std::error_code(ENAMETOOLONG, std::generic_category());
   }
   ResultPath.set_size(static_cast<size_t>(CharCount));
 #endif
   return std::error_code();
 }
 
+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;
+    file_status st;
+    EC = Item.status(st);
+    if (EC && !IgnoreErrors)
+      return EC;
+
+    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;
+
+    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();
+}
+
 } // end namespace fs
 
 namespace path {
 
 bool home_directory(SmallVectorImpl<char> &result) {
   if (char *RequestedDir = getenv("HOME")) {
     result.clear();
     result.append(RequestedDir, RequestedDir + strlen(RequestedDir));

llvm/lib/Support/Windows/Path.inc

 #include <fcntl.h>
 #include <io.h>
 #include <sys/stat.h>
 #include <sys/types.h>
 
 // These two headers must be included last, and make sure shlobj is required
 // after Windows.h to make sure it picks up our definition of _WIN32_WINNT
 #include "WindowsSupport.h"
+#include <shellapi.h>
 #include <shlobj.h>
 
 #undef max
 
 // MinGW doesn't define this.
 #ifndef _ERRNO_T_DEFINED
 #define _ERRNO_T_DEFINED
 typedef int errno_t;

 
 int mapped_file_region::alignment() {
   SYSTEM_INFO SysInfo;
   ::GetSystemInfo(&SysInfo);
   return SysInfo.dwAllocationGranularity;
 }
 
 std::error_code detail::directory_iterator_construct(detail::DirIterState &it,
-                                                StringRef path){
+                                                     StringRef path,
+                                                     bool follow_symlinks) {
   SmallVector<wchar_t, 128> path_utf16;
 
   if (std::error_code ec = widenPath(path, path_utf16))
     return ec;
 
   // Convert path to the format that Windows is happy with.
   if (path_utf16.size() > 0 &&
       !is_separator(path_utf16[path.size() - 1]) &&
std::error_code detail::directory_iterator_construct(detail::DirIterState &it,
   if (std::error_code ec =
           UTF16ToUTF8(FirstFind.cFileName, ::wcslen(FirstFind.cFileName),
                       directory_entry_name_utf8))
     return ec;
 
   it.IterationHandle = intptr_t(FindHandle.take());
   SmallString<128> directory_entry_path(path);
   path::append(directory_entry_path, directory_entry_name_utf8);
-  it.CurrentEntry = directory_entry(directory_entry_path);
+  it.CurrentEntry = directory_entry(directory_entry_path, follow_symlinks);
 
   return std::error_code();
 }
 
 std::error_code detail::directory_iterator_destruct(detail::DirIterState &it) {
   if (it.IterationHandle != 0)
     // Closes the handle if it's valid.
     ScopedFindHandle close(HANDLE(it.IterationHandle));
std::error_code getPathFromOpenFD(int FD, SmallVectorImpl<char> &ResultPath) {
   // null.
   if (TempPath.back() == L'\0') {
     --CharCount;
     TempPath.pop_back();
   }
 
   return windows::UTF16ToUTF8(TempPath.data(), CharCount, ResultPath);
 }
+
+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();
+}
+
 } // end namespace fs
 
 namespace path {
 static bool getKnownFolderPath(KNOWNFOLDERID folderId,
                                SmallVectorImpl<char> &result) {
   wchar_t *path = nullptr;
   if (::SHGetKnownFolderPath(folderId, KF_FLAG_CREATE, nullptr, &path) != S_OK)
     return false;

llvm/unittests/Support/Path.cpp

TEST_F(FileSystemTest, DirectoryIteration) {
   }
   EXPECT_EQ(I, E);
   ASSERT_NO_ERROR(fs::remove(Twine(TestDirectory) + "/reclevel/a/b/c"));
   ASSERT_NO_ERROR(fs::remove(Twine(TestDirectory) + "/reclevel/a/b"));
   ASSERT_NO_ERROR(fs::remove(Twine(TestDirectory) + "/reclevel/a"));
   ASSERT_NO_ERROR(fs::remove(Twine(TestDirectory) + "/reclevel"));
 }
 
+TEST_F(FileSystemTest, Remove) {
+  SmallString<64> BaseDir;
+  SmallString<64> Paths[4];
+  int fds[4];
+  ASSERT_NO_ERROR(fs::createUniqueDirectory("fs_remove", BaseDir));
+
+  ASSERT_NO_ERROR(fs::create_directories(Twine(BaseDir) + "/foo/bar/baz"));
+  ASSERT_NO_ERROR(fs::create_directories(Twine(BaseDir) + "/foo/bar/buzz"));
+  ASSERT_NO_ERROR(fs::createUniqueFile(
+      Twine(BaseDir) + "/foo/bar/baz/%%%%%%.tmp", fds[0], Paths[0]));
+  ASSERT_NO_ERROR(fs::createUniqueFile(
+      Twine(BaseDir) + "/foo/bar/baz/%%%%%%.tmp", fds[1], Paths[1]));
+  ASSERT_NO_ERROR(fs::createUniqueFile(
+      Twine(BaseDir) + "/foo/bar/buzz/%%%%%%.tmp", fds[2], Paths[2]));
+  ASSERT_NO_ERROR(fs::createUniqueFile(
+      Twine(BaseDir) + "/foo/bar/buzz/%%%%%%.tmp", fds[3], Paths[3]));
+
+  for (int fd : fds)
+    ::close(fd);
+
+  EXPECT_TRUE(fs::exists(Twine(BaseDir) + "/foo/bar/baz"));
+  EXPECT_TRUE(fs::exists(Twine(BaseDir) + "/foo/bar/buzz"));
+  EXPECT_TRUE(fs::exists(Paths[0]));
+  EXPECT_TRUE(fs::exists(Paths[1]));
+  EXPECT_TRUE(fs::exists(Paths[2]));
+  EXPECT_TRUE(fs::exists(Paths[3]));
+
+  ASSERT_NO_ERROR(fs::remove_directories("D:/footest"));
+
+  ASSERT_NO_ERROR(fs::remove_directories(BaseDir));
+  ASSERT_FALSE(fs::exists(BaseDir));
+}
+
 const char archive[] = "!<arch>\x0A";
 const char bitcode[] = "\xde\xc0\x17\x0b";
 const char coff_object[] = "\x00\x00......";
 const char coff_bigobj[] = "\x00\x00\xff\xff\x00\x02......"
     "\xc7\xa1\xba\xd1\xee\xba\xa9\x4b\xaf\x20\xfa\xf6\x6a\xa4\xdc\xb8";
 const char coff_import_library[] = "\x00\x00\xff\xff....";
 const char elf_relocatable[] = { 0x7f, 'E', 'L', 'F', 1, 2, 1, 0, 0,
                                  0,    0,   0,   0,   0, 0, 0, 0, 1 };