[llvm-cov] Prevent llvm-cov from hanging when a symblink doesn't exist.

[liaoyuke]

Summary

Previous code hangs indefinitely when trying to iterate through a
symbol link file that points to an non-exist directory. This change
fixes the bug to make the addCollectedPath function exit ealier and
print out correct warning messages.

Patch by Yuke Liao (@liaoyuke).

[vsk]

Thanks for the patch. Some inline comments --

include/llvm/Support/FileSystem.h

1036 ↗

(On Diff #140011)

Could you add a test for this in unittests/Support/Path.cpp?

tools/llvm-cov/CodeCoverage.cpp

202 ↗

(On Diff #140011)

Nitpicks: could you capitalize 'source', and change 'exit' -> 'exist'?

222 ↗

(On Diff #140011)

Nitpick, please capitalize 'code'.

[Dor1s]

tools/llvm-cov/CodeCoverage.cpp

213 ↗

(On Diff #140011)

int: I think PascalCase is preferred, so this needs to be PathBeforeErrorOccurs

[liaoyuke]

Addressed comments and added unit test.

[liaoyuke]

Thanks for the comments, please take another look. Thank you very much!

include/llvm/Support/FileSystem.h

1036 ↗

(On Diff #140011)

Just found out there are already a test covering broken symlinks, but unfortunately, that test was artificially manipulated to pass, so it didn't catch the hang, I've modified the test.

[vsk]

unittests/Support/Path.cpp

911 ↗

(On Diff #140028)

Could you keep the checks for what the visited set should be? I think we'd get the behavior we're interested in by changing the 'break' here to a 'continue'.

[liaoyuke]

unittests/Support/Path.cpp

911 ↗

(On Diff #140028)

This is the part that gets tricky, we can't call i.increment() again because it will hang. We can do what the original test did to help this test pass, however, this won't help us catch real bugs.

Another option is that we can make make the recursive_directory_iterator more robust that it is capable of skipping broken symlink files by itself.

WDYT?

[liaoyuke]

unittests/Support/Path.cpp

911 ↗

(On Diff #140028)

With the new behavior, a broken symlink won't prevent other paths from being collected, which actually makes more sense to me.

[vsk]

Ideally, if the directory iterator finds a broken symlink, it would be able to report the error and continue its walk. I haven't looked at the code closely enough to say definitively, but I think this should be possible without changing the API.

[Dor1s]

unittests/Support/Path.cpp

911 ↗

(On Diff #140028)

That indeed sounds better. Have you already looked into that?

[liaoyuke]

unittests/Support/Path.cpp

911 ↗

(On Diff #140028)

Yes, will upload a new patch later.

[liaoyuke]

When recursive directory iterator encounters an error, it either hangs
or exits immediately in previous implementations.

This change modifies the behavior to report the error and continue
its walk, and added corresponding unit tests to verify it.

[vsk]

This is looking really good, thanks for digging into it. Without this patch, clients of the directory iterator need to check for broken symlinks explicitly and issue a call to "i.no_push()". That clearly isn't happening everywhere it should. This patch really improves the API.

Note that after this lands, we'll need to fix up clients of this API in llvm-profdata.cpp and in lldb (inside of FileSpec::EnumerateDirectory).

I have a few more suggestions inline --

include/llvm/Support/FileSystem.h

959 ↗

(On Diff #140100)

Why not sink the "!ec" test into check_current_entry?

1006 ↗

(On Diff #140100)

Could you rename this to something more explicit, such as update_error_code_for_current_entry?

unittests/Support/Path.cpp

914 ↗

(On Diff #140100)

You can just write: "find(NamesOfBrokenSymlinks, CurrentFileName)". LLVM defines range-based algorithms in llvm/ADT/STLExtras.h.

918 ↗

(On Diff #140100)

Should this be something like, ASSERT_EQUAL(ec.category(), std::errc::no_such_file_or_directory)?

925 ↗

(On Diff #140100)

Could you retain the behavior of the original test and continue when a broken symlink is found?

As you did above, you can check that the set of broken symlink names is what you expect.

[liaoyuke]

Address comments.

[vsk]

@liaoyuke I tested this out and found that it breaks the BrokenSymlinkRealFSRecursiveIteration unit test in clang, among some other modules tests. I couldn't find an issue with the test. Mind taking a look?

[liaoyuke]

Of course! I'll look at it later, sorry about the inconvenience.

include/llvm/Support/FileSystem.h

959 ↗

(On Diff #140100)

Good idea!

unittests/Support/Path.cpp

914 ↗

(On Diff #140100)

This is definitely more concise, thanks for letting me know!

[liaoyuke]

Fix unit test errors.

[liaoyuke]

Please hold on reviewing, I've some problem uploading the changes under clang/

[liaoyuke]

This change is now blocked on https://reviews.llvm.org/D45178.

[vsk]

Looks great!

I tested this (and D45178) out with 'check-{llvm,clang,lldb-lit,lldb-unit}'. While applying the patch, I noticed some tab/space inconsistencies which Phab isn't rendering, but that should be easy to fix before committing.

[liaoyuke]

Convert tabs to spaces and fix format.

[liaoyuke]

I've converted all the tabs to spaces, sorry about the trouble. Please take another look. Thank you very much!

[vsk]

No worries, feel free to commit whitespace/formatting fixes without review.

[sammccall]

Sorry for the late followup here, I just found this patch while investigating some misbehavior.

It appears the update_error_code_for_current_entry prevents iterating over a directory without stat()ing every file in it.
This is bad for performance in the common case where only the name/type is required.
(There were other things in fs/vfs causing every file to be stat()ed, they are now fixed).

Moreover, the idea that a dangling symlink is an error for a *directory listing* operation is surprising, and seems like solving problems at the wrong layer.
(e.g. recursive_directory_iterator needs to know if an entry is a dangling symlink, but it should find out using opendir() rather than with stat())

If there are no objections, I'd like to remove this behavior of directory_iterator so we can have fast directory iteration, and find a different fix for recursive_directory_iterator.

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);
+    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 {
-      ErrorOr<basic_file_status> st = State->Stack.top()->status();
-      if (!st) return *this;
-      if (is_directory(*st)) {
+      ErrorOr<basic_file_status> status = State->Stack.top()->status();
+      if (status && is_directory(*status)) {
         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/trunk/tools/llvm-cov/CodeCoverage.cpp

 
 void CodeCoverageTool::collectPaths(const std::string &Path) {
   llvm::sys::fs::file_status Status;
   llvm::sys::fs::status(Path, Status);
   if (!llvm::sys::fs::exists(Status)) {
     if (PathRemapping)
       addCollectedPath(Path);
     else
-      error("Missing source file", Path);
+      warning("Source file doesn't exist, proceeded by ignoring it.", Path);
     return;
   }
 
   if (llvm::sys::fs::is_regular_file(Status)) {
     addCollectedPath(Path);
     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 && !EC; F.increment(EC)) {
+         F != E; F.increment(EC)) {
+
+      if (EC) {
+        warning(EC.message(), F->path());
+        continue;
+      }
+
       if (llvm::sys::fs::is_regular_file(F->path()))
         addCollectedPath(F->path());
     }
-    if (EC)
-      warning(EC.message(), Path);
   }
 }
 
 ErrorOr<const MemoryBuffer &>
 CodeCoverageTool::getSourceFile(StringRef SourceFile) {
   // If we've remapped filenames, look up the real location for this file.
   std::unique_lock<std::mutex> Guard{LoadedSourceFilesLock};
   if (!RemappedFilenames.empty()) {

llvm/trunk/unittests/Support/Path.cpp

TEST_F(FileSystemTest, BrokenSymlinkDirectoryIteration) {
   ASSERT_NO_ERROR(
       fs::create_directories(Twine(TestDirectory) + "/symlink/d/dd/ddd"));
   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 visited;
-
-  // The directory iterator doesn't stat the file, so we should be able to
-  // iterate over the whole directory.
+  v_t VisitedNonBrokenSymlinks;
+  v_t VisitedBrokenSymlinks;
   std::error_code ec;
+
+  // Broken symbol links are expected to throw an error.
   for (fs::directory_iterator i(Twine(TestDirectory) + "/symlink", ec), e;
        i != e; i.increment(ec)) {
-    ASSERT_NO_ERROR(ec);
-    visited.push_back(path::filename(i->path()));
+    if (ec == std::make_error_code(std::errc::no_such_file_or_directory)) {
+      VisitedBrokenSymlinks.push_back(path::filename(i->path()));
+      continue;
     }
-  std::sort(visited.begin(), visited.end());
-  v_t expected = {"a", "b", "c", "d", "e"};
-  ASSERT_TRUE(visited.size() == expected.size());
-  ASSERT_TRUE(std::equal(visited.begin(), visited.end(), expected.begin()));
-  visited.clear();
-
-  // The recursive directory iterator has to stat the file, so we need to skip
-  // the broken symlinks.
-  for (fs::recursive_directory_iterator
-           i(Twine(TestDirectory) + "/symlink", ec),
-       e;
-       i != e; i.increment(ec)) {
-    ASSERT_NO_ERROR(ec);
 
-    ErrorOr<fs::basic_file_status> status = i->status();
-    if (status.getError() ==
-        std::make_error_code(std::errc::no_such_file_or_directory)) {
-      i.no_push();
+    ASSERT_NO_ERROR(ec);
+    VisitedNonBrokenSymlinks.push_back(path::filename(i->path()));
+  }
+  std::sort(VisitedNonBrokenSymlinks.begin(), VisitedNonBrokenSymlinks.end());
+  std::sort(VisitedBrokenSymlinks.begin(), VisitedBrokenSymlinks.end());
+  v_t ExpectedNonBrokenSymlinks = {"b", "d"};
+  ASSERT_EQ(ExpectedNonBrokenSymlinks.size(), VisitedNonBrokenSymlinks.size());
+  ASSERT_TRUE(std::equal(VisitedNonBrokenSymlinks.begin(),
+                         VisitedNonBrokenSymlinks.end(),
+                         ExpectedNonBrokenSymlinks.begin()));
+  VisitedNonBrokenSymlinks.clear();
+
+  v_t ExpectedBrokenSymlinks = {"a", "c", "e"};
+  ASSERT_EQ(ExpectedBrokenSymlinks.size(), VisitedBrokenSymlinks.size());
+  ASSERT_TRUE(std::equal(VisitedBrokenSymlinks.begin(),
+                         VisitedBrokenSymlinks.end(),
+                         ExpectedBrokenSymlinks.begin()));
+  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)) {
+      VisitedBrokenSymlinks.push_back(path::filename(i->path()));
       continue;
     }
 
-    visited.push_back(path::filename(i->path()));
+    ASSERT_NO_ERROR(ec);
+    VisitedNonBrokenSymlinks.push_back(path::filename(i->path()));
   }
-  std::sort(visited.begin(), visited.end());
-  expected = {"b", "bb", "d", "da", "dd", "ddd", "ddd"};
-  ASSERT_TRUE(visited.size() == expected.size());
-  ASSERT_TRUE(std::equal(visited.begin(), visited.end(), expected.begin()));
-  visited.clear();
-
-  // This recursive directory iterator doesn't follow symlinks, so we don't need
-  // to skip them.
-  for (fs::recursive_directory_iterator
-           i(Twine(TestDirectory) + "/symlink", ec, /*follow_symlinks=*/false),
-       e;
+  std::sort(VisitedNonBrokenSymlinks.begin(), VisitedNonBrokenSymlinks.end());
+  std::sort(VisitedBrokenSymlinks.begin(), VisitedBrokenSymlinks.end());
+  ExpectedNonBrokenSymlinks = {"b", "bb", "d", "da", "dd", "ddd", "ddd"};
+  ASSERT_EQ(ExpectedNonBrokenSymlinks.size(), VisitedNonBrokenSymlinks.size());
+  ASSERT_TRUE(std::equal(VisitedNonBrokenSymlinks.begin(),
+                         VisitedNonBrokenSymlinks.end(),
+                         ExpectedNonBrokenSymlinks.begin()));
+  VisitedNonBrokenSymlinks.clear();
+
+  ExpectedBrokenSymlinks = {"a", "ba", "bc", "c", "e"};
+  ASSERT_EQ(ExpectedBrokenSymlinks.size(), VisitedBrokenSymlinks.size());
+  ASSERT_TRUE(std::equal(VisitedBrokenSymlinks.begin(),
+                         VisitedBrokenSymlinks.end(),
+                         ExpectedBrokenSymlinks.begin()));
+  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)) {
+      VisitedBrokenSymlinks.push_back(path::filename(i->path()));
+      continue;
+    }
+
     ASSERT_NO_ERROR(ec);
-    visited.push_back(path::filename(i->path()));
+    VisitedNonBrokenSymlinks.push_back(path::filename(i->path()));
   }
-  std::sort(visited.begin(), visited.end());
-  expected = {"a", "b", "ba", "bb", "bc", "c", "d", "da", "dd", "ddd", "e"};
-  ASSERT_TRUE(visited.size() == expected.size());
-  ASSERT_TRUE(std::equal(visited.begin(), visited.end(), expected.begin()));
+  std::sort(VisitedNonBrokenSymlinks.begin(), VisitedNonBrokenSymlinks.end());
+  std::sort(VisitedBrokenSymlinks.begin(), VisitedBrokenSymlinks.end());
+  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()));
+  VisitedNonBrokenSymlinks.clear();
+
+  ExpectedBrokenSymlinks = {};
+  ASSERT_EQ(ExpectedBrokenSymlinks.size(), VisitedBrokenSymlinks.size());
+  ASSERT_TRUE(std::equal(VisitedBrokenSymlinks.begin(),
+                         VisitedBrokenSymlinks.end(),
+                         ExpectedBrokenSymlinks.begin()));
+  VisitedBrokenSymlinks.clear();
 
   ASSERT_NO_ERROR(fs::remove_directories(Twine(TestDirectory) + "/symlink"));
 }
 #endif
 
 TEST_F(FileSystemTest, Remove) {
   SmallString<64> BaseDir;
   SmallString<64> Paths[4];