[clang-tools-extra] r366455 - [clangd] Get rid of dots and dotsdots within GlobalCompilationDatabase

Hans Wennborg via cfe-commits cfe-commits at lists.llvm.org
Mon Jul 22 11:17:43 PDT 2019


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 at 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 at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


More information about the cfe-commits mailing list