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; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
