This is an archive of the discontinued LLVM Phabricator instance.

Differential D152696

Prevent deadlocks in death tests.
ClosedPublic

Authored by mboehme on Jun 12 2023, 4:09 AM.

Details

Reviewers
thieta
jdenny
hans
Commits
rGdfbcee286b9b: Prevent deadlocks in death tests.
Summary

We have recently started seeing deadlocks in death tests while running in an internal test environment.

Per the documentation here, there are issues with death tests in the presence of threads:

https://github.com/google/googletest/blob/main/docs/advanced.md#death-tests-and-threads

To avoid the deadlocks, I first tried appending DeathTest to the relevant test suite names, which has the effect of running these test suites before all other tests. However, this did not prevent the deadlocks.

This patch therefore uses the option of setting the death_test_style flag to "threadsafe" (see description in the page linked above under "Death Test Styles"), and this prevents the deadlocks.

The documentation notes that the "threadsafe" death test style "trades increased test execution time (potentially dramatically so) for improved thread safety". This is because, to execute a death test, "threadsafe" does a "fork + exec", then re-executes the current test in the child process, whereas the default "fast" death test style does only a fork (on those platforms that support it). However, as we have relatively few death tests, the increased execution time does not make a big difference in total test execution time in my testing.

Note that other projects, such as Chromium, also choose to set the "threadsafe" death test style globally:

https://source.chromium.org/chromium/chromium/src/+/main:base/test/test_suite.cc;l=367

Diff Detail

Event Timeline

mboehme created this revision.Jun 12 2023, 4:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 12 2023, 4:09 AM
Herald added a subscriber: thopre. · View Herald Transcript
mboehme requested review of this revision.Jun 12 2023, 4:09 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 12 2023, 4:09 AM
Herald added subscribers: llvm-commits, cfe-commits. · View Herald Transcript
Harbormaster completed remote builds in B238149: Diff 530453.Jun 12 2023, 5:23 AM
Herald added a subscriber: ormris. · View Herald TranscriptJun 12 2023, 5:23 AM
hans added a subscriber: hans.Jun 12 2023, 5:46 AM

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 ?

MaskRay added reviewers: thieta, jdenny.Jun 12 2023, 1:17 PM
mboehme added a comment.Jun 12 2023, 11:41 PM
In 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 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.

mboehme updated this revision to Diff 530789.Jun 13 2023, 12:06 AM

Set the death_test_style flag globally in TestMain.cpp instead of each
individual test.

mboehme edited the summary of this revision. (Show Details)Jun 13 2023, 12:07 AM
hans accepted this revision.Jun 13 2023, 12:14 AM

lgtm

This revision is now accepted and ready to land.Jun 13 2023, 12:14 AM
Harbormaster completed remote builds in B238405: Diff 530789.Jun 13 2023, 1:22 AM
Closed by commit rGdfbcee286b9b: Prevent deadlocks in death tests. (authored by mboehme). · Explain WhyJun 14 2023, 9:29 PM
This revision was automatically updated to reflect the committed changes.
mboehme added a commit: rGdfbcee286b9b: Prevent deadlocks in death tests..
mgorny added a subscriber: mgorny.Jun 17 2023, 4:03 AM

This change is causing a lot of unittests to fail on Gentoo. I've tested both on amd64 and arm64; on amd64 additionally the test suite seems to hang.

Failed Tests (75):
  LLVM-Unit :: ADT/./ADTTests/APFloatTest/SemanticsDeath
  LLVM-Unit :: ADT/./ADTTests/APIntTest/StringDeath
  LLVM-Unit :: ADT/./ADTTests/APSIntTest/StringDeath
  LLVM-Unit :: ADT/./ADTTests/BitVectorTest/0/DenseSet
  LLVM-Unit :: ADT/./ADTTests/BitVectorTest/1/DenseSet
  LLVM-Unit :: ADT/./ADTTests/BitfieldsTest/ValueTooBigBool
  LLVM-Unit :: ADT/./ADTTests/BitfieldsTest/ValueTooBigBounded
  LLVM-Unit :: ADT/./ADTTests/BitfieldsTest/ValueTooBigInt
  LLVM-Unit :: ADT/./ADTTests/BumpPtrListTest/resetAlloc
  LLVM-Unit :: ADT/./ADTTests/FallibleIteratorTest/RegularLoopExitRequiresErrorCheck
  LLVM-Unit :: ADT/./ADTTests/PackedVectorTest/SignedValues
  LLVM-Unit :: ADT/./ADTTests/PackedVectorTest/UnsignedValues
  LLVM-Unit :: ADT/./ADTTests/STLExtrasTest/EarlyIncrementTest
  LLVM-Unit :: ADT/./ADTTests/STLExtrasTest/EarlyIncrementTestCustomPointerIterator
  LLVM-Unit :: ADT/./ADTTests/STLExtrasTest/EnumerateDifferentLengths
  LLVM-Unit :: ADT/./ADTTests/SmallVectorReferenceInvalidationTest/0/AppendRange
  LLVM-Unit :: ADT/./ADTTests/SmallVectorReferenceInvalidationTest/0/AssignRange
  LLVM-Unit :: ADT/./ADTTests/SmallVectorReferenceInvalidationTest/0/InsertRange
  LLVM-Unit :: ADT/./ADTTests/SmallVectorReferenceInvalidationTest/1/AppendRange
  LLVM-Unit :: ADT/./ADTTests/SmallVectorReferenceInvalidationTest/1/AssignRange
  LLVM-Unit :: ADT/./ADTTests/SmallVectorReferenceInvalidationTest/1/InsertRange
  LLVM-Unit :: ADT/./ADTTests/SmallVectorTest/0/TruncateTest
  LLVM-Unit :: ADT/./ADTTests/SmallVectorTest/1/TruncateTest
  LLVM-Unit :: ADT/./ADTTests/SmallVectorTest/2/TruncateTest
  LLVM-Unit :: ADT/./ADTTests/SmallVectorTest/3/TruncateTest
  LLVM-Unit :: ADT/./ADTTests/SmallVectorTest/4/TruncateTest
  LLVM-Unit :: ADT/./ADTTests/StrongIntDeathTest/OutOfBounds
  LLVM-Unit :: ADT/./ADTTests/ZipIteratorTest/ZipEqualNotEqual
  LLVM-Unit :: ADT/./ADTTests/ZipIteratorTest/ZipFirstNotShortest
  LLVM-Unit :: Analysis/./AnalysisTests/CGSCCPassManagerTest/TestUpdateCGAndAnalysisManagerForPasses1
  LLVM-Unit :: Analysis/./AnalysisTests/CGSCCPassManagerTest/TestUpdateCGAndAnalysisManagerForPasses3
  LLVM-Unit :: Analysis/./AnalysisTests/CGSCCPassManagerTest/TestUpdateCGAndAnalysisManagerForPasses5
  LLVM-Unit :: Analysis/./AnalysisTests/VFShapeAPITest/Parameters_Invalid
  LLVM-Unit :: AsmParser/./AsmParserTests/AsmParserTest/NonNullTerminatedInput
  LLVM-Unit :: BinaryFormat/./BinaryFormatTests/MsgPackWriter/TestWriteCompatibleNoBin
  LLVM-Unit :: ExecutionEngine/JITLink/./JITLinkTests/LinkGraphTest/ContentAccessAndUpdate
  LLVM-Unit :: FileCheck/./FileCheckTests/FileCheckTest/FileCheckContext
  LLVM-Unit :: IR/./IRTests/ConstantsTest/ReplaceWithConstantTest
  LLVM-Unit :: IR/./IRTests/GlobalTest/AlignDeath
  LLVM-Unit :: IR/./IRTests/ValueHandle/AssertingVH_Asserts
  LLVM-Unit :: IR/./IRTests/ValueHandle/PoisoningVH_Asserts
  LLVM-Unit :: IR/./IRTests/ValueHandle/TrackingVH_Asserts
  LLVM-Unit :: IR/./IRTests/ValueTest/getLocalSlotDeath
  LLVM-Unit :: IR/./IRTests/VectorBuilderTest/TestFail_ReportAndAbort
  LLVM-Unit :: Support/./SupportTests/AlignmentDeathTest/AlignAddr
  LLVM-Unit :: Support/./SupportTests/AlignmentDeathTest/ComparisonsWithZero
  LLVM-Unit :: Support/./SupportTests/AlignmentDeathTest/InvalidCTors
  LLVM-Unit :: Support/./SupportTests/CastingTest/assertion_check_const_ref
  LLVM-Unit :: Support/./SupportTests/CastingTest/assertion_check_ptr
  LLVM-Unit :: Support/./SupportTests/CastingTest/assertion_check_ref
  LLVM-Unit :: Support/./SupportTests/CastingTest/assertion_check_unique_ptr
  LLVM-Unit :: Support/./SupportTests/DataExtractorDeathTest/Cursor
  LLVM-Unit :: Support/./SupportTests/Error/AccessExpectedInFailureMode
  LLVM-Unit :: Support/./SupportTests/Error/CantFailDeath
  LLVM-Unit :: Support/./SupportTests/Error/ErrorAsOutParameterUnchecked
  LLVM-Unit :: Support/./SupportTests/Error/FailureFromHandler
  LLVM-Unit :: Support/./SupportTests/Error/FailureToHandle
  LLVM-Unit :: Support/./SupportTests/Error/UncheckedError
  LLVM-Unit :: Support/./SupportTests/Error/UncheckedExpectedInSuccessModeAccess
  LLVM-Unit :: Support/./SupportTests/Error/UncheckedExpectedInSuccessModeAssignment
  LLVM-Unit :: Support/./SupportTests/Error/UncheckedExpectedInSuccessModeDestruction
  LLVM-Unit :: Support/./SupportTests/Error/UncheckedSuccess
  LLVM-Unit :: Support/./SupportTests/Error/UnhandledExpectedInFailureMode
  LLVM-Unit :: Support/./SupportTests/ErrorDeathTest/ExitOnError
  LLVM-Unit :: Support/./SupportTests/ErrorOr/SimpleValue
  LLVM-Unit :: Support/./SupportTests/FileSystemTest/FileMapping
  LLVM-Unit :: Support/./SupportTests/JSONTest/Types
  LLVM-Unit :: Support/./SupportTests/YAMLIO/TestReadWritePolymorphicScalar
  LLVM-Unit :: Support/./SupportTests/raw_pwrite_ostreamTest/TestFD
  LLVM-Unit :: Support/./SupportTests/raw_pwrite_ostreamTest/TestSVector
  LLVM-Unit :: Target/AArch64/./AArch64Tests/SMEAttributes/Constructors
  LLVM-Unit :: Testing/Annotations/./TestingAnnotationTests/AnnotationsTest/Errors
  LLVM-Unit :: Transforms/Utils/./UtilsTests/BasicBlockUtils/splitBasicBlockBefore_ex2
  LLVM-Unit :: Transforms/Utils/./UtilsTests/ValueMapperTest/mapMetadataLocalAsMetadata
  LLVM-Unit :: Transforms/Vectorize/./VectorizeTests/VPRecipeTest/dump

The failures seem to be roughly the same, e.g.:

FAIL: LLVM-Unit :: AsmParser/./AsmParserTests/1/8 (47212 of 49607)
******************** TEST 'LLVM-Unit :: AsmParser/./AsmParserTests/1/8' FAILED ********************
Script(shard):
--
GTEST_OUTPUT=json:/var/tmp/portage/sys-devel/llvm-17.0.0.9999/work/llvm_build-.arm64/unittests/AsmParser/./AsmParserTests-LLVM-Unit-2378-1-8.json GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=8 GTEST_SHARD_INDEX=1 /var/tmp/portage/sys-devel/llvm-17.0.0.9999/work/llvm_build-.arm64/unittests/AsmParser/./AsmParserTests
--

Script:
--
/var/tmp/portage/sys-devel/llvm-17.0.0.9999/work/llvm_build-.arm64/unittests/AsmParser/./AsmParserTests --gtest_filter=AsmParserTest.NonNullTerminatedInput
--
/var/tmp/portage/sys-devel/llvm-17.0.0.9999/work/llvm/unittests/AsmParser/AsmParserTest.cpp:42: Failure
Death test: Mod = parseAssemblyString(Source.substr(0, Source.size() - 2), Error, Ctx)
    Result: died but not with expected error.
  Expected: contains regular expression "Buffer is not null terminated!"
Actual msg:
[  DEATH   ] #0 0x0000ffff90ddd614 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/var/tmp/portage/sys-devel/llvm-17.0.0.9999/work/llvm_build-.arm64/lib64/libLLVM-17gitdfbcee28.so+0xccd614)
[  DEATH   ] #1 0x0000ffff90ddb330 llvm::sys::RunSignalHandlers() (/var/tmp/portage/sys-devel/llvm-17.0.0.9999/work/llvm_build-.arm64/lib64/libLLVM-17gitdfbcee28.so+0xccb330)
[  DEATH   ] #2 0x0000ffff90ddb4dc SignalHandler(int) Signals.cpp:0:0
[  DEATH   ] #3 0x0000ffff98f8e7cc (linux-vdso.so.1+0x7cc)
[  DEATH   ] #4 0x0000ffff98f14540 (/usr/lib64/libsandbox.so+0x4540)
[  DEATH   ] #5 0x0000ffff98f1a5e4 execve (/usr/lib64/libsandbox.so+0xa5e4)
[  DEATH   ] #6 0x0000aaaae19d65a8 testing::internal::ExecDeathTestChildMain(void*) gtest-all.cc:0:0
[  DEATH   ] #7 0x0000ffff8fde49dc (/lib64/libc.so.6+0xe49dc)
[  DEATH   ] 

/var/tmp/portage/sys-devel/llvm-17.0.0.9999/work/llvm/unittests/AsmParser/AsmParserTest.cpp:42
Death test: Mod = parseAssemblyString(Source.substr(0, Source.size() - 2), Error, Ctx)
    Result: died but not with expected error.
  Expected: contains regular expression "Buffer is not null terminated!"
Actual msg:
[  DEATH   ] #0 0x0000ffff90ddd614 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/var/tmp/portage/sys-devel/llvm-17.0.0.9999/work/llvm_build-.arm64/lib64/libLLVM-17gitdfbcee28.so+0xccd614)
[  DEATH   ] #1 0x0000ffff90ddb330 llvm::sys::RunSignalHandlers() (/var/tmp/portage/sys-devel/llvm-17.0.0.9999/work/llvm_build-.arm64/lib64/libLLVM-17gitdfbcee28.so+0xccb330)
[  DEATH   ] #2 0x0000ffff90ddb4dc SignalHandler(int) Signals.cpp:0:0
[  DEATH   ] #3 0x0000ffff98f8e7cc (linux-vdso.so.1+0x7cc)
[  DEATH   ] #4 0x0000ffff98f14540 (/usr/lib64/libsandbox.so+0x4540)
[  DEATH   ] #5 0x0000ffff98f1a5e4 execve (/usr/lib64/libsandbox.so+0xa5e4)
[  DEATH   ] #6 0x0000aaaae19d65a8 testing::internal::ExecDeathTestChildMain(void*) gtest-all.cc:0:0
[  DEATH   ] #7 0x0000ffff8fde49dc (/lib64/libc.so.6+0xe49dc)
[  DEATH   ] 


********************

Full build log (from arm64):

build.log9 MBDownload

mboehme added a comment.Jun 20 2023, 12:13 AM
In D152696#4430238, @mgorny wrote:

This change is causing a lot of unittests to fail on Gentoo. I've tested both on amd64 and arm64; on amd64 additionally the test suite seems to hang.
...

Sorry, only just saw this now. I'll revert.

mgorny added a comment.Jun 20 2023, 12:36 AM

Thank you.

mboehme added a reverting change: rGaad514174524: Revert "Prevent deadlocks in death tests.".Jun 20 2023, 12:39 AM

Revision Contents

PathSize
third-party/
unittest/
UnitTestMain/
4 lines

Diff 531605

third-party/unittest/UnitTestMain/TestMain.cpp

Show All 23 Lines
int main(int argc, char **argv) { int main(int argc, char **argv) {
// Skip setting up signal handlers for tests that need to test things without // Skip setting up signal handlers for tests that need to test things without
// them configured. // them configured.
if (!getenv("LLVM_PROGRAM_TEST_NO_STACKTRACE_HANDLER")) { if (!getenv("LLVM_PROGRAM_TEST_NO_STACKTRACE_HANDLER")) {
llvm::sys::PrintStackTraceOnErrorSignal(argv[0], llvm::sys::PrintStackTraceOnErrorSignal(argv[0],
true /* Disable crash reporting */); true /* Disable crash reporting */);
} }
// Use the "threadsafe" test style for death tests -- the "fast" test style
// can cause deadlocks.
testing::GTEST_FLAG(death_test_style) = "threadsafe";
// Initialize both gmock and gtest. // Initialize both gmock and gtest.
testing::InitGoogleMock(&argc, argv); testing::InitGoogleMock(&argc, argv);
llvm::cl::ParseCommandLineOptions(argc, argv); llvm::cl::ParseCommandLineOptions(argc, argv);
// Make it easy for a test to re-execute itself by saving argv[0]. // Make it easy for a test to re-execute itself by saving argv[0].
TestMainArgv0 = argv[0]; TestMainArgv0 = argv[0];
Show All 17 Lines