Skip to content

Commit

Permalink
Catch floating point exceptions on MSVC for gtest + add cmake `FORCE_…
Browse files Browse the repository at this point in the history 
…DEBUG_ARITHM_MSVC` to enable in release builds too
  • Loading branch information
@jmarrec
jmarrec committed Aug 13, 2024
1 parent 023b7f8 commit 64018cf
Show file tree
Hide file tree
Showing 4 changed files with 43 additions and 24 deletions.
16 changes: 10 additions & 6 deletions cmake/CompilerFlags.cmake
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,15 +76,19 @@ if(MSVC AND NOT ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Intel")) # Visual C++ (VS
target_compile_options(project_options INTERFACE $<$<CONFIG:Debug>:/Ob0>) # Disable inlining
target_compile_options(project_options INTERFACE $<$<CONFIG:Debug>:/RTCsu>) # Runtime checks
target_compile_options(project_fp_options INTERFACE $<$<CONFIG:Debug>:/fp:strict>) # Floating point model
target_compile_options(project_options INTERFACE $<$<CONFIG:Debug>:/DMSVC_DEBUG>) # Triggers code in main.cc to catch floating point NaNs

target_compile_options(turn_off_warnings INTERFACE /W0)

option(FORCE_DEBUG_ARITHM_MSVC "Enable trapping floating point exceptions in non Debug mode" OFF)

This comment has been minimized.

Sign in to view

Copy link
@rraustad

rraustad Aug 13, 2024 •
edited
Loading

Contributor

I turn on this switch in CMake and get this error. I added the path to the Python libs folder and still get the error. I am not linking with Python.

3>LINK : fatal error LNK1104: cannot open file 'python312.lib'
3>Done building project "energyplus_tests.vcxproj" -- FAILED.
========== Build: 2 succeeded, 1 failed, 29 up-to-date, 0 skipped ==========

This comment has been minimized.

Sign in to view

Copy link
@rraustad

rraustad Aug 13, 2024

Contributor

If I include LINK_WITH_PYTHON the DXCoils_Test2 unit test runs and I get:

image

This comment has been minimized.

Sign in to view

Copy link
@rraustad

rraustad Aug 13, 2024

Contributor

This unit test is actually testing for a 0 capacity, so this->autoSizedValue = 0 at line 400 and the FPE occurs. If I fix the problem the unit test passes in the debugger.

EXPECT_DOUBLE_EQ(0.0, state->dataDXCoils->DXCoil(2).RatedTotCap(1));
mark_as_advanced(FORCE_DEBUG_ARITHM_MSVC)

set(need_arithm_debug_genex "$<OR:$<BOOL:${FORCE_DEBUG_ARITHM_MSVC}>,$<CONFIG:Debug>>")

# in main.cc for E+ (actual: api/EnergyPlusPgm.cc) and gtest: will catch _EM_ZERODIVIDE | _EM_INVALID | _EM_OVERFLOW
target_compile_definitions(project_fp_options INTERFACE $<${need_arithm_debug_genex}:DEBUG_ARITHM_MSVC>)

elseif(CMAKE_COMPILER_IS_GNUCXX OR "${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang" OR "${CMAKE_CXX_COMPILER_ID}" STREQUAL "AppleClang") # g++/Clang

# TODO: after we fix all test, enable this by default on Debug builds
# option(FORCE_DEBUG_ARITHM_GCC_OR_CLANG "Enable trapping floating point exceptions in non Debug mode" OFF)
option(FORCE_DEBUG_ARITHM_GCC_OR_CLANG "Enable trapping floating point exceptions" OFF)
option(FORCE_DEBUG_ARITHM_GCC_OR_CLANG "Enable trapping floating point exceptions in non Debug mode" OFF)
mark_as_advanced(FORCE_DEBUG_ARITHM_GCC_OR_CLANG)

# COMPILER FLAGS
Expand Down Expand Up @@ -126,7 +130,7 @@ elseif(CMAKE_COMPILER_IS_GNUCXX OR "${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang" O

set(need_arithm_debug_genex "$<OR:$<BOOL:${FORCE_DEBUG_ARITHM_GCC_OR_CLANG}>,$<CONFIG:Debug>>")

# in main.cc for E+ and gtest: feenableexcept(FE_DIVBYZERO | FE_INVALID | FE_OVERFLOW)
# in main.cc for E+ (actual: api/EnergyPlusPgm.cc) and gtest: feenableexcept(FE_DIVBYZERO | FE_INVALID | FE_OVERFLOW)
target_compile_definitions(project_fp_options INTERFACE $<${need_arithm_debug_genex}:DEBUG_ARITHM_GCC_OR_CLANG>)
include(CheckCXXSymbolExists)
check_cxx_symbol_exists(feenableexcept "fenv.h" HAVE_FEENABLEEXCEPT)
Expand Down
27 changes: 17 additions & 10 deletions src/EnergyPlus/api/EnergyPlusPgm.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -182,10 +182,12 @@
#include <string>
#include <vector>

#ifndef NDEBUG
#ifdef __unix__
#include <cfenv>
#ifdef DEBUG_ARITHM_GCC_OR_CLANG
#include <EnergyPlus/fenv_missing.h>
#endif

#ifdef DEBUG_ARITHM_MSVC
#include <cfloat>
#endif

// ObjexxFCL Headers
Expand Down Expand Up @@ -248,17 +250,22 @@ void commonInitialize(EnergyPlus::EnergyPlusData &state)
// std::cin.tie(nullptr); // Untie cin and cout: Could cause odd behavior for interactive prompts

// Enable floating point exceptions
#ifndef NDEBUG
#ifdef __unix__
#ifdef DEBUG_ARITHM_GCC_OR_CLANG
feenableexcept(FE_DIVBYZERO | FE_INVALID | FE_OVERFLOW); // These exceptions are enabled (FE_INEXACT and FE_UNDERFLOW will not throw)
#endif
#endif

#ifdef MSVC_DEBUG
// the following line enables NaN detection in Visual Studio debug builds. See
#ifdef DEBUG_ARITHM_MSVC
// the following enables NaN detection in Visual Studio debug builds. See
// https://github.com/NREL/EnergyPlus/wiki/Debugging-Tips
int fp_control_state =
_controlfp(_EM_INEXACT | _EM_UNDERFLOW, _MCW_EM); // These exceptions are disabled (_EM_INEXACT and _EM_UNDERFLOW will not throw)

// Note: what you need to pass to the _controlfp_s is actually the opposite
// By default all bits are 1, and the exceptions are turned off, so you need to turn off the bits for the exceptions you want to enable
// > For the _MCW_EM mask, clearing it sets the exception, which allows the hardware exception; setting it hides the exception.
unsigned int fpcntrl = 0;
_controlfp_s(&fpcntrl, 0, 0);
unsigned int new_exceptions = _EM_ZERODIVIDE | _EM_INVALID | _EM_OVERFLOW;
unsigned int new_control = fpcntrl & ~new_exceptions;
_controlfp_s(&fpcntrl, new_control, _MCW_EM);
#endif

#ifdef _MSC_VER
Expand Down
8 changes: 0 additions & 8 deletions src/EnergyPlus/main.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,16 +50,8 @@

#include <CLI/CLI11.hpp>

#ifdef DEBUG_ARITHM_GCC_OR_CLANG
#include <EnergyPlus/fenv_missing.h>
#endif

int main(int argc, char **argv)
{
#ifdef DEBUG_ARITHM_GCC_OR_CLANG
feenableexcept(FE_DIVBYZERO | FE_INVALID | FE_OVERFLOW);
#endif

#ifdef _WIN32
const std::vector<std::string> args = CLI::detail::compute_win32_argv();
#else
Expand Down
16 changes: 16 additions & 0 deletions tst/EnergyPlus/unit/main.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,10 @@
#include <EnergyPlus/fenv_missing.h>
#endif

#ifdef DEBUG_ARITHM_MSVC
#include <cfloat>
#endif

// Google Test main
int main(int argc, char **argv)
{
Expand All @@ -68,5 +72,17 @@ int main(int argc, char **argv)
#ifdef DEBUG_ARITHM_GCC_OR_CLANG
feenableexcept(FE_DIVBYZERO | FE_INVALID | FE_OVERFLOW);
#endif

#ifdef DEBUG_ARITHM_MSVC
// Note: what you need to pass to the _controlfp_s is actually the opposite
// By default all bits are 1, and the exceptions are turned off, so you need to turn off the bits for the exceptions you want to enable
// > For the _MCW_EM mask, clearing it sets the exception, which allows the hardware exception; setting it hides the exception.
unsigned int fpcntrl = 0;
_controlfp_s(&fpcntrl, 0, 0);
unsigned int new_exceptions = _EM_ZERODIVIDE | _EM_INVALID | _EM_OVERFLOW;
unsigned int new_control = fpcntrl & ~new_exceptions;
_controlfp_s(&fpcntrl, new_control, _MCW_EM);
#endif

return RUN_ALL_TESTS();
}

4 comments on commit 64018cf

@nrel-bot-2c
Copy link

Choose a reason for hiding this comment

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

catch_fe_exceptions_release_msvc_and_gtest (jmarrec) - x86_64-Linux-Ubuntu-22.04-gcc-11.4: OK (3575 of 3695 tests passed, 473 test warnings)

Messages:\n

  • 593 tests had: AUD diffs.
  • 563 tests had: EIO diffs.
  • 283 tests had: RDD diffs.
  • 123 tests had: Table small diffs.
  • 120 tests had: Table big diffs.
  • 2 tests had: ESO small diffs.
  • 1 test had: MTR small diffs.
  • 1 test had: MTD diffs.
  • 1 test had: ERR diffs.

Failures:\n

regression Test Summary

  • Passed: 691
  • Failed: 120

Build Badge Test Badge

@nrel-bot-3
Copy link

Choose a reason for hiding this comment

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

catch_fe_exceptions_release_msvc_and_gtest (jmarrec) - x86_64-MacOS-10.18-clang-15.0.0: OK (3534 of 3654 tests passed, 472 test warnings)

Messages:\n

  • 592 tests had: AUD diffs.
  • 562 tests had: EIO diffs.
  • 283 tests had: RDD diffs.
  • 123 tests had: Table small diffs.
  • 120 tests had: Table big diffs.
  • 2 tests had: ESO small diffs.
  • 1 test had: MTR small diffs.
  • 1 test had: MTD diffs.
  • 1 test had: ERR diffs.

Failures:\n

regression Test Summary

  • Passed: 671
  • Failed: 120

Build Badge Test Badge

@nrel-bot-2b
Copy link

Choose a reason for hiding this comment

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

catch_fe_exceptions_release_msvc_and_gtest (jmarrec) - x86_64-Linux-Ubuntu-22.04-gcc-11.4-UnitTestsCoverage-Debug: OK (2056 of 2070 tests passed, 0 test warnings)

Failures:\n

EnergyPlusFixture Test Summary

  • Passed: 1563
  • NUMERICAL: 14

Build Badge Test Badge Coverage Badge

@nrel-bot-2
Copy link

Choose a reason for hiding this comment

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

catch_fe_exceptions_release_msvc_and_gtest (jmarrec) - x86_64-Linux-Ubuntu-22.04-gcc-11.4-IntegrationCoverage-Debug: OK (795 of 795 tests passed, 0 test warnings)

Build Badge Test Badge Coverage Badge

Please sign in to comment.