[PATCH] D152696: Prevent deadlocks in death tests.

[Martin Böhme via Phabricator via cfe-commits]

mboehme added a comment.

In D152696#4413626 <https://reviews.llvm.org/D152696#4413626>, @hans wrote:

> I'm no expert here, but I went to check what Chromium does, and it seems to set the death_test_style to "threadsafe" for the whole test suite: https://source.chromium.org/chromium/chromium/src/+/main:base/test/test_suite.cc;l=367
>
>> As the page linked above notes, "flags are saved before each test and restored
>> afterwards", so the flag affects only the tests where it is set. This is
>> important because the "threadsafe" death test style "trades increased test
>> execution time (potentially dramatically so) for improved thread safety", so we
>> likely don't want to set it for all tests. The tests where I've added the flag
>> don't appear to suffer a significantly increated execution time.
>
> I assume the flag only affects death tests though, and we probably do want it on all of those, so perhaps we could just set it once in third-party/unittest/UnitTestMain/TestMain.cpp ?

I've checked, and you're right -- the flag does affect only death tests (which makes sense, I guess):

https://github.com/llvm/llvm-project/blob/main/third-party/unittest/googletest/src/gtest-death-test.cc#L1508

The difference is essentially this: For a "threadsafe" death test, the code does a "fork + exec" and re-executes only the current test in the new process, whereas for a "fast" death test, it only does a fork (on platforms that can fork).

The scary warnings in the documentation <https://github.com/google/googletest/blob/main/docs/advanced.md#death-tests-and-threads> about "increased test execution time (potentially dramatically so)" had made me wary of enabling the flag everywhere, but as it turns out, the increased execution time only applies to death tests -- so setting the flag globally in `main()` would have exactly the same effect as setting it locally in every test, the way I'm doing at the moment.

I"ll update the patch to do this but first wanted to respond to the discussion.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152696/new/

https://reviews.llvm.org/D152696