Merged this and r366559 to the 9 branch in r366718.
On Thu, Jul 18, 2019 at 9:13 AM Kadir Cetinkaya via cfe-commits <cfe-commits@lists.llvm.org> wrote: > > Author: kadircet > Date: Thu Jul 18 09:13:23 2019 > New Revision: 366455 > > URL: http://llvm.org/viewvc/llvm-project?rev=366455&view=rev > Log: > [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 > > Modified: > clang-tools-extra/trunk/clangd/FS.cpp > clang-tools-extra/trunk/clangd/FS.h > clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.cpp > > clang-tools-extra/trunk/clangd/unittests/GlobalCompilationDatabaseTests.cpp > > Modified: clang-tools-extra/trunk/clangd/FS.cpp > URL: > http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/FS.cpp?rev=366455&r1=366454&r2=366455&view=diff > ============================================================================== > --- clang-tools-extra/trunk/clangd/FS.cpp (original) > +++ clang-tools-extra/trunk/clangd/FS.cpp Thu Jul 18 09:13:23 2019 > @@ -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 > > Modified: clang-tools-extra/trunk/clangd/FS.h > URL: > http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/FS.h?rev=366455&r1=366454&r2=366455&view=diff > ============================================================================== > --- clang-tools-extra/trunk/clangd/FS.h (original) > +++ clang-tools-extra/trunk/clangd/FS.h Thu Jul 18 09:13:23 2019 > @@ -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 > > > Modified: clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.cpp > URL: > http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.cpp?rev=366455&r1=366454&r2=366455&view=diff > ============================================================================== > --- clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.cpp (original) > +++ clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.cpp Thu Jul 18 > 09:13:23 2019 > @@ -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: > 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 DirectoryBasedGlobalCompilationData > 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 Fi > 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::getF > > 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{}; > } > > Modified: > clang-tools-extra/trunk/clangd/unittests/GlobalCompilationDatabaseTests.cpp > URL: > http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/unittests/GlobalCompilationDatabaseTests.cpp?rev=366455&r1=366454&r2=366455&view=diff > ============================================================================== > --- > clang-tools-extra/trunk/clangd/unittests/GlobalCompilationDatabaseTests.cpp > (original) > +++ > clang-tools-extra/trunk/clangd/unittests/GlobalCompilationDatabaseTests.cpp > Thu Jul 18 09:13:23 2019 > @@ -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, Disc > }); > > 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, Disc > } > } > > +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 > > > _______________________________________________ > cfe-commits mailing list > cfe-commits@lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits