This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by kadircet on Mar 9 2021, 2:21 AM.

Details

Summary

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.

Diff Detail

Event Timeline

kadircet created this revision.Mar 9 2021, 2:21 AM
kadircet requested review of this revision.Mar 9 2021, 2:21 AM
sammccall accepted this revision.Mar 9 2021, 2:43 AM

Thanks for working this out!

And sorry for the stupid private inheritance tricks :-(

This revision is now accepted and ready to land.Mar 9 2021, 2:43 AM
This revision was landed with ongoing or failed builds.Mar 9 2021, 2:57 AM
This revision was automatically updated to reflect the committed changes.