[PATCH][VFS] Ignore broken symlinks in the directory iterator.

[ributzka]

Summary

The VFS directory iterator and recursive directory iterator behave differently
from the LLVM counterparts. Once the VFS iterators hit a broken symlink they
immediately abort. The LLVM counterparts allow to recover from this issue by
clearing the error code and skipping to the next entry.

This change adds the same functionality to the VFS iterators. There should be
no change in current behavior in the current CLANG source base, because all
clients have loop exit conditions that also check the error code.

[bruno]

Hi Juergen,

Thanks for working on this.

include/clang/Basic/VirtualFileSystem.h

164 ↗

(On Diff #91113)

I would rather we don't drop checks for ECs - it's commonly assumed that whenever they are passed in we wanna check them for errors, can't you just skip the specific case you want to avoid?

lib/Basic/VirtualFileSystem.cpp

1860 ↗

(On Diff #91113)

Same here.

1873 ↗

(On Diff #91113)

Can you add a comment explaining why you are doing it? I would prefer if we reset the EC state here than having the callers ignoring EC results.

[ributzka]

lib/Basic/VirtualFileSystem.cpp

1873 ↗

(On Diff #91113)

If we reset the EC here, then the caller won't know that there was an issue. The idea is that we still want the caller to check EC. It should be up to the caller to decide how to act on this particular error.

I guess since the caller has to check for the error anyway I could even remove this check completely and not check EC at all here.

[ributzka]

include/clang/Basic/VirtualFileSystem.h

164 ↗

(On Diff #91113)

EC is an output parameter here. The client is supposed to check it.

[ributzka]

Remove the EC check completely.

[bruno]

lib/Basic/VirtualFileSystem.cpp

1873 ↗

(On Diff #91113)

Right, this should be fine with the latest changes and per discussions offline. LGTM

[ributzka]

Thanks Bruno. Committed in r297510.

cfe/trunk/include/clang/Basic/VirtualFileSystem.h

public:
 
   /// \brief Construct an 'end' iterator.
   directory_iterator() = default;
 
   /// \brief Equivalent to operator++, with an error code.
   directory_iterator &increment(std::error_code &EC) {
     assert(Impl && "attempting to increment past end");
     EC = Impl->increment();
-    if (EC || !Impl->CurrentEntry.isStatusKnown())
+    if (!Impl->CurrentEntry.isStatusKnown())
       Impl.reset(); // Normalize the end iterator to Impl == nullptr.
     return *this;
   }
 
   const Status &operator*() const { return Impl->CurrentEntry; }
   const Status *operator->() const { return &Impl->CurrentEntry; }
 
   bool operator==(const directory_iterator &RHS) const {

cfe/trunk/lib/Basic/VirtualFileSystem.cpp

class RealFSDirIter : public clang::vfs::detail::DirIterImpl {
   std::string Path;
   llvm::sys::fs::directory_iterator Iter;
 public:
   RealFSDirIter(const Twine &_Path, std::error_code &EC)
       : Path(_Path.str()), Iter(Path, EC) {
     if (!EC && Iter != llvm::sys::fs::directory_iterator()) {
       llvm::sys::fs::file_status S;
       EC = Iter->status(S);
-      if (!EC)
-        CurrentEntry = Status::copyWithNewName(S, Iter->path());
     }
   }
 
   std::error_code increment() override {
     std::error_code EC;
     Iter.increment(EC);
     if (EC) {
       return EC;
std::error_code VFSFromYamlDirIterImpl::increment() {
   return std::error_code();
 }
 
 vfs::recursive_directory_iterator::recursive_directory_iterator(FileSystem &FS_,
                                                            const Twine &Path,
                                                            std::error_code &EC)
     : FS(&FS_) {
   directory_iterator I = FS->dir_begin(Path, EC);
-  if (!EC && I != directory_iterator()) {
+  if (I != directory_iterator()) {
     State = std::make_shared<IterState>();
     State->push(I);
   }
 }
 
 vfs::recursive_directory_iterator &
 recursive_directory_iterator::increment(std::error_code &EC) {
   assert(FS && State && !State->empty() && "incrementing past end");
   assert(State->top()->isStatusKnown() && "non-canonical end iterator");
   vfs::directory_iterator End;
   if (State->top()->isDirectory()) {
     vfs::directory_iterator I = FS->dir_begin(State->top()->getName(), EC);
-    if (EC)
-      return *this;
     if (I != End) {
       State->push(I);
       return *this;
     }
   }
 
   while (!State->empty() && State->top().increment(EC) == End)
     State->pop();
 
   if (State->empty())
     State.reset(); // end iterator
 
   return *this;
 }

cfe/trunk/unittests/Basic/VirtualFileSystemTest.cpp

ScopedDir(const Twine &Name, bool Unique=false) {
     EXPECT_FALSE(EC);
   }
   ~ScopedDir() {
     if (Path != "")
       EXPECT_FALSE(llvm::sys::fs::remove(Path.str()));
   }
   operator StringRef() { return Path.str(); }
 };
+
+struct ScopedLink {
+  SmallString<128> Path;
+  ScopedLink(const Twine &To, const Twine &From) {
+    Path = From.str();
+    std::error_code EC = sys::fs::create_link(To, From);
+    if (EC)
+      Path = "";
+    EXPECT_FALSE(EC);
+  }
+  ~ScopedLink() {
+    if (Path != "")
+      EXPECT_FALSE(llvm::sys::fs::remove(Path.str()));
+  }
+  operator StringRef() { return Path.str(); }
+};
 } // end anonymous namespace
 
 TEST(VirtualFileSystemTest, BasicRealFSIteration) {
   ScopedDir TestDirectory("virtual-file-system-test", /*Unique*/true);
   IntrusiveRefCntPtr<vfs::FileSystem> FS = vfs::getRealFileSystem();
 
   std::error_code EC;
   vfs::directory_iterator I = FS->dir_begin(Twine(TestDirectory), EC);
TEST(VirtualFileSystemTest, BasicRealFSIteration) {
   I.increment(EC);
   ASSERT_FALSE(EC);
   ASSERT_NE(vfs::directory_iterator(), I);
   EXPECT_TRUE(I->getName().endswith("a") || I->getName().endswith("c"));
   I.increment(EC);
   EXPECT_EQ(vfs::directory_iterator(), I);
 }
 
+TEST(VirtualFileSystemTest, BrokenSymlinkRealFSIteration) {
+  ScopedDir TestDirectory("virtual-file-system-test", /*Unique*/ true);
+  IntrusiveRefCntPtr<vfs::FileSystem> FS = vfs::getRealFileSystem();
+
+  ScopedLink _a("no_such_file", TestDirectory + "/a");
+  ScopedDir _b(TestDirectory + "/b");
+  ScopedLink _c("no_such_file", TestDirectory + "/c");
+
+  std::error_code EC;
+  vfs::directory_iterator I = FS->dir_begin(Twine(TestDirectory), EC);
+  EXPECT_TRUE(EC);
+  EXPECT_NE(vfs::directory_iterator(), I);
+  EC = std::error_code();
+  EXPECT_TRUE(I->getName() == _a);
+  I.increment(EC);
+  EXPECT_FALSE(EC);
+  EXPECT_NE(vfs::directory_iterator(), I);
+  EXPECT_TRUE(I->getName() == _b);
+  I.increment(EC);
+  EXPECT_TRUE(EC);
+  EXPECT_NE(vfs::directory_iterator(), I);
+  EC = std::error_code();
+  EXPECT_NE(vfs::directory_iterator(), I);
+  EXPECT_TRUE(I->getName() == _c);
+  I.increment(EC);
+  EXPECT_FALSE(EC);
+  EXPECT_EQ(vfs::directory_iterator(), I);
+}
+
 TEST(VirtualFileSystemTest, BasicRealFSRecursiveIteration) {
   ScopedDir TestDirectory("virtual-file-system-test", /*Unique*/true);
   IntrusiveRefCntPtr<vfs::FileSystem> FS = vfs::getRealFileSystem();
 
   std::error_code EC;
   auto I = vfs::recursive_directory_iterator(*FS, Twine(TestDirectory), EC);
   ASSERT_FALSE(EC);
   EXPECT_EQ(vfs::recursive_directory_iterator(), I); // empty directory is empty
for (const std::string &Name : Contents) {
     Counts[Index]++;
   }
   EXPECT_EQ(1, Counts[0]); // a
   EXPECT_EQ(1, Counts[1]); // b
   EXPECT_EQ(1, Counts[2]); // c
   EXPECT_EQ(1, Counts[3]); // d
 }
 
+TEST(VirtualFileSystemTest, BrokenSymlinkRealFSRecursiveIteration) {
+  ScopedDir TestDirectory("virtual-file-system-test", /*Unique*/ true);
+  IntrusiveRefCntPtr<vfs::FileSystem> FS = vfs::getRealFileSystem();
+
+  ScopedLink _a("no_such_file", TestDirectory + "/a");
+  ScopedDir _b(TestDirectory + "/b");
+  ScopedLink _ba("no_such_file", TestDirectory + "/b/a");
+  ScopedDir _bb(TestDirectory + "/b/b");
+  ScopedLink _bc("no_such_file", TestDirectory + "/b/c");
+  ScopedLink _c("no_such_file", TestDirectory + "/c");
+  ScopedDir _d(TestDirectory + "/d");
+  ScopedDir _dd(TestDirectory + "/d/d");
+  ScopedDir _ddd(TestDirectory + "/d/d/d");
+  ScopedLink _e("no_such_file", TestDirectory + "/e");
+
+  std::vector<std::string> Contents;
+  std::error_code EC;
+  for (vfs::recursive_directory_iterator I(*FS, Twine(TestDirectory), EC), E;
+       I != E; I.increment(EC)) {
+    // Skip broken symlinks.
+    if (EC == std::errc::no_such_file_or_directory) {
+      EC = std::error_code();
+      continue;
+    } else if (EC) {
+      break;
+    }
+    Contents.push_back(I->getName());
+  }
+
+  // Check contents.
+  EXPECT_EQ(5U, Contents.size());
+  EXPECT_TRUE(Contents[0] == _b);
+  EXPECT_TRUE(Contents[1] == _bb);
+  EXPECT_TRUE(Contents[2] == _d);
+  EXPECT_TRUE(Contents[3] == _dd);
+  EXPECT_TRUE(Contents[4] == _ddd);
+}
+
 template <typename DirIter>
 static void checkContents(DirIter I, ArrayRef<StringRef> ExpectedOut) {
   std::error_code EC;
   SmallVector<StringRef, 4> Expected(ExpectedOut.begin(), ExpectedOut.end());
   SmallVector<std::string, 4> InputToCheck;
 
   // Do not rely on iteration order to check for contents, sort both
   // content vectors before comparison.