[clang-tools-extra] f25e3c2 - Revert "[clangd] Resolve driver symlinks, and look up unknown relative drivers in PATH."
Nico Weber via cfe-commits
cfe-commits at lists.llvm.org
Mon Jun 8 12:21:16 PDT 2020
Author: Nico Weber
Date: 2020-06-08T15:20:16-04:00
New Revision: f25e3c2d0e8553e6640ca5e0d1933c0e9455bd71
URL: https://github.com/llvm/llvm-project/commit/f25e3c2d0e8553e6640ca5e0d1933c0e9455bd71
DIFF: https://github.com/llvm/llvm-project/commit/f25e3c2d0e8553e6640ca5e0d1933c0e9455bd71.diff
LOG: Revert "[clangd] Resolve driver symlinks, and look up unknown relative drivers in PATH."
This reverts commit 806342b8ef54ec07511d0ce5d3d1335451e952da.
Breaks check-clangd on macOS, https://reviews.llvm.org/D75414#2080076
Added:
Modified:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/CompileCommands.cpp
clang-tools-extra/clangd/CompileCommands.h
clang-tools-extra/clangd/QueryDriverDatabase.cpp
clang-tools-extra/clangd/support/Threading.h
clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
clang-tools-extra/clangd/unittests/support/ThreadingTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp
index db9932a43e5a..b3b2bdd976bf 100644
--- a/clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -514,7 +514,7 @@ void ClangdLSPServer::onInitialize(const InitializeParams &Params,
if (ClangdServerOpts.ResourceDir)
Mangler.ResourceDir = *ClangdServerOpts.ResourceDir;
CDB.emplace(BaseCDB.get(), Params.initializationOptions.fallbackFlags,
- tooling::ArgumentsAdjuster(std::move(Mangler)));
+ tooling::ArgumentsAdjuster(Mangler));
{
// Switch caller's context with LSPServer's background context. Since we
// rather want to propagate information from LSPServer's context into the
diff --git a/clang-tools-extra/clangd/CompileCommands.cpp b/clang-tools-extra/clangd/CompileCommands.cpp
index a73312a521e8..84f72f5f58c7 100644
--- a/clang-tools-extra/clangd/CompileCommands.cpp
+++ b/clang-tools-extra/clangd/CompileCommands.cpp
@@ -119,48 +119,6 @@ std::string detectStandardResourceDir() {
return CompilerInvocation::GetResourcesPath("clangd", (void *)&Dummy);
}
-// The path passed to argv[0] is important:
-// - its parent directory is Driver::Dir, used for library discovery
-// - its basename affects CLI parsing (clang-cl) and other settings
-// Where possible it should be an absolute path with sensible directory, but
-// with the original basename.
-static std::string resolveDriver(llvm::StringRef Driver, bool FollowSymlink,
- llvm::Optional<std::string> ClangPath) {
- auto SiblingOf = [&](llvm::StringRef AbsPath) {
- llvm::SmallString<128> Result = llvm::sys::path::parent_path(AbsPath);
- llvm::sys::path::append(Result, llvm::sys::path::filename(Driver));
- return Result.str().str();
- };
-
- // First, eliminate relative paths.
- std::string Storage;
- if (!llvm::sys::path::is_absolute(Driver)) {
- // If the driver is a generic like "g++" with no path, add clang dir.
- if (ClangPath &&
- (Driver == "clang" || Driver == "clang++" || Driver == "gcc" ||
- Driver == "g++" || Driver == "cc" || Driver == "c++")) {
- return SiblingOf(*ClangPath);
- }
- // Otherwise try to look it up on PATH. This won't change basename.
- auto Absolute = llvm::sys::findProgramByName(Driver);
- if (Absolute && llvm::sys::path::is_absolute(*Absolute))
- Driver = Storage = std::move(*Absolute);
- else if (ClangPath) // If we don't find it, use clang dir again.
- return SiblingOf(*ClangPath);
- else // Nothing to do: can't find the command and no detected dir.
- return Driver.str();
- }
-
- // Now we have an absolute path, but it may be a symlink.
- assert(llvm::sys::path::is_absolute(Driver));
- if (FollowSymlink) {
- llvm::SmallString<256> Resolved;
- if (!llvm::sys::fs::real_path(Driver, Resolved))
- return SiblingOf(Resolved);
- }
- return Driver.str();
-}
-
} // namespace
CommandMangler CommandMangler::detect() {
@@ -204,22 +162,25 @@ void CommandMangler::adjust(std::vector<std::string> &Cmd) const {
Cmd.push_back(*Sysroot);
}
- if (!Cmd.empty()) {
- bool FollowSymlink = !Has("-no-canonical-prefixes");
- Cmd.front() =
- (FollowSymlink ? ResolvedDrivers : ResolvedDriversNoFollow)
- .get(Cmd.front(), [&, this] {
- return resolveDriver(Cmd.front(), FollowSymlink, ClangPath);
- });
+ // If the driver is a generic name like "g++" with no path, add a clang path.
+ // This makes it easier for us to find the standard libraries on mac.
+ if (ClangPath && llvm::sys::path::is_absolute(*ClangPath) && !Cmd.empty()) {
+ std::string &Driver = Cmd.front();
+ if (Driver == "clang" || Driver == "clang++" || Driver == "gcc" ||
+ Driver == "g++" || Driver == "cc" || Driver == "c++") {
+ llvm::SmallString<128> QualifiedDriver =
+ llvm::sys::path::parent_path(*ClangPath);
+ llvm::sys::path::append(QualifiedDriver, Driver);
+ Driver = std::string(QualifiedDriver.str());
+ }
}
}
-CommandMangler::operator clang::tooling::ArgumentsAdjuster() && {
- // ArgumentsAdjuster is a std::function and so must be copyable.
- return [Mangler = std::make_shared<CommandMangler>(std::move(*this))](
- const std::vector<std::string> &Args, llvm::StringRef File) {
+CommandMangler::operator clang::tooling::ArgumentsAdjuster() {
+ return [Mangler{*this}](const std::vector<std::string> &Args,
+ llvm::StringRef File) {
auto Result = Args;
- Mangler->adjust(Result);
+ Mangler.adjust(Result);
return Result;
};
}
diff --git a/clang-tools-extra/clangd/CompileCommands.h b/clang-tools-extra/clangd/CompileCommands.h
index 51a5574d13d3..02b1a76c5bff 100644
--- a/clang-tools-extra/clangd/CompileCommands.h
+++ b/clang-tools-extra/clangd/CompileCommands.h
@@ -8,10 +8,8 @@
#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_COMPILECOMMANDS_H
#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_COMPILECOMMANDS_H
-#include "support/Threading.h"
#include "clang/Tooling/ArgumentsAdjusters.h"
#include "clang/Tooling/CompilationDatabase.h"
-#include "llvm/ADT/StringMap.h"
#include <string>
#include <vector>
@@ -42,12 +40,10 @@ struct CommandMangler {
static CommandMangler detect();
void adjust(std::vector<std::string> &Cmd) const;
- explicit operator clang::tooling::ArgumentsAdjuster() &&;
+ explicit operator clang::tooling::ArgumentsAdjuster();
private:
CommandMangler() = default;
- Memoize<llvm::StringMap<std::string>> ResolvedDrivers;
- Memoize<llvm::StringMap<std::string>> ResolvedDriversNoFollow;
};
} // namespace clangd
diff --git a/clang-tools-extra/clangd/QueryDriverDatabase.cpp b/clang-tools-extra/clangd/QueryDriverDatabase.cpp
index 11d74203ae94..2ab217dac155 100644
--- a/clang-tools-extra/clangd/QueryDriverDatabase.cpp
+++ b/clang-tools-extra/clangd/QueryDriverDatabase.cpp
@@ -88,7 +88,7 @@ std::vector<std::string> parseDriverOutput(llvm::StringRef Output) {
std::vector<std::string>
extractSystemIncludes(PathRef Driver, llvm::StringRef Lang,
llvm::ArrayRef<std::string> CommandLine,
- const llvm::Regex &QueryDriverRegex) {
+ llvm::Regex &QueryDriverRegex) {
trace::Span Tracer("Extract system includes");
SPAN_ATTACH(Tracer, "driver", Driver);
SPAN_ATTACH(Tracer, "lang", Lang);
@@ -267,12 +267,19 @@ class QueryDriverDatabase : public GlobalCompilationDatabase {
llvm::SmallString<128> Driver(Cmd->CommandLine.front());
llvm::sys::fs::make_absolute(Cmd->Directory, Driver);
+ auto Key = std::make_pair(Driver.str().str(), Lang.str());
- std::vector<std::string> SystemIncludes =
- QueriedDrivers.get(/*Key=*/(Driver + ":" + Lang).str(), [&] {
- return extractSystemIncludes(Driver, Lang, Cmd->CommandLine,
- QueryDriverRegex);
- });
+ std::vector<std::string> SystemIncludes;
+ {
+ std::lock_guard<std::mutex> Lock(Mu);
+
+ auto It = DriverToIncludesCache.find(Key);
+ if (It != DriverToIncludesCache.end())
+ SystemIncludes = It->second;
+ else
+ DriverToIncludesCache[Key] = SystemIncludes = extractSystemIncludes(
+ Key.first, Key.second, Cmd->CommandLine, QueryDriverRegex);
+ }
return addSystemIncludes(*Cmd, SystemIncludes);
}
@@ -282,9 +289,12 @@ class QueryDriverDatabase : public GlobalCompilationDatabase {
}
private:
- // Caches includes extracted from a driver. Key is driver:lang.
- Memoize<llvm::StringMap<std::vector<std::string>>> QueriedDrivers;
- llvm::Regex QueryDriverRegex;
+ mutable std::mutex Mu;
+ // Caches includes extracted from a driver.
+ mutable std::map<std::pair<std::string, std::string>,
+ std::vector<std::string>>
+ DriverToIncludesCache;
+ mutable llvm::Regex QueryDriverRegex;
std::unique_ptr<GlobalCompilationDatabase> Base;
CommandChanged::Subscription BaseChanged;
diff --git a/clang-tools-extra/clangd/support/Threading.h b/clang-tools-extra/clangd/support/Threading.h
index 5155ac193fd1..310dd7bc5a24 100644
--- a/clang-tools-extra/clangd/support/Threading.h
+++ b/clang-tools-extra/clangd/support/Threading.h
@@ -131,44 +131,6 @@ std::future<T> runAsync(llvm::unique_function<T()> Action) {
std::move(Action), Context::current().clone());
}
-/// Memoize is a cache to store and reuse computation results based on a key.
-///
-/// Memoize<DenseMap<int, bool>> PrimeCache;
-/// for (int I : RepetitiveNumbers)
-/// if (PrimeCache.get(I, [&] { return expensiveIsPrime(I); }))
-/// llvm::errs() << "Prime: " << I << "\n";
-///
-/// The computation will only be run once for each key.
-/// This class is threadsafe. Concurrent calls for the same key may run the
-/// computation multiple times, but each call will return the same result.
-template <typename Container> class Memoize {
- mutable Container Cache;
- std::unique_ptr<std::mutex> Mu;
-
-public:
- Memoize() : Mu(std::make_unique<std::mutex>()) {}
-
- template <typename T, typename Func>
- typename Container::mapped_type get(T &&Key, Func Compute) const {
- {
- std::lock_guard<std::mutex> Lock(*Mu);
- auto It = Cache.find(Key);
- if (It != Cache.end())
- return It->second;
- }
- // Don't hold the mutex while computing.
- auto V = Compute();
- {
- std::lock_guard<std::mutex> Lock(*Mu);
- auto R = Cache.try_emplace(std::forward<T>(Key), V);
- // Insert into cache may fail if we raced with another thread.
- if (!R.second)
- return R.first->second; // Canonical value, from other thread.
- }
- return V;
- }
-};
-
} // namespace clangd
} // namespace clang
#endif
diff --git a/clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp b/clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
index ffef28b92d3d..cad987291623 100644
--- a/clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
+++ b/clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
@@ -9,11 +9,7 @@
#include "CompileCommands.h"
#include "TestFS.h"
-#include "llvm/ADT/ScopeExit.h"
#include "llvm/ADT/StringExtras.h"
-#include "llvm/Support/FileSystem.h"
-#include "llvm/Support/Path.h"
-#include "llvm/Support/Process.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
@@ -107,81 +103,12 @@ TEST(CommandMangler, ClangPath) {
Cmd = {"unknown-binary", "foo.cc"};
Mangler.adjust(Cmd);
- EXPECT_EQ(testPath("fake/unknown-binary"), Cmd.front());
+ EXPECT_EQ("unknown-binary", Cmd.front());
Cmd = {testPath("path/clang++"), "foo.cc"};
Mangler.adjust(Cmd);
EXPECT_EQ(testPath("path/clang++"), Cmd.front());
-
- Cmd = {"foo/unknown-binary", "foo.cc"};
- Mangler.adjust(Cmd);
- EXPECT_EQ(testPath("fake/unknown-binary"), Cmd.front());
-}
-
-// Only run the PATH/symlink resolving test on unix, we need to fiddle
-// with permissions and environment variables...
-#ifdef LLVM_ON_UNIX
-MATCHER(Ok, "") {
- if (arg) {
- *result_listener << arg.message();
- return false;
- }
- return true;
-}
-
-TEST(CommandMangler, ClangPathResolve) {
- // Set up filesystem:
- // /temp/
- // bin/
- // foo -> temp/lib/bar
- // lib/
- // bar
- llvm::SmallString<256> TempDir;
- ASSERT_THAT(llvm::sys::fs::createUniqueDirectory("ClangPathResolve", TempDir),
- Ok());
- auto CleanDir = llvm::make_scope_exit(
- [&] { llvm::sys::fs::remove_directories(TempDir); });
- ASSERT_THAT(llvm::sys::fs::create_directory(TempDir + "/bin"), Ok());
- ASSERT_THAT(llvm::sys::fs::create_directory(TempDir + "/lib"), Ok());
- int FD;
- ASSERT_THAT(llvm::sys::fs::openFileForWrite(TempDir + "/lib/bar", FD), Ok());
- ASSERT_THAT(llvm::sys::Process::SafelyCloseFileDescriptor(FD), Ok());
- ::chmod((TempDir + "/lib/bar").str().c_str(), 0755); // executable
- ASSERT_THAT(
- llvm::sys::fs::create_link(TempDir + "/lib/bar", TempDir + "/bin/foo"),
- Ok());
-
- // Test the case where the driver is an absolute path to a symlink.
- auto Mangler = CommandMangler::forTests();
- Mangler.ClangPath = testPath("fake/clang");
- std::vector<std::string> Cmd = {(TempDir + "/bin/foo").str(), "foo.cc"};
- Mangler.adjust(Cmd);
- // Directory based on resolved symlink, basename preserved.
- EXPECT_EQ((TempDir + "/lib/foo").str(), Cmd.front());
-
- // Set PATH to point to temp/bin so we can find 'foo' on it.
- ASSERT_TRUE(::getenv("PATH"));
- auto RestorePath =
- llvm::make_scope_exit([OldPath = std::string(::getenv("PATH"))] {
- ::setenv("PATH", OldPath.c_str(), 1);
- });
- ::setenv("PATH", (TempDir + "/bin").str().c_str(), /*overwrite=*/1);
-
- // Test the case where the driver is a $PATH-relative path to a symlink.
- Mangler = CommandMangler::forTests();
- Mangler.ClangPath = testPath("fake/clang");
- // Driver found on PATH.
- Cmd = {"foo", "foo.cc"};
- Mangler.adjust(Cmd);
- // Found the symlink and resolved the path as above.
- EXPECT_EQ((TempDir + "/lib/foo").str(), Cmd.front());
-
- // Symlink not resolved with -no-canonical-prefixes.
- Cmd = {"foo", "-no-canonical-prefixes", "foo.cc"};
- Mangler.adjust(Cmd);
- EXPECT_EQ((TempDir + "/bin/foo").str(), Cmd.front());
}
-#endif
} // namespace
} // namespace clangd
diff --git a/clang-tools-extra/clangd/unittests/support/ThreadingTests.cpp b/clang-tools-extra/clangd/unittests/support/ThreadingTests.cpp
index e265ad2eabea..015af092012a 100644
--- a/clang-tools-extra/clangd/unittests/support/ThreadingTests.cpp
+++ b/clang-tools-extra/clangd/unittests/support/ThreadingTests.cpp
@@ -7,8 +7,6 @@
//===----------------------------------------------------------------------===//
#include "support/Threading.h"
-#include "llvm/ADT/DenseMap.h"
-#include "gmock/gmock.h"
#include "gtest/gtest.h"
#include <mutex>
@@ -62,64 +60,5 @@ TEST_F(ThreadingTest, TaskRunner) {
std::lock_guard<std::mutex> Lock(Mutex);
ASSERT_EQ(Counter, TasksCnt * IncrementsPerTask);
}
-
-TEST_F(ThreadingTest, Memoize) {
- const unsigned NumThreads = 5;
- const unsigned NumKeys = 100;
- const unsigned NumIterations = 100;
-
- Memoize<llvm::DenseMap<int, int>> Cache;
- std::atomic<unsigned> ComputeCount(0);
- std::atomic<int> ComputeResult[NumKeys];
- std::fill(std::begin(ComputeResult), std::end(ComputeResult), -1);
-
- AsyncTaskRunner Tasks;
- for (unsigned I = 0; I < NumThreads; ++I)
- Tasks.runAsync("worker" + std::to_string(I), [&] {
- for (unsigned J = 0; J < NumIterations; J++)
- for (unsigned K = 0; K < NumKeys; K++) {
- int Result = Cache.get(K, [&] { return ++ComputeCount; });
- EXPECT_THAT(ComputeResult[K].exchange(Result),
- testing::AnyOf(-1, Result))
- << "Got inconsistent results from memoize";
- }
- });
- Tasks.wait();
- EXPECT_GE(ComputeCount, NumKeys) << "Computed each key once";
- EXPECT_LE(ComputeCount, NumThreads * NumKeys)
- << "Worst case, computed each key in every thread";
- for (int Result : ComputeResult)
- EXPECT_GT(Result, 0) << "All results in expected domain";
-}
-
-TEST_F(ThreadingTest, MemoizeDeterministic) {
- Memoize<llvm::DenseMap<int, char>> Cache;
-
- // Spawn two parallel computations, A and B.
- // Force concurrency: neither can finish until both have started.
- // Verify that cache returns consistent results.
- AsyncTaskRunner Tasks;
- std::atomic<char> ValueA(0), ValueB(0);
- Notification ReleaseA, ReleaseB;
- Tasks.runAsync("A", [&] {
- ValueA = Cache.get(0, [&] {
- ReleaseB.notify();
- ReleaseA.wait();
- return 'A';
- });
- });
- Tasks.runAsync("A", [&] {
- ValueB = Cache.get(0, [&] {
- ReleaseA.notify();
- ReleaseB.wait();
- return 'B';
- });
- });
- Tasks.wait();
-
- ASSERT_EQ(ValueA, ValueB);
- ASSERT_THAT(ValueA.load(), testing::AnyOf('A', 'B'));
-}
-
} // namespace clangd
} // namespace clang
More information about the cfe-commits
mailing list