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] Make fenv and math tests preserve fenv_t state #89658

Merged
merged 4 commits into from
Apr 23, 2024

Conversation

frobtech
Copy link
Contributor

@frobtech frobtech commented Apr 22, 2024

This adds a new test fixture class FEnvSafeTest (usable as a base
class for other fixtures) that ensures each test doesn't perturb
the fenv_t state that the next test will start with. It also
provides types and methods tests can use to explicitly wrap code
under test either to check that it doesn't perturb the state or
to save and restore the state around particular test code.

All the fenv and math tests are updated to use this so that none
can affect another. Expectations that code under test and/or
tests themselves don't perturb state can be added later.

This adds a new test fixture class FEnvSafeTest (usable as a base
class for other fixtures) that ensures each test doesn't perturb
the `fenv_t` state that the next test will start with.  It also
provides types and methods tests can use to explicitly wrap code
under test either to check that it doesn't perturb the state or
to save and restore the state around particular test code.

All the fenv and math tests are updated to use this so that none
can affect another.  Expectations that code under test and/or
tests themselves don't perturb state can be added later.
Copy link

github-actions bot commented Apr 22, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@frobtech frobtech requested a review from Caslyn April 22, 2024 20:10
@frobtech frobtech marked this pull request as ready for review April 22, 2024 20:15
@llvmbot llvmbot added the libc label Apr 22, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Apr 22, 2024

@llvm/pr-subscribers-libc

Author: Roland McGrath (frobtech)

Changes
  • [mlir] fix typo in mem2reg [NFC]
  • [InstCombine] Update BranchProbabilityAnalysis cache result (#86470)
  • [SPIRV][HLSL] Add mad intrinsic lowering for spirv (#89130)
  • [AArch64] Add phase ordering tests for some basic interleaving vectorization patterns. NFC
  • [LV] Fix warning about Mask being set twice. NFC
  • [nfc][github] subscribe myself to MLIR Mem2Reg PRs
  • [clang][Interp] Change array index types in OffsetHelper
  • [clang-repl] Keep the first llvm::Module empty to avoid invalid memory access. (#89031)
  • [memprof] Accept Schema in the constructor of RecordWriterTrait (NFC) (#89486)
  • [Interpreter] Fix warnings
  • Revert "[clang-repl] Keep the first llvm::Module empty to avoid invalid memory access. (#89031)"
  • Reland "[clang-repl] Keep the first llvm::Module empty to avoid invalid memory access. (#89031)"
  • Revert "Reland "[python] Bump Python minimum version to 3.8 (#78828)""
  • [X86] X86LowerTileCopy: Find dead register to use to prevent save-reload of tile register (#83628)
  • [mlir][python] Fix generation of Python bindings for async dialect (#75960)
  • [memprof] Omit the key length for the call stack table (#89510)
  • [RISCV] Remove extra indentation from RISCVProcessors.td.
  • [tidy] update check list [NFC]
  • [InstCombine] Add check to avoid dependent optimization order, NFC
  • [InstCombine] Optimize powi(X, Y)/ (X * Z) with Ofast
  • [clang][Interp][NFC] Change pointer offset to uint64
  • Revert "[X86] X86LowerTileCopy: Find dead register to use to prevent save-reload of tile register (#83628)"
  • [Clang] Do not try to diagnose parameter packs in invalid pack expressions (#89257)
  • [MLIR] Generalize expand_shape to take shape as explicit input (#69267)
  • Revert "[MLIR] Generalize expand_shape to take shape as explicit input" (#89540)
  • [AArch64] Add costs for LD3/LD4 shuffles.
  • [InstCombine] Fix unexpected overwriting in foldSelectWithSRem (#89539)
  • [Transforms] Remove extraneous ArrayRef (NFC) (#89535)
  • [ValueTracking] Combine variable declaration with its only assignment. NFC (#89526)
  • [X86][MC] Error out for CMPCCXADD on 32 bit targets. (#88672)
  • [MLIR][Linalg] Enable fuse consumer (#85528)
  • [LTO] Allow target-specific module splittting (#83128)
  • [clang][dataflow] Model conditional operator correctly. (#89213)
  • Revert "[clang][dataflow] Model conditional operator correctly." (#89577)
  • [InstCombine] Fold fabs over selects (#86390)
  • [llvm-split] Require x86-registered-target for target-specific-split.ll
  • [SelectionDAG] Remove redundant KnownBits smin and smax operations (#89519)
  • [AArch64] Add tests for more undef lanes on zip/uzp. NFC
  • [VectorCombine] Add test coverage for #89390
  • [VectorCombine] foldShuffleOfBinops - don't fold shuffle(divrem(x,y),divrem(z,w)) if mask contains poison
  • [clang][Interp] Create full type info for dummy pointers
  • Reland "[clang] CTAD: Fix require-clause is not transformed." (#89476)
  • Mark mlir::Value::isa/dyn_cast/cast/... member functions deprecated. (#89238)
  • CodeGen: Strengthen definition of F{MIN|MAX}NUM_IEEE nodes (#85195)
  • [llvm][bazel] Fix BUILD after e86ebe4.
  • [llvm-split] Add missing TargetParser dependency
  • Revert "[clang][Interp] Create full type info for dummy pointers"
  • [clang][Interp][NFC] Get ComplexType directly
  • [clang][Interp][NFC] Test out-of-bounds access on vectors
  • [libclc] Fix build with Unix Makefiles (#89147)
  • [clang][Interp] Support ImplicitArrayInitExpr for vectors
  • [flang] de-duplicate AbstractResult pass (#88867)
  • [DAG] visitOR/visitORLike - merge repeated SDLoc calls.
  • clang: Remove unnecessary pointer bitcast
  • [DAG] visitORCommutative - use sd_match to reduce the need for commutative operand matching. NFCI.
  • [AArch64][GlobalISel] Combine Shuffles of G_CONCAT_VECTORS (#87489)
  • Revert "[TBAA] Add verifier for tbaa.struct metadata (#86709)"
  • [flang][driver] Avoid mentions of Clang in Flang's command line reference. (#88932)
  • [flang] Default -g to full debug info. (#89418)
  • AMDGPU: Simplify DS atomicrmw fadd handling (#89468)
  • [Clang] Fix template alias default DWARF version (#89594)
  • Fix DW_TAG_template_alias refs in llvm-dwarfdump --verify (#89589)
  • [llvm-split] Correctly deallocate TargetMachine
  • [KnownBitsTest] Common up isCorrect and isOptimal. NFC. (#89585)
  • [VPlan] Remove custom checks for EVL placement in verifier (NFCI).
  • [KnownBitsTest] Print name of function when exhaustive tests fail (#89588)
  • AMDGPU: Refactor atomicrmw fadd expansion logic (NFC) (#89469)
  • [NFC] Fix comments in PassBuilder functions (#89513)
  • [clang] MveEmitter: Pass Args as a const reference (#89551)
  • [RemoveDIs] Make verify-uselistorder preserve the input debug info format (#87789)
  • [clang][Interp][NFC] getRecord() might return null
  • [clang][Interp] Fix casting pointers to int128
  • [KnownBitsTest] Standardize variable names in exhaustive tests
  • [libc++] Don't commit libcxx.imp (#89391)
  • [libc++] Remove stray CMake install step for modulemap file (#89394)
  • [mlir] fix latex formulas in the tutorial
  • [mlir][Hoisting] Hoisting vector.extract/vector.broadcast pairs (#86108)
  • [X86] Allow input vector extracted from larger vector when combining to VPMADDUBSW (#89584)
  • [AArch64] Add some tests for reassociated addressing modes. NFC
  • [Clang] Fix a crash introduced in PR#88666 (#89567)
  • [LAA] Document reasoning in multiple places in isDependent (NFC). (#89381)
  • [flang][OpenMP] Allow common blocks in nested directives (#88430)
  • [mlir] fix polynomial dialect docs
  • [clang][Interp][NFC] Refactor Program::getGlobal()
  • [clang][Interp] Remove faulty assertion
  • [clang][Interp] Implement C++23 [[assume]] support
  • [SLP]Fix PR89438: check for all tree entries for the resized value.
  • [gn] port e86ebe4 (llvm-split target dep)
  • [libc++][NFC] Fix unparenthesized comma expression in mem-initializer (#89605)
  • [lldb][DWARF] Remove m_forward_decl_die_to_compiler_type as it never actually being used. (#89427)
  • [AST] Dump argument types for TypeTraitExpr. (#89370)
  • [LAA] Add etra tests with strides with different signs.
  • [VectorCombine] foldShuffleOfShuffles - fold "shuffle (shuffle x, undef), (shuffle y, undef)" -> "shuffle x, y" (#88743)
  • [SystemZ][z/OS] Implement llvm.returnaddress for XPLINK (#89440)
  • [SLP][NFC]Add a test with incorrect size of the external user detection.
  • [SLP]Fix a check for multi-users for icmp user.
  • [SLP]Fix PR89614: phis can be reordered, if reuses are not empty.
  • Reapply "[Clang][Sema] Fix crash when 'this' is used in a dependent class scope function template specialization that instantiates to a static member function (#87541, #88311)" (#88731)
  • [OpenACC] Implement 'num_gangs' sema for compute constructs (#89460)
  • [C23] Select the correct promoted type for a bit-field (#89254)
  • [NVPTX][NFCI] Use DataLayout to determine short shared/local/const pointers (#89404)
  • [RISCV] Precommit stack protector checks for Linux and Android (#87679)
  • [Clang][Sema] Remove unused function after #88731 (#89618)
  • [compiler-rt][ctx_instr] Add ctx_profile component (#89304)
  • [SLP]Introduce transformNodes() and transform loads + reverse to strided loads.
  • [X86] gfni-funnel-shifts.ll - add vXi8 variable/splat/constant test coverage
  • Revert "[compiler-rt][ctx_instr] Add ctx_profile component" (#89625)
  • [SPIRV][HLSL] map lerp to Fmix (#88976)
  • [Clang] Fix crash on invalid size in user-defined static_assert message (#89420)
  • [Offload] Move /openmp/libomptarget to /offload (#75125)
  • [RemoveDIs] Preserve debug info format in llvm-reduce (#89220)
  • [flang][cuda] fix parsing of cuda_kernel (#89613)
  • Carving out -Wformat warning about scoped enums into a subwarning (#88595)
  • [SystemZ][NFC] Use new getPointerSize function (#89623)
  • [clangd] Fix unittests in TargetDeclTest bucket (#89630)
  • [Clang] Fix __is_trivially_equaltiy_comparable documentation (#88528)
  • [RISCV] Add extension information to RISCVFeatures.td. NFC (#89326)
  • [clang][CoverageMapping] do not emit a gap region when either end doesn't have valid source locations (#89564)
  • [lldb][MinidumpFileBuilder] Fix addition of MemoryList steam (#88564)
  • [SPIR-V] Emit SPIR-V generator magic number and version (#87951)
  • [flang][OpenMP] Concatenate begin and end clauses into single list (#89090)
  • [clang-tidy] Improved --verify-config when using literal style in config file (#85591)
  • [MIPS]: Rework atomic max/min expand for subword (#89575)
  • [lldb][NFC] Remove unused pexpect/ptyprocess (#89609)
  • [RISCV] Add freeze when expanding mul by constant to two or more uses (#89290)
  • [BitInt] Expose a _BitInt literal suffix in C++ (#86586)
  • [libc] don't over include stdlib in the hdr declaring bsearch (#89471)
  • [llvm] Add support for zero-width integers in MathExtras.h (#87193)
  • [libc][docs] codify Policy on Assembler Sources (#88185)
  • [libc][POSIX][pthreads] implement pthread_rwlockattr_t functions (#89322)
  • [RISCV][clang] Don't enable -mrelax-all for -O0 on RISC-V (#88538)
  • [Offload] Fix per-target install directory (#89645)
  • [llvm-readobj] Remove --raw-relr
  • [libc] Make fenv and math tests preserve fenv_t state

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

81 Files Affected:

  • (modified) libc/test/UnitTest/CMakeLists.txt (+2)
  • (added) libc/test/UnitTest/FEnvSafeTest.cpp (+84)
  • (added) libc/test/UnitTest/FEnvSafeTest.h (+101)
  • (modified) libc/test/src/fenv/CMakeLists.txt (+16)
  • (modified) libc/test/src/fenv/enabled_exceptions_test.cpp (+7-11)
  • (modified) libc/test/src/fenv/exception_flags_test.cpp (+7-5)
  • (modified) libc/test/src/fenv/exception_status_test.cpp (+21-22)
  • (added) libc/test/src/fenv/excepts.h (+24)
  • (modified) libc/test/src/fenv/feclearexcept_test.cpp (+12-9)
  • (modified) libc/test/src/fenv/feenableexcept_test.cpp (+6-1)
  • (modified) libc/test/src/fenv/feholdexcept_test.cpp (+6-1)
  • (modified) libc/test/src/fenv/feupdateenv_test.cpp (+3-2)
  • (modified) libc/test/src/fenv/getenv_and_setenv_test.cpp (+8-6)
  • (modified) libc/test/src/fenv/rounding_mode_test.cpp (+6-3)
  • (modified) libc/test/src/math/CeilTest.h (+3-1)
  • (modified) libc/test/src/math/CopySignTest.h (+2-1)
  • (modified) libc/test/src/math/FAbsTest.h (+3-1)
  • (modified) libc/test/src/math/FDimTest.h (+2-1)
  • (modified) libc/test/src/math/FMaxTest.h (+3-1)
  • (modified) libc/test/src/math/FMinTest.h (+3-1)
  • (modified) libc/test/src/math/FModTest.h (+3-1)
  • (modified) libc/test/src/math/FloorTest.h (+3-1)
  • (modified) libc/test/src/math/FmaTest.h (+2-1)
  • (modified) libc/test/src/math/FrexpTest.h (+3-1)
  • (modified) libc/test/src/math/HypotTest.h (+2-1)
  • (modified) libc/test/src/math/ILogbTest.h (+2-1)
  • (modified) libc/test/src/math/LdExpTest.h (+2-1)
  • (modified) libc/test/src/math/LogbTest.h (+3-1)
  • (modified) libc/test/src/math/ModfTest.h (+3-1)
  • (modified) libc/test/src/math/NextAfterTest.h (+2-1)
  • (modified) libc/test/src/math/RIntTest.h (+2-1)
  • (modified) libc/test/src/math/RemQuoTest.h (+2-1)
  • (modified) libc/test/src/math/RoundEvenTest.h (+2-1)
  • (modified) libc/test/src/math/RoundTest.h (+3-1)
  • (modified) libc/test/src/math/RoundToIntegerTest.h (+5-1)
  • (modified) libc/test/src/math/SqrtTest.h (+3-1)
  • (modified) libc/test/src/math/TruncTest.h (+3-1)
  • (modified) libc/test/src/math/exhaustive/fmod_generic_impl_test.cpp (+2-1)
  • (modified) libc/test/src/math/smoke/CanonicalizeTest.h (+2-1)
  • (modified) libc/test/src/math/smoke/CeilTest.h (+3-1)
  • (modified) libc/test/src/math/smoke/CopySignTest.h (+2-1)
  • (modified) libc/test/src/math/smoke/FAbsTest.h (+3-1)
  • (modified) libc/test/src/math/smoke/FDimTest.h (+2-1)
  • (modified) libc/test/src/math/smoke/FMaxTest.h (+3-1)
  • (modified) libc/test/src/math/smoke/FMaximumMagNumTest.h (+2-1)
  • (modified) libc/test/src/math/smoke/FMaximumMagTest.h (+2-1)
  • (modified) libc/test/src/math/smoke/FMaximumNumTest.h (+2-1)
  • (modified) libc/test/src/math/smoke/FMaximumTest.h (+2-1)
  • (modified) libc/test/src/math/smoke/FMinTest.h (+3-1)
  • (modified) libc/test/src/math/smoke/FMinimumMagNumTest.h (+2-1)
  • (modified) libc/test/src/math/smoke/FMinimumMagTest.h (+2-1)
  • (modified) libc/test/src/math/smoke/FMinimumNumTest.h (+2-1)
  • (modified) libc/test/src/math/smoke/FMinimumTest.h (+2-1)
  • (modified) libc/test/src/math/smoke/FModTest.h (+3-1)
  • (modified) libc/test/src/math/smoke/FloorTest.h (+3-1)
  • (modified) libc/test/src/math/smoke/FmaTest.h (+2-1)
  • (modified) libc/test/src/math/smoke/FrexpTest.h (+3-1)
  • (modified) libc/test/src/math/smoke/FromfpTest.h (+2-1)
  • (modified) libc/test/src/math/smoke/FromfpxTest.h (+2-1)
  • (modified) libc/test/src/math/smoke/HypotTest.h (+2-1)
  • (modified) libc/test/src/math/smoke/ILogbTest.h (+2-1)
  • (modified) libc/test/src/math/smoke/LdExpTest.h (+2-1)
  • (modified) libc/test/src/math/smoke/LogbTest.h (+3-1)
  • (modified) libc/test/src/math/smoke/ModfTest.h (+3-1)
  • (modified) libc/test/src/math/smoke/NextAfterTest.h (+2-1)
  • (modified) libc/test/src/math/smoke/NextDownTest.h (+2-1)
  • (modified) libc/test/src/math/smoke/NextTowardTest.h (+2-1)
  • (modified) libc/test/src/math/smoke/NextUpTest.h (+2-1)
  • (modified) libc/test/src/math/smoke/RIntTest.h (+2-1)
  • (modified) libc/test/src/math/smoke/RemQuoTest.h (+2-1)
  • (modified) libc/test/src/math/smoke/RoundEvenTest.h (+2-1)
  • (modified) libc/test/src/math/smoke/RoundTest.h (+3-1)
  • (modified) libc/test/src/math/smoke/RoundToIntegerTest.h (+5-1)
  • (modified) libc/test/src/math/smoke/SqrtTest.h (+3-1)
  • (modified) libc/test/src/math/smoke/TruncTest.h (+3-1)
  • (modified) libc/test/src/math/smoke/UfromfpTest.h (+2-1)
  • (modified) libc/test/src/math/smoke/UfromfpxTest.h (+2-1)
  • (modified) libc/test/src/math/smoke/nan_test.cpp (+2-1)
  • (modified) libc/test/src/math/smoke/nanf128_test.cpp (+2-1)
  • (modified) libc/test/src/math/smoke/nanf_test.cpp (+2-1)
  • (modified) libc/test/src/math/smoke/nanl_test.cpp (+2-1)
diff --git a/libc/test/UnitTest/CMakeLists.txt b/libc/test/UnitTest/CMakeLists.txt
index 9113eca388e05b..302af3044ca3d6 100644
--- a/libc/test/UnitTest/CMakeLists.txt
+++ b/libc/test/UnitTest/CMakeLists.txt
@@ -111,8 +111,10 @@ add_header_library(
 add_unittest_framework_library(
   LibcFPTestHelpers
   SRCS
+    FEnvSafeTest.cpp
     RoundingModeUtils.cpp
   HDRS
+    FEnvSafeTest.h
     FPMatcher.h
     RoundingModeUtils.h
   DEPENDS
diff --git a/libc/test/UnitTest/FEnvSafeTest.cpp b/libc/test/UnitTest/FEnvSafeTest.cpp
new file mode 100644
index 00000000000000..43aebc3f36e7b3
--- /dev/null
+++ b/libc/test/UnitTest/FEnvSafeTest.cpp
@@ -0,0 +1,84 @@
+//===-- FEnvSafeTest.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 "FEnvSafeTest.h"
+
+#include "src/__support/FPUtil/FEnvImpl.h"
+#include "src/__support/macros/properties/architectures.h"
+
+namespace LIBC_NAMESPACE::testing {
+
+void FEnvSafeTest::PreserveFEnv::check() {
+  fenv_t after;
+  test.get_fenv(after);
+  test.expect_fenv_eq(before, after);
+}
+
+void FEnvSafeTest::TearDown() {
+  if (!should_be_unchanged) {
+    restore_fenv();
+  }
+}
+
+void FEnvSafeTest::get_fenv(fenv_t &fenv) {
+  ASSERT_EQ(LIBC_NAMESPACE::fputil::get_env(&fenv), 0);
+}
+
+void FEnvSafeTest::set_fenv(const fenv_t &fenv) {
+  ASSERT_EQ(LIBC_NAMESPACE::fputil::set_env(&fenv), 0);
+}
+
+void FEnvSafeTest::expect_fenv_eq(const fenv_t &before_fenv,
+                                  const fenv_t &after_fenv) {
+#if defined(LIBC_TARGET_ARCH_IS_AARCH64)
+  using LIBC_NAMESPACE::fputil::FEnv::FPState;
+  const FPState &before_state = reinterpret_cast<const FPState &>(before_fenv);
+  const FPState &after_state = reinterpret_cast<const FPState &>(after_fenv);
+
+  EXPECT_EQ(before_state.ControlWord, after_state.ControlWord);
+  EXPECT_EQ(before_state.StatusWord, after_state.StatusWord);
+
+#elif defined(LIBC_TARGET_ARCH_IS_X86) && !defined(__APPLE__)
+  using LIBC_NAMESPACE::fputil::internal::FPState;
+  const FPState &before_state = reinterpret_cast<const FPState &>(before_fenv);
+  const FPState &after_state = reinterpret_cast<const FPState &>(after_fenv);
+
+#if defined(_WIN32)
+  EXPECT_EQ(before_state.control_word, after_state.control_word);
+  EXPECT_EQ(before_state.status_word, after_state.status_word);
+#elif defined(__APPLE__)
+  EXPECT_EQ(before_state.control_word, after_state.control_word);
+  EXPECT_EQ(before_state.status_word, after_state.status_word);
+  EXPECT_EQ(before_state.mxcsr, after_state.mxcsr);
+#else
+  EXPECT_EQ(before_state.x87_status.control_word,
+            after_state.x87_status.control_word);
+  EXPECT_EQ(before_state.x87_status.status_word,
+            after_state.x87_status.status_word);
+  EXPECT_EQ(before_state.mxcsr, after_state.mxcsr);
+#endif
+
+#elif defined(LIBC_TARGET_ARCH_IS_ARM) && defined(__ARM_FP)
+  using LIBC_NAMESPACE::fputil::FEnv;
+  const FEnv &before_state = reinterpret_cast<const FEnv &>(before_fenv);
+  const FEnv &after_state = reinterpret_cast<const FEnv &>(after_fenv);
+
+  EXPECT_EQ(before_state.fpscr, after_state.fpscr);
+
+#elif defined(LIBC_TARGET_ARCH_IS_ANY_RISCV)
+  const uint32_t &before_fcsr = reinterpret_cast<const uint32_t &>(before_fenv);
+  const uint32_t &after_fcsr = reinterpret_cast<const uint32_t &>(after_fenv);
+  EXPECT_EQ(before_fcsr, after_fcsr);
+
+#else
+  // No arch-specific `fenv_t` support, so nothing to compare.
+
+#endif
+}
+
+} // namespace LIBC_NAMESPACE::testing
diff --git a/libc/test/UnitTest/FEnvSafeTest.h b/libc/test/UnitTest/FEnvSafeTest.h
new file mode 100644
index 00000000000000..d5a8bb7ee667ce
--- /dev/null
+++ b/libc/test/UnitTest/FEnvSafeTest.h
@@ -0,0 +1,101 @@
+//===-- FEnvSafeTest.h -----------------------------------------*- 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
+//
+//===---------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_TEST_UNITTEST_FPENVSAFE_H
+#define LLVM_LIBC_TEST_UNITTEST_FPENVSAFE_H
+
+#include "hdr/types/fenv_t.h"
+#include "src/__support/CPP/utility.h"
+#include "test/UnitTest/Test.h"
+
+namespace LIBC_NAMESPACE::testing {
+
+// This provides a test fixture (or base class for other test fixtures) that
+// asserts that each test does not leave the FPU state represented by `fenv_t`
+// (aka `FPState`) perturbed from its initial state.
+class FEnvSafeTest : public Test {
+public:
+  void TearDown() override;
+
+protected:
+  // This is an RAII type where `PreserveFEnv preserve{this};` will sample the
+  // `fenv_t` state and restore it when `preserve` goes out of scope.
+  class PreserveFEnv {
+    fenv_t before;
+    FEnvSafeTest &test;
+
+  public:
+    explicit PreserveFEnv(FEnvSafeTest *self) : test{*self} {
+      test.get_fenv(before);
+    }
+
+    // Cause test expectation failures if the current state doesn't match what
+    // was captured in the constructor.
+    void check();
+
+    // Restore the state captured in the constructor.
+    void restore() { test.set_fenv(before); }
+
+    ~PreserveFEnv() { restore(); }
+  };
+
+  // This is an RAII type where `CheckFEnv check{this};` will sample the
+  // `fenv_t` state and require it be the same when `check` goes out of scope.
+  struct CheckFEnv : public PreserveFEnv {
+    using PreserveFEnv::PreserveFEnv;
+
+    ~CheckFEnv() { check(); }
+  };
+
+  // This calls callable() and returns its value, but has EXPECT_* failures if
+  // the `fenv_t` state is not preserved by the call.
+  template <typename T> decltype(auto) check_fenv_preserved(T &&callable) {
+    CheckFEnv check{this};
+    return cpp::forward<T>(callable)();
+  }
+
+  // This calls callable() and returns its value, but saves and restores the
+  // `fenv_t` state around the call.
+  template <typename T>
+  auto with_fenv_preserved(T &&callable)
+      -> decltype(cpp::forward<decltype(callable)>(callable)()) {
+    PreserveFEnv preserve{this};
+    return cpp::forward<T>(callable)();
+  }
+
+  // A test can call these to indicate it will or won't change `fenv_t` state.
+  void will_change_fenv() { should_be_unchanged = false; }
+  void will_not_change_fenv() { should_be_unchanged = true; }
+
+  // This explicitly resets back to the "before" state captured in SetUp().
+  // TearDown() always does this, but should_be_unchanged controls whether
+  // it also causes test failures if a test fails to restore it.
+  void restore_fenv() { check.restore(); }
+
+private:
+  void get_fenv(fenv_t &fenv);
+  void set_fenv(const fenv_t &fenv);
+  void expect_fenv_eq(const fenv_t &before_fenv, const fenv_t &after_fenv);
+
+  CheckFEnv check{this};
+
+  // TODO: Many tests fail if this is true. It needs to be figured out whether
+  // the state should be preserved by each library function under test, and
+  // separately whether each test itself should preserve the state.  It
+  // probably isn't important that tests be explicitly written to preserve the
+  // state, as the fixture can (and does) reset it--the next test can rely on
+  // getting "normal" ambient state initially.  For library functions that
+  // should preserve the state, that should be checked after each call, not
+  // just after the whole test.  So they can use check_fenv_preserved or
+  // with_fenv_preserved as appropriate.
+  bool should_be_unchanged = false;
+};
+
+} // namespace LIBC_NAMESPACE::testing
+
+#endif // LLVM_LIBC_TEST_UNITTEST_FPENVSAFE_H
diff --git a/libc/test/src/fenv/CMakeLists.txt b/libc/test/src/fenv/CMakeLists.txt
index f277b65e2d42be..b776f9a0706e86 100644
--- a/libc/test/src/fenv/CMakeLists.txt
+++ b/libc/test/src/fenv/CMakeLists.txt
@@ -9,6 +9,8 @@ add_libc_unittest(
   DEPENDS
     libc.src.fenv.fegetround
     libc.src.fenv.fesetround
+  LINK_LIBRARIES
+    LibcFPTestHelpers
 )
 
 add_libc_unittest(
@@ -23,6 +25,8 @@ add_libc_unittest(
     libc.src.fenv.fesetexcept
     libc.src.fenv.fetestexcept
     libc.src.__support.FPUtil.fenv_impl
+  LINK_LIBRARIES
+    LibcFPTestHelpers
 )
 
 add_libc_unittest(
@@ -37,6 +41,8 @@ add_libc_unittest(
     libc.src.fenv.fesetenv
     libc.src.fenv.fesetround
     libc.src.__support.FPUtil.fenv_impl
+  LINK_LIBRARIES
+    LibcFPTestHelpers
 )
 
 add_libc_unittest(
@@ -50,6 +56,8 @@ add_libc_unittest(
     libc.src.fenv.fesetexceptflag
     libc.src.fenv.fetestexceptflag
     libc.src.__support.FPUtil.fenv_impl
+  LINK_LIBRARIES
+    LibcFPTestHelpers
 )
 
 add_libc_unittest(
@@ -62,6 +70,8 @@ add_libc_unittest(
     libc.include.signal
     libc.src.fenv.feupdateenv
     libc.src.__support.FPUtil.fenv_impl
+  LINK_LIBRARIES
+    LibcFPTestHelpers
 )
 
 add_libc_unittest(
@@ -73,6 +83,8 @@ add_libc_unittest(
   DEPENDS
     libc.src.fenv.feclearexcept
     libc.src.__support.FPUtil.fenv_impl
+  LINK_LIBRARIES
+    LibcFPTestHelpers
 )
 
 add_libc_unittest(
@@ -85,6 +97,8 @@ add_libc_unittest(
     libc.src.fenv.fedisableexcept
     libc.src.fenv.feenableexcept
     libc.src.fenv.fegetexcept
+  LINK_LIBRARIES
+    LibcFPTestHelpers
 )
 
 if (NOT (LLVM_USE_SANITIZER OR (${LIBC_TARGET_OS} STREQUAL "windows")
@@ -109,6 +123,7 @@ if (NOT (LLVM_USE_SANITIZER OR (${LIBC_TARGET_OS} STREQUAL "windows")
       libc.src.__support.FPUtil.fenv_impl
     LINK_LIBRARIES
       LibcFPExceptionHelpers
+      LibcFPTestHelpers
   )
 
   add_fp_unittest(
@@ -124,5 +139,6 @@ if (NOT (LLVM_USE_SANITIZER OR (${LIBC_TARGET_OS} STREQUAL "windows")
       libc.src.__support.FPUtil.fenv_impl
     LINK_LIBRARIES
       LibcFPExceptionHelpers
+      LibcFPTestHelpers
   )
 endif()
diff --git a/libc/test/src/fenv/enabled_exceptions_test.cpp b/libc/test/src/fenv/enabled_exceptions_test.cpp
index 53440b704ca761..7d26eab5695bce 100644
--- a/libc/test/src/fenv/enabled_exceptions_test.cpp
+++ b/libc/test/src/fenv/enabled_exceptions_test.cpp
@@ -12,15 +12,20 @@
 
 #include "src/__support/FPUtil/FEnvImpl.h"
 #include "src/__support/macros/properties/architectures.h"
+#include "test/UnitTest/FEnvSafeTest.h"
 #include "test/UnitTest/FPExceptMatcher.h"
 #include "test/UnitTest/Test.h"
 
 #include "hdr/fenv_macros.h"
 #include <signal.h>
 
+#include "excepts.h"
+
+using LlvmLibcExceptionStatusTest = LIBC_NAMESPACE::testing::FEnvSafeTest;
+
 // This test enables an exception and verifies that raising that exception
 // triggers SIGFPE.
-TEST(LlvmLibcExceptionStatusTest, RaiseAndCrash) {
+TEST_F(LlvmLibcExceptionStatusTest, RaiseAndCrash) {
 #if defined(LIBC_TARGET_ARCH_IS_ANY_ARM) ||                                    \
     defined(LIBC_TARGET_ARCH_IS_ANY_RISCV)
   // Few Arm HW implementations do not trap exceptions. We skip this test
@@ -41,16 +46,7 @@ TEST(LlvmLibcExceptionStatusTest, RaiseAndCrash) {
   // 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 ALL_EXCEPTS =
-      FE_DIVBYZERO | FE_INVALID | FE_INEXACT | FE_OVERFLOW | FE_UNDERFLOW;
-
-  for (int e : excepts) {
+  for (int e : EXCEPTS) {
     LIBC_NAMESPACE::fputil::disable_except(FE_ALL_EXCEPT);
     LIBC_NAMESPACE::fputil::enable_except(e);
     ASSERT_EQ(LIBC_NAMESPACE::feclearexcept(FE_ALL_EXCEPT), 0);
diff --git a/libc/test/src/fenv/exception_flags_test.cpp b/libc/test/src/fenv/exception_flags_test.cpp
index 9d2be6426a6d0b..2f4332df861fec 100644
--- a/libc/test/src/fenv/exception_flags_test.cpp
+++ b/libc/test/src/fenv/exception_flags_test.cpp
@@ -12,18 +12,20 @@
 #include "src/fenv/fetestexceptflag.h"
 
 #include "src/__support/FPUtil/FEnvImpl.h"
+#include "test/UnitTest/FEnvSafeTest.h"
 #include "test/UnitTest/Test.h"
 
-TEST(LlvmLibcFenvTest, GetSetTestExceptFlag) {
+#include "excepts.h"
+
+using LlvmLibcFEnvTest = LIBC_NAMESPACE::testing::FEnvSafeTest;
+
+TEST_F(LlvmLibcFEnvTest, GetSetTestExceptFlag) {
   // We will disable all exceptions to prevent invocation of the exception
   // handler.
   LIBC_NAMESPACE::fputil::disable_except(FE_ALL_EXCEPT);
   LIBC_NAMESPACE::fputil::clear_except(FE_ALL_EXCEPT);
 
-  int excepts[] = {FE_DIVBYZERO, FE_INVALID, FE_INEXACT, FE_OVERFLOW,
-                   FE_UNDERFLOW};
-
-  for (int e : excepts) {
+  for (int e : EXCEPTS) {
     // The overall idea is to raise an except and save the exception flags.
     // Next, clear the flags and then set the saved exception flags. This
     // should set the flag corresponding to the previously raised exception.
diff --git a/libc/test/src/fenv/exception_status_test.cpp b/libc/test/src/fenv/exception_status_test.cpp
index a7000020b1a3c8..fdf9421457866e 100644
--- a/libc/test/src/fenv/exception_status_test.cpp
+++ b/libc/test/src/fenv/exception_status_test.cpp
@@ -13,24 +13,23 @@
 #include "src/fenv/fetestexcept.h"
 
 #include "src/__support/FPUtil/FEnvImpl.h"
+#include "test/UnitTest/FEnvSafeTest.h"
 #include "test/UnitTest/Test.h"
 
 #include "hdr/fenv_macros.h"
 
-TEST(LlvmLibcExceptionStatusTest, RaiseAndTest) {
+#include "excepts.h"
+
+using LlvmLibcExceptionStatusTest = LIBC_NAMESPACE::testing::FEnvSafeTest;
+
+TEST_F(LlvmLibcExceptionStatusTest, RaiseAndTest) {
   // This test raises a set of exceptions and checks that the exception
   // status flags are updated. The intention is really not to invoke the
   // exception handler. Hence, we will disable all exceptions at the
   // beginning.
   LIBC_NAMESPACE::fputil::disable_except(FE_ALL_EXCEPT);
 
-  int excepts[] = {FE_DIVBYZERO, FE_INVALID, FE_INEXACT, FE_OVERFLOW,
-                   FE_UNDERFLOW};
-
-  constexpr int ALL_EXCEPTS =
-      FE_DIVBYZERO | FE_INVALID | FE_INEXACT | FE_OVERFLOW | FE_UNDERFLOW;
-
-  for (int e : excepts) {
+  for (int e : EXCEPTS) {
     int r = LIBC_NAMESPACE::feraiseexcept(e);
     ASSERT_EQ(r, 0);
     int s = LIBC_NAMESPACE::fetestexcept(e);
@@ -47,8 +46,8 @@ TEST(LlvmLibcExceptionStatusTest, RaiseAndTest) {
     ASSERT_EQ(s, e);
   }
 
-  for (int e1 : excepts) {
-    for (int e2 : excepts) {
+  for (int e1 : EXCEPTS) {
+    for (int e2 : EXCEPTS) {
       int e = e1 | e2;
       int r = LIBC_NAMESPACE::feraiseexcept(e);
       ASSERT_EQ(r, 0);
@@ -67,9 +66,9 @@ TEST(LlvmLibcExceptionStatusTest, RaiseAndTest) {
     }
   }
 
-  for (int e1 : excepts) {
-    for (int e2 : excepts) {
-      for (int e3 : excepts) {
+  for (int e1 : EXCEPTS) {
+    for (int e2 : EXCEPTS) {
+      for (int e3 : EXCEPTS) {
         int e = e1 | e2 | e3;
         int r = LIBC_NAMESPACE::feraiseexcept(e);
         ASSERT_EQ(r, 0);
@@ -89,10 +88,10 @@ TEST(LlvmLibcExceptionStatusTest, RaiseAndTest) {
     }
   }
 
-  for (int e1 : excepts) {
-    for (int e2 : excepts) {
-      for (int e3 : excepts) {
-        for (int e4 : excepts) {
+  for (int e1 : EXCEPTS) {
+    for (int e2 : EXCEPTS) {
+      for (int e3 : EXCEPTS) {
+        for (int e4 : EXCEPTS) {
           int e = e1 | e2 | e3 | e4;
           int r = LIBC_NAMESPACE::feraiseexcept(e);
           ASSERT_EQ(r, 0);
@@ -113,11 +112,11 @@ TEST(LlvmLibcExceptionStatusTest, RaiseAndTest) {
     }
   }
 
-  for (int e1 : excepts) {
-    for (int e2 : excepts) {
-      for (int e3 : excepts) {
-        for (int e4 : excepts) {
-          for (int e5 : excepts) {
+  for (int e1 : EXCEPTS) {
+    for (int e2 : EXCEPTS) {
+      for (int e3 : EXCEPTS) {
+        for (int e4 : EXCEPTS) {
+          for (int e5 : EXCEPTS) {
             int e = e1 | e2 | e3 | e4 | e5;
             int r = LIBC_NAMESPACE::feraiseexcept(e);
             ASSERT_EQ(r, 0);
diff --git a/libc/test/src/fenv/excepts.h b/libc/test/src/fenv/excepts.h
new file mode 100644
index 00000000000000..e9517d319a9b79
--- /dev/null
+++ b/libc/test/src/fenv/excepts.h
@@ -0,0 +1,24 @@
+//===-- List of all FE_* constants for tests -----------------------------===//
+//
+// 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
+//
+//===---------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_TEST_SRC_FENV_EXCEPTS_H
+#define LLVM_LIBC_TEST_SRC_FENV_EXCEPTS_H
+
+#include "hdr/fenv_macros.h"
+
+constexpr 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 ALL_EXCEPTS =
+    FE_DIVBYZERO | FE_INVALID | FE_INEXACT | FE_OVERFLOW | FE_UNDERFLOW;
+
+#endif // LLVM_LIBC_TEST_SRC_FENV_EXCEPTS_H
diff --git a/libc/test/src/fenv/feclearexcept_test.cpp b/libc/test/src/fenv/feclearexcept_test.cpp
index bb42d9070358ef..52adda46adf2f0 100644
--- a/libc/test/src/fenv/feclearexcept_test.cpp
+++ b/libc/test/src/fenv/feclearexcept_test.cpp
@@ -9,27 +9,30 @@
 #include "src/fenv/feclearexcept.h"
 
 #include "src/__support/FPUtil/FEnvImpl.h"
+#include "test/UnitTest/FEnvSafeTest.h"
 #include "test/UnitTest/Test.h"
 
 #include "hdr/fenv_macros.h"
 #include <stdint.h>
 
-TEST(LlvmLibcFEnvTest, ClearTest) {
-  uint16_t excepts[] = {FE_DIVBYZERO, FE_INVALID, FE_INEXACT, FE_OVERFLOW,
-                        FE_UNDERFLOW};
+#include "excepts.h"
+
+using LlvmLibcFEnvTest = LIBC_NAMESPACE::testing::FEnvSafeTest;
+
+TEST_F(LlvmLibcFEnvTest, ClearTest) {
   LIBC_NAMESPACE::fputil::disable_except(FE_ALL_EXCEPT);
   LIBC_NAMESPACE::fputil::clear_except(FE_ALL_EXCEPT);
 
-  for (uint16_t e : excepts)
+  for (int e : EXCEPTS)
     ASSERT_EQ(LIBC_NAMESPACE::fputil::test_except(e), 0);
 
   LIBC_NAMESPACE::fputil::raise_except(FE_ALL_EXCEPT);
 
-  for (uint16_t e1 : excepts) {
-    for (uint16_t e2 : excepts) {
-      for (uint16_t e3 : excepts) {
-        for (uint16_t e4 : excepts) {
-          for (uint16_t e5 : excepts) {
+  for (int e1 : EXCEPTS) {
+    for (int e2 : EXCEPTS) {
+      for (int e3 : EXCEPTS) {
+        for (int e4 : EXCEPTS) {
+          for (int e5 : EXCEPTS) {
             // We clear one exception and test to verify that it was cleared.
             LIBC_NAMESPACE::feclearexcept(e1 | e2 | e3 | e4 | e5);
             ASSERT_EQ(
diff --git a/libc/test/src/fenv/feenableexcept_test.cpp b/libc/test/src/fenv/feenableexcept_test.cpp
index aeb4f955fd69b6..232e2a1c8316c6 100644
--- a/libc/test/src/fenv/feenableexcept_test.cpp
+++ b/libc/test/src/fenv/feenableexcept_test.cpp
@@ -11,11 +11,16 @@
 #include "src/fenv/feenableexcept.h"
 #include "src/fenv/fegetexcept.h"
 
+#include "test/UnitTest/FEnvSafeTest.h"
 #include "test/UnitTest/Test.h"
 
 #include "hdr/fenv_macros.h"
 
-TEST(LlvmLibcFEnvTest, EnableTest) {
+#include "excepts.h"
+
+using LlvmLibcFEnvTest = LIBC_NAMESPACE::testing::FEnvSafeTest;
+
+TEST_F(LlvmLibcFEnvTest, EnableTest) {
 #if defined(LIBC_TARGET_ARCH_IS_ANY_ARM) ||                                    \
     defined(LIBC_TARGET_ARCH_IS_ANY_RISCV)
   // Few Arm HW implementations do not trap exceptions. We skip this test
diff --git a/libc/test/src/fenv/feholdexcept_test.cpp b/libc/test/src/fenv/feholdexcept_test.cpp
index 0689d89ab233a3..f3e05d4a5b6c0d 100644
--- a/libc/test/src/fenv/feholdexcept_test.cpp
+++ b/libc/test/src/fenv/feholdexcept_test.cpp
@@ -11,10 +11,15 @@
 
 #include "src/__support/FPUtil/FEnvImpl.h"
 #include "src/__support/macros/properties/architectures.h"
+#include "test/UnitTest/FEnvSafeTest.h"
 #include "test/UnitTest/FPExceptMatcher.h"
 #include "test/UnitTest/Test.h"
 
-TEST(LlvmLibcFEnvTest, RaiseAndCrash) {
+#include "excepts.h"
+
+using LlvmLibcFEnvTest = LIBC_NAMESPACE::testing::FEnvSafeTest;
+
+TEST_F(LlvmLibcFEnvTest, RaiseAndCrash) {
 #if defined(LIBC_TARGET_ARCH_IS_ANY_ARM) ||                                    \
     defined(LIBC_TARGET_ARCH_IS_ANY_RISCV)
   // Few Arm HW implementations do not trap exceptions. We skip this test
diff --gi...
[truncated]

@nickdesaulniers
Copy link
Member

Mind updating the PR description? Not sure what happened there.

@frobtech frobtech changed the title p/libc fenv safe test [libc] Make fenv and math tests preserve fenv_t state Apr 22, 2024
Copy link
Contributor

@michaelrj-google michaelrj-google left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

libc/test/UnitTest/CMakeLists.txt Show resolved Hide resolved
libc/test/src/fenv/excepts.h Show resolved Hide resolved
@llvmbot llvmbot added the bazel "Peripheral" support tier build system: utils/bazel label Apr 23, 2024
@frobtech frobtech merged commit 837dab9 into llvm:main Apr 23, 2024
3 of 4 checks passed
michaelrj-google added a commit to michaelrj-google/llvm-project that referenced this pull request Apr 23, 2024
The PR llvm#89658 updated up the fenv tests work, this patch updates the
bazel to match.
Flandini added a commit to Flandini/llvm-project that referenced this pull request Apr 30, 2024
…in atan* tests

Moving towards llvm#89658 style matchers and test fixtures instead of macros
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bazel "Peripheral" support tier build system: utils/bazel libc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants