[all-commits] [llvm/llvm-project] d1531b: [clangd] Move logging out of LSPTest base class in...

[kadir çetinkaya via All-commits]

  Branch: refs/heads/main
  Home:   https://github.com/llvm/llvm-project
  Commit: d1531b08c3d1bd46829a91313e7d6eb24d04c0d0
      https://github.com/llvm/llvm-project/commit/d1531b08c3d1bd46829a91313e7d6eb24d04c0d0
  Author: Kadir Cetinkaya <kadircet at google.com>
  Date:   2021-03-09 (Tue, 09 Mar 2021)

  Changed paths:
    M clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp

  Log Message:
  -----------
  [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