[clang-tools-extra] d1531b0 - [clangd] Move logging out of LSPTest base class into a separate one.

[Kadir Cetinkaya via cfe-commits]

Author: Kadir Cetinkaya
Date: 2021-03-09T11:57:05+01:00
New Revision: d1531b08c3d1bd46829a91313e7d6eb24d04c0d0

URL: https://github.com/llvm/llvm-project/commit/d1531b08c3d1bd46829a91313e7d6eb24d04c0d0
DIFF: https://github.com/llvm/llvm-project/commit/d1531b08c3d1bd46829a91313e7d6eb24d04c0d0.diff

LOG: [clangd] Move logging out of LSPTest base class into a separate one.

This was causing TSan failures due to a race on vptr during destruction,
see
https://lab.llvm.org/buildbot/#/builders/131/builds/6579/steps/6/logs/FAIL__Clangd_Unit_Tests__LSPTest_FeatureModulesThr.

The story is, during the execution of a destructor all the virtual
dispatches should resolve to implementations in the class being
destroyed, not the derived ones. And LSPTests will log some stuff during
destruction (we send shutdown/exit requests, which are logged), which is
a virtual dispatch when LSPTest is derived from clang::clangd::Logger.

It is a benign race in our case, as gtests that derive from LSPTest
doesn't override the `log` method. But still during destruction, we
might try to update vtable ptr (due to being done with destruction of
test and starting destruction of LSPTest) and read from it to dispatch a
log message at the same time.

This patch fixes that race by moving `log` out of the vtable of
`LSPTest`.

Differential Revision: https://reviews.llvm.org/D98241

Added: 
    

Modified: 
    clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp b/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
index c01a1d2ebc88..f596f94bd6cd 100644
--- a/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
@@ -35,9 +35,9 @@ MATCHER_P(DiagMessage, M, "") {
   return false;
 }
 
-class LSPTest : public ::testing::Test, private clangd::Logger {
+class LSPTest : public ::testing::Test {
 protected:
-  LSPTest() : LogSession(*this) {
+  LSPTest() : LogSession(L) {
     ClangdServer::Options &Base = Opts;
     Base = ClangdServer::optsForTest();
     // This is needed to we can test index-based operations like call hierarchy.
@@ -73,26 +73,29 @@ class LSPTest : public ::testing::Test, private clangd::Logger {
   FeatureModuleSet FeatureModules;
 
 private:
-  // Color logs so we can distinguish them from test output.
-  void log(Level L, const char *Fmt,
-           const llvm::formatv_object_base &Message) override {
-    raw_ostream::Colors Color;
-    switch (L) {
-    case Level::Verbose:
-      Color = raw_ostream::BLUE;
-      break;
-    case Level::Error:
-      Color = raw_ostream::RED;
-      break;
-    default:
-      Color = raw_ostream::YELLOW;
-      break;
+  class Logger : public clang::clangd::Logger {
+    // Color logs so we can distinguish them from test output.
+    void log(Level L, const char *Fmt,
+             const llvm::formatv_object_base &Message) override {
+      raw_ostream::Colors Color;
+      switch (L) {
+      case Level::Verbose:
+        Color = raw_ostream::BLUE;
+        break;
+      case Level::Error:
+        Color = raw_ostream::RED;
+        break;
+      default:
+        Color = raw_ostream::YELLOW;
+        break;
+      }
+      std::lock_guard<std::mutex> Lock(LogMu);
+      (llvm::outs().changeColor(Color) << Message << "\n").resetColor();
     }
-    std::lock_guard<std::mutex> Lock(LogMu);
-    (llvm::outs().changeColor(Color) << Message << "\n").resetColor();
-  }
-  std::mutex LogMu;
+    std::mutex LogMu;
+  };
 
+  Logger L;
   LoggingSession LogSession;
   llvm::Optional<ClangdLSPServer> Server;
   llvm::Optional<std::thread> ServerThread;