[Support] Listing a directory containing dangling symlinks is not an error.

[sammccall]

Summary

Reporting this as an error required stat()ing every file, as well as seeming
semantically questionable.

[vsk]

Thanks for the patch.

include/llvm/Support/FileSystem.h

1259 ↗

(On Diff #167471)

I'm struggling to understand how work is saved here. Do you mind explaining in some more detail? Specifically, I'm not sure why determining file_type is cheaper than getting a basic_file_status. Don't both stat()?

tools/llvm-cov/CodeCoverage.cpp

218 ↗

(On Diff #167471)

Do all clients of the directory iterator need to check status(), or is llvm-cov the weird one?

unittests/Support/Path.cpp

887 ↗

(On Diff #167471)

Nice!

[Dor1s]

include/llvm/Support/FileSystem.h

1220–1221 ↗

(On Diff #167471)

is this comment still relevant?

1263 ↗

(On Diff #167471)

Can this status be different from the value obtained on line 1259?

[sammccall]

Thanks for looking at this, good questions! I could have given more background.

Are there places you think better doc comments would help?

include/llvm/Support/FileSystem.h

1259 ↗

(On Diff #167471)

since r342232 file_type is often better:

• on linux/bsd/mac with most FSs, you get d_type as part of the struct dirent, so file_type is free when traversing, while basic_file_status costs a stat()
• on solaris (and others?), d_type is not present. On some filesystems (e.g. certain configurations of XFS), d_type is "unknown". In these cases type() falls back to status(), so this change is a no-op.
• on windows, both are free (you get basic_file_status as part of the directory listing)

tools/llvm-cov/CodeCoverage.cpp

218 ↗

(On Diff #167471)

I don't think llvm-cov particularly needs to check or warn here, I'm just trying to preserve the existing behavior as I'm not so familiar with it.

The original bug ("llvm-cov hangs") was a problem in recursive_directory_iterator, not llvm-cov itself. When it hit a dangling symlink (or anything that failed to stat), increment() would become a no-op and return no error code.

D44960 fixed this by having the iterator report the stat() failure as an error_code, and continue to advance. At this point the natural loop works fine. I believe the changes to llvm-cov just add a warning, and prevent the dangling symlink itself from being added as a collected path. (The latter shouldn't matter for most programs, but I preserved it to be safe).

Reporting the stat() error code had a kind of logic: stat() produces ErrorOr<file_status> and directory_iterator yielded (basic_file_status, error_code) so it seems to fit. But having directory_iterator return something closer to (path, type) and not error on dangling links is a better match for most OSes.

I don't think this changed API is error-prone: for a dangling symlink you get a directory_entry with just a path, and to do anything you must open/stat it etc which can fail. So it gets handled in the same way the application handles files you can see but can't read for any other reason, which seems appropriate.

[sammccall]

include/llvm/Support/FileSystem.h

1220–1221 ↗

(On Diff #167471)

In both directory_iterator and recursive_directory_iterator I can't tell if this comment refers to the above members or is a placeholder for future expansion. But it's been this way for at least 8 years. I'll remove them.

1263 ↗

(On Diff #167471)

Line 1259 gets the type (not status) from the directory listing, which will be symlink_file if the thing in the dir is a symlink.

The stat() is a resolving stat on the symlink (notice here type is symlink_file and Follow is true), so the new type is of the symlink *target*. This allows us to traverse into it if it's a directory.

[sammccall]

Zap unclear comments, clarify extra stat.

[vsk]

include/llvm/Support/FileSystem.h

1259 ↗

(On Diff #167471)

Thanks for the explanation, this change makes sense to me now.

1257 ↗

(On Diff #167502)

Might be worth noting here that on most OSes, checking the type of a file doesn't cost a stat().

tools/llvm-cov/CodeCoverage.cpp

218 ↗

(On Diff #167471)

I agree that the API change is safe. llvm-cov can handle situations in which it can't read a file. Not collecting paths to broken symlinks might actually be a problem, though, because it means summary statistics might not be emitted for files that can't (& don't need to) be read. I'll look into this as a follow-up -- for this patch, preserving the current behavior sgtm.

[Dor1s]

include/llvm/Support/FileSystem.h

1263 ↗

(On Diff #167471)

Thanks for clarifying!

llvm/trunk/include/llvm/Support/FileSystem.h

 public:
   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), FollowSymlinks);
-    update_error_code_for_current_entry(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(), FollowSymlinks);
-    update_error_code_for_current_entry(ec);
   }
 
   /// 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);
-    update_error_code_for_current_entry(ec);
     return *this;
   }
 
   const directory_entry &operator*() const { return State->CurrentEntry; }
   const directory_entry *operator->() const { return &State->CurrentEntry; }
 
   bool operator==(const directory_iterator &RHS) const {
     if (State == RHS.State)
       return true;
     if (!RHS.State)
       return State->CurrentEntry == directory_entry();
     if (!State)
       return RHS.State->CurrentEntry == directory_entry();
     return State->CurrentEntry == RHS.State->CurrentEntry;
   }
 
   bool operator!=(const directory_iterator &RHS) const {
     return !(*this == RHS);
   }
-  // Other members as required by
-  // C++ Std, 24.1.1 Input iterators [input.iterators]
-
-private:
-  // Checks if current entry is valid and populates error code. For example,
-  // current entry may not exist due to broken symbol links.
-  void update_error_code_for_current_entry(std::error_code &ec) {
-    // Bail out if error has already occured earlier to avoid overwriting it.
-    if (ec)
-      return;
-
-    // Empty directory entry is used to mark the end of an interation, it's not
-    // an error.
-    if (State->CurrentEntry == directory_entry())
-      return;
-
-    ErrorOr<basic_file_status> status = State->CurrentEntry.status();
-    if (!status)
-      ec = status.getError();
-  }
 };
 
 namespace detail {
 
   /// Keeps state for the recursive_directory_iterator.
   struct RecDirIterState {
     std::stack<directory_iterator, std::vector<directory_iterator>> Stack;
     uint16_t Level = 0;
public:
 
   // 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_type type = State->Stack.top()->type();
+      if (type == file_type::symlink_file && Follow) {
+        // Resolve the symlink: is it a directory to recurse into?
         ErrorOr<basic_file_status> status = State->Stack.top()->status();
-      if (status && is_directory(*status)) {
+        if (status)
+          type = status->type();
+        // Otherwise broken symlink, and we'll continue.
+      }
+      if (type == file_type::directory_file) {
         State->Stack.push(directory_iterator(*State->Stack.top(), ec, Follow));
         if (State->Stack.top() != end_itr) {
           ++State->Level;
           return *this;
         }
         State->Stack.pop();
       }
     }
public:
 
   bool operator==(const recursive_directory_iterator &RHS) const {
     return State == RHS.State;
   }
 
   bool operator!=(const recursive_directory_iterator &RHS) const {
     return !(*this == RHS);
   }
-  // Other members as required by
-  // C++ Std, 24.1.1 Input iterators [input.iterators]
 };
 
 /// @}
 
 } // end namespace fs
 } // end namespace sys
 } // end namespace llvm
 
 #endif // LLVM_SUPPORT_FILESYSTEM_H

llvm/trunk/tools/llvm-cov/CodeCoverage.cpp

if (llvm::sys::fs::is_regular_file(Status)) {
     return;
   }
 
   if (llvm::sys::fs::is_directory(Status)) {
     std::error_code EC;
     for (llvm::sys::fs::recursive_directory_iterator F(Path, EC), E;
          F != E; F.increment(EC)) {
 
-      if (EC) {
-        warning(EC.message(), F->path());
+      auto Status = F->status();
+      if (!Status) {
+        warning(Status.getError().message(), F->path());
         continue;
       }
 
-      if (llvm::sys::fs::is_regular_file(F->path()))
+      if (Status->type() == llvm::sys::fs::file_type::regular_file)
         addCollectedPath(F->path());
     }
   }
 }
 
 ErrorOr<const MemoryBuffer &>
 CodeCoverageTool::getSourceFile(StringRef SourceFile) {
   // If we've remapped filenames, look up the real location for this file.

llvm/trunk/unittests/Support/Path.cpp

ASSERT_NO_ERROR(fs::create_link(Twine(TestDirectory) + "/symlink/d/dd",
                                   Twine(TestDirectory) + "/symlink/d/da"));
   ASSERT_NO_ERROR(
       fs::create_link("no_such_file", Twine(TestDirectory) + "/symlink/e"));
 
   typedef std::vector<std::string> v_t;
   v_t VisitedNonBrokenSymlinks;
   v_t VisitedBrokenSymlinks;
   std::error_code ec;
+  using testing::UnorderedElementsAre;
+  using testing::UnorderedElementsAreArray;
 
   // Broken symbol links are expected to throw an error.
   for (fs::directory_iterator i(Twine(TestDirectory) + "/symlink", ec), e;
        i != e; i.increment(ec)) {
-    if (ec == std::make_error_code(std::errc::no_such_file_or_directory)) {
+    ASSERT_NO_ERROR(ec);
+    if (i->status().getError() ==
+        std::make_error_code(std::errc::no_such_file_or_directory)) {
       VisitedBrokenSymlinks.push_back(path::filename(i->path()));
       continue;
     }
-
-    ASSERT_NO_ERROR(ec);
     VisitedNonBrokenSymlinks.push_back(path::filename(i->path()));
   }
-  llvm::sort(VisitedNonBrokenSymlinks);
-  llvm::sort(VisitedBrokenSymlinks);
-  v_t ExpectedNonBrokenSymlinks = {"b", "d"};
-  ASSERT_EQ(ExpectedNonBrokenSymlinks.size(), VisitedNonBrokenSymlinks.size());
-  ASSERT_TRUE(std::equal(VisitedNonBrokenSymlinks.begin(),
-                         VisitedNonBrokenSymlinks.end(),
-                         ExpectedNonBrokenSymlinks.begin()));
+  EXPECT_THAT(VisitedNonBrokenSymlinks, UnorderedElementsAre("b", "d"));
   VisitedNonBrokenSymlinks.clear();
 
-  v_t ExpectedBrokenSymlinks = {"a", "c", "e"};
-  ASSERT_EQ(ExpectedBrokenSymlinks.size(), VisitedBrokenSymlinks.size());
-  ASSERT_TRUE(std::equal(VisitedBrokenSymlinks.begin(),
-                         VisitedBrokenSymlinks.end(),
-                         ExpectedBrokenSymlinks.begin()));
+  EXPECT_THAT(VisitedBrokenSymlinks, UnorderedElementsAre("a", "c", "e"));
   VisitedBrokenSymlinks.clear();
 
   // Broken symbol links are expected to throw an error.
   for (fs::recursive_directory_iterator i(
       Twine(TestDirectory) + "/symlink", ec), e; i != e; i.increment(ec)) {
-    if (ec == std::make_error_code(std::errc::no_such_file_or_directory)) {
+    ASSERT_NO_ERROR(ec);
+    if (i->status().getError() ==
+        std::make_error_code(std::errc::no_such_file_or_directory)) {
       VisitedBrokenSymlinks.push_back(path::filename(i->path()));
       continue;
     }
-
-    ASSERT_NO_ERROR(ec);
     VisitedNonBrokenSymlinks.push_back(path::filename(i->path()));
   }
-  llvm::sort(VisitedNonBrokenSymlinks);
-  llvm::sort(VisitedBrokenSymlinks);
-  ExpectedNonBrokenSymlinks = {"b", "bb", "d", "da", "dd", "ddd", "ddd"};
-  ASSERT_EQ(ExpectedNonBrokenSymlinks.size(), VisitedNonBrokenSymlinks.size());
-  ASSERT_TRUE(std::equal(VisitedNonBrokenSymlinks.begin(),
-                         VisitedNonBrokenSymlinks.end(),
-                         ExpectedNonBrokenSymlinks.begin()));
+  EXPECT_THAT(VisitedNonBrokenSymlinks,
+              UnorderedElementsAre("b", "bb", "d", "da", "dd", "ddd", "ddd"));
   VisitedNonBrokenSymlinks.clear();
 
-  ExpectedBrokenSymlinks = {"a", "ba", "bc", "c", "e"};
-  ASSERT_EQ(ExpectedBrokenSymlinks.size(), VisitedBrokenSymlinks.size());
-  ASSERT_TRUE(std::equal(VisitedBrokenSymlinks.begin(),
-                         VisitedBrokenSymlinks.end(),
-                         ExpectedBrokenSymlinks.begin()));
+  EXPECT_THAT(VisitedBrokenSymlinks,
+              UnorderedElementsAre("a", "ba", "bc", "c", "e"));
   VisitedBrokenSymlinks.clear();
 
   for (fs::recursive_directory_iterator i(
       Twine(TestDirectory) + "/symlink", ec, /*follow_symlinks=*/false), e;
        i != e; i.increment(ec)) {
-    if (ec == std::make_error_code(std::errc::no_such_file_or_directory)) {
+    ASSERT_NO_ERROR(ec);
+    if (i->status().getError() ==
+        std::make_error_code(std::errc::no_such_file_or_directory)) {
       VisitedBrokenSymlinks.push_back(path::filename(i->path()));
       continue;
     }
-
-    ASSERT_NO_ERROR(ec);
     VisitedNonBrokenSymlinks.push_back(path::filename(i->path()));
   }
-  llvm::sort(VisitedNonBrokenSymlinks);
-  llvm::sort(VisitedBrokenSymlinks);
-  ExpectedNonBrokenSymlinks = {"a", "b", "ba", "bb", "bc", "c", "d", "da", "dd",
-                               "ddd", "e"};
-  ASSERT_EQ(ExpectedNonBrokenSymlinks.size(), VisitedNonBrokenSymlinks.size());
-  ASSERT_TRUE(std::equal(VisitedNonBrokenSymlinks.begin(),
-                         VisitedNonBrokenSymlinks.end(),
-                         ExpectedNonBrokenSymlinks.begin()));
+  EXPECT_THAT(VisitedNonBrokenSymlinks,
+              UnorderedElementsAreArray({"a", "b", "ba", "bb", "bc", "c", "d",
+                                         "da", "dd", "ddd", "e"}));
   VisitedNonBrokenSymlinks.clear();
 
-  ExpectedBrokenSymlinks = {};
-  ASSERT_EQ(ExpectedBrokenSymlinks.size(), VisitedBrokenSymlinks.size());
-  ASSERT_TRUE(std::equal(VisitedBrokenSymlinks.begin(),
-                         VisitedBrokenSymlinks.end(),
-                         ExpectedBrokenSymlinks.begin()));
+  EXPECT_THAT(VisitedBrokenSymlinks, UnorderedElementsAre());
   VisitedBrokenSymlinks.clear();
 
   ASSERT_NO_ERROR(fs::remove_directories(Twine(TestDirectory) + "/symlink"));
 }
 #endif
 
 TEST_F(FileSystemTest, Remove) {
   SmallString<64> BaseDir;