[libc] Add a new test matcher for tests raising floating point exceptions.

[sivachandra]

Summary

This new matcher does not use death tests to check if SIGFPE is raised.
Instead, that a SIGFPE was raised is checked using a SIGFPE signal handler.

[sivachandra]

I am not sure how this will play with Fuchsia's zxtest framework.

[abrachet]

Do you anticipate this will be used later to match against if any exception was raised? I imagine in that case the current logic would be moved from the constructor into match.

If not, I think this would be a lot simpler if the logic inside of FPExceptMatcher::FPExceptMatcher was just a function called testSigfpeRaised which returned caughtExcept and then RAISES_FP_EXCEPT would just be ASSERT_TRUE(testSigfpeRaised(func))

Also, what does this solve that the death tests do not, Windows support?

libc/utils/FPUtil/TestHelpers.cpp

19

Not used

86

siglongjmp takes a sigjmp_buf not jmp_buf. I'm guessing [sig]jmp_buf are macros to the same type but we should still use sigjmp_buf when not in Windows.

libc/utils/FPUtil/TestHelpers.h

86

Not used

134

Should this be called ASSERT_RAISES_FP_EXCEPT?

138

Can this be __llvm_libc::testing::Test::createCallable(func)

[sivachandra]

Address comments.

[sivachandra]

In D106086#2881171, @abrachet wrote:

Do you anticipate this will be used later to match against if any exception was raised? I imagine in that case the current logic would be moved from the constructor into match.

Yes, I would like to extend this to match the actual raised exception. So, some kind of reordering might be required.

If not, I think this would be a lot simpler if the logic inside of FPExceptMatcher::FPExceptMatcher was just a function called testSigfpeRaised which returned caughtExcept and then RAISES_FP_EXCEPT would just be ASSERT_TRUE(testSigfpeRaised(func))

The appeal of the matcher over an ASSERT_TRUE(...) kind of check is the custom error message.

Also, what does this solve that the death tests do not, Windows support?

That is the primary motivation. However, I am not yet sure how this will fit with Fuchsia's scheme of things.

libc/utils/FPUtil/TestHelpers.h

138

Yes, we could. I am trying not to grow a dependency from testutils/ExecuteFunction.h. I want to move testutils in to UnitTest as soon as I can. So, we can do that cleanup at that time.

[mcgrathr]

For Fuchsia you should just define the new macro to continue to use zxtest's ASSERT_DEATH macro.

libc/utils/FPUtil/TestHelpers.cpp

77

It seems like it would be cleaner to just do

#define sigjmp_buf jmp_buf
#define sigsetjmp(buf, save) setjmp(buf)
#define siglongjmp(buf) longjmp(buf)

and not have any more conditionals below.

86

Your handler is only called for the signals you've set it on, so this will never be entered with a different value and you don't need to test it.

98

Just make unique_ptr the argument type so callers construct it in place.

105

It's probably wise to use fegetenv / fesetenv here.

libc/utils/FPUtil/TestHelpers.h

79

This ctor is superfluous if you just construct with Callable{func}.

[sivachandra]

Address comments.

libc/utils/FPUtil/TestHelpers.cpp

98

We cannot use std::unique_ptr in the API as we will then have to include <memory> in the header file which will lead to header-mixups in the unit test. That is why I have arranged it this way. Added a comment now in the header file about "ownership transfer".

libc/utils/FPUtil/TestHelpers.h

79

My understanding about brace initializers could be wrong or incomplete here but since the class is dynamic (because of the virtual methods), its objects cannot be initialized with {...}? Clang infact complains that there is no appropriate constructor for it when I use the brace initializer.

[sivachandra]

In D106086#2888664, @mcgrathr wrote:

For Fuchsia you should just define the new macro to continue to use zxtest's ASSERT_DEATH macro.

I have added an alternate definition in TestHelpers.h.

#ifdef LLVM_LIBC_TEST_USE_FUCHSIA
#define ASSERT_RAISES_FP_EXCEPT(func) ASSERT_DEATH(func, WITH_SIGNAL(SIGFPE))
#else
#define ASSERT_RAISES_FP_EXCEPT(func)
...
#endif // LLVM_LIBC_TEST_USE_FUCHSIA

[mcgrathr]

lgtm

libc/test/src/CMakeLists.txt

+function(add_fp_unittest name)
+  cmake_parse_arguments(
+    "MATH_UNITTEST"
+    "NEED_MPFR" # Optional arguments
+    "" # Single value arguments
+    "" # Multi-value arguments
+    ${ARGN}
+  )
+
+  if(MATH_UNITTEST_NEED_MPFR)
+    if(NOT LIBC_TESTS_CAN_USE_MPFR)
+      message("WARNING: Math test ${name} will be skipped as MPFR library is not available.")
+      return()
+    endif()
+  endif()
+
+  add_libc_unittest(${name} ${MATH_UNITTEST_UNPARSED_ARGUMENTS})
+  get_fq_target_name(${name} fq_target_name)
+  if (NOT TARGET ${fq_target_name})
+    return()
+  endif()
+  if(MATH_UNITTEST_NEED_MPFR)
+    target_link_libraries(${fq_target_name} PRIVATE libcMPFRWrapper -lmpfr -lgmp)
+  endif()
+  target_link_libraries(${fq_target_name} PRIVATE LibcFPTestHelpers)
+endfunction(add_fp_unittest)
+
 add_subdirectory(__support)
 add_subdirectory(ctype)
 add_subdirectory(errno)
 add_subdirectory(fenv)
 add_subdirectory(math)
 add_subdirectory(string)
 
 if(NOT LLVM_LIBC_FULL_BUILD)

libc/test/src/fenv/CMakeLists.txt

add_libc_unittest(
   DEPENDS
     libc.src.fenv.feclearexcept
     libc.utils.FPUtil.fputil
 )
 
 if (NOT LLVM_USE_SANITIZER)
   # Sanitizers don't like SIGFPE. So, we will run the 
   # tests which raise SIGFPE only in non-sanitizer builds.
-  add_libc_unittest(
+  add_fp_unittest(
     enabled_exceptions_test
     SUITE
       libc_fenv_unittests
     SRCS
       enabled_exceptions_test.cpp
     DEPENDS
       libc.include.signal
       libc.src.fenv.feclearexcept
       libc.src.fenv.feraiseexcept
       libc.src.fenv.fetestexcept
       libc.utils.FPUtil.fputil
   )
 
-  add_libc_unittest(
+  add_fp_unittest(
     feholdexcept_test
     SUITE
       libc_fenv_unittests
     SRCS
       feholdexcept_test.cpp
     DEPENDS
-      libc.include.signal
+      libc.include.fenv
       libc.src.fenv.feholdexcept
       libc.utils.FPUtil.fputil
   )
 endif()

libc/test/src/fenv/enabled_exceptions_test.cpp

 //===-- Unittests for feraiseexcept with exceptions enabled ---------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "src/fenv/feclearexcept.h"
 #include "src/fenv/feraiseexcept.h"
 #include "src/fenv/fetestexcept.h"
 
 #include "utils/FPUtil/FEnv.h"
+#include "utils/FPUtil/TestHelpers.h"
 #include "utils/UnitTest/Test.h"
 
 #include <fenv.h>
 #include <signal.h>
 
 // This test enables an exception and verifies that raising that exception
 // triggers SIGFPE.
 TEST(LlvmLibcExceptionStatusTest, RaiseAndCrash) {
   // TODO: Install a floating point exception handler and verify that the
   // the expected exception was raised. One will have to longjmp back from
   // that exception handler, so such a testing can be done after we have
   // longjmp implemented.
 
   int excepts[] = {FE_DIVBYZERO, FE_INVALID, FE_INEXACT, FE_OVERFLOW,
                    FE_UNDERFLOW};
 
   // We '|' the individual exception flags instead of using FE_ALL_EXCEPT
   // as it can include non-standard extensions. Note that we should be able
   // to compile this file with headers from other libcs as well.
   constexpr int allExcepts =
       FE_DIVBYZERO | FE_INVALID | FE_INEXACT | FE_OVERFLOW | FE_UNDERFLOW;
 
   for (int e : excepts) {
-    ASSERT_DEATH(
-        [=] {
-          __llvm_libc::fputil::disableExcept(FE_ALL_EXCEPT);
-          __llvm_libc::fputil::enableExcept(e);
-          ASSERT_EQ(__llvm_libc::feclearexcept(FE_ALL_EXCEPT), 0);
-          // Raising all exceptions except |e| should not call the
-          // SIGFPE handler. They should set the exception flag though,
-          // so we verify that.
-          int others = allExcepts & ~e;
-          ASSERT_EQ(__llvm_libc::feraiseexcept(others), 0);
-          ASSERT_EQ(__llvm_libc::fetestexcept(others), others);
 
-          // We don't check the return value when raising |e| as
-          // feraiseexcept will not return when it raises an enabled
-          // exception.
-          __llvm_libc::feraiseexcept(e);
-        },
-        WITH_SIGNAL(SIGFPE));
+    ASSERT_RAISES_FP_EXCEPT([=] { __llvm_libc::feraiseexcept(e); });
   }
 }

libc/test/src/fenv/feholdexcept_test.cpp

 //===-- Unittests for feholdexcept with exceptions enabled ----------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "src/fenv/feholdexcept.h"
 
 #include "utils/FPUtil/FEnv.h"
+#include "utils/FPUtil/TestHelpers.h"
 #include "utils/UnitTest/Test.h"
 
 #include <fenv.h>
-#include <signal.h>
 
 TEST(LlvmLibcFEnvTest, RaiseAndCrash) {
   int excepts[] = {FE_DIVBYZERO, FE_INVALID, FE_INEXACT, FE_OVERFLOW,
                    FE_UNDERFLOW};
 
   for (int e : excepts) {
     fenv_t env;
     __llvm_libc::fputil::disableExcept(FE_ALL_EXCEPT);
     __llvm_libc::fputil::enableExcept(e);
     ASSERT_EQ(__llvm_libc::fputil::clearExcept(FE_ALL_EXCEPT), 0);
     ASSERT_EQ(__llvm_libc::feholdexcept(&env), 0);
     // feholdexcept should disable all excepts so raising an exception
     // should not crash/invoke the exception handler.
     ASSERT_EQ(__llvm_libc::fputil::raiseExcept(e), 0);
 
     // When we put back the saved env which has the exception enabled, it
     // should crash with SIGFPE.
     __llvm_libc::fputil::setEnv(&env);
-    ASSERT_DEATH([=] { __llvm_libc::fputil::raiseExcept(e); },
-                 WITH_SIGNAL(SIGFPE));
+    ASSERT_RAISES_FP_EXCEPT([=] { __llvm_libc::fputil::raiseExcept(e); });
   }
 }

libc/test/src/math/CMakeLists.txt

 add_libc_testsuite(libc_math_unittests)
 
-function(add_fp_unittest name)
-  cmake_parse_arguments(
-    "MATH_UNITTEST"
-    "NEED_MPFR" # Optional arguments
-    "" # Single value arguments
-    "" # Multi-value arguments
-    ${ARGN}
-  )
-
-  if(MATH_UNITTEST_NEED_MPFR)
-    if(NOT LIBC_TESTS_CAN_USE_MPFR)
-      message("WARNING: Math test ${name} will be skipped as MPFR library is not available.")
-      return()
-    endif()
-  endif()
-
-  add_libc_unittest(${name} ${MATH_UNITTEST_UNPARSED_ARGUMENTS})
-  get_fq_target_name(${name} fq_target_name)
-  if (NOT TARGET ${fq_target_name})
-    return()
-  endif()
-  if(MATH_UNITTEST_NEED_MPFR)
-    target_link_libraries(${fq_target_name} PRIVATE libcMPFRWrapper -lmpfr -lgmp)
-  endif()
-  target_link_libraries(${fq_target_name} PRIVATE LibcFPTestHelpers)
-endfunction(add_fp_unittest)
-
 add_fp_unittest(
   cosf_test
   NEED_MPFR
   SUITE
     libc_math_unittests
   SRCS
     cosf_test.cpp
   HDRS

libc/utils/FPUtil/TestHelpers.h

public:
   }
 };
 
 template <__llvm_libc::testing::TestCondition C, typename T>
 FPMatcher<T, C> getMatcher(T expectedValue) {
   return FPMatcher<T, C>(expectedValue);
 }
 
+// TODO: Make the matcher match specific exceptions instead of just identifying
+// that an exception was raised.
+class FPExceptMatcher : public __llvm_libc::testing::Matcher<bool> {
+  bool exceptionRaised;
+
+public:
+  class FunctionCaller {
+  public:
+    virtual ~FunctionCaller(){};
+    virtual void call() = 0;
+  };
+
+  template <typename Func> static FunctionCaller *getFunctionCaller(Func func) {
+    struct Callable : public FunctionCaller {
+      Func f;
+      explicit Callable(Func theFunc) : f(theFunc) {}
+      void call() override { f(); }
+    };
+
+    return new Callable(func);
+  }
+
+  // Takes ownership of func.
+  explicit FPExceptMatcher(FunctionCaller *func);
+
+  bool match(bool unused) { return exceptionRaised; }
+
+  void explainError(testutils::StreamWrapper &stream) override {
+    stream << "A floating point exception should have been raised but it "
+           << "wasn't\n";
+  }
+};
+
 } // namespace testing
 } // namespace fputil
 } // namespace __llvm_libc
 
 #define DECLARE_SPECIAL_CONSTANTS(T)                                           \
   using FPBits = __llvm_libc::fputil::FPBits<T>;                               \
   using UIntType = typename FPBits::UIntType;                                  \
   const T zero = T(FPBits::zero());                                            \
EXPECT_THAT(                                                                 \
           expected))
 
 #define ASSERT_FP_NE(expected, actual)                                         \
   ASSERT_THAT(                                                                 \
       actual,                                                                  \
       __llvm_libc::fputil::testing::getMatcher<__llvm_libc::testing::Cond_NE>( \
           expected))
 
+#ifdef LLVM_LIBC_TEST_USE_FUCHSIA
+#define ASSERT_RAISES_FP_EXCEPT(func) ASSERT_DEATH(func, WITH_SIGNAL(SIGFPE))
+#else
+#define ASSERT_RAISES_FP_EXCEPT(func)                                          \
+  ASSERT_THAT(                                                                 \
+      true,                                                                    \
+      __llvm_libc::fputil::testing::FPExceptMatcher(                           \
+          __llvm_libc::fputil::testing::FPExceptMatcher::getFunctionCaller(    \
+              func)))
+#endif // LLVM_LIBC_TEST_USE_FUCHSIA
+
 #endif // LLVM_LIBC_UTILS_FPUTIL_TEST_HELPERS_H

libc/utils/FPUtil/TestHelpers.cpp

 //===-- TestMatchers.cpp ----------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "TestHelpers.h"
 
+#include "FEnv.h"
 #include "FPBits.h"
 
+#include <memory>
+#include <setjmp.h>
+#include <signal.h>
 #include <string>
 
 namespace __llvm_libc {
 namespace fputil {
 namespace testing {
 
 // Return the first N hex digits of an integer as a string in upper case.
 template <typename T>
 cpp::EnableIfType<cpp::IsIntegral<T>::Value, std::string>
 uintToHex(T X, size_t Length = sizeof(T) * 2) {
   std::string s(Length, '0');

 
 template void describeValue<float>(const char *, float,
                                    testutils::StreamWrapper &);
 template void describeValue<double>(const char *, double,
                                     testutils::StreamWrapper &);
 template void describeValue<long double>(const char *, long double,
                                          testutils::StreamWrapper &);
 
+#if defined(__WIN32) || defined(_WIN64)
+#define sigjmp_buf jmp_buf
+#define sigsetjmp(buf, save) setjmp(buf)
+#define siglongjmp(buf) longjmp(buf)
+#endif
+
+static thread_local sigjmp_buf jumpBuffer;
+static thread_local bool caughtExcept;
+
+static void sigfpeHandler(int sig) {
+  caughtExcept = true;
+  siglongjmp(jumpBuffer, -1);
+}
+
+FPExceptMatcher::FPExceptMatcher(FunctionCaller *func) {
+  auto oldSIGFPEHandler = signal(SIGFPE, &sigfpeHandler);
+  std::unique_ptr<FunctionCaller> funcUP(func);
+
+  caughtExcept = false;
+  fenv_t oldEnv;
+  fegetenv(&oldEnv);
+  if (sigsetjmp(jumpBuffer, 1) == 0)
+    funcUP->call();
+  // We restore the previous floating point environment after
+  // the call to the function which can potentially raise SIGFPE.
+  fesetenv(&oldEnv);
+  signal(SIGFPE, oldSIGFPEHandler);
+  exceptionRaised = caughtExcept;
+}
+
 } // namespace testing
 } // namespace fputil
 } // namespace __llvm_libc