[clangd] Get rid of dots and dotsdots within GlobalCompilationDatabase

[kadircet]

Summary

These bugs has surfaced during the refactoring D64712.
There are some test cases that passes paths with ".." in them to OverlayCDB and DirectoryBasedCDB, which were not failing before because (tested) users of these functions were handling the canonicalization.

It is also possible to have an LSP client sending over compile commands over the wire without canonicalizing them.
As for the second bug, it is easier to trigger for example a json comp db with an entry ("file": "/abs/path/build/../a.cc") will trigger that.

I am not sure if there are build systems generating this kind of output, but it looks valid from a comp db perspective, whereas it doesn't comply with clangd's assumption.

[sammccall]

clang-tools-extra/clangd/GlobalCompilationDatabase.cpp

77 ↗

(On Diff #210299)

This patch lacks a bit of context (why are we changing this behavior).
Based on offline discussion, it's not clear to me why this particular change to getFallbackCommand is necessary or desirable. (Compared to others, which seem like fairly obvious bugs, even if the reason to fix them isn't obvious)

144 ↗

(On Diff #210299)

nit: can you pull out a local (or FS.h) RemoveDots helper that accepts StringRef and returns std::string?

Most of the usages in this file could be inlined and I think that would read better. Not concerned about performance I think. (But returning SmallString is OK if you are)

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

307 ↗

(On Diff #210299)

Is this actually important/desirable?

[kadircet]

• Add removeDots helper to FS.h
• Revert changes in getFallbackCommands.
• Add comments for the reasoning behind removeDots calls.

clang-tools-extra/clangd/GlobalCompilationDatabase.cpp

77 ↗

(On Diff #210299)

updating the summary for reasoning

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

307 ↗

(On Diff #210299)

yea agreed this is not necessary.

[sammccall]

As discussed, the long-term fix might be to avoid ever encountering these paths (filtering everything we get over LSP, CDBs etc).
Maybe add a fixme on removeDots?

[kadircet]

• Address comments

clang-tools-extra/trunk/clangd/FS.h

 //===--- FS.h - File system related utils ------------------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #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"
 
 namespace clang {
 namespace clangd {
 
 /// Records status information for files open()ed or stat()ed during preamble
public:
   IntrusiveRefCntPtr<llvm::vfs::FileSystem>
   getConsumingFS(IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS) const;
 
 private:
   std::string MainFilePath;
   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
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_FS_H

clang-tools-extra/trunk/clangd/FS.cpp

public:
     }
 
   private:
     const PreambleFileStatusCache &StatCache;
   };
   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/trunk/clangd/GlobalCompilationDatabase.cpp

 //===--- GlobalCompilationDatabase.cpp ---------------------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "GlobalCompilationDatabase.h"
+#include "FS.h"
 #include "Logger.h"
 #include "Path.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Tooling/ArgumentsAdjusters.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #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>
 #include <tuple>
 #include <vector>
 
 namespace clang {
 namespace clangd {
DirectoryBasedGlobalCompilationDatabase::lookupCDB(
 
   {
     std::lock_guard<std::mutex> Lock(Mutex);
     if (CompileCommandsDir) {
       std::tie(Result.CDB, SentBroadcast) =
           getCDBInDirLocked(*CompileCommandsDir);
       Result.PI.SourceRoot = *CompileCommandsDir;
     } else {
-      actOnAllParentDirectories(
-          Request.FileName, [this, &SentBroadcast, &Result](PathRef Path) {
-            std::tie(Result.CDB, SentBroadcast) = getCDBInDirLocked(Path);
+      // 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)
       return llvm::None;
 
     // Mark CDB as broadcasted to make sure discovery is performed once.
     if (Request.ShouldBroadcast && !SentBroadcast)
       CompilationDatabases[Result.PI.SourceRoot].SentBroadcast = true;
void DirectoryBasedGlobalCompilationDatabase::broadcastCDB(
 
   std::vector<std::string> GovernedFiles;
   for (llvm::StringRef File : AllFiles) {
     // A file is governed by this CDB if lookup for the file would find it.
     // Independent of whether it has an entry for that file or not.
     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;
       }
       return false;
     });
   }
 
   OnCommandChanged.broadcast(std::move(GovernedFiles));
if (Base)
     });
 }
 
 llvm::Optional<tooling::CompileCommand>
 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;
   }
   if (!Cmd && Base)
     Cmd = Base->getCompileCommand(File);
   if (!Cmd)
     return llvm::None;
   adjustArguments(*Cmd, ResourceDir);
   return Cmd;
 }
 
 tooling::CompileCommand OverlayCDB::getFallbackCommand(PathRef File) const {
   auto Cmd = Base ? Base->getFallbackCommand(File)
                   : GlobalCompilationDatabase::getFallbackCommand(File);
   std::lock_guard<std::mutex> Lock(Mutex);
   Cmd.CommandLine.insert(Cmd.CommandLine.end(), FallbackFlags.begin(),
                          FallbackFlags.end());
   adjustArguments(Cmd, ResourceDir);
   return Cmd;
 }
 
 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{};
   }
   if (Base)
     return Base->getProjectInfo(File);
 
   return llvm::None;
 }
 } // namespace clangd
 } // namespace clang

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

 //===-- GlobalCompilationDatabaseTests.cpp ----------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "GlobalCompilationDatabase.h"
 
 #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"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include <fstream>
 #include <string>
 
 namespace clang {
 namespace clangd {
 namespace {
 using ::testing::AllOf;
 using ::testing::Contains;
 using ::testing::ElementsAre;
 using ::testing::EndsWith;
+using ::testing::HasSubstr;
 using ::testing::IsEmpty;
 using ::testing::Not;
 using ::testing::StartsWith;
 using ::testing::UnorderedElementsAre;
 
 TEST(GlobalCompilationDatabaseTest, FallbackCommand) {
   DirectoryBasedGlobalCompilationDatabase DB(None);
   auto Cmd = DB.getFallbackCommand(testPath("foo/bar.cc"));
// happens in practice though.
     DirectoryBasedGlobalCompilationDatabase DB(llvm::None);
     std::vector<std::string> DiscoveredFiles;
     auto Sub =
         DB.watch([&DiscoveredFiles](const std::vector<std::string> Changes) {
           DiscoveredFiles = Changes;
         });
 
     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;
     llvm::sys::path::append(File, "build", "gen.cc");
     DB.getCompileCommand(File.str());
     EXPECT_THAT(DiscoveredFiles, UnorderedElementsAre(EndsWith("gen.cc")));
   }
 
// With a custom compile commands dir.
 
     File = FS.Root;
     llvm::sys::path::append(File, "build", "gen.cc");
     DB.getCompileCommand(File.str());
     EXPECT_THAT(DiscoveredFiles, IsEmpty());
   }
 }
 
+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