From b8d438567d6babea1560c9d1f8ba7f44f4014f97 Mon Sep 17 00:00:00 2001 From: Jonah Miller Date: Thu, 28 Nov 2024 18:20:15 -0700 Subject: [PATCH 01/13] run singularity tests through clang warnings, clang address sanitizer, clang undefined behavior sanitizer --- singularity-eos/closure/mixed_cell_models.hpp | 6 +++--- singularity-eos/eos/eos_gruneisen.hpp | 4 ++-- singularity-eos/eos/eos_mgusup.hpp | 2 +- singularity-eos/eos/eos_powermg.hpp | 2 +- singularity-eos/eos/eos_sap_polynomial.hpp | 4 ++-- singularity-eos/eos/eos_vinet.hpp | 2 +- singularity-eos/eos/get_sg_eos_rho_e.cpp | 14 +++++++++++--- singularity-eos/eos/get_sg_eos_rho_p.cpp | 12 +++++++++--- singularity-eos/eos/get_sg_eos_rho_t.cpp | 13 ++++++++++--- 9 files changed, 40 insertions(+), 19 deletions(-) diff --git a/singularity-eos/closure/mixed_cell_models.hpp b/singularity-eos/closure/mixed_cell_models.hpp index 69cdf80807..f26ed23a02 100644 --- a/singularity-eos/closure/mixed_cell_models.hpp +++ b/singularity-eos/closure/mixed_cell_models.hpp @@ -209,9 +209,9 @@ class PTESolverBase { const RealIndexer &vfrac_, const RealIndexer &sie_, const RealIndexer &temp_, const RealIndexer &press_, Real *&scratch, Real Tnorm, const MixParams ¶ms = MixParams()) - : nmat(nmats), neq(neqs), niter(0), eos(eos_), vfrac_total(vfrac_tot), - sie_total(sie_tot), rho(rho_), vfrac(vfrac_), sie(sie_), temp(temp_), - press(press_), Tnorm(Tnorm), params_(params) { + : params_(params), nmat(nmats), neq(neqs), niter(0), vfrac_total(vfrac_tot), + sie_total(sie_tot), eos(eos_), rho(rho_), vfrac(vfrac_), sie(sie_), temp(temp_), + press(press_), Tnorm(Tnorm) { jacobian = AssignIncrement(scratch, neq * neq); dx = AssignIncrement(scratch, neq); sol_scratch = AssignIncrement(scratch, 2 * neq); diff --git a/singularity-eos/eos/eos_gruneisen.hpp b/singularity-eos/eos/eos_gruneisen.hpp index 131e523d7f..93570a37e7 100644 --- a/singularity-eos/eos/eos_gruneisen.hpp +++ b/singularity-eos/eos/eos_gruneisen.hpp @@ -359,14 +359,14 @@ PORTABLE_INLINE_FUNCTION Real Gruneisen::PressureFromDensityInternalEnergy( template PORTABLE_INLINE_FUNCTION Real Gruneisen::MinInternalEnergyFromDensity(const Real rho_in, Indexer_t &&lambda) const { - const Real rho = std::min(rho_in, _rho_max); + // const Real rho = std::min(rho_in, _rho_max); MinInternalEnergyIsNotEnabled("Gruneisen"); return 0.0; } template PORTABLE_INLINE_FUNCTION Real Gruneisen::EntropyFromDensityInternalEnergy( const Real rho_in, const Real sie, Indexer_t &&lambda) const { - const Real rho = std::min(rho_in, _rho_max); + // const Real rho = std::min(rho_in, _rho_max); EntropyIsNotEnabled("Gruneisen"); return 1.0; } diff --git a/singularity-eos/eos/eos_mgusup.hpp b/singularity-eos/eos/eos_mgusup.hpp index 8fae069056..409e596fe1 100644 --- a/singularity-eos/eos/eos_mgusup.hpp +++ b/singularity-eos/eos/eos_mgusup.hpp @@ -370,7 +370,7 @@ MGUsup::FillEos(Real &rho, Real &temp, Real &sie, Real &press, Real &cv, Real &b } if (output & thermalqs::temperature) temp = TemperatureFromDensityInternalEnergy(rho, sie); - if (output & thermalqs::specific_internal_energy) sie = sie; + // if (output & thermalqs::specific_internal_energy) sie = sie; if (output & thermalqs::pressure) press = PressureFromDensityInternalEnergy(rho, sie); if (output & thermalqs::specific_heat) cv = SpecificHeatFromDensityInternalEnergy(rho, sie); diff --git a/singularity-eos/eos/eos_powermg.hpp b/singularity-eos/eos/eos_powermg.hpp index 3b17e8aae9..ea8904285a 100644 --- a/singularity-eos/eos/eos_powermg.hpp +++ b/singularity-eos/eos/eos_powermg.hpp @@ -452,7 +452,7 @@ PowerMG::FillEos(Real &rho, Real &temp, Real &sie, Real &press, Real &cv, Real & } if (output & thermalqs::temperature) temp = TemperatureFromDensityInternalEnergy(rho, sie); - if (output & thermalqs::specific_internal_energy) sie = sie; + // if (output & thermalqs::specific_internal_energy) sie = sie; if (output & thermalqs::pressure) press = PressureFromDensityInternalEnergy(rho, sie); if (output & thermalqs::specific_heat) cv = SpecificHeatFromDensityInternalEnergy(rho, sie); diff --git a/singularity-eos/eos/eos_sap_polynomial.hpp b/singularity-eos/eos/eos_sap_polynomial.hpp index 29681300e8..c26e77d429 100644 --- a/singularity-eos/eos/eos_sap_polynomial.hpp +++ b/singularity-eos/eos/eos_sap_polynomial.hpp @@ -40,8 +40,8 @@ class SAP_Polynomial : public EosBase { SAP_Polynomial(const Real rho0, const Real a0, const Real a1, const Real a2c, const Real a2e, const Real a3, const Real b0, const Real b1, const Real b2c, const Real b2e, const Real b3) - : _rho0(rho0), _a0(a0), _a1(a1), _a2c(a2c), _a2e(a2e), _a3(a3), _b0(b0), _b1(b1), - _b2c(b2c), _b2e(b2e), _b3(b3) { + : _a0(a0), _a1(a1), _a2c(a2c), _a2e(a2e), _a3(a3), _b0(b0), _b1(b1), _b2c(b2c), + _b2e(b2e), _b3(b3), _rho0(rho0) { CheckParams(); } diff --git a/singularity-eos/eos/eos_vinet.hpp b/singularity-eos/eos/eos_vinet.hpp index 3b5e0204b6..570d1bb319 100644 --- a/singularity-eos/eos/eos_vinet.hpp +++ b/singularity-eos/eos/eos_vinet.hpp @@ -395,7 +395,7 @@ Vinet::FillEos(Real &rho, Real &temp, Real &sie, Real &press, Real &cv, Real &bm } Real Vout[8]; Vinet_F_DT_func(rho, temp, Vout); - if (output & thermalqs::temperature) temp = temp; + // if (output & thermalqs::temperature) temp = temp; if (output & thermalqs::specific_internal_energy) sie = Vout[0]; if (output & thermalqs::pressure) press = Vout[1]; if (output & thermalqs::specific_heat) cv = Vout[4]; diff --git a/singularity-eos/eos/get_sg_eos_rho_e.cpp b/singularity-eos/eos/get_sg_eos_rho_e.cpp index adfa23ec81..8501c4cde8 100644 --- a/singularity-eos/eos/get_sg_eos_rho_e.cpp +++ b/singularity-eos/eos/get_sg_eos_rho_e.cpp @@ -54,10 +54,18 @@ void get_sg_eos_rho_e(const char *name, int ncell, indirection_v &offsets_v, // eos accessor singularity::EOSAccessor_ eos_inx(eos_v, &pte_idxs(tid, 0)); // reset inputs + // JMM: Address sanitizer likes these named. My guess is + // that the forwarding references are not being resolved + // properly. + Real *prho_pte = &rho_pte(tid, 0); + Real *pvfrac_pte = &vfrac_pte(tid, 0); + Real *psie_pte = &sie_pte(tid, 0); + Real *ptemp_pte = &temp_pte(tid, 0); + Real *ppress_pte = &press_pte(tid, 0); + Real *pscratch = &solver_scratch(tid, 0); PTESolverRhoT method( - npte, eos_inx, vfrac_sum, sie_v(i), &rho_pte(tid, 0), &vfrac_pte(tid, 0), - &sie_pte(tid, 0), &temp_pte(tid, 0), &press_pte(tid, 0), cache, - &solver_scratch(tid, 0)); + npte, eos_inx, vfrac_sum, sie_v(i), prho_pte, pvfrac_pte, psie_pte, + ptemp_pte, ppress_pte, cache, pscratch); auto status = PTESolver(method); pte_converged = status.converged; } else { diff --git a/singularity-eos/eos/get_sg_eos_rho_p.cpp b/singularity-eos/eos/get_sg_eos_rho_p.cpp index 24ab5119eb..a6c5a8ea4a 100644 --- a/singularity-eos/eos/get_sg_eos_rho_p.cpp +++ b/singularity-eos/eos/get_sg_eos_rho_p.cpp @@ -54,10 +54,16 @@ void get_sg_eos_rho_p(const char *name, int ncell, indirection_v &offsets_v, // create solver lambda // eos accessor singularity::EOSAccessor_ eos_inx(eos_v, &pte_idxs(tid, 0)); + // JMM: Address sanitizer likes these named. + Real *prho_pte = &rho_pte(tid, 0); + Real *pvfrac_pte = &vfrac_pte(tid, 0); + Real *psie_pte = &sie_pte(tid, 0); + Real *ptemp_pte = &temp_pte(tid, 0); + Real *ppress_pte = &press_pte(tid, 0); + Real *pscratch = &solver_scratch(tid, 0); PTESolverFixedP method( - npte, eos_inx, vfrac_sum, press_pte(tid, 0), &rho_pte(tid, 0), - &vfrac_pte(tid, 0), &sie_pte(tid, 0), &temp_pte(tid, 0), &press_pte(tid, 0), - cache[0], &solver_scratch(tid, 0)); + npte, eos_inx, vfrac_sum, press_pte(tid, 0), prho_pte, pvfrac_pte, psie_pte, + ptemp_pte, ppress_pte, cache[0], pscratch); auto status = PTESolver(method); pte_converged = status.converged; // calculate total sie diff --git a/singularity-eos/eos/get_sg_eos_rho_t.cpp b/singularity-eos/eos/get_sg_eos_rho_t.cpp index adedbea921..9735a4c1a1 100644 --- a/singularity-eos/eos/get_sg_eos_rho_t.cpp +++ b/singularity-eos/eos/get_sg_eos_rho_t.cpp @@ -55,10 +55,17 @@ void get_sg_eos_rho_t(const char *name, int ncell, indirection_v &offsets_v, // create solver lambda // eos accessor singularity::EOSAccessor_ eos_inx(eos_v, &pte_idxs(tid, 0)); + // JMM: Address sanitizer does not like passing these in + // through the function call. + Real *prho_pte = &rho_pte(tid, 0); + Real *pvfrac_pte = &vfrac_pte(tid, 0); + Real *psie_pte = &sie_pte(tid, 0); + Real *ptemp_pte = &temp_pte(tid, 0); + Real *ppress_pte = &press_pte(tid, 0); + Real *pscratch = &solver_scratch(tid, 0); PTESolverFixedT method( - npte, eos_inx, vfrac_sum, temp_pte(tid, 0), &rho_pte(tid, 0), - &vfrac_pte(tid, 0), &sie_pte(tid, 0), &temp_pte(tid, 0), &press_pte(tid, 0), - cache, &solver_scratch(tid, 0)); + npte, eos_inx, vfrac_sum, temp_pte(tid, 0), prho_pte, pvfrac_pte, psie_pte, + ptemp_pte, ppress_pte, cache, pscratch); auto status = PTESolver(method); pte_converged = status.converged; // calculate total internal energy From 49bb334371781894db77d43eb09d70e555426360 Mon Sep 17 00:00:00 2001 From: Jonah Miller Date: Fri, 29 Nov 2024 12:45:04 -0700 Subject: [PATCH 02/13] sneak in kokkos update in submodules --- utils/kokkos | 2 +- utils/kokkos-kernels | 2 +- utils/ports-of-call | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/utils/kokkos b/utils/kokkos index 08ceff92bc..15dc143e5f 160000 --- a/utils/kokkos +++ b/utils/kokkos @@ -1 +1 @@ -Subproject commit 08ceff92bcf3a828844480bc1e6137eb74028517 +Subproject commit 15dc143e5f39949eece972a798e175c4b463d4b8 diff --git a/utils/kokkos-kernels b/utils/kokkos-kernels index 0608a33738..957ac84922 160000 --- a/utils/kokkos-kernels +++ b/utils/kokkos-kernels @@ -1 +1 @@ -Subproject commit 0608a337389ceb7e2695570db830230c2703bfe0 +Subproject commit 957ac84922a25ac76d0c67815f4cdfc243897f6a diff --git a/utils/ports-of-call b/utils/ports-of-call index 58ce1181b2..4c4cbab492 160000 --- a/utils/ports-of-call +++ b/utils/ports-of-call @@ -1 +1 @@ -Subproject commit 58ce1181b2d835bd32673ad70550c9130381f91b +Subproject commit 4c4cbab492b850756bfbf27b36426da29f53b4bf From 27b3a8a6b37a55923dad6e3e1bb284f1b10bdadb Mon Sep 17 00:00:00 2001 From: Jonah Miller Date: Fri, 29 Nov 2024 12:45:36 -0700 Subject: [PATCH 03/13] clean up some more warnings clang found --- test/test_closure_pte.cpp | 6 +++--- test/test_eos_ideal.cpp | 2 +- test/test_eos_modifiers.cpp | 1 + 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/test/test_closure_pte.cpp b/test/test_closure_pte.cpp index fce46ac9ca..a66aa478d3 100644 --- a/test/test_closure_pte.cpp +++ b/test/test_closure_pte.cpp @@ -30,12 +30,12 @@ #include #include -constexpr Real GPa = 1.0e10; -constexpr Real MJ_per_kg = 1.0e10; - #ifdef SINGULARITY_TEST_SESAME #ifdef SINGULARITY_USE_SPINER_WITH_HDF5 +constexpr Real GPa = 1.0e10; +constexpr Real MJ_per_kg = 1.0e10; + using singularity::DavisProducts; using singularity::DavisReactants; using singularity::IdealGas; diff --git a/test/test_eos_ideal.cpp b/test/test_eos_ideal.cpp index 9e52c81769..11b8b6447a 100644 --- a/test/test_eos_ideal.cpp +++ b/test/test_eos_ideal.cpp @@ -103,10 +103,10 @@ class CheckPofRE { int nwrong = 0; private: - int N_; Real *P_; Real *rho_; Real *sie_; + int N_; }; SCENARIO("Ideal gas vector Evaluate call", "[IdealGas][Evaluate]") { GIVEN("An ideal gas, and some device memory") { diff --git a/test/test_eos_modifiers.cpp b/test/test_eos_modifiers.cpp index 631d906ba9..f13773dc74 100644 --- a/test/test_eos_modifiers.cpp +++ b/test/test_eos_modifiers.cpp @@ -134,6 +134,7 @@ SCENARIO("EOS Builder and Modifiers", "[EOSBuilder][Modifiers][IdealGas]") { Real c = 0; THEN("The EOS is constructed correctly") { auto eos_ramped = Modify(eos, r0, a, b, c); + eos_ramped.PrintParams(); } } } From ec77b9dce54029f74353d4f8a2466cf83f56efe8 Mon Sep 17 00:00:00 2001 From: Jonah Miller Date: Fri, 29 Nov 2024 12:46:15 -0700 Subject: [PATCH 04/13] add ability to build fortran backend code without fortran. Useful for clang-sanitize builds, since flang is not super well supported yet --- CMakeLists.txt | 13 +++++++++++++ singularity-eos/CMakeLists.txt | 4 ++-- test/CMakeLists.txt | 2 +- 3 files changed, 16 insertions(+), 3 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 56178b2fba..8b09b1868c 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -46,6 +46,14 @@ cmake_dependent_option( "SINGULARITY_USE_SPINER" OFF) option(SINGULARITY_USE_FORTRAN "Enable fortran bindings" ON) +# Optionally build these if you want to build/test the fortran +# infrastructure without invoking a fortran compiler If fortran is +# off, you can set this. If fortran is on, it's forced to ON and is +# not set-able. +cmake_dependent_option(SINGULARITY_BUILD_FORTRAN_BACKEND + "Build the C++ code to which the fortran bindings bind" + OFF "NOT SINGULARITY_USE_FORTRAN" ON) + option(SINGULARITY_USE_KOKKOS "Use Kokkos for portability" OFF) option(SINGULARITY_USE_EOSPAC "Enable eospac backend" OFF) option(SINGULARITY_EOSPAC_ENABLE_SHMEM @@ -95,6 +103,7 @@ cmake_dependent_option( # modify flags options option(SINGULARITY_BETTER_DEBUG_FLAGS "Better debug flags for singularity" ON) option(SINGULARITY_HIDE_MORE_WARNINGS "hide more warnings" OFF) +option(SINGULARITY_STRICT_WARNINGS "Make warnings strict" OFF) # toggle code options option(SINGULARITY_USE_TRUE_LOG_GRIDDING @@ -552,6 +561,10 @@ target_compile_options( > # release > # cuda ) +if (SINGULARITY_STRICT_WARNINGS) + target_compile_options(singularity-eos_Interface INTERFACE + -Wall -Werror) +endif() if(TARGET singularity-eos_Library) target_compile_options(singularity-eos_Library PRIVATE ${xlfix}) diff --git a/singularity-eos/CMakeLists.txt b/singularity-eos/CMakeLists.txt index 87b2614717..4f0c5e3220 100644 --- a/singularity-eos/CMakeLists.txt +++ b/singularity-eos/CMakeLists.txt @@ -73,13 +73,13 @@ if (SINGULARITY_BUILD_CLOSURE) closure/kinetic_phasetransition_utils.hpp closure/kinetic_phasetransition_methods.hpp ) - if (SINGULARITY_USE_FORTRAN OR SINGULARITY_BUILD_TESTS) + if (SINGULARITY_BUILD_FORTRAN_BACKEND OR SINGULARITY_BUILD_TESTS) # while these are C++ files, they # are only needed for the fortran backend or unit testing register_srcs(eos/singularity_eos.cpp) register_headers(eos/singularity_eos.hpp) endif() - if (SINGULARITY_USE_FORTRAN) + if (SINGULARITY_BUILD_FORTRAN_BACKEND) register_srcs(eos/get_sg_eos.cpp) if (SINGULARITY_USE_KOKKOS) register_srcs( diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index e01d0a01e1..318f469387 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -134,7 +134,7 @@ if(SINGULARITY_BUILD_CLOSURE) endif() endif() # disable fortran tests when using OpenMP due to segfaults - if(SINGULARITY_USE_KOKKOS AND SINGULARITY_USE_FORTRAN AND NOT Kokkos_ENABLE_OPENMP) + if(SINGULARITY_USE_KOKKOS AND SINGULARITY_BUILD_FORTRAN_BACKEND AND NOT Kokkos_ENABLE_OPENMP) add_executable(test_get_sg_eos test_get_sg_eos.cpp) target_link_libraries(test_get_sg_eos PRIVATE Catch2::Catch2 singularity-eos::singularity-eos) From 520f1dcab7e65eb5632d76fb834a87f7e5959487 Mon Sep 17 00:00:00 2001 From: Jonah Miller Date: Fri, 29 Nov 2024 12:52:41 -0700 Subject: [PATCH 05/13] enable_testing was missing... how did tests ever run???? --- CMakeLists.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/CMakeLists.txt b/CMakeLists.txt index 8b09b1868c..fbfa81e865 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -603,6 +603,7 @@ endif() if(SINGULARITY_BUILD_TESTS) include(CTest) + enable_testing() add_subdirectory(test) endif() From 5d053c782e2a6d8c2e428ca17e07df0625df2193 Mon Sep 17 00:00:00 2001 From: Jonah Miller Date: Fri, 29 Nov 2024 12:53:16 -0700 Subject: [PATCH 06/13] add sanitizer test --- .github/workflows/sanitizer.yml | 40 +++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) create mode 100644 .github/workflows/sanitizer.yml diff --git a/.github/workflows/sanitizer.yml b/.github/workflows/sanitizer.yml new file mode 100644 index 0000000000..218ab7179f --- /dev/null +++ b/.github/workflows/sanitizer.yml @@ -0,0 +1,40 @@ +name: Run clang sanitizer on minimal code subset + +on: + push: + branches: [ main ] + pull_request: + branches: [ main ] + +jobs: + sanitizer: + name: Run clang sanitizer on minimal code subset + runs-on: ubuntu-latest + + steps: + - name: Checkout code + uses: actions/checkout@v3 + with: + submodules: recursive + - name: Set system to non-interactive mode + run: export DEBIAN_FRONTEND=noninteractive + - name: install dependencies + run: | + sudo apt-get update -y -qq + sudo apt-get install -y --allow-downgrades --allow-remove-essential --allow-change-held-packages -qq build-essential clang llvm + - name: build and run tests + run: | + mkdir -p bin + cd bin + cmake -DCMAKE_CXX_COMPILER=clang++ \ + -DCMAKE_CXX_FLAGS="-fsanitize=address -fsanitize=undefined -fno-omit-frame-pointer" \ + -DCMAKE_BUILD_TYPE=Debug \ + -DSINGULARITY_STRICT_WARNINGS=ON \ + -DSINGULARITY_USE_FORTRAN=OFF \ + -DSINGULARITY_BUILD_FORTRAN_BACKEND=ON \ + -DSINGULARITY_BUILD_TESTS=ON \ + -DSINGULARITY_FORCE_SUBMODULE_MODE=ON \ + -DSINGULARITY_USE_KOKKOS=ON \ + .. + make -j4 + ctest --output-on-failure From ac4cf33331fd72e8140426b7c44750e1ad978de8 Mon Sep 17 00:00:00 2001 From: Jonah Miller Date: Fri, 29 Nov 2024 12:54:35 -0700 Subject: [PATCH 07/13] rename sanitizer --- .github/workflows/sanitizer.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/sanitizer.yml b/.github/workflows/sanitizer.yml index 218ab7179f..819501b426 100644 --- a/.github/workflows/sanitizer.yml +++ b/.github/workflows/sanitizer.yml @@ -1,4 +1,4 @@ -name: Run clang sanitizer on minimal code subset +name: Sanitizer on: push: From 75519faa3e733a7f1c31770bbdb8328bf1ac57db Mon Sep 17 00:00:00 2001 From: Jonah Miller Date: Fri, 29 Nov 2024 13:04:25 -0700 Subject: [PATCH 08/13] Add new build flags --- doc/sphinx/src/building.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/doc/sphinx/src/building.rst b/doc/sphinx/src/building.rst index 6dc37ce57c..602b620a35 100644 --- a/doc/sphinx/src/building.rst +++ b/doc/sphinx/src/building.rst @@ -118,6 +118,8 @@ The main CMake options to configure building are in the following table: ``SINGULARITY_USE_SINGLE_LOGS`` OFF Use single precision logarithms (may degrade accuracy). ``SINGULARITY_NQT_ORDER_1`` OFF For fast logs, use the less accurate but faster 1st-order version. ``SINGULARITY_NQT_PORTABLE`` OFF For fast logs, use the slower but endianness-independent implementation. + ``SINGULARITY_BUILD_FORTRAN_BACKEND`` OFF For testing, you may build the C++ code to which the fortran bindings bind + ``SINGULARITY_STRICT_WARNINGS`` OFF For testing. Adds -Wall and -Werror to builds. ====================================== ======= =========================================== More options are available to modify only if certain other options or From 07684af1e4188588f027ad9fcfe66df19639d825 Mon Sep 17 00:00:00 2001 From: Jonah Miller Date: Fri, 29 Nov 2024 13:06:11 -0700 Subject: [PATCH 09/13] correction --- doc/sphinx/src/building.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/sphinx/src/building.rst b/doc/sphinx/src/building.rst index 602b620a35..2567f1f266 100644 --- a/doc/sphinx/src/building.rst +++ b/doc/sphinx/src/building.rst @@ -118,7 +118,6 @@ The main CMake options to configure building are in the following table: ``SINGULARITY_USE_SINGLE_LOGS`` OFF Use single precision logarithms (may degrade accuracy). ``SINGULARITY_NQT_ORDER_1`` OFF For fast logs, use the less accurate but faster 1st-order version. ``SINGULARITY_NQT_PORTABLE`` OFF For fast logs, use the slower but endianness-independent implementation. - ``SINGULARITY_BUILD_FORTRAN_BACKEND`` OFF For testing, you may build the C++ code to which the fortran bindings bind ``SINGULARITY_STRICT_WARNINGS`` OFF For testing. Adds -Wall and -Werror to builds. ====================================== ======= =========================================== @@ -161,6 +160,7 @@ preconditions: ``SINGULARITY_TEST_PYTHON`` ``SINGULARITY_BUILD_TESTS=ON`` ``SINGULARITY_BUILD_PYTHON=ON`` Test the Python bindings. ``SINGULARITY_USE_HELMHOLTZ`` ``SINGULARITY_USE_SPINER=ON`` ``SINGULARITY_USE_SPINER_WITH_HDF5=ON`` Use Helmholtz equation of state. ``SINGULARITY_TEST_HELMHOLTZ`` ``SINGULARITY_USE_HELMHOLTZ`` Build Helmholtz equation of state tests. + ``SINGULARITY_BUILD_FORTRAN_BACKEND`` ``NOT SINGULARITY_USE_FORTRAN`` For testing, you may build the C++ code to which the fortran bindings bind without building the bindings themselves. ============================================== ================================================================================= =========================================== When installing ``singularity-eos``, data files are also installed. The From 20e966164a2ae85117fab5e1509cd89e63067f9a Mon Sep 17 00:00:00 2001 From: Jonah Miller Date: Fri, 29 Nov 2024 13:50:33 -0700 Subject: [PATCH 10/13] changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4cc15acee6..e2e855636a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### Added (new features/APIs/variables/...) ### Fixed (Repair bugs, etc) +- [[OR437]](https://github.com/lanl/singularity-eos/pull/437) Fix segfault on HIP, clean up warnings, add strict sanitizer test ### Changed (changing behavior/API/variables/...) From 5b90563c7802b4679ab4caea7e7cba21c5fb66f8 Mon Sep 17 00:00:00 2001 From: Jonah Miller Date: Fri, 29 Nov 2024 13:51:02 -0700 Subject: [PATCH 11/13] add check for warnings on gcc as well as clang --- .github/workflows/warnings.yml | 37 ++++++++++++++ CMakeLists.txt | 2 +- .../base/root-finding-1d/root_finding.hpp | 50 +++++++++---------- singularity-eos/eos/eos_base.hpp | 7 +-- singularity-eos/eos/get_sg_eos_p_t.cpp | 2 +- singularity-eos/eos/get_sg_eos_rho_e.cpp | 2 +- singularity-eos/eos/get_sg_eos_rho_p.cpp | 2 +- singularity-eos/eos/get_sg_eos_rho_t.cpp | 2 +- .../eos/modifiers/eos_unitsystem.hpp | 3 ++ 9 files changed, 74 insertions(+), 33 deletions(-) create mode 100644 .github/workflows/warnings.yml diff --git a/.github/workflows/warnings.yml b/.github/workflows/warnings.yml new file mode 100644 index 0000000000..98316b7ccf --- /dev/null +++ b/.github/workflows/warnings.yml @@ -0,0 +1,37 @@ +name: Warnings + +on: + push: + branches: [ main ] + pull_request: + branches: [ main ] + +jobs: + warnings-gcc: + name: Ensure no warnings from gcc + runs-on: ubuntu-latest + + steps: + - name: Checkout code + uses: actions/checkout@v3 + with: + submodules: recursive + - name: Set system to non-interactive mode + run: export DEBIAN_FRONTEND=noninteractive + - name: install dependencies + run: | + sudo apt-get update -y -qq + sudo apt-get install -y --allow-downgrades --allow-remove-essential --allow-change-held-packages -qq build-essential + - name: build and run tests + run: | + mkdir -p bin + cd bin + cmake -DCMAKE_BUILD_TYPE=Debug \ + -DSINGULARITY_STRICT_WARNINGS=ON \ + -DSINGULARITY_USE_FORTRAN=OFF \ + -DSINGULARITY_BUILD_FORTRAN_BACKEND=ON \ + -DSINGULARITY_BUILD_TESTS=ON \ + -DSINGULARITY_FORCE_SUBMODULE_MODE=ON \ + -DSINGULARITY_USE_KOKKOS=ON \ + .. + make -j4 diff --git a/CMakeLists.txt b/CMakeLists.txt index fbfa81e865..ab5d62bd02 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -563,7 +563,7 @@ target_compile_options( ) if (SINGULARITY_STRICT_WARNINGS) target_compile_options(singularity-eos_Interface INTERFACE - -Wall -Werror) + -Wall -Werror -Wno-unknown-pragmas) endif() if(TARGET singularity-eos_Library) diff --git a/singularity-eos/base/root-finding-1d/root_finding.hpp b/singularity-eos/base/root-finding-1d/root_finding.hpp index e62615a7d1..7305141475 100644 --- a/singularity-eos/base/root-finding-1d/root_finding.hpp +++ b/singularity-eos/base/root-finding-1d/root_finding.hpp @@ -70,20 +70,20 @@ enum class Status { SUCCESS = 0, FAIL = 1 }; */ class RootCounts { private: - static constexpr int nbins_{15}; + static constexpr std::size_t nbins_{15}; mutable Real counts_[nbins_]; public: PORTABLE_INLINE_FUNCTION RootCounts() { - for (int i{0}; i < nbins_; ++i) + for (std::size_t i{0}; i < nbins_; ++i) counts_[i] = 0; } PORTABLE_INLINE_FUNCTION void reset() { - for (int i{0}; i < nbins_; ++i) + for (std::size_t i{0}; i < nbins_; ++i) counts_[i] = 0; } - PORTABLE_INLINE_FUNCTION void increment(int i) const { + PORTABLE_INLINE_FUNCTION void increment(std::size_t i) const { assert(i < nbins_ && i >= 0); #ifdef PORTABILITY_STRATEGY_NONE counts_[i] += 1; @@ -91,24 +91,24 @@ class RootCounts { } PORTABLE_INLINE_FUNCTION Real total() const { Real tot{1.e-20}; - for (int i{0}; i < nbins_; ++i) + for (std::size_t i{0}; i < nbins_; ++i) tot += counts_[i]; return tot; } - PORTABLE_INLINE_FUNCTION const Real &operator[](const int i) const { + PORTABLE_INLINE_FUNCTION const Real &operator[](const std::size_t i) const { assert(i < nbins_ && i >= 0); return counts_[i]; } - PORTABLE_INLINE_FUNCTION Real &operator[](const int i) { + PORTABLE_INLINE_FUNCTION Real &operator[](const std::size_t i) { assert(i < nbins_ && i >= 0); return counts_[i]; } PORTABLE_INLINE_FUNCTION void print_counts() const { - for (int i{0}; i < nbins_; ++i) + for (std::size_t i{0}; i < nbins_; ++i) printf("%e\n", counts_[i]); } - PORTABLE_INLINE_FUNCTION int nBins() const { return nbins_; } - PORTABLE_INLINE_FUNCTION int more() const { return nbins_ - 1; } + PORTABLE_INLINE_FUNCTION std::size_t nBins() const { return nbins_; } + PORTABLE_INLINE_FUNCTION std::size_t more() const { return nbins_ - 1; } }; PORTABLE_INLINE_FUNCTION bool check_bracket(const Real ya, const Real yb) { @@ -119,11 +119,11 @@ template PORTABLE_INLINE_FUNCTION bool set_bracket(const T &f, Real &a, const Real guess, Real &b, Real &ya, const Real yg, Real &yb, const bool &verbose = false) { - constexpr int max_search_depth = 6; + constexpr std::size_t max_search_depth = 6; Real dx = b - a; - for (int level = 0; level < max_search_depth; level++) { - const int nlev = (1 << level); - for (int i = 0; i < nlev; i++) { + for (std::size_t level = 0; level < max_search_depth; level++) { + const std::size_t nlev = (1 << level); + for (std::size_t i = 0; i < nlev; i++) { const Real x = a + (i + 0.5) * dx; const Real yx = f(x); if (check_bracket(yx, yg)) { @@ -158,7 +158,7 @@ PORTABLE_INLINE_FUNCTION Status regula_falsi(const T &f, const Real ytarget, Real &xroot, const RootCounts *counts = nullptr, const bool &verbose = false) { - constexpr int max_iter = SECANT_NITER_MAX; + constexpr std::size_t max_iter = SECANT_NITER_MAX; auto func = [&](const Real x) { return f(x) - ytarget; }; Real ya = func(a); Real yg = func(guess); @@ -187,9 +187,9 @@ PORTABLE_INLINE_FUNCTION Status regula_falsi(const T &f, const Real ytarget, ya *= sign; yb *= sign; - int b1 = 0; - int b2 = 0; - int iteration_count = 0; + std::size_t b1 = 0; + std::size_t b2 = 0; + std::size_t iteration_count = 0; while (b - a > 2.0 * xtol && (std::abs(ya) > ytol || std::abs(yb) > ytol) && iteration_count < max_iter) { Real c = (a * yb - b * ya) / (yb - ya); @@ -251,7 +251,7 @@ PORTABLE_INLINE_FUNCTION Status newton_raphson(const T &f, const Real ytarget, const bool &verbose = false, const bool &fail_on_bound_root = true) { - constexpr int max_iter = NEWTON_RAPHSON_NITER_MAX; + constexpr std::size_t max_iter = NEWTON_RAPHSON_NITER_MAX; Real _x = guess; Real _xold = 0.0; auto status = Status::SUCCESS; @@ -259,7 +259,7 @@ PORTABLE_INLINE_FUNCTION Status newton_raphson(const T &f, const Real ytarget, Real yg; Real dfunc; - int iter; + std::size_t iter; for (iter = 0; iter < max_iter; iter++) { std::tie(yg, dfunc) = f(_x); // C++11 tuple unpacking @@ -383,7 +383,7 @@ PORTABLE_INLINE_FUNCTION Status secant(const T &f, const Real ytarget, const Rea Real x_last, y, yp, ym, dyNum, dyDen, dy; Real x = xguess; - unsigned int iter{0}; + std::size_t iter{0}; for (iter = 0; iter < SECANT_NITER_MAX; ++iter) { x_last = x; dx = fabs(1.e-7 * x) + xtol; @@ -490,7 +490,7 @@ PORTABLE_INLINE_FUNCTION Status bisect(const T &f, const Real ytarget, const Rea x += 2. * xtol; } // do { // Try to find reasonable region for bisection - for (int i{0}; i < BISECT_REG_MAX; ++i) { + for (std::size_t i{0}; i < BISECT_REG_MAX; ++i) { dx = fabs(grow * x); xl = x - dx; xr = x + dx; @@ -547,10 +547,10 @@ PORTABLE_INLINE_FUNCTION Status bisect(const T &f, const Real ytarget, const Rea "\til = %.10e\n" "\tir = %.10e\n", xguess, ytarget, xl, xr, fl, fr, il, ir); - int nx = 300; + std::size_t nx = 300; Real dx = (xmax - xmin) / (nx - 1); fprintf(stderr, "Area map:\nx\ty\n"); - for (int i = 0; i < nx; i++) { + for (std::size_t i = 0; i < nx; i++) { fprintf(stderr, "%.4f\t%.4e\n", x + i * dx, f(x + i * dx)); } #endif @@ -559,7 +559,7 @@ PORTABLE_INLINE_FUNCTION Status bisect(const T &f, const Real ytarget, const Rea } } - for (int i{0}; i < BISECT_NITER_MAX; ++i) { + for (std::size_t i{0}; i < BISECT_NITER_MAX; ++i) { Real xm = 0.5 * (xl + xr); Real fm = f(xm) - ytarget; if (fl * fm <= 0) { diff --git a/singularity-eos/eos/eos_base.hpp b/singularity-eos/eos/eos_base.hpp index 6e838d108d..83d0bf03a9 100644 --- a/singularity-eos/eos/eos_base.hpp +++ b/singularity-eos/eos/eos_base.hpp @@ -41,7 +41,7 @@ constexpr std::size_t MAX_NUM_CHARS = 121; // Cuda doesn't have strcat, so we implement it ourselves PORTABLE_FORCEINLINE_FUNCTION char *StrCat(char *destination, const char *source) { - int i, j; // not in loops because they're re-used. + std::size_t i, j; // not in loops because they're re-used. // specifically avoid strlen, which isn't on GPU for (i = 0; destination[i] != '\0'; i++) { @@ -49,9 +49,10 @@ char *StrCat(char *destination, const char *source) { // assumes destination has enough memory allocated for (j = 0; source[j] != '\0'; j++) { // MAX_NUM_CHARS-1 to leave room for null terminator - PORTABLE_REQUIRE((i + j) < MAX_NUM_CHARS - 1, + std::size_t ipj = i + j; + PORTABLE_REQUIRE(ipj < MAX_NUM_CHARS - 1, "Concat string must be within allowed size"); - destination[i + j] = source[j]; + destination[ipj] = source[j]; } // null terminate destination string destination[i + j] = '\0'; diff --git a/singularity-eos/eos/get_sg_eos_p_t.cpp b/singularity-eos/eos/get_sg_eos_p_t.cpp index befe6ff0a7..8b0857edf5 100644 --- a/singularity-eos/eos/get_sg_eos_p_t.cpp +++ b/singularity-eos/eos/get_sg_eos_p_t.cpp @@ -40,7 +40,7 @@ void get_sg_eos_p_t(const char *name, int ncell, int nmat, indirection_v &offset const int32_t token{tokens.acquire()}; const int32_t tid{small_loop ? iloop : token}; // need to initialize the scratch before it's used to avoid undefined behavior - for (int idx = 0; idx < solver_scratch.extent(1); ++idx) { + for (std::size_t idx = 0; idx < solver_scratch.extent(1); ++idx) { solver_scratch(tid, idx) = 0.0; } // caching mechanism diff --git a/singularity-eos/eos/get_sg_eos_rho_e.cpp b/singularity-eos/eos/get_sg_eos_rho_e.cpp index 8501c4cde8..29a6516caf 100644 --- a/singularity-eos/eos/get_sg_eos_rho_e.cpp +++ b/singularity-eos/eos/get_sg_eos_rho_e.cpp @@ -41,7 +41,7 @@ void get_sg_eos_rho_e(const char *name, int ncell, indirection_v &offsets_v, // initialize values for solver / lookup i_func(i, tid, mass_sum, npte, vfrac_sum, 0.0, 1.0, 0.0); // need to initialize the scratch before it's used to avoid undefined behavior - for (int idx = 0; idx < solver_scratch.extent(1); ++idx) { + for (std::size_t idx = 0; idx < solver_scratch.extent(1); ++idx) { solver_scratch(tid, idx) = 0.0; } // get cache from offsets into scratch diff --git a/singularity-eos/eos/get_sg_eos_rho_p.cpp b/singularity-eos/eos/get_sg_eos_rho_p.cpp index a6c5a8ea4a..65400ae7b9 100644 --- a/singularity-eos/eos/get_sg_eos_rho_p.cpp +++ b/singularity-eos/eos/get_sg_eos_rho_p.cpp @@ -43,7 +43,7 @@ void get_sg_eos_rho_p(const char *name, int ncell, indirection_v &offsets_v, i_func(i, tid, mass_sum, npte, vfrac_sum, 0.0, 0.0, 1.0); Real sie_tot_true{0.0}; // need to initialize the scratch before it's used to avoid undefined behavior - for (int idx = 0; idx < solver_scratch.extent(1); ++idx) { + for (std::size_t idx = 0; idx < solver_scratch.extent(1); ++idx) { solver_scratch(tid, idx) = 0.0; } const int neq = npte + 1; diff --git a/singularity-eos/eos/get_sg_eos_rho_t.cpp b/singularity-eos/eos/get_sg_eos_rho_t.cpp index 9735a4c1a1..692e76b409 100644 --- a/singularity-eos/eos/get_sg_eos_rho_t.cpp +++ b/singularity-eos/eos/get_sg_eos_rho_t.cpp @@ -44,7 +44,7 @@ void get_sg_eos_rho_t(const char *name, int ncell, indirection_v &offsets_v, // calculate pte condition (lookup for 1 mat cell) Real sie_tot_true{0.0}; // need to initialize the scratch before it's used to avoid undefined behavior - for (int idx = 0; idx < solver_scratch.extent(1); ++idx) { + for (std::size_t idx = 0; idx < solver_scratch.extent(1); ++idx) { solver_scratch(tid, idx) = 0.0; } const int neq = npte; diff --git a/singularity-eos/eos/modifiers/eos_unitsystem.hpp b/singularity-eos/eos/modifiers/eos_unitsystem.hpp index cb98a373ae..e69092bf8c 100644 --- a/singularity-eos/eos/modifiers/eos_unitsystem.hpp +++ b/singularity-eos/eos/modifiers/eos_unitsystem.hpp @@ -34,12 +34,15 @@ namespace singularity { using namespace eos_base; // tag dispatch for constructors for UnitSystem +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wunused-variable" namespace eos_units_init { static struct ThermalUnitsInit { } thermal_units_init_tag; static struct LengthTimeUnitsInit { } length_time_units_init_tag; } // namespace eos_units_init +#pragma GCC diagnostic pop template class UnitSystem : public EosBase> { From 1d938cf931774f4d89816f63c10353a711a6854a Mon Sep 17 00:00:00 2001 From: Jonah Miller Date: Fri, 29 Nov 2024 13:56:41 -0700 Subject: [PATCH 12/13] switch to [[maybe_unused]] because were on C++17 --- singularity-eos/eos/modifiers/eos_unitsystem.hpp | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/singularity-eos/eos/modifiers/eos_unitsystem.hpp b/singularity-eos/eos/modifiers/eos_unitsystem.hpp index e69092bf8c..53d9435aab 100644 --- a/singularity-eos/eos/modifiers/eos_unitsystem.hpp +++ b/singularity-eos/eos/modifiers/eos_unitsystem.hpp @@ -34,15 +34,12 @@ namespace singularity { using namespace eos_base; // tag dispatch for constructors for UnitSystem -#pragma GCC diagnostic push -#pragma GCC diagnostic ignored "-Wunused-variable" namespace eos_units_init { -static struct ThermalUnitsInit { +[[maybe_unused]] static struct ThermalUnitsInit { } thermal_units_init_tag; -static struct LengthTimeUnitsInit { +[[maybe_unused]] static struct LengthTimeUnitsInit { } length_time_units_init_tag; } // namespace eos_units_init -#pragma GCC diagnostic pop template class UnitSystem : public EosBase> { From 596b08626ecf37bfbabad66a73b3c525ee057d63 Mon Sep 17 00:00:00 2001 From: Jonah Miller Date: Mon, 2 Dec 2024 16:49:08 -0700 Subject: [PATCH 13/13] more descriptive comment --- singularity-eos/eos/get_sg_eos_rho_e.cpp | 6 +++--- singularity-eos/eos/get_sg_eos_rho_t.cpp | 5 +++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/singularity-eos/eos/get_sg_eos_rho_e.cpp b/singularity-eos/eos/get_sg_eos_rho_e.cpp index 29a6516caf..e9fe4a3d0e 100644 --- a/singularity-eos/eos/get_sg_eos_rho_e.cpp +++ b/singularity-eos/eos/get_sg_eos_rho_e.cpp @@ -54,9 +54,9 @@ void get_sg_eos_rho_e(const char *name, int ncell, indirection_v &offsets_v, // eos accessor singularity::EOSAccessor_ eos_inx(eos_v, &pte_idxs(tid, 0)); // reset inputs - // JMM: Address sanitizer likes these named. My guess is - // that the forwarding references are not being resolved - // properly. + // JMM: The solver constructor is (deep under the hood) + // capturing by reference. So to avoid out-of-scope access, + // these must be "anchored" at caller scope. Real *prho_pte = &rho_pte(tid, 0); Real *pvfrac_pte = &vfrac_pte(tid, 0); Real *psie_pte = &sie_pte(tid, 0); diff --git a/singularity-eos/eos/get_sg_eos_rho_t.cpp b/singularity-eos/eos/get_sg_eos_rho_t.cpp index 692e76b409..f7bc3cf8b8 100644 --- a/singularity-eos/eos/get_sg_eos_rho_t.cpp +++ b/singularity-eos/eos/get_sg_eos_rho_t.cpp @@ -55,8 +55,9 @@ void get_sg_eos_rho_t(const char *name, int ncell, indirection_v &offsets_v, // create solver lambda // eos accessor singularity::EOSAccessor_ eos_inx(eos_v, &pte_idxs(tid, 0)); - // JMM: Address sanitizer does not like passing these in - // through the function call. + // JMM: The solver constructor is (deep under the hood) + // capturing by reference. So to avoid out-of-scope access, + // these must be "anchored" at caller scope. Real *prho_pte = &rho_pte(tid, 0); Real *pvfrac_pte = &vfrac_pte(tid, 0); Real *psie_pte = &sie_pte(tid, 0);