[clangd] Get rid of dots and dotsdots within GlobalCompilationDatabase

Reviewers: sammccall

Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D64860

llvm-svn: 366455

clang-tools-extra/clangd/FS.cpp

@@ -111,5 +111,11 @@ PreambleFileStatusCache::getConsumingFS(
   return llvm::IntrusiveRefCntPtr<CacheVFS>(new CacheVFS(std::move(FS), *this));
 }
 
+Path removeDots(PathRef File) {
+  llvm::SmallString<128> CanonPath(File);
+  llvm::sys::path::remove_dots(CanonPath, /*remove_dot_dot=*/true);
+  return CanonPath.str().str();
+}
+
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/FS.h

@@ -9,6 +9,7 @@
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_FS_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_FS_H
 
+#include "Path.h"
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/Support/VirtualFileSystem.h"
@@ -65,6 +66,13 @@ private:
   llvm::StringMap<llvm::vfs::Status> StatCache;
 };
 
+/// Returns a version of \p File that doesn't contain dots and dot dots.
+/// e.g /a/b/../c -> /a/c
+///     /a/b/./c -> /a/b/c
+/// FIXME: We should avoid encountering such paths in clangd internals by
+/// filtering everything we get over LSP, CDB, etc.
+Path removeDots(PathRef File);
+
 } // namespace clangd
 } // namespace clang
 

clang-tools-extra/clangd/GlobalCompilationDatabase.cpp

@@ -7,6 +7,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "GlobalCompilationDatabase.h"
+#include "FS.h"
 #include "Logger.h"
 #include "Path.h"
 #include "clang/Frontend/CompilerInvocation.h"
@@ -15,6 +16,7 @@
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallString.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 #include <string>
@@ -147,12 +149,15 @@ DirectoryBasedGlobalCompilationDatabase::lookupCDB(
           getCDBInDirLocked(*CompileCommandsDir);
       Result.PI.SourceRoot = *CompileCommandsDir;
     } else {
-      actOnAllParentDirectories(
-          Request.FileName, [this, &SentBroadcast, &Result](PathRef Path) {
-            std::tie(Result.CDB, SentBroadcast) = getCDBInDirLocked(Path);
-            Result.PI.SourceRoot = Path;
-            return Result.CDB != nullptr;
-          });
+      // Traverse the canonical version to prevent false positives. i.e.:
+      // src/build/../a.cc can detect a CDB in /src/build if not canonicalized.
+      actOnAllParentDirectories(removeDots(Request.FileName),
+                                [this, &SentBroadcast, &Result](PathRef Path) {
+                                  std::tie(Result.CDB, SentBroadcast) =
+                                      getCDBInDirLocked(Path);
+                                  Result.PI.SourceRoot = Path;
+                                  return Result.CDB != nullptr;
+                                });
     }
 
     if (!Result.CDB)
@@ -209,7 +214,8 @@ void DirectoryBasedGlobalCompilationDatabase::broadcastCDB(
     actOnAllParentDirectories(File, [&](PathRef Path) {
       if (DirectoryHasCDB.lookup(Path)) {
         if (Path == Result.PI.SourceRoot)
-          GovernedFiles.push_back(File);
+          // Make sure listeners always get a canonical path for the file.
+          GovernedFiles.push_back(removeDots(File));
         // Stop as soon as we hit a CDB.
         return true;
       }
@@ -248,7 +254,7 @@ OverlayCDB::getCompileCommand(PathRef File) const {
   llvm::Optional<tooling::CompileCommand> Cmd;
   {
     std::lock_guard<std::mutex> Lock(Mutex);
-    auto It = Commands.find(File);
+    auto It = Commands.find(removeDots(File));
     if (It != Commands.end())
       Cmd = It->second;
   }
@@ -272,20 +278,24 @@ tooling::CompileCommand OverlayCDB::getFallbackCommand(PathRef File) const {
 
 void OverlayCDB::setCompileCommand(
     PathRef File, llvm::Optional<tooling::CompileCommand> Cmd) {
+  // We store a canonical version internally to prevent mismatches between set
+  // and get compile commands. Also it assures clients listening to broadcasts
+  // doesn't receive different names for the same file.
+  std::string CanonPath = removeDots(File);
   {
     std::unique_lock<std::mutex> Lock(Mutex);
     if (Cmd)
-      Commands[File] = std::move(*Cmd);
+      Commands[CanonPath] = std::move(*Cmd);
     else
-      Commands.erase(File);
+      Commands.erase(CanonPath);
   }
-  OnCommandChanged.broadcast({File});
+  OnCommandChanged.broadcast({CanonPath});
 }
 
 llvm::Optional<ProjectInfo> OverlayCDB::getProjectInfo(PathRef File) const {
   {
     std::lock_guard<std::mutex> Lock(Mutex);
-    auto It = Commands.find(File);
+    auto It = Commands.find(removeDots(File));
     if (It != Commands.end())
       return ProjectInfo{};
   }

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

@@ -10,6 +10,7 @@
 
 #include "Path.h"
 #include "TestFS.h"
+#include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringExtras.h"
@@ -31,6 +32,7 @@ using ::testing::AllOf;
 using ::testing::Contains;
 using ::testing::ElementsAre;
 using ::testing::EndsWith;
+using ::testing::HasSubstr;
 using ::testing::IsEmpty;
 using ::testing::Not;
 using ::testing::StartsWith;
@@ -247,9 +249,10 @@ TEST(GlobalCompilationDatabaseTest, DiscoveryWithNestedCDBs) {
         });
 
     File = FS.Root;
-    llvm::sys::path::append(File, "a.cc");
+    llvm::sys::path::append(File, "build", "..", "a.cc");
     DB.getCompileCommand(File.str());
-    EXPECT_THAT(DiscoveredFiles, UnorderedElementsAre(EndsWith("a.cc")));
+    EXPECT_THAT(DiscoveredFiles, UnorderedElementsAre(AllOf(
+                                     EndsWith("a.cc"), Not(HasSubstr("..")))));
     DiscoveredFiles.clear();
 
     File = FS.Root;
@@ -282,6 +285,27 @@ TEST(GlobalCompilationDatabaseTest, DiscoveryWithNestedCDBs) {
   }
 }
 
+TEST(GlobalCompilationDatabaseTest, NonCanonicalFilenames) {
+  OverlayCDB DB(nullptr);
+  std::vector<std::string> DiscoveredFiles;
+  auto Sub =
+      DB.watch([&DiscoveredFiles](const std::vector<std::string> Changes) {
+        DiscoveredFiles = Changes;
+      });
+
+  llvm::SmallString<128> Root(testRoot());
+  llvm::sys::path::append(Root, "build", "..", "a.cc");
+  DB.setCompileCommand(Root.str(), tooling::CompileCommand());
+  EXPECT_THAT(DiscoveredFiles, UnorderedElementsAre(testPath("a.cc")));
+  DiscoveredFiles.clear();
+
+  llvm::SmallString<128> File(testRoot());
+  llvm::sys::path::append(File, "blabla", "..", "a.cc");
+
+  EXPECT_TRUE(DB.getCompileCommand(File));
+  EXPECT_TRUE(DB.getProjectInfo(File));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang