[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