Index: clang-tools-extra/trunk/clangd/FS.h =================================================================== --- clang-tools-extra/trunk/clangd/FS.h +++ clang-tools-extra/trunk/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 @@ llvm::StringMap 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 Index: clang-tools-extra/trunk/clangd/FS.cpp =================================================================== --- clang-tools-extra/trunk/clangd/FS.cpp +++ clang-tools-extra/trunk/clangd/FS.cpp @@ -111,5 +111,11 @@ return llvm::IntrusiveRefCntPtr(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 Index: clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.cpp =================================================================== --- clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.cpp +++ clang-tools-extra/trunk/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 @@ -147,12 +149,15 @@ 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 @@ 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 @@ llvm::Optional Cmd; { std::lock_guard Lock(Mutex); - auto It = Commands.find(File); + auto It = Commands.find(removeDots(File)); if (It != Commands.end()) Cmd = It->second; } @@ -272,20 +278,24 @@ void OverlayCDB::setCompileCommand( PathRef File, llvm::Optional 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 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 OverlayCDB::getProjectInfo(PathRef File) const { { std::lock_guard Lock(Mutex); - auto It = Commands.find(File); + auto It = Commands.find(removeDots(File)); if (It != Commands.end()) return ProjectInfo{}; } Index: clang-tools-extra/trunk/clangd/unittests/GlobalCompilationDatabaseTests.cpp =================================================================== --- clang-tools-extra/trunk/clangd/unittests/GlobalCompilationDatabaseTests.cpp +++ clang-tools-extra/trunk/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::Contains; using ::testing::ElementsAre; using ::testing::EndsWith; +using ::testing::HasSubstr; using ::testing::IsEmpty; using ::testing::Not; using ::testing::StartsWith; @@ -247,9 +249,10 @@ }); 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, NonCanonicalFilenames) { + OverlayCDB DB(nullptr); + std::vector DiscoveredFiles; + auto Sub = + DB.watch([&DiscoveredFiles](const std::vector 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