[clang-tools-extra] 8bd000a - [clangd] Config: loading and caching config from disk.
Sam McCall via cfe-commits
cfe-commits at lists.llvm.org
Sat Jul 4 01:53:02 PDT 2020
Author: Sam McCall
Date: 2020-07-04T10:48:31+02:00
New Revision: 8bd000a65fe4452c09855115d5204a2a46838004
URL: https://github.com/llvm/llvm-project/commit/8bd000a65fe4452c09855115d5204a2a46838004
DIFF: https://github.com/llvm/llvm-project/commit/8bd000a65fe4452c09855115d5204a2a46838004.diff
LOG: [clangd] Config: loading and caching config from disk.
Summary:
The Provider extension point is designed to also be implemented by
ClangdLSPServer (to inject config-over-lsp) and likely by embedders.
Reviewers: kadircet
Subscribers: mgorny, ilya-biryukov, MaskRay, jkorous, arphaman, jfb, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D82964
Added:
clang-tools-extra/clangd/ConfigProvider.cpp
clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
Modified:
clang-tools-extra/clangd/CMakeLists.txt
clang-tools-extra/clangd/ConfigProvider.h
clang-tools-extra/clangd/unittests/CMakeLists.txt
clang-tools-extra/clangd/unittests/TestFS.cpp
clang-tools-extra/clangd/unittests/TestFS.h
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/CMakeLists.txt b/clang-tools-extra/clangd/CMakeLists.txt
index 916826a8679b..9eb06941e4dd 100644
--- a/clang-tools-extra/clangd/CMakeLists.txt
+++ b/clang-tools-extra/clangd/CMakeLists.txt
@@ -38,6 +38,7 @@ add_clang_library(clangDaemon
Compiler.cpp
Config.cpp
ConfigCompile.cpp
+ ConfigProvider.cpp
ConfigYAML.cpp
Diagnostics.cpp
DraftStore.cpp
diff --git a/clang-tools-extra/clangd/ConfigProvider.cpp b/clang-tools-extra/clangd/ConfigProvider.cpp
new file mode 100644
index 000000000000..4b466d53e293
--- /dev/null
+++ b/clang-tools-extra/clangd/ConfigProvider.cpp
@@ -0,0 +1,207 @@
+//===--- ConfigProvider.cpp - Loading of user configuration ---------------===//
+//
+// 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 "ConfigProvider.h"
+#include "Config.h"
+#include "ConfigFragment.h"
+#include "support/ThreadsafeFS.h"
+#include "support/Trace.h"
+#include "llvm/ADT/StringMap.h"
+#include "llvm/Support/Path.h"
+#include <mutex>
+
+namespace clang {
+namespace clangd {
+namespace config {
+
+// Threadsafe cache around reading a YAML config file from disk.
+class FileConfigCache {
+ std::mutex Mu;
+ llvm::SmallVector<CompiledFragment, 1> CachedValue;
+ llvm::sys::TimePoint<> MTime = {};
+ unsigned Size = -1;
+
+ void updateCacheLocked(const llvm::vfs::Status &Stat,
+ llvm::vfs::FileSystem &FS, DiagnosticCallback DC) {
+ if (Size == Stat.getSize() && MTime == Stat.getLastModificationTime())
+ return; // Already valid.
+
+ Size = Stat.getSize();
+ MTime = Stat.getLastModificationTime();
+ CachedValue.clear();
+
+ auto Buf = FS.getBufferForFile(Path);
+ // If stat() succeeds but we failed to read, don't cache failure.
+ if (!Buf) {
+ Size = -1;
+ MTime = {};
+ return;
+ }
+
+ // If file changed between stat and open, we don't know its mtime.
+ // For simplicity, don't cache the value in this case (use a bad key).
+ if (Buf->get()->getBufferSize() != Size) {
+ Size = -1;
+ MTime = {};
+ }
+
+ // Finally parse and compile the actual fragments.
+ for (auto &Fragment :
+ Fragment::parseYAML(Buf->get()->getBuffer(), Path, DC))
+ CachedValue.push_back(std::move(Fragment).compile(DC));
+ }
+
+public:
+ // Must be set before the cache is used. Not a constructor param to allow
+ // computing ancestor-relative paths to be deferred.
+ std::string Path;
+
+ // Retrieves up-to-date config fragments from disk.
+ // A cached result may be reused if the mtime and size are unchanged.
+ // (But several concurrent read()s can miss the cache after a single change).
+ // Future performance ideas:
+ // - allow caches to be reused based on short elapsed walltime
+ // - allow latency-sensitive operations to skip revalidating the cache
+ void read(const ThreadsafeFS &TFS, DiagnosticCallback DC,
+ std::vector<CompiledFragment> &Out) {
+ assert(llvm::sys::path::is_absolute(Path));
+ auto FS = TFS.view(/*CWD=*/llvm::None);
+ auto Stat = FS->status(Path);
+ if (!Stat || !Stat->isRegularFile()) {
+ // No point taking the lock to clear the cache. We know what to return.
+ // If the file comes back we'll invalidate the cache at that point.
+ return;
+ }
+
+ std::lock_guard<std::mutex> Lock(Mu);
+ updateCacheLocked(*Stat, *FS, DC);
+ llvm::copy(CachedValue, std::back_inserter(Out));
+ }
+};
+
+std::unique_ptr<Provider> Provider::fromYAMLFile(llvm::StringRef AbsPath,
+ const ThreadsafeFS &FS) {
+ class AbsFileProvider : public Provider {
+ mutable FileConfigCache Cache; // threadsafe
+ const ThreadsafeFS &FS;
+
+ std::vector<CompiledFragment>
+ getFragments(const Params &P, DiagnosticCallback DC) const override {
+ std::vector<CompiledFragment> Result;
+ Cache.read(FS, DC, Result);
+ return Result;
+ };
+
+ public:
+ AbsFileProvider(llvm::StringRef Path, const ThreadsafeFS &FS) : FS(FS) {
+ assert(llvm::sys::path::is_absolute(Path));
+ Cache.Path = Path.str();
+ }
+ };
+
+ return std::make_unique<AbsFileProvider>(AbsPath, FS);
+}
+
+std::unique_ptr<Provider>
+Provider::fromAncestorRelativeYAMLFiles(llvm::StringRef RelPath,
+ const ThreadsafeFS &FS) {
+ class RelFileProvider : public Provider {
+ std::string RelPath;
+ const ThreadsafeFS &FS;
+
+ mutable std::mutex Mu;
+ // Keys are the ancestor directory, not the actual config path within it.
+ // We only insert into this map, so pointers to values are stable forever.
+ // Mutex guards the map itself, not the values (which are threadsafe).
+ mutable llvm::StringMap<FileConfigCache> Cache;
+
+ std::vector<CompiledFragment>
+ getFragments(const Params &P, DiagnosticCallback DC) const override {
+ namespace path = llvm::sys::path;
+
+ if (P.Path.empty())
+ return {};
+
+ // Compute absolute paths to all ancestors (substrings of P.Path).
+ llvm::StringRef Parent = path::parent_path(P.Path);
+ llvm::SmallVector<llvm::StringRef, 8> Ancestors;
+ for (auto I = path::begin(Parent, path::Style::posix),
+ E = path::end(Parent);
+ I != E; ++I) {
+ // Avoid weird non-substring cases like phantom "." components.
+ // In practice, Component is a substring for all "normal" ancestors.
+ if (I->end() < Parent.begin() && I->end() > Parent.end())
+ continue;
+ Ancestors.emplace_back(Parent.begin(), I->end() - Parent.begin());
+ }
+ // Ensure corresponding cache entries exist in the map.
+ llvm::SmallVector<FileConfigCache *, 8> Caches;
+ {
+ std::lock_guard<std::mutex> Lock(Mu);
+ for (llvm::StringRef Ancestor : Ancestors) {
+ auto R = Cache.try_emplace(Ancestor);
+ // Assemble the actual config file path only once.
+ if (R.second) {
+ llvm::SmallString<256> ConfigPath = Ancestor;
+ path::append(ConfigPath, RelPath);
+ R.first->second.Path = ConfigPath.str().str();
+ }
+ Caches.push_back(&R.first->second);
+ }
+ }
+ // Finally query each individual file.
+ // This will take a (per-file) lock for each file that actually exists.
+ std::vector<CompiledFragment> Result;
+ for (FileConfigCache *Cache : Caches)
+ Cache->read(FS, DC, Result);
+ return Result;
+ };
+
+ public:
+ RelFileProvider(llvm::StringRef RelPath, const ThreadsafeFS &FS)
+ : RelPath(RelPath), FS(FS) {
+ assert(llvm::sys::path::is_relative(RelPath));
+ }
+ };
+
+ return std::make_unique<RelFileProvider>(RelPath, FS);
+}
+
+std::unique_ptr<Provider>
+Provider::combine(std::vector<std::unique_ptr<Provider>> Providers) {
+ struct CombinedProvider : Provider {
+ std::vector<std::unique_ptr<Provider>> Providers;
+
+ std::vector<CompiledFragment>
+ getFragments(const Params &P, DiagnosticCallback DC) const override {
+ std::vector<CompiledFragment> Result;
+ for (const auto &Provider : Providers) {
+ for (auto &Fragment : Provider->getFragments(P, DC))
+ Result.push_back(std::move(Fragment));
+ }
+ return Result;
+ }
+ };
+ auto Result = std::make_unique<CombinedProvider>();
+ Result->Providers = std::move(Providers);
+ return Result;
+}
+
+Config Provider::getConfig(const Params &P, DiagnosticCallback DC) const {
+ trace::Span Tracer("getConfig");
+ if (!P.Path.empty())
+ SPAN_ATTACH(Tracer, "path", P.Path);
+ Config C;
+ for (const auto &Fragment : getFragments(P, DC))
+ Fragment(P, C);
+ return C;
+}
+
+} // namespace config
+} // namespace clangd
+} // namespace clang
diff --git a/clang-tools-extra/clangd/ConfigProvider.h b/clang-tools-extra/clangd/ConfigProvider.h
index a81e7eb855b4..a773e56b3bd7 100644
--- a/clang-tools-extra/clangd/ConfigProvider.h
+++ b/clang-tools-extra/clangd/ConfigProvider.h
@@ -26,6 +26,7 @@
namespace clang {
namespace clangd {
struct Config;
+class ThreadsafeFS;
namespace config {
/// Describes the context used to evaluate configuration fragments.
@@ -47,6 +48,48 @@ using DiagnosticCallback = llvm::function_ref<void(const llvm::SMDiagnostic &)>;
/// Returns true if the condition was met and the settings were used.
using CompiledFragment = std::function<bool(const Params &, Config &)>;
+/// A source of configuration fragments.
+/// Generally these providers reflect a fixed policy for obtaining config,
+/// but return
diff erent concrete configuration over time.
+/// e.g. a provider that reads config from files is responsive to file changes.
+class Provider {
+public:
+ virtual ~Provider() = default;
+
+ // Reads fragments from a single YAML file with a fixed path.
+ static std::unique_ptr<Provider> fromYAMLFile(llvm::StringRef AbsPathPath,
+ const ThreadsafeFS &);
+ // Reads fragments from YAML files found relative to ancestors of Params.Path.
+ //
+ // All fragments that exist are returned, starting from distant ancestors.
+ // For instance, given RelPath of ".clangd", then for source file /foo/bar.cc,
+ // the searched fragments are [/.clangd, /foo/.clangd].
+ //
+ // If Params does not specify a path, no fragments are returned.
+ static std::unique_ptr<Provider>
+ fromAncestorRelativeYAMLFiles(llvm::StringRef RelPath, const ThreadsafeFS &);
+
+ /// A provider that includes fragments from all the supplied providers.
+ /// Order is preserved; later providers take precedence over earlier ones.
+ static std::unique_ptr<Provider>
+ combine(std::vector<std::unique_ptr<Provider>>);
+
+ /// Build a config based on this provider.
+ Config getConfig(const Params &, DiagnosticCallback) const;
+
+private:
+ /// Provide fragments that may be relevant to the file.
+ /// The configuration provider is not responsible for testing conditions.
+ ///
+ /// Providers are expected to cache compiled fragments, and only
+ /// reparse/recompile when the source data has changed.
+ /// Despite the need for caching, this function must be threadsafe.
+ ///
+ /// When parsing/compiling, the DiagnosticCallback is used to report errors.
+ virtual std::vector<CompiledFragment>
+ getFragments(const Params &, DiagnosticCallback) const = 0;
+};
+
} // namespace config
} // namespace clangd
} // namespace clang
diff --git a/clang-tools-extra/clangd/unittests/CMakeLists.txt b/clang-tools-extra/clangd/unittests/CMakeLists.txt
index 698e27a86008..c25e2b7f8103 100644
--- a/clang-tools-extra/clangd/unittests/CMakeLists.txt
+++ b/clang-tools-extra/clangd/unittests/CMakeLists.txt
@@ -42,6 +42,7 @@ add_unittest(ClangdUnitTests ClangdTests
CompileCommandsTests.cpp
CompilerTests.cpp
ConfigCompileTests.cpp
+ ConfigProviderTests.cpp
ConfigYAMLTests.cpp
DexTests.cpp
DiagnosticsTests.cpp
diff --git a/clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp b/clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
new file mode 100644
index 000000000000..122b55cf64e0
--- /dev/null
+++ b/clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
@@ -0,0 +1,156 @@
+//===-- ConfigProviderTests.cpp -------------------------------------------===//
+//
+// 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 "Config.h"
+#include "ConfigProvider.h"
+#include "ConfigTesting.h"
+#include "TestFS.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include "llvm/Support/SourceMgr.h"
+#include <atomic>
+
+namespace clang {
+namespace clangd {
+namespace config {
+namespace {
+using ::testing::ElementsAre;
+using ::testing::IsEmpty;
+
+// Provider that appends an arg to compile flags.
+// The arg is prefix<N>, where N is the times getFragments() was called.
+// It also yields a diagnostic each time it's called.
+class FakeProvider : public Provider {
+ std::string Prefix;
+ mutable std::atomic<unsigned> Index = {0};
+
+ std::vector<CompiledFragment>
+ getFragments(const Params &, DiagnosticCallback DC) const override {
+ DC(llvm::SMDiagnostic("", llvm::SourceMgr::DK_Error, Prefix));
+ CompiledFragment F =
+ [Arg(Prefix + std::to_string(++Index))](const Params &P, Config &C) {
+ C.CompileFlags.Edits.push_back(
+ [Arg](std::vector<std::string> &Argv) { Argv.push_back(Arg); });
+ return true;
+ };
+ return {F};
+ }
+
+public:
+ FakeProvider(llvm::StringRef Prefix) : Prefix(Prefix) {}
+};
+
+std::vector<std::string> getAddedArgs(Config &C) {
+ std::vector<std::string> Argv;
+ for (auto &Edit : C.CompileFlags.Edits)
+ Edit(Argv);
+ return Argv;
+}
+
+// The provider from combine() should invoke its providers in order, and not
+// cache their results.
+TEST(ProviderTest, Combine) {
+ CapturedDiags Diags;
+ std::vector<std::unique_ptr<Provider>> Providers;
+ Providers.push_back(std::make_unique<FakeProvider>("foo"));
+ Providers.push_back(std::make_unique<FakeProvider>("bar"));
+ auto Combined = Provider::combine(std::move(Providers));
+ Config Cfg = Combined->getConfig(Params(), Diags.callback());
+ EXPECT_THAT(Diags.Diagnostics,
+ ElementsAre(DiagMessage("foo"), DiagMessage("bar")));
+ EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo1", "bar1"));
+ Diags.Diagnostics.clear();
+
+ Cfg = Combined->getConfig(Params(), Diags.callback());
+ EXPECT_THAT(Diags.Diagnostics,
+ ElementsAre(DiagMessage("foo"), DiagMessage("bar")));
+ EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo2", "bar2"));
+}
+
+const char *AddFooWithErr = R"yaml(
+CompileFlags:
+ Add: foo
+ Unknown: 42
+)yaml";
+
+const char *AddBarBaz = R"yaml(
+CompileFlags:
+ Add: bar
+---
+CompileFlags:
+ Add: baz
+)yaml";
+
+TEST(ProviderTest, FromYAMLFile) {
+ MockFS FS;
+ FS.Files["foo.yaml"] = AddFooWithErr;
+
+ CapturedDiags Diags;
+ auto P = Provider::fromYAMLFile(testPath("foo.yaml"), FS);
+ auto Cfg = P->getConfig(Params(), Diags.callback());
+ EXPECT_THAT(Diags.Diagnostics,
+ ElementsAre(DiagMessage("Unknown CompileFlags key Unknown")));
+ EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo"));
+ Diags.Diagnostics.clear();
+
+ Cfg = P->getConfig(Params(), Diags.callback());
+ EXPECT_THAT(Diags.Diagnostics, IsEmpty()) << "Cached, not re-parsed";
+ EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo"));
+
+ FS.Files["foo.yaml"] = AddBarBaz;
+ Cfg = P->getConfig(Params(), Diags.callback());
+ EXPECT_THAT(Diags.Diagnostics, IsEmpty()) << "New config, no errors";
+ EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("bar", "baz"));
+
+ FS.Files.erase("foo.yaml");
+ Cfg = P->getConfig(Params(), Diags.callback());
+ EXPECT_THAT(Diags.Diagnostics, IsEmpty()) << "Missing file is not an error";
+ EXPECT_THAT(getAddedArgs(Cfg), IsEmpty());
+}
+
+TEST(ProviderTest, FromAncestorRelativeYAMLFiles) {
+ MockFS FS;
+ FS.Files["a/b/c/foo.yaml"] = AddBarBaz;
+ FS.Files["a/foo.yaml"] = AddFooWithErr;
+
+ std::string ABCPath =
+ testPath("a/b/c/d/test.cc", llvm::sys::path::Style::posix);
+ Params ABCParams;
+ ABCParams.Path = ABCPath;
+ std::string APath =
+ testPath("a/b/e/f/test.cc", llvm::sys::path::Style::posix);
+ Params AParams;
+ AParams.Path = APath;
+
+ CapturedDiags Diags;
+ auto P = Provider::fromAncestorRelativeYAMLFiles("foo.yaml", FS);
+
+ auto Cfg = P->getConfig(Params(), Diags.callback());
+ EXPECT_THAT(Diags.Diagnostics, IsEmpty());
+ EXPECT_THAT(getAddedArgs(Cfg), IsEmpty());
+
+ Cfg = P->getConfig(ABCParams, Diags.callback());
+ EXPECT_THAT(Diags.Diagnostics,
+ ElementsAre(DiagMessage("Unknown CompileFlags key Unknown")));
+ EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo", "bar", "baz"));
+ Diags.Diagnostics.clear();
+
+ Cfg = P->getConfig(AParams, Diags.callback());
+ EXPECT_THAT(Diags.Diagnostics, IsEmpty()) << "Cached config";
+ EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo"));
+
+ FS.Files.erase("a/foo.yaml");
+ Cfg = P->getConfig(ABCParams, Diags.callback());
+ EXPECT_THAT(Diags.Diagnostics, IsEmpty());
+ EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("bar", "baz"));
+}
+
+} // namespace
+} // namespace config
+} // namespace clangd
+} // namespace clang
diff --git a/clang-tools-extra/clangd/unittests/TestFS.cpp b/clang-tools-extra/clangd/unittests/TestFS.cpp
index c436e9a7a951..3b2fbc142a28 100644
--- a/clang-tools-extra/clangd/unittests/TestFS.cpp
+++ b/clang-tools-extra/clangd/unittests/TestFS.cpp
@@ -79,13 +79,13 @@ const char *testRoot() {
#endif
}
-std::string testPath(PathRef File) {
+std::string testPath(PathRef File, llvm::sys::path::Style Style) {
assert(llvm::sys::path::is_relative(File) && "FileName should be relative");
llvm::SmallString<32> NativeFile = File;
- llvm::sys::path::native(NativeFile);
+ llvm::sys::path::native(NativeFile, Style);
llvm::SmallString<32> Path;
- llvm::sys::path::append(Path, testRoot(), NativeFile);
+ llvm::sys::path::append(Path, Style, testRoot(), NativeFile);
return std::string(Path.str());
}
diff --git a/clang-tools-extra/clangd/unittests/TestFS.h b/clang-tools-extra/clangd/unittests/TestFS.h
index 1c713c78f7bf..7972fe37c7fe 100644
--- a/clang-tools-extra/clangd/unittests/TestFS.h
+++ b/clang-tools-extra/clangd/unittests/TestFS.h
@@ -69,7 +69,8 @@ class MockCompilationDatabase : public GlobalCompilationDatabase {
const char *testRoot();
// Returns a suitable absolute path for this OS.
-std::string testPath(PathRef File);
+std::string testPath(PathRef File,
+ llvm::sys::path::Style = llvm::sys::path::Style::native);
// unittest: is a scheme that refers to files relative to testRoot()
// This anchor is used to force the linker to link in the generated object file
More information about the cfe-commits
mailing list