[Lldb-commits] [lldb] [lldb][test] Make MemoryTest use the threadsafe gtest death-test style (PR #181127)

[via lldb-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-lldb

Author: Michael Buch (Michael137)

<details>
<summary>Changes</summary>

These assertions would occasionally crash on macOS CI with following stacktrace:
```
06:53:31  Death test: { read_results = process_sp-&gt;ReadMemoryRanges(ranges, 
buffer); }
06:53:31      Result: died but not with expected error.
06:53:31    Expected: contains regular expression "read more than requested 
bytes"
06:53:31  Actual msg:
06:53:31  [  DEATH   ] Stack dump without symbol names (ensure you have 
llvm-symbolizer in your PATH or set the environment var `LLVM_SYMBOLIZER_PATH` 
to point to it):
06:53:31  [  DEATH   ] 0  TargetTests              0x000000010055bb80 
llvm::sys::PrintStackTrace(llvm::raw_ostream&amp;, int) + 56
06:53:31  [  DEATH   ] 1  TargetTests              0x0000000100559778 
llvm::sys::RunSignalHandlers() + 64
06:53:31  [  DEATH   ] 2  TargetTests              0x000000010055c668 
SignalHandler(int, __siginfo*, void*) + 344
06:53:31  [  DEATH   ] 3  libsystem_platform.dylib 0x0000000196993744 _sigtramp 
+ 56
06:53:31  [  DEATH   ] 4  libsystem_trace.dylib    0x00000001966b5180 
_os_log_preferences_refresh + 36
06:53:31  [  DEATH   ] 5  libsystem_trace.dylib    0x00000001966b5740 
os_signpost_enabled + 300
06:53:31  [  DEATH   ] 6  TargetTests              0x0000000100500930 
llvm::SignpostEmitter::startInterval(void const*, llvm::StringRef) + 68
06:53:31  [  DEATH   ] 7  TargetTests              0x0000000100782c24 
lldb_private::Timer::Timer(lldb_private::Timer::Category&amp;, char const*, 
...) + 168
06:53:31  [  DEATH   ] 8  TargetTests              0x000000010069cd44 
lldb_private::Process::ReadMemoryFromInferior(unsigned long long, void*, 
unsigned long, lldb_private::Status&amp;) + 100
06:53:31  [  DEATH   ] 9  TargetTests              0x000000010069cf68 
lldb_private::Process::ReadMemoryRanges(llvm::ArrayRef&lt;lldb_private::Range&lt;unsigned
 long long, unsigned long&gt;&gt;, llvm::MutableArrayRef&lt;unsigned char&gt;) 
+ 300
06:53:31  [  DEATH   ] 10 TargetTests              0x000000010045482c 
MemoryDeathTest_TestReadMemoryRangesReturnsTooMuch_Test::TestBody() + 1748
```

By default, Google death-tests executes the code under test in a sub-process 
(e.g., via `fork`, see [official 
docs](https://google.github.io/googletest/reference/assertions.html#death)). 
However, the os_log APIs are not safe across forks, and using the os_log 
handles of a parent process is not guaranteed to work (read undefined). This is 
why we sometimes non-deterministically crash.

On Darwin we compile with signposts enabled, and there's no mechanism of 
avoiding calling into os_log once you've compiled with them enabled (e.g., in 
our case we just check the runtime `os_signpost_enabled`).

GoogleTest provides an alternative mechanism to spawn the death tests. It 
"re-executes the unit binary but only runs the death tests". This should avoid 
sharing the os_log handles with the parent. This mode is called "threadsafe".

This patch sets this style for the only 2 death-tests currently in the LLDB 
test-suite.

Since this is quite the foot-gun we should disable it for the entire LLDB 
unit-test suite. But to make CI less flakey for now, do this for only the tests 
in question.

---
Full diff: https://github.com/llvm/llvm-project/pull/181127.diff


1 Files Affected:

- (modified) lldb/unittests/Target/MemoryTest.cpp (+14) 


``````````diff
diff --git a/lldb/unittests/Target/MemoryTest.cpp 
b/lldb/unittests/Target/MemoryTest.cpp
index 15b22a47e30e8..47aa105cdfecd 100644
--- a/lldb/unittests/Target/MemoryTest.cpp
+++ b/lldb/unittests/Target/MemoryTest.cpp
@@ -381,6 +381,13 @@ TEST_F(MemoryTest, TestReadMemoryRanges) {
 using MemoryDeathTest = MemoryTest;
 
 TEST_F(MemoryDeathTest, TestReadMemoryRangesReturnsTooMuch) {
+  // gtest death-tests execute in a sub-process (fork), which invalidates
+  // any signpost handles and would cause spurious crashes if used. Use the
+  // "threadsafe" style of death-test to work around this.
+  // FIXME: we should set this only if signposts are enabled, and do so
+  // for the entire test-suite.
+  GTEST_FLAG_SET(death_test_style, "threadsafe");
+
   ArchSpec arch("x86_64-apple-macosx-");
   Platform::SetHostPlatform(PlatformRemoteMacOSX::CreateInstance(true, &arch));
   DebuggerSP debugger_sp = Debugger::CreateInstance();
@@ -409,6 +416,13 @@ TEST_F(MemoryDeathTest, 
TestReadMemoryRangesReturnsTooMuch) {
 }
 
 TEST_F(MemoryDeathTest, TestReadMemoryRangesWithShortBuffer) {
+  // gtest death-tests execute in a sub-process (fork), which invalidates
+  // any signpost handles and would cause spurious crashes if used. Use the
+  // "threadsafe" style of death-test to work around this.
+  // FIXME: we should set this only if signposts are enabled, and do so
+  // for the entire test-suite.
+  GTEST_FLAG_SET(death_test_style, "threadsafe");
+
   ArchSpec arch("x86_64-apple-macosx-");
   Platform::SetHostPlatform(PlatformRemoteMacOSX::CreateInstance(true, &arch));
   DebuggerSP debugger_sp = Debugger::CreateInstance();

``````````

</details>


https://github.com/llvm/llvm-project/pull/181127
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits