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