diff --git a/clang-tools-extra/clangd/CMakeLists.txt b/clang-tools-extra/clangd/CMakeLists.txt --- a/clang-tools-extra/clangd/CMakeLists.txt +++ b/clang-tools-extra/clangd/CMakeLists.txt @@ -38,6 +38,7 @@ Compiler.cpp Config.cpp ConfigCompile.cpp + ConfigProvider.cpp ConfigYAML.cpp Diagnostics.cpp DraftStore.cpp diff --git a/clang-tools-extra/clangd/ConfigProvider.h b/clang-tools-extra/clangd/ConfigProvider.h --- 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 @@ /// Returns true if the condition was met and the settings were used. using CompiledFragment = std::function; +/// A source of configuration fragments. +/// Generally these providers reflect a fixed policy for obtaining config, +/// but return different 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 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 + 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 + combine(std::vector>); + + /// 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 + getFragments(const Params &, DiagnosticCallback) const = 0; +}; + } // namespace config } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/ConfigProvider.cpp b/clang-tools-extra/clangd/ConfigProvider.cpp new file mode 100644 --- /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 + +namespace clang { +namespace clangd { +namespace config { + +// Threadsafe cache around reading a YAML config file from disk. +class FileConfigCache { + std::mutex Mu; + llvm::SmallVector 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 &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 Lock(Mu); + updateCacheLocked(*Stat, *FS, DC); + llvm::copy(CachedValue, std::back_inserter(Out)); + } +}; + +std::unique_ptr Provider::fromYAMLFile(llvm::StringRef AbsPath, + const ThreadsafeFS &FS) { + class AbsFileProvider : public Provider { + mutable FileConfigCache Cache; // threadsafe + const ThreadsafeFS &FS; + + std::vector + getFragments(const Params &P, DiagnosticCallback DC) const override { + std::vector 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(AbsPath, FS); +} + +std::unique_ptr +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 Cache; + + std::vector + 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 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 Caches; + { + std::lock_guard 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 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(RelPath, FS); +} + +std::unique_ptr +Provider::combine(std::vector> Providers) { + struct CombinedProvider : Provider { + std::vector> Providers; + + std::vector + getFragments(const Params &P, DiagnosticCallback DC) const override { + std::vector 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(); + 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/unittests/CMakeLists.txt b/clang-tools-extra/clangd/unittests/CMakeLists.txt --- a/clang-tools-extra/clangd/unittests/CMakeLists.txt +++ b/clang-tools-extra/clangd/unittests/CMakeLists.txt @@ -42,6 +42,7 @@ 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 --- /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 + +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, 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 Index = {0}; + + std::vector + 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 &Argv) { Argv.push_back(Arg); }); + return true; + }; + return {F}; + } + +public: + FakeProvider(llvm::StringRef Prefix) : Prefix(Prefix) {} +}; + +std::vector getAddedArgs(Config &C) { + std::vector 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> Providers; + Providers.push_back(std::make_unique("foo")); + Providers.push_back(std::make_unique("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.h b/clang-tools-extra/clangd/unittests/TestFS.h --- a/clang-tools-extra/clangd/unittests/TestFS.h +++ b/clang-tools-extra/clangd/unittests/TestFS.h @@ -69,7 +69,8 @@ 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 diff --git a/clang-tools-extra/clangd/unittests/TestFS.cpp b/clang-tools-extra/clangd/unittests/TestFS.cpp --- a/clang-tools-extra/clangd/unittests/TestFS.cpp +++ b/clang-tools-extra/clangd/unittests/TestFS.cpp @@ -79,13 +79,13 @@ #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()); }