[libcxx-commits] [PATCH] D154557: [libc++] Add more general death tests and make them work on more platforms

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

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:





================
Comment at: libcxx/test/libcxx/containers/sequences/array/array.zero/assert.subscript.pass.cpp:20
 
 int main(int, char**) {
   {
----------------
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`.


================
Comment at: libcxx/test/support/terminating_test_helper.h:9
+
+#ifndef TEST_TERMINATING_TEST_HELPER_H
+#define TEST_TERMINATING_TEST_HELPER_H
----------------



================
Comment at: libcxx/test/support/terminating_test_helper.h:37
+  std_terminate,
+  nondetermenistic_test_count,
+#ifdef _LIBCPP_VERSION
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154557