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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet created this revision.
kadircet added a reviewer: sammccall.
Herald added subscribers: usaxena95, arphaman.
kadircet requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang.

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`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D98241

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


Index: clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
@@ -35,9 +35,9 @@
   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 @@
   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;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D98241.329258.patch
Type: text/x-patch
Size: 2090 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20210309/035baf53/attachment-0001.bin>