This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Add more general death tests and make them work on more platforms
AbandonedPublic

Authored by philnik on Jul 5 2023, 6:29 PM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Summary

This version of death tests spawns processes instead of forking the current process. This makes it much easier to port the tests to more platforms, since a lot of platforms don't support forking since it's a very intrusive and expensive operation.

Diff Detail

Event Timeline

philnik created this revision.Jul 5 2023, 6:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2023, 6:29 PM
Herald added a subscriber: mgrang. · View Herald Transcript
philnik requested review of this revision.Jul 5 2023, 6:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2023, 6:29 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

@mstorsjo @fsb4000 Could one of you help me out with spawning processes on windows? It should be fairly simple. You just have to return the output of stdout and stderr and the return value from run_test_process.

philnik updated this revision to Diff 537831.Jul 6 2023, 12:24 PM

Try to fix CI

ldionne added inline comments.Jul 7 2023, 12:01 PM
libcxx/test/libcxx/algorithms/debug_less.inconsistent.pass.cpp
16–17

This seems unrelated, and it creates confusion in the patch. Can you please audit and only keep what's relevant to the patch?

I'm ambivalent on this patch because it leaves us in a state where we have two frameworks for death tests and both are reasonably heavyweight. Instead, if we wanted to improve significantly on top of what we have today, I think we would need to implement support for these tests in the test harness (Lit).

We explored this a bit and it could look something like this:

#define TEST_EXPECTED_ERROR(expr, message)
  do {
    if (gather_tests) {
      print(f"{test_counter} [[[{message}]]]");
    } else if (test_counter-- == 0) {
      expr();
    }
  } while (false)

// From lit:
// 1. Run the test with no arguments.
// 2. Parse the output and build a mapping like this:
//      {test-number => expected-error-message}
// 3. Then you run the executable with each test number and you check the error message.

TEST_RUN_VERIFY_MAIN() {
  {
    typedef std::array<int, 0> C;
    C c = {};
    C const& cc = c;
    TEST_EXPECTED_ERROR(c[0], "cannot call array<T, 0>::operator[] on a zero-sized array");
    TEST_EXPECTED_ERROR(c[1], "cannot call array<T, 0>::operator[] on a zero-sized array");
    TEST_EXPECTED_ERROR(cc[0], "cannot call array<T, 0>::operator[] on a zero-sized array");
    TEST_EXPECTED_ERROR(cc[1], "cannot call array<T, 0>::operator[] on a zero-sized array");
  }
  {
    typedef std::array<const int, 0> C;
    C c = {{}};
    C const& cc = c;
    TEST_EXPECTED_ERROR(c[0], "cannot call array<T, 0>::operator[] on a zero-sized array");
    TEST_EXPECTED_ERROR(c[1], "cannot call array<T, 0>::operator[] on a zero-sized array");
    TEST_EXPECTED_ERROR(cc[0], "cannot call array<T, 0>::operator[] on a zero-sized array");
    TEST_EXPECTED_ERROR(cc[1], "cannot call array<T, 0>::operator[] on a zero-sized array");
  }

  return 0;
}

Unfortunately, the "From Lit" part ends up being quite complicated (we prototyped) and so it's not clear it's worth pursuing at this time. Barring that, I think we should instead improve the current death test framework to support intentional calls to std::terminate, which should be doable roughly with

diff --git a/libcxx/test/support/check_assertion.h b/libcxx/test/support/check_assertion.h
index 741dc4a78828..263413b50b62 100644
--- a/libcxx/test/support/check_assertion.h
+++ b/libcxx/test/support/check_assertion.h
@@ -111,7 +111,7 @@ inline AssertionInfoMatcher& GlobalMatcher() {
 
 struct DeathTest {
   enum ResultKind {
-    RK_DidNotDie, RK_MatchFound, RK_MatchFailure, RK_SetupFailure, RK_Unknown
+    RK_DidNotDie, RK_MatchFound, RK_MatchFailure, RK_SetupFailure, RK_TerminatedAsExpected, RK_Unknown
   };
 
   static const char* ResultKindToString(ResultKind RK) {
@@ -121,6 +121,7 @@ struct DeathTest {
     CASE(RK_DidNotDie);
     CASE(RK_SetupFailure);
     CASE(RK_MatchFound);
+    CASE(RK_TerminatedAsExpected);
     CASE(RK_Unknown);
     }
     return "not a result kind";
@@ -256,6 +257,11 @@ void std::__libcpp_verbose_abort(char const* format, ...) {
 template <class Func>
 inline bool ExpectDeath(const char* stmt, Func&& func, AssertionInfoMatcher Matcher) {
   DeathTest DT(Matcher);
+  set_terminate([] {
+    if (__expect_to_terminate__)
+      std::exit(RK_TerminatedAsExpected);
+    std::abort();
+  });
   DeathTest::ResultKind RK = DT.Run(func);
   auto OnFailure = [&](const char* msg) {
     std::fprintf(stderr, "EXPECT_DEATH( %s ) failed! (%s)\n\n", stmt, msg);
@@ -273,6 +279,8 @@ inline bool ExpectDeath(const char* stmt, Func&& func, AssertionInfoMatcher Matc
   switch (RK) {
   case DeathTest::RK_MatchFound:
     return true;
+  case DeathTest::RK_TerminatedAsExpected:
+    return true;
   case DeathTest::RK_SetupFailure:
     return OnFailure("child failed to setup test environment");
   case DeathTest::RK_Unknown:
libcxx/test/libcxx/containers/sequences/array/array.zero/assert.subscript.pass.cpp
20

Should this be TEST_TERMINATIONS? It's also problematic that the test is going to appear to work if you misuse the "test framework" like this. Could probably be detected by referencing something that TEST_TERMINATIONS defines from EXPECT_LIBCPP_ASSERT_FAILURE.

libcxx/test/support/terminating_test_helper.h
9
37
philnik planned changes to this revision.Aug 7 2023, 3:11 PM
philnik abandoned this revision.Sep 18 2023, 9:49 AM