Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[libc][math] fix loose except check in {EXPECT,ASSERT}_FP_EXCEPTION macros #88816

Merged
merged 6 commits into from
May 6, 2024

Conversation

Flandini
Copy link
Contributor

@Flandini Flandini commented Apr 15, 2024

Adds more FP test macros for the upcoming test adds for #61092 and the issues opened from it: #88768, #88769, #88770, #88771, #88772.

Fix bug in {EXPECT,ASSERT}_FP_EXCEPTION. EXPECT_FP_EXCEPTION(0) seems to be used to test that an exception did not happen, but it always does EXPECT_GE(... & 0, 0) which never fails.

Update and refactor tests that break after the above bug fix. An interesting way things broke after the above change is that ForceRoundingMode and quick_get_round() were raising the inexact exception, breaking a lot of the atan* tests.

The changes for all files other than FPMatcher.h and libc/test/src/math/smoke/RoundToIntegerTest.h should have the same semantics as before. For RoundToIntegerTest.h, lines 56-58 before the changes do not always hold since this test is used for functions with different exception and errno behavior like lrint and lround. I've deleted those lines for now, but tests for those cases should be added for the different nearest int functions to account for this.

Adding @nickdesaulniers for review.

@llvmbot llvmbot added the libc label Apr 15, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Apr 15, 2024

@llvm/pr-subscribers-libc

Author: Michael Flanders (Flandini)

Changes

Adds more FPMatchers for the upcoming test adds for #61092 and the issues opened from it: #88768, #88769, #88770, #88771, #88772.

Fix bug in FPMatcher's {EXPECT,ASSERT}_FP_EXCEPTION. EXPECT_FP_EXCEPTION(0) seems to be used to test that an exception did not happen, but it always does EXPECT_GE(... & 0, 0) which never fails.

Update and refactor tests that break after the above bug fix. An interesting way things broke after the above change is that ForceRoundingMode and quick_get_round() were raising the inexact exception, breaking a lot of the atan* tests.

The changes for all files other than FPMatcher.h and libc/test/src/math/smoke/RoundToIntegerTest.h should have the same semantics as before. For RoundToIntegerTest.h, lines 56-58 before the changes do not always hold since this test is used for functions with different exception and errno behavior like lrint and lround. I've deleted those lines for now, but tests for those cases should be added for the different nearest int functions to account for this.

Adding @nickdesaulniers for review.


Patch is 25.99 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/88816.diff

6 Files Affected:

  • (modified) libc/test/UnitTest/FPMatcher.h (+101-18)
  • (modified) libc/test/src/math/smoke/NextAfterTest.h (+36-59)
  • (modified) libc/test/src/math/smoke/RoundToIntegerTest.h (+2-12)
  • (modified) libc/test/src/math/smoke/atan2f_test.cpp (+17-31)
  • (modified) libc/test/src/math/smoke/atanf_test.cpp (+6-14)
  • (modified) libc/test/src/math/smoke/atanhf_test.cpp (+22-34)
diff --git a/libc/test/UnitTest/FPMatcher.h b/libc/test/UnitTest/FPMatcher.h
index a76e0b8ef6f6f0..aab10d8a540c21 100644
--- a/libc/test/UnitTest/FPMatcher.h
+++ b/libc/test/UnitTest/FPMatcher.h
@@ -159,43 +159,92 @@ template <typename T> struct FPTest : public Test {
 #define EXPECT_FP_EXCEPTION(expected)                                          \
   do {                                                                         \
     if (math_errhandling & MATH_ERREXCEPT) {                                   \
-      EXPECT_GE(LIBC_NAMESPACE::fputil::test_except(FE_ALL_EXCEPT) &           \
-                    (expected),                                                \
-                expected);                                                     \
+      EXPECT_EQ(LIBC_NAMESPACE::fputil::test_except(FE_ALL_EXCEPT) &           \
+                    ((expected) ? (expected) : FE_ALL_EXCEPT),                 \
+                (expected));                                                   \
     }                                                                          \
   } while (0)
 
 #define ASSERT_FP_EXCEPTION(expected)                                          \
   do {                                                                         \
     if (math_errhandling & MATH_ERREXCEPT) {                                   \
-      ASSERT_GE(LIBC_NAMESPACE::fputil::test_except(FE_ALL_EXCEPT) &           \
-                    (expected),                                                \
-                expected);                                                     \
+      ASSERT_EQ(LIBC_NAMESPACE::fputil::test_except(FE_ALL_EXCEPT) &           \
+                    ((expected) ? (expected) : FE_ALL_EXCEPT),                 \
+                (expected));                                                   \
     }                                                                          \
   } while (0)
 
+#define EXPECT_FP_EQ_NO_ERRNO_EXCEPTION(expected_val, actual_val)              \
+  do {                                                                         \
+    LIBC_NAMESPACE::libc_errno = 0;                                            \
+    LIBC_NAMESPACE::fputil::clear_except(FE_ALL_EXCEPT);                       \
+    EXPECT_FP_EQ(expected_val, actual_val);                                    \
+    EXPECT_FP_EXCEPTION(0);                                                    \
+    EXPECT_MATH_ERRNO(0);                                                      \
+  } while (0)
+
+#define ASSERT_FP_EQ_NO_ERRNO_EXCEPTION(expected_val, actual_val)              \
+  do {                                                                         \
+    LIBC_NAMESPACE::libc_errno = 0;                                            \
+    LIBC_NAMESPACE::fputil::clear_except(FE_ALL_EXCEPT);                       \
+    ASSERT_FP_EQ(expected_val, actual_val);                                    \
+    ASSERT_FP_EXCEPTION(0);                                                    \
+    ASSERT_MATH_ERRNO(0);                                                      \
+  } while (0)
+
 #define EXPECT_FP_EQ_WITH_EXCEPTION(expected_val, actual_val, expected_except) \
   do {                                                                         \
     LIBC_NAMESPACE::fputil::clear_except(FE_ALL_EXCEPT);                       \
     EXPECT_FP_EQ(expected_val, actual_val);                                    \
-    if (math_errhandling & MATH_ERREXCEPT) {                                   \
-      EXPECT_GE(LIBC_NAMESPACE::fputil::test_except(FE_ALL_EXCEPT) &           \
-                    (expected_except),                                         \
-                expected_except);                                              \
-      LIBC_NAMESPACE::fputil::clear_except(FE_ALL_EXCEPT);                     \
-    }                                                                          \
+    EXPECT_FP_EXCEPTION(expected_except);                                      \
+  } while (0)
+
+#define ASSERT_FP_EQ_WITH_EXCEPTION(expected_val, actual_val, expected_except) \
+  do {                                                                         \
+    LIBC_NAMESPACE::fputil::clear_except(FE_ALL_EXCEPT);                       \
+    ASSERT_FP_EQ(expected_val, actual_val);                                    \
+    ASSERT_FP_EXCEPTION(expected_except);                                      \
   } while (0)
 
 #define EXPECT_FP_IS_NAN_WITH_EXCEPTION(actual_val, expected_except)           \
   do {                                                                         \
     LIBC_NAMESPACE::fputil::clear_except(FE_ALL_EXCEPT);                       \
     EXPECT_FP_IS_NAN(actual_val);                                              \
-    if (math_errhandling & MATH_ERREXCEPT) {                                   \
-      EXPECT_GE(LIBC_NAMESPACE::fputil::test_except(FE_ALL_EXCEPT) &           \
-                    (expected_except),                                         \
-                expected_except);                                              \
-      LIBC_NAMESPACE::fputil::clear_except(FE_ALL_EXCEPT);                     \
-    }                                                                          \
+    EXPECT_FP_EXCEPTION(expected_except);                                      \
+  } while (0)
+
+#define EXPECT_FP_EQ_WITH_ERRNO(expected_val, actual_val, expected_errno)      \
+  do {                                                                         \
+    LIBC_NAMESPACE::libc_errno = 0;                                            \
+    EXPECT_FP_EQ(expected_val, actual_val);                                    \
+    EXPECT_MATH_ERRNO(expected_errno);                                         \
+  } while (0)
+
+#define ASSERT_FP_EQ_WITH_ERRNO(expected_val, actual_val, expected_errno)      \
+  do {                                                                         \
+    LIBC_NAMESPACE::libc_errno = 0;                                            \
+    ASSERT_FP_EQ(expected_val, actual_val);                                    \
+    ASSERT_MATH_ERRNO(expected_errno);                                         \
+  } while (0)
+
+#define EXPECT_FP_EQ_WITH_ERRNO_EXCEPTION(expected_val, actual_val,            \
+                                          expected_errno, expected_except)     \
+  do {                                                                         \
+    LIBC_NAMESPACE::libc_errno = 0;                                            \
+    LIBC_NAMESPACE::fputil::clear_except(FE_ALL_EXCEPT);                       \
+    EXPECT_FP_EQ(expected_val, actual_val);                                    \
+    EXPECT_MATH_ERRNO(expected_errno);                                         \
+    EXPECT_FP_EXCEPTION(expected_except);                                      \
+  } while (0)
+
+#define ASSERT_FP_EQ_WITH_ERRNO_EXCEPTION(expected_val, actual_val,            \
+                                          expected_errno, expected_except)     \
+  do {                                                                         \
+    LIBC_NAMESPACE::libc_errno = 0;                                            \
+    LIBC_NAMESPACE::fputil::clear_except(FE_ALL_EXCEPT);                       \
+    ASSERT_FP_EQ(expected_val, actual_val);                                    \
+    ASSERT_MATH_ERRNO(expected_errno);                                         \
+    ASSERT_FP_EXCEPTION(expected_except);                                      \
   } while (0)
 
 #define EXPECT_FP_EQ_ALL_ROUNDING(expected, actual)                            \
@@ -219,4 +268,38 @@ template <typename T> struct FPTest : public Test {
     }                                                                          \
   } while (0)
 
+#define EXPECT_FP_EQ_ALL_ROUNDING_NO_ERRNO_EXCEPTION(expected, actual)         \
+  do {                                                                         \
+    using namespace LIBC_NAMESPACE::fputil::testing;                           \
+    ForceRoundingMode __r1(RoundingMode::Nearest);                             \
+    if (__r1.success) {                                                        \
+      EXPECT_FP_EQ_NO_ERRNO_EXCEPTION((expected), (actual));                   \
+    }                                                                          \
+    ForceRoundingMode __r2(RoundingMode::Upward);                              \
+    if (__r2.success) {                                                        \
+      EXPECT_FP_EQ_NO_ERRNO_EXCEPTION((expected), (actual));                   \
+    }                                                                          \
+    ForceRoundingMode __r3(RoundingMode::Downward);                            \
+    if (__r3.success) {                                                        \
+      EXPECT_FP_EQ_NO_ERRNO_EXCEPTION((expected), (actual));                   \
+    }                                                                          \
+    ForceRoundingMode __r4(RoundingMode::TowardZero);                          \
+    if (__r4.success) {                                                        \
+      EXPECT_FP_EQ_NO_ERRNO_EXCEPTION((expected), (actual));                   \
+    }                                                                          \
+  } while (0)
+
+// Instantiations of the above macros to specific cases
+
+#define ASSERT_FP_EQ_WITH_UNDERFLOW(expected_val, actual_val)                  \
+  ASSERT_FP_EQ_WITH_EXCEPTION(expected_val, actual_val,                        \
+                              FE_INEXACT | FE_UNDERFLOW)
+
+#define ASSERT_FP_EQ_WITH_OVERFLOW(expected_val, actual_val)                   \
+  ASSERT_FP_EQ_WITH_EXCEPTION(expected_val, actual_val,                        \
+                              FE_INEXACT | FE_OVERFLOW)
+
+#define ASSERT_FP_EQ_WITH_INEXACT(expected_val, actual_val)                    \
+  ASSERT_FP_EQ_WITH_EXCEPTION(expected_val, actual_val, FE_INEXACT)
+
 #endif // LLVM_LIBC_TEST_UNITTEST_FPMATCHER_H
diff --git a/libc/test/src/math/smoke/NextAfterTest.h b/libc/test/src/math/smoke/NextAfterTest.h
index d9c50c8109d803..37edceffd8182c 100644
--- a/libc/test/src/math/smoke/NextAfterTest.h
+++ b/libc/test/src/math/smoke/NextAfterTest.h
@@ -17,17 +17,6 @@
 #include "test/UnitTest/FPMatcher.h"
 #include "test/UnitTest/Test.h"
 
-#define ASSERT_FP_EQ_WITH_EXCEPTION(result, expected, expected_exception)      \
-  ASSERT_FP_EQ(result, expected);                                              \
-  ASSERT_FP_EXCEPTION(expected_exception);                                     \
-  LIBC_NAMESPACE::fputil::clear_except(FE_ALL_EXCEPT)
-
-#define ASSERT_FP_EQ_WITH_UNDERFLOW(result, expected)                          \
-  ASSERT_FP_EQ_WITH_EXCEPTION(result, expected, FE_INEXACT | FE_UNDERFLOW)
-
-#define ASSERT_FP_EQ_WITH_OVERFLOW(result, expected)                           \
-  ASSERT_FP_EQ_WITH_EXCEPTION(result, expected, FE_INEXACT | FE_OVERFLOW)
-
 template <typename T>
 class NextAfterTestTemplate : public LIBC_NAMESPACE::testing::Test {
   using FPBits = LIBC_NAMESPACE::fputil::FPBits<T>;
@@ -60,139 +49,127 @@ class NextAfterTestTemplate : public LIBC_NAMESPACE::testing::Test {
 
     // 'from' is zero|neg_zero.
     T x = zero;
-    T result = func(x, T(1));
     StorageType expected_bits = 1;
     T expected = LIBC_NAMESPACE::cpp::bit_cast<T>(expected_bits);
-    ASSERT_FP_EQ_WITH_UNDERFLOW(result, expected);
+    ASSERT_FP_EQ_WITH_UNDERFLOW(expected, func(x, T(1)));
 
-    result = func(x, T(-1));
     expected_bits = FPBits::SIGN_MASK + 1;
     expected = LIBC_NAMESPACE::cpp::bit_cast<T>(expected_bits);
-    ASSERT_FP_EQ_WITH_UNDERFLOW(result, expected);
+    ASSERT_FP_EQ_WITH_UNDERFLOW(expected, func(x, T(-1)));
 
     x = neg_zero;
-    result = func(x, 1);
     expected_bits = 1;
     expected = LIBC_NAMESPACE::cpp::bit_cast<T>(expected_bits);
-    ASSERT_FP_EQ_WITH_UNDERFLOW(result, expected);
+    ASSERT_FP_EQ_WITH_UNDERFLOW(expected, func(x, 1));
 
-    result = func(x, -1);
     expected_bits = FPBits::SIGN_MASK + 1;
     expected = LIBC_NAMESPACE::cpp::bit_cast<T>(expected_bits);
-    ASSERT_FP_EQ_WITH_UNDERFLOW(result, expected);
+    ASSERT_FP_EQ_WITH_UNDERFLOW(expected, func(x, -1));
 
     // 'from' is max subnormal value.
     x = LIBC_NAMESPACE::cpp::bit_cast<T>(max_subnormal);
-    result = func(x, 1);
+    T result = func(x, 1);
     expected = LIBC_NAMESPACE::cpp::bit_cast<T>(min_normal);
-    ASSERT_FP_EQ(result, expected);
+    ASSERT_FP_EQ(expected, result);
 
-    result = func(x, 0);
     expected_bits = max_subnormal - 1;
     expected = LIBC_NAMESPACE::cpp::bit_cast<T>(expected_bits);
-    ASSERT_FP_EQ_WITH_UNDERFLOW(result, expected);
+    ASSERT_FP_EQ_WITH_UNDERFLOW(expected, func(x, 0));
 
     x = -x;
 
     result = func(x, -1);
     expected_bits = FPBits::SIGN_MASK + min_normal;
     expected = LIBC_NAMESPACE::cpp::bit_cast<T>(expected_bits);
-    ASSERT_FP_EQ(result, expected);
+    ASSERT_FP_EQ(expected, result);
 
-    result = func(x, 0);
     expected_bits = FPBits::SIGN_MASK + max_subnormal - 1;
     expected = LIBC_NAMESPACE::cpp::bit_cast<T>(expected_bits);
-    ASSERT_FP_EQ_WITH_UNDERFLOW(result, expected);
+    ASSERT_FP_EQ_WITH_UNDERFLOW(expected, func(x, 0));
 
     // 'from' is min subnormal value.
     x = LIBC_NAMESPACE::cpp::bit_cast<T>(min_subnormal);
-    result = func(x, 1);
     expected_bits = min_subnormal + 1;
     expected = LIBC_NAMESPACE::cpp::bit_cast<T>(expected_bits);
-    ASSERT_FP_EQ_WITH_UNDERFLOW(result, expected);
-    ASSERT_FP_EQ_WITH_UNDERFLOW(func(x, 0), 0);
+    ASSERT_FP_EQ_WITH_UNDERFLOW(expected, func(x, 1));
+    ASSERT_FP_EQ_WITH_UNDERFLOW(0.0, func(x, 0));
 
     x = -x;
-    result = func(x, -1);
     expected_bits = FPBits::SIGN_MASK + min_subnormal + 1;
     expected = LIBC_NAMESPACE::cpp::bit_cast<T>(expected_bits);
-    ASSERT_FP_EQ_WITH_UNDERFLOW(result, expected);
-    ASSERT_FP_EQ_WITH_UNDERFLOW(func(x, 0), T(-0.0));
+    ASSERT_FP_EQ_WITH_UNDERFLOW(expected, func(x, -1));
+    ASSERT_FP_EQ_WITH_UNDERFLOW(T(-0.0), func(x, 0));
 
     // 'from' is min normal.
     x = LIBC_NAMESPACE::cpp::bit_cast<T>(min_normal);
-    result = func(x, 0);
     expected_bits = max_subnormal;
     expected = LIBC_NAMESPACE::cpp::bit_cast<T>(expected_bits);
-    ASSERT_FP_EQ_WITH_UNDERFLOW(result, expected);
+    ASSERT_FP_EQ_WITH_UNDERFLOW(expected, func(x, 0));
 
     result = func(x, inf);
     expected_bits = min_normal + 1;
     expected = LIBC_NAMESPACE::cpp::bit_cast<T>(expected_bits);
-    ASSERT_FP_EQ(result, expected);
+    ASSERT_FP_EQ(expected, result);
 
     x = -x;
-    result = func(x, 0);
     expected_bits = FPBits::SIGN_MASK + max_subnormal;
     expected = LIBC_NAMESPACE::cpp::bit_cast<T>(expected_bits);
-    ASSERT_FP_EQ_WITH_UNDERFLOW(result, expected);
+    ASSERT_FP_EQ_WITH_UNDERFLOW(expected, func(x, 0));
 
     result = func(x, -inf);
     expected_bits = FPBits::SIGN_MASK + min_normal + 1;
     expected = LIBC_NAMESPACE::cpp::bit_cast<T>(expected_bits);
-    ASSERT_FP_EQ(result, expected);
+    ASSERT_FP_EQ(expected, result);
 
     // 'from' is max normal and 'to' is infinity.
     x = LIBC_NAMESPACE::cpp::bit_cast<T>(max_normal);
-    result = func(x, inf);
-    ASSERT_FP_EQ_WITH_OVERFLOW(result, inf);
-
-    result = func(-x, -inf);
-    ASSERT_FP_EQ_WITH_OVERFLOW(result, -inf);
+    ASSERT_FP_EQ_WITH_OVERFLOW(inf, func(x, inf));
+    ASSERT_FP_EQ_WITH_OVERFLOW(-inf, func(-x, -inf));
 
     // 'from' is infinity.
     x = inf;
     result = func(x, 0);
     expected_bits = max_normal;
     expected = LIBC_NAMESPACE::cpp::bit_cast<T>(expected_bits);
-    ASSERT_FP_EQ(result, expected);
-    ASSERT_FP_EQ(func(x, inf), inf);
+    ASSERT_FP_EQ(expected, result);
+    ASSERT_FP_EQ(inf, func(x, inf));
 
     x = neg_inf;
     result = func(x, 0);
     expected_bits = FPBits::SIGN_MASK + max_normal;
     expected = LIBC_NAMESPACE::cpp::bit_cast<T>(expected_bits);
-    ASSERT_FP_EQ(result, expected);
-    ASSERT_FP_EQ(func(x, neg_inf), neg_inf);
+    ASSERT_FP_EQ(expected, result);
+    ASSERT_FP_EQ(neg_inf, func(x, neg_inf));
 
     // 'from' is a power of 2.
     x = T(32.0);
     result = func(x, 0);
     FPBits x_bits = FPBits(x);
     FPBits result_bits = FPBits(result);
-    ASSERT_EQ(result_bits.get_biased_exponent(),
-              uint16_t(x_bits.get_biased_exponent() - 1));
-    ASSERT_EQ(result_bits.get_mantissa(), FPBits::FRACTION_MASK);
+    ASSERT_EQ(uint16_t(x_bits.get_biased_exponent() - 1),
+              result_bits.get_biased_exponent());
+    ASSERT_EQ(FPBits::FRACTION_MASK, result_bits.get_mantissa());
 
     result = func(x, T(33.0));
     result_bits = FPBits(result);
-    ASSERT_EQ(result_bits.get_biased_exponent(), x_bits.get_biased_exponent());
-    ASSERT_EQ(result_bits.get_mantissa(),
-              x_bits.get_mantissa() + StorageType(1));
+    ASSERT_EQ(x_bits.get_biased_exponent(), result_bits.get_biased_exponent());
+    ASSERT_EQ(x_bits.get_mantissa() + StorageType(1),
+              result_bits.get_mantissa());
 
     x = -x;
 
     result = func(x, 0);
     result_bits = FPBits(result);
-    ASSERT_EQ(result_bits.get_biased_exponent(),
-              uint16_t(x_bits.get_biased_exponent() - 1));
-    ASSERT_EQ(result_bits.get_mantissa(), FPBits::FRACTION_MASK);
+    ASSERT_EQ(uint16_t(x_bits.get_biased_exponent() - 1),
+              result_bits.get_biased_exponent());
+
+    ASSERT_EQ(FPBits::FRACTION_MASK, result_bits.get_mantissa());
 
     result = func(x, T(-33.0));
     result_bits = FPBits(result);
-    ASSERT_EQ(result_bits.get_biased_exponent(), x_bits.get_biased_exponent());
-    ASSERT_EQ(result_bits.get_mantissa(),
-              x_bits.get_mantissa() + StorageType(1));
+    ASSERT_EQ(x_bits.get_biased_exponent(), result_bits.get_biased_exponent());
+    ASSERT_EQ(x_bits.get_mantissa() + StorageType(1),
+              result_bits.get_mantissa());
   }
 };
 
diff --git a/libc/test/src/math/smoke/RoundToIntegerTest.h b/libc/test/src/math/smoke/RoundToIntegerTest.h
index 863cf75f05ff6b..a738a3f7eb314a 100644
--- a/libc/test/src/math/smoke/RoundToIntegerTest.h
+++ b/libc/test/src/math/smoke/RoundToIntegerTest.h
@@ -26,14 +26,7 @@ class RoundToIntegerTestTemplate : public LIBC_NAMESPACE::testing::Test {
   typedef I (*RoundToIntegerFunc)(F);
 
 private:
-  using FPBits = LIBC_NAMESPACE::fputil::FPBits<F>;
-  using StorageType = typename FPBits::StorageType;
-
-  const F zero = FPBits::zero(Sign::POS).get_val();
-  const F neg_zero = FPBits::zero(Sign::NEG).get_val();
-  const F inf = FPBits::inf(Sign::POS).get_val();
-  const F neg_inf = FPBits::inf(Sign::NEG).get_val();
-  const F nan = FPBits::quiet_nan().get_val();
+  DECLARE_SPECIAL_CONSTANTS(F)
 
   static constexpr StorageType MAX_SUBNORMAL =
       FPBits::max_subnormal().uintval();
@@ -53,9 +46,6 @@ class RoundToIntegerTestTemplate : public LIBC_NAMESPACE::testing::Test {
     if (expectError) {
       ASSERT_FP_EXCEPTION(FE_INVALID);
       ASSERT_MATH_ERRNO(EDOM);
-    } else {
-      ASSERT_FP_EXCEPTION(0);
-      ASSERT_MATH_ERRNO(0);
     }
   }
 
@@ -77,7 +67,7 @@ class RoundToIntegerTestTemplate : public LIBC_NAMESPACE::testing::Test {
     // libc/CMakeLists.txt is not forwarded to C++.
 #if LIBC_COPT_IMPLEMENTATION_DEFINED_TEST_BEHAVIOR
     // Result is not well-defined, we always returns INTEGER_MAX
-    test_one_input(func, nan, INTEGER_MAX, true);
+    test_one_input(func, aNaN, INTEGER_MAX, true);
 #endif // LIBC_COPT_IMPLEMENTATION_DEFINED_TEST_BEHAVIOR
   }
 
diff --git a/libc/test/src/math/smoke/atan2f_test.cpp b/libc/test/src/math/smoke/atan2f_test.cpp
index f81d140fefc5ea..a5310488c144e1 100644
--- a/libc/test/src/math/smoke/atan2f_test.cpp
+++ b/libc/test/src/math/smoke/atan2f_test.cpp
@@ -16,35 +16,21 @@
 using LlvmLibcAtan2fTest = LIBC_NAMESPACE::testing::FPTest<float>;
 
 TEST_F(LlvmLibcAtan2fTest, SpecialNumbers) {
-  LIBC_NAMESPACE::libc_errno = 0;
-
-  LIBC_NAMESPACE::fputil::clear_except(FE_ALL_EXCEPT);
-  EXPECT_FP_EQ_ALL_ROUNDING(aNaN, LIBC_NAMESPACE::atan2f(aNaN, zero));
-  EXPECT_FP_EXCEPTION(0);
-  EXPECT_MATH_ERRNO(0);
-
-  LIBC_NAMESPACE::fputil::clear_except(FE_ALL_EXCEPT);
-  EXPECT_FP_EQ_ALL_ROUNDING(aNaN, LIBC_NAMESPACE::atan2f(1.0f, aNaN));
-  EXPECT_FP_EXCEPTION(0);
-  EXPECT_MATH_ERRNO(0);
-
-  LIBC_NAMESPACE::fputil::clear_except(FE_ALL_EXCEP...
[truncated]

@Flandini Flandini changed the title [libc][math] adds more FPMatchers and fixes bug in {EXPECT,ASSERT}_FP_EXCEPTION [libc][math] adds more FP test macros and fixes bug in {EXPECT,ASSERT}_FP_EXCEPTION Apr 23, 2024
@lntue
Copy link
Contributor

lntue commented Apr 29, 2024

Sorry for the late review! Can you double check if this PR might be related / intersected with #89658? Thanks,

@Flandini
Copy link
Contributor Author

Flandini commented Apr 29, 2024

Sorry for the late review! Can you double check if this PR might be related / intersected with #89658? Thanks,

This PR adds the {EXCEPT,ASSERT}_FP_EXCEPTION* macros for individual invocations of a function call, #89658 adds a wrapper for the TEST_F calls to check that fenv_t state hasn't changed. Noted in #89658, here, we still want both of these, so this PR is still relevant.

Also, this PR starts to address the point in that above comment to start strengthening expectations of more individual function invocations to also check their errno/exceptions, along the lines of #61092 and #88819.

If we move towards better organizing tests around function invocations that should set errno/exceptions, i.e., move them to their own TEST_F, then an approach like #89658 but for checking that fenv_t has changed, would probably be preferable, and this PR could drop some of the macros like EXPECT_FP_EQ_WITH_ERRNO and EXPECT_FP_EQ_WITH_EXCEPTION. Thoughts?

For functions that should not set errno/exceptions, I think we still want the individual ASSERT_FP_EQ_NO_ERRNO_EXCEPTION and EXPECT_FP_EQ_ALL_ROUNDING_NO_ERRNO_EXCEPTION from this PR. There is an intersection noted in #89658, that test fixtures already reset fenv_t on test entry, so do we want all FP tests wrapped in FEnvSafeTest? Actually, I see the with_fenv_preserved and check_fenv_preserved, I think these are nicer than the macro approach.

@Flandini
Copy link
Contributor Author

Flandini commented Apr 29, 2024

Yeah, I think there is a lot of overlap, and the #89658 is much nicer. The changes in this PR to test/src/math/* and the change from ASSERT_GE -> EXPECT_GE and ASSERT_EQ -> EXPECT_EQ are still relevant. @lntue what do you think about the following: I will drop the rest of the macros and leave this PR as changes to {EXPECT,ASSERT}_FP_EXCEPTION, then add another PR to replace the dropped macros with FEnvSafeTest style wrappers. Then, onto addressing #88819 and #88768, #88769, #88770, #88771, #88772.

@lntue
Copy link
Contributor

lntue commented Apr 29, 2024

Yeah, I think there is a lot of overlap, and the #89658 is much nicer. The changes in this PR to test/src/math/* and the change from ASSERT_GE -> EXPECT_GE and ASSERT_EQ -> EXPECT_EQ are still relevant. @lntue what do you think about the following: I will drop the rest of the macros and leave this PR as changes to {EXPECT,ASSERT}_FP_EXCEPTION, then add another PR to replace the dropped macros with FEnvSafeTest style wrappers. Then, onto addressing #88819 and #88768, #88769, #88770, #88771, #88772.

SGTM.

@Flandini
Copy link
Contributor Author

Added issue #90653 to deal with some of these TODO comments.

@Flandini
Copy link
Contributor Author

Ready for review @lntue

@Flandini Flandini changed the title [libc][math] adds more FP test macros and fixes bug in {EXPECT,ASSERT}_FP_EXCEPTION [libc][math] fix loose except check in {EXPECT,ASSERT}_FP_EXCEPTION macros May 2, 2024
@lntue lntue merged commit ecfb5d9 into llvm:main May 6, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants