Use the canonical path for the include header search.

[felberj]

Summary

This is done so that clangd can propose the correct include path when
a include directory contains a symlink.

[felberj]

This addresses things like

• https://github.com/hedronvision/bazel-compile-commands-extractor/issues/136
• https://github.com/clangd/clangd/issues/124

llvm/lib/Support/VirtualFileSystem.cpp

1165

FYI: This is the first time I worked with this codebase. Is there a better way to do this?

[felberj]

Sorry, I removed the reviewers again, because this might still need some work e2e anymore. I will update this once I resolved that.

[sammccall]

I'm out on vacation the next two weeks, happy to look once back, otherwise @kadircet would be the person to review this on the clangd side.

A couple of general notes:

• people use symlinks in different ways, to give files multiple names, the idea that we can tell which path is "better" is mostly false. Improving one case probably makes another worse.
• at first glance, this seems to change the behavior that FileEntry::getName() starts with the include path as spelled in -I etc. This seems like a significant change that is likely to impact a lot of places that may be under-tested, like spellings of filenames in diagnostics.

clang/lib/Lex/InitHeaderSearch.cpp

179

Non-equality doesn't mean it's a symlink, some parent could be, or it could simply have been a relative path (can those get here?)

[felberj]

Thanks a lot for your notes! Unfortunately I noticed that while the test that I created passed, my actual issue [0] still persists and I sent out the change too early. It seems you are right and the symlink handling is in fact more nuanced.

Enjoy your vacation! I will go on vacation soon as well, so no hurries with the review.

[0] https://github.com/hedronvision/bazel-compile-commands-extractor/issues/136

clang-tools-extra/clangd/unittests/HeadersTests.cpp

TEST_F(HeadersTest, SearchPath) {
   FS.Files["foo/bar.h"] = "x";
   FS.Files["foo/bar/baz.h"] = "y";
   CDB.ExtraClangFlags.push_back("-Ifoo/bar");
   CDB.ExtraClangFlags.push_back("-Ifoo/bar/..");
   EXPECT_THAT(collectIncludes().SearchPathsCanonical,
               ElementsAre(Subdir, testPath("foo/bar"), testPath("foo")));
 }
 
+TEST_F(HeadersTest, SymlinkedSearchPath) {
+  MainFile = testPath("sub/main.cpp");
+  std::string Path = testPath("outer/foo/bar.h");
+  FS.Files[Path] = "";
+
+  std::string Symlink = testPath("sub/symlink");
+  FS.Symlinks[Symlink] = "../outer";
+
+  SearchDirArg = (llvm::Twine("-I") + Symlink).str();
+  CDB.ExtraClangFlags = {SearchDirArg.c_str()};
+  std::string test = testPath("outer/foo/bar.h");
+  EXPECT_EQ(calculate(test), "\"foo/bar.h\"");
+}
+
 TEST_F(HeadersTest, InsertInclude) {
   std::string Path = testPath("sub/bar.h");
   FS.Files[Path] = "";
   EXPECT_EQ(calculate(Path), "\"bar.h\"");
 }
 
 TEST_F(HeadersTest, DoNotInsertIfInSameFile) {
   MainFile = testPath("main.h");

clang-tools-extra/clangd/unittests/TestFS.h

 
 namespace clang {
 namespace clangd {
 
 // Builds a VFS that provides access to the provided files, plus temporary
 // directories.
 llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>
 buildTestFS(llvm::StringMap<std::string> const &Files,
+            llvm::StringMap<std::string> const &Symlinks = {},
             llvm::StringMap<time_t> const &Timestamps = {});
 
 // A VFS provider that returns TestFSes containing a provided set of files.
 class MockFS : public ThreadsafeFS {
 public:
   IntrusiveRefCntPtr<llvm::vfs::FileSystem> viewImpl() const override {
-    auto MemFS = buildTestFS(Files, Timestamps);
+    auto MemFS = buildTestFS(Files, Symlinks, Timestamps);
     if (!OverlayRealFileSystemForModules)
       return MemFS;
     llvm::IntrusiveRefCntPtr<llvm::vfs::OverlayFileSystem> OverlayFileSystem =
         new llvm::vfs::OverlayFileSystem(llvm::vfs::getRealFileSystem());
     OverlayFileSystem->pushOverlay(MemFS);
     return OverlayFileSystem;
   }
 
   // If relative paths are used, they are resolved with testPath().
   llvm::StringMap<std::string> Files;
+  llvm::StringMap<std::string> Symlinks;
   llvm::StringMap<time_t> Timestamps;
   // If true, real file system will be used as fallback for the in-memory one.
   // This is useful for testing module support.
   bool OverlayRealFileSystemForModules = false;
 };
 
 // A Compilation database that returns a fixed set of compile flags.
 class MockCompilationDatabase : public GlobalCompilationDatabase {

clang-tools-extra/clangd/unittests/TestFS.cpp

if (!pathStartsWith(Prefix, Path))
     return false;
   Path = Path.drop_front(Prefix.size());
   return true;
 }
 } // namespace
 
 llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>
 buildTestFS(llvm::StringMap<std::string> const &Files,
+            llvm::StringMap<std::string> const &Symlinks,
             llvm::StringMap<time_t> const &Timestamps) {
   llvm::IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> MemFS(
       new llvm::vfs::InMemoryFileSystem);
   MemFS->setCurrentWorkingDirectory(testRoot());
   for (auto &FileAndContents : Files) {
     llvm::StringRef File = FileAndContents.first();
     MemFS->addFile(
         File, Timestamps.lookup(File),
         llvm::MemoryBuffer::getMemBufferCopy(FileAndContents.second, File));
   }
+  for (auto &SymlinkAndContents : Symlinks) {
+    llvm::StringRef Src = SymlinkAndContents.first();
+    llvm::StringRef Dest = SymlinkAndContents.second;
+    MemFS->addSymbolicLink(Src, Dest, Timestamps.lookup(Src));
+  }
   return MemFS;
 }
 
 MockCompilationDatabase::MockCompilationDatabase(llvm::StringRef Directory,
                                                  llvm::StringRef RelPathPrefix)
     : ExtraClangFlags({"-ffreestanding"}), Directory(Directory),
       RelPathPrefix(RelPathPrefix) {
   // -ffreestanding avoids implicit stdc-predef.h.

clang/lib/Lex/InitHeaderSearch.cpp

bool InitHeaderSearch::AddUnmappedPath(const Twine &Path, IncludeDirGroup Group,
   } else if (Group == ExternCSystem) {
     Type = SrcMgr::C_ExternCSystem;
   } else {
     Type = SrcMgr::C_System;
   }
 
   // If the directory exists, add it.
   if (auto DE = FM.getOptionalDirectoryRef(MappedPathStr)) {
+    StringRef canonical = FM.getCanonicalName(*DE);
+    if (canonical == MappedPathStr) {
+      // It is a normal directory
       IncludePath.emplace_back(Group, DirectoryLookup(*DE, Type, isFramework),
                                UserEntryIdx);
       return true;
     }
+    if (Verbose) {
+      llvm::errs() << "rewrite the include " << MappedPathStr
+                   << " to its canonical path " << canonical << "\n";
+    }
+    // If it is a symlink, we add the canonical path.
+    if (auto cDE = FM.getOptionalDirectoryRef(canonical)) {
+      IncludePath.emplace_back(Group, DirectoryLookup(*cDE, Type, isFramework),
+                               UserEntryIdx);
+      return true;
+    }
+  }
 
   // Check to see if this is an apple-style headermap (which are not allowed to
   // be frameworks).
   if (!isFramework) {
     if (auto FE = FM.getFile(MappedPathStr)) {
       if (const HeaderMap *HM = Headers.CreateHeaderMap(*FE)) {
         // It is a headermap, add it to the search path.
         IncludePath.emplace_back(

llvm/lib/Support/VirtualFileSystem.cpp

InMemoryFileSystem::getRealPath(const Twine &Path,
                                 SmallVectorImpl<char> &Output) const {
   auto CWD = getCurrentWorkingDirectory();
   if (!CWD || CWD->empty())
     return errc::operation_not_permitted;
   Path.toVector(Output);
   if (auto EC = makeAbsolute(Output))
     return EC;
   llvm::sys::path::remove_dots(Output, /*remove_dot_dot=*/true);
+  if (auto Node = lookupNode(Output, true)) {
+    auto Name = Node.getName();
+    Output.clear();
+    Output.append(Name.begin(), Name.end());
+  }
   return {};
 }
 
 std::error_code InMemoryFileSystem::isLocal(const Twine &Path, bool &Result) {
   Result = false;
   return {};
 }