[PATCH] D152696: Prevent deadlocks in death tests.

2023-06-20 Thread Michał Górny via Phabricator via cfe-commits 
mgorny added a comment.

Thank you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152696

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152696: Prevent deadlocks in death tests.

2023-06-20 Thread Martin Böhme via Phabricator via cfe-commits 
mboehme added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152696

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152696: Prevent deadlocks in death tests.

2023-06-17 Thread Michał Górny via Phabricator via cfe-commits 
mgorny added a comment.

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 :

[PATCH] D152696: Prevent deadlocks in death tests.

2023-06-14 Thread Martin Böhme via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rGdfbcee286b9b: Prevent deadlocks in death tests. (authored by 
mboehme).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152696

Files:
  third-party/unittest/UnitTestMain/TestMain.cpp


Index: third-party/unittest/UnitTestMain/TestMain.cpp
===
--- third-party/unittest/UnitTestMain/TestMain.cpp
+++ third-party/unittest/UnitTestMain/TestMain.cpp
@@ -29,6 +29,10 @@
 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.
   testing::InitGoogleMock(&argc, argv);
 


Index: third-party/unittest/UnitTestMain/TestMain.cpp
===
--- third-party/unittest/UnitTestMain/TestMain.cpp
+++ third-party/unittest/UnitTestMain/TestMain.cpp
@@ -29,6 +29,10 @@
 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.
   testing::InitGoogleMock(&argc, argv);
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152696: Prevent deadlocks in death tests.

2023-06-13 Thread Hans Wennborg via Phabricator via cfe-commits 
hans accepted this revision.
hans added a comment.
This revision is now accepted and ready to land.

lgtm


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152696

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152696: Prevent deadlocks in death tests.

2023-06-13 Thread Martin Böhme via Phabricator via cfe-commits 
mboehme updated this revision to Diff 530789.
mboehme added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152696

Files:
  third-party/unittest/UnitTestMain/TestMain.cpp


Index: third-party/unittest/UnitTestMain/TestMain.cpp
===
--- third-party/unittest/UnitTestMain/TestMain.cpp
+++ third-party/unittest/UnitTestMain/TestMain.cpp
@@ -29,6 +29,10 @@
 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.
   testing::InitGoogleMock(&argc, argv);
 


Index: third-party/unittest/UnitTestMain/TestMain.cpp
===
--- third-party/unittest/UnitTestMain/TestMain.cpp
+++ third-party/unittest/UnitTestMain/TestMain.cpp
@@ -29,6 +29,10 @@
 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.
   testing::InitGoogleMock(&argc, argv);
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152696: Prevent deadlocks in death tests.

2023-06-12 Thread Martin Böhme via Phabricator via cfe-commits 
mboehme added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152696

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152696: Prevent deadlocks in death tests.

2023-06-12 Thread Hans Wennborg via Phabricator via cfe-commits 
hans added a comment.

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 ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152696

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152696: Prevent deadlocks in death tests.

2023-06-12 Thread Martin Böhme via Phabricator via cfe-commits 
mboehme created this revision.
Herald added a subscriber: thopre.
Herald added a project: All.
mboehme requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

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.

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.

This patch changes a number of unit tests across LLVM and Clang. As this patch
fixes a test infrastructure issue and doesn't affect the behavior of the tests
themselves, it seemed appropriate to change all death tests in a single patch.
However, if reviewers would prefer me to split this up into separate patches
form LLVM and Clang, I'm happy to do that.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D152696

Files:
  clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp
  clang/unittests/ASTMatchers/Dynamic/VariantValueTest.cpp
  clang/unittests/Basic/FileManagerTest.cpp
  clang/unittests/Basic/LineOffsetMappingTest.cpp
  clang/unittests/Basic/SarifTest.cpp
  clang/unittests/Lex/LexerTest.cpp
  clang/unittests/Serialization/InMemoryModuleCacheTest.cpp
  clang/unittests/Tooling/TransformerTest.cpp
  llvm/unittests/ADT/APFloatTest.cpp
  llvm/unittests/ADT/APIntTest.cpp
  llvm/unittests/ADT/APSIntTest.cpp
  llvm/unittests/ADT/AnyTest.cpp
  llvm/unittests/ADT/BumpPtrListTest.cpp
  llvm/unittests/ADT/IteratorTest.cpp
  llvm/unittests/ADT/STLExtrasTest.cpp
  llvm/unittests/ADT/SequenceTest.cpp
  llvm/unittests/ADT/SmallVectorTest.cpp
  llvm/unittests/AsmParser/AsmParserTest.cpp
  llvm/unittests/BinaryFormat/MsgPackWriterTest.cpp
  llvm/unittests/CodeGen/GlobalISel/MachineIRBuilderTest.cpp
  llvm/unittests/CodeGen/LowLevelTypeTest.cpp
  llvm/unittests/CodeGen/MachineInstrBundleIteratorTest.cpp
  llvm/unittests/FileCheck/FileCheckTest.cpp
  llvm/unittests/IR/ConstantsTest.cpp
  llvm/unittests/IR/MetadataTest.cpp
  llvm/unittests/IR/ValueHandleTest.cpp
  llvm/unittests/IR/ValueTest.cpp
  llvm/unittests/Support/DataExtractorTest.cpp
  llvm/unittests/Support/ErrorTest.cpp
  llvm/unittests/Support/Path.cpp
  llvm/unittests/Support/YAMLIOTest.cpp
  llvm/unittests/Support/raw_pwrite_stream_test.cpp
  llvm/unittests/Transforms/Utils/ValueMapperTest.cpp

Index: llvm/unittests/Transforms/Utils/ValueMapperTest.cpp
===
--- llvm/unittests/Transforms/Utils/ValueMapperTest.cpp
+++ llvm/unittests/Transforms/Utils/ValueMapperTest.cpp
@@ -265,6 +265,8 @@
 #ifdef GTEST_HAS_DEATH_TEST
 #ifndef NDEBUG
 TEST(ValueMapperTest, mapMetadataLocalAsMetadata) {
+  testing::GTEST_FLAG(death_test_style) = "threadsafe";
+
   LLVMContext C;
   FunctionType *FTy =
   FunctionType::get(Type::getVoidTy(C), Type::getInt8Ty(C), false);
Index: llvm/unittests/Support/raw_pwrite_stream_test.cpp
===
--- llvm/unittests/Support/raw_pwrite_stream_test.cpp
+++ llvm/unittests/Support/raw_pwrite_stream_test.cpp
@@ -38,6 +38,8 @@
 
 #ifdef GTEST_HAS_DEATH_TEST
 #ifndef NDEBUG
+  testing::GTEST_FLAG(death_test_style) = "threadsafe";
+
   EXPECT_DEATH(OS.pwrite("12345", 5, 0),
"We don't support extending the stream");
 #endif
@@ -74,6 +76,8 @@
 
 #ifdef GTEST_HAS_DEATH_TEST
 #ifndef NDEBUG
+  testing::GTEST_FLAG(death_test_style) = "threadsafe";
+
   EXPECT_DEATH(OS.pwrite("12345", 5, 0),
"We don't support extending the stream");
 #endif
Index: llvm/unittests/Support/YAMLIOTest.cpp
===
--- llvm/unittests/Support/YAMLIOTest.cpp
+++ llvm/unittests/Support/YAMLIOTest.cpp
@@ -3064,6 +3064,8 @@
   Output yout(ostr);
 #ifdef GTEST_HAS_DEATH_TEST
 #ifndef NDEBUG
+  testing::GTEST_FLAG(death_test_style) = "threadsafe";
+
   EXPECT_DEATH(yout << node, "plain scalar documents are not supported");
 #endif
 #endif
Index: llvm/unittests/Support/Path.cpp
===