Run singularity-eos through clang address sanitizer... and fix the HIP segfault! #437

Yurlungur · 2024-11-29T02:11:37Z

PR Summary

I ran singularity-eos through the clang address and undefined behavior sanitizer on my laptop. It found a few minor things like undefined variables, but it also found the source of the HIP segfault. The issue was that the constructors for the pte solvers use forwarding references, e.g.,

  template <typename EOS_t, typename Real_t, typename CReal_t, typename Lambda_t>
  PORTABLE_INLINE_FUNCTION
  PTESolverFixedT(const std::size_t nmat, EOS_t &&eos, const Real vfrac_tot,
                  const Real T_true, Real_t &&rho, Real_t &&vfrac, Real_t &&sie,
                  CReal_t &&temp, Real_t &&press, Lambda_t &&lambda, Real *scratch,
                  const MixParams &params = MixParams())

but the accessors in get_sg_eos all call the constructor by passing in accessors to 2d views, e.g., &rho_pte(tid, 0) which are located from the unique token logic used to prevent over-allocated scratch memory (for example).

The problem is that the compiler is capturing these in the function call as temprorary variables, but the perfect forwarding references are then capturing them by reference, not value, meaning they go out of scope before the solver is actually used.

The issue is resolved by explicitly declaring variables for each of the accessors passed in. e.g.,

          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<singularity::EOSAccessor_, Real *, Real **> method(
              npte, eos_inx, vfrac_sum, temp_pte(tid, 0), prho_pte,
              pvfrac_pte, psie_pte, ptemp_pte, ppress_pte,
              cache, pscratch);

We may wish to remove perfect forwarding down the line and instead insist these constructors capture by value. See #438 .

Here's the build command I used.

cmake -DSINGULARITY_FORCE_SUBMODULE_MODE=ON -DSINGULARITY_USE_KOKKOS=ON -DSINGULARITY_BUILD_TESTS=ON -DCMAKE_CXX_COMPILER=clang++-15 -DCMAKE_CXX_FLAGS="-fsanitize=address -fsanitize=undefined -Wall -fno-omit-frame-pointer" -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DCMAKE_BUILD_TYPE=Debug ..

I added this to the github-workflows as a test. In the same test, I also add a strict check for warnings. If there are any warnings in the subset of code built, the test fails. Naturally, I also went through the code and eliminated said warnings. :)

PR Checklist

Adds a test for any bugs fixed. Adds tests for new features.
Format your changes by using the make format command after configuring with cmake.
Document any new features, update documentation for changes made.
Make sure the copyright notice on any files you modified is up to date.
After creating a pull request, note it in the CHANGELOG.md file.
LANL employees: make sure tests pass both on the github CI and on the Darwin CI

If preparing for a new release, in addition please check the following:

Update the version in cmake.
Move the changes in the CHANGELOG.md file under a new header for the new release, and reset the categories.
Ensure that any when='@main' dependencies are updated to the release version in the package.py

…, clang undefined behavior sanitizer

rbberger · 2024-11-29T09:35:23Z

@Yurlungur I think the issue here is that &rho_pte(tid, 0) is a pointer, but the memory location storing that pointer is a temporary if you pass it within the function call. That temporary is then passed as reference and stored as reference. However, when the call statement completes, that temporary goes out of scope and you therefore end up with a reference to an invalid memory location that was storing the pointer value.

Yurlungur · 2024-11-29T18:27:53Z

@Yurlungur I think the issue here is that &rho_pte(tid, 0) is a pointer, but the memory location storing that pointer is a temporary if you pass it within the function call. That temporary is then passed as reference and stored as reference. However, when the call statement completes, that temporary goes out of scope and you therefore end up with a reference to an invalid memory location that was storing the pointer value.

Thanks @rbberger . Right this makes sense. If we were passing by value, this wouldn't be a problem right? Its because we use forwarding references in our accessors, which allows the compiler to bind them as references.

I wonder if this suggests that we should insist our accessors be passed by value. And we just assume reference semantics under the hood, which is how C-style arrays and views work anyway.

… clang-sanitize builds, since flang is not super well supported yet

Yurlungur

Ok I went through and cleaned up more warnings. I also added the clang-sanitizer test as a github workflow. This required adding some new cmake build options to support that. I made this test also fail if it encounters any warnings, which I think will help us catch problematic code going forward. The clang sanitizer checks address/memory errors as well as looks for undefined behavior.

I also snuck in some submodule updates (which shouldn't impact the spack builds at all).

Yurlungur · 2024-11-29T20:08:44Z

singularity-eos/eos/eos_gruneisen.hpp

@@ -359,14 +359,14 @@ PORTABLE_INLINE_FUNCTION Real Gruneisen::PressureFromDensityInternalEnergy(
 template <typename Indexer_t>
 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);


These are unused var warnings but I want to keep the vars here as a comment, for if/when we fill this in

Yurlungur · 2024-11-29T20:09:27Z

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;


Silences warning from self-assignment. But I want to leave it in as a comment.

Yurlungur · 2024-11-29T20:09:44Z

singularity-eos/eos/eos_sap_polynomial.hpp

+      : _a0(a0), _a1(a1), _a2c(a2c), _a2e(a2e), _a3(a3), _b0(b0), _b1(b1), _b2c(b2c),
+        _b2e(b2e), _b3(b3), _rho0(rho0) {


Silences warning about initializer list ordering.

Yurlungur · 2024-11-29T20:10:24Z

singularity-eos/eos/get_sg_eos_rho_e.cpp

+          // 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<singularity::EOSAccessor_, Real *, Real **> 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);


This (and similar changes) silence the segfault. :) We may want to do more, though. See issue #438

Yurlungur · 2024-11-29T20:11:49Z

CMakeLists.txt

+# 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)


I added this mainly to support building with clang, because flang is still not super well supported, and I wanted to be able to run the C++ code through clang sanitizer.

I think this is a great idea! Thanks for adding it

Yurlungur · 2024-11-29T20:12:33Z

CMakeLists.txt

@@ -590,6 +603,7 @@ endif()

 if(SINGULARITY_BUILD_TESTS)
  include(CTest)
+  enable_testing()


I don't know how tests ever ran without this. Did cmake get stricter at some point?

Yurlungur · 2024-11-29T20:12:54Z

test/test_closure_pte.cpp

-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;


Silences unused variable warning when this test is disabled.

Yurlungur · 2024-11-29T20:13:09Z

test/test_eos_ideal.cpp

-  int N_;
  Real *P_;
  Real *rho_;
  Real *sie_;
+  int N_;


Silences initializer list warning

Yurlungur · 2024-11-29T20:14:01Z

utils/kokkos

I snuck in submodule updates to more recent versions of kokkos, ports-of-call, and kokkos-kernels. Only impacts submodule mode, but the versions we were pinned to were pretty old. This also silences some warnings from ports of call. @pdmullen this should fix the warnings you guys were seeing in artemis. :)

Yurlungur · 2024-11-29T20:14:52Z

.github/workflows/sanitizer.yml

Test will fail if the sanitizer is unhappy OR if there are any warnings in the set of code the code tests. I want to be stricter about this going forward, as I think it will help us catch problems like this in the future.

Yurlungur · 2024-11-29T20:18:01Z

@dholladay00 @jhp-lanl @rbberger @mauneyc-LANL this is ready for review.

@jdolence this resolves the HIP segfault. I don't think it was going to be an issue for riot, but now we can be confident.

@pdmullen this should silence the warnings you were seeing in artemis.

Yurlungur · 2024-11-29T20:22:11Z

All tests now passing on re-git including the HIP tests that were previously failing.

Yurlungur · 2024-11-29T20:52:13Z

OK also added checks for compiler warnings from GCC, which caught a few more things, now cleaned up.

Yurlungur · 2024-11-29T20:59:23Z

.github/workflows/warnings.yml

+                  -DSINGULARITY_FORCE_SUBMODULE_MODE=ON \
+                  -DSINGULARITY_USE_KOKKOS=ON \
+                  ..
+            make -j4


No need to run the code, this is just to check for warnings.

singularity-eos/base/root-finding-1d/root_finding.hpp

Yurlungur · 2024-11-29T21:01:43Z

singularity-eos/eos/modifiers/eos_unitsystem.hpp

-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;


clang didn't care that these tags might be unused, maybe because it could tell they were for tag dispatch. But gcc did care. [maybe_unused] is a C++17 feature, so we can safely use it and assume it's compiler agnostic.

jhp-lanl

Approving with some minor comments/requests.

singularity-eos/eos/get_sg_eos_rho_e.cpp

jhp-lanl · 2024-12-02T22:09:12Z

CMakeLists.txt

+# 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)


I think this is a great idea! Thanks for adding it

CMakeLists.txt

Yurlungur · 2024-12-03T00:34:04Z

Merging this now, but still open to feedback if anyone else reviews. I can submit an MR later.

Yurlungur · 2024-12-03T00:34:11Z

Also thanks for the review @jhp-lanl !

run singularity tests through clang warnings, clang address sanitizer…

b8d4385

…, clang undefined behavior sanitizer

Yurlungur requested review from dholladay00, rbberger, mauneyc-LANL and jhp-lanl November 29, 2024 02:11

Yurlungur self-assigned this Nov 29, 2024

jonahm-LANL added 6 commits November 29, 2024 12:45

sneak in kokkos update in submodules

49bb334

clean up some more warnings clang found

27b3a8a

add ability to build fortran backend code without fortran. Useful for…

ec77b9d

… clang-sanitize builds, since flang is not super well supported yet

enable_testing was missing... how did tests ever run????

520f1dc

add sanitizer test

5d053c7

rename sanitizer

ac4cf33

Yurlungur linked an issue Nov 29, 2024 that may be closed by this pull request

upgrade kokkos and kokkos kernels submodules #436

Closed

Yurlungur mentioned this pull request Nov 29, 2024

Should we remove forwarding references from PTE solvers? #438

Open

jonahm-LANL added 2 commits November 29, 2024 13:04

Add new build flags

75519fa

correction

07684af

Yurlungur commented Nov 29, 2024

View reviewed changes

Yurlungur requested review from jdolence and pdmullen November 29, 2024 20:18

Yurlungur added bug Something isn't working build Something to do with the build system clean-up Changes that don't affect code execution but make the code cleaner Testing Additions/changes to the testing infrastruture labels Nov 29, 2024

jonahm-LANL added 2 commits November 29, 2024 13:50

changelog

20e9661

add check for warnings on gcc as well as clang

5b90563

switch to [[maybe_unused]] because were on C++17

1d938cf

Yurlungur commented Nov 29, 2024

View reviewed changes

jhp-lanl approved these changes Dec 2, 2024

View reviewed changes

more descriptive comment

596b086

Yurlungur merged commit 668d71a into main Dec 3, 2024
8 checks passed

Yurlungur deleted the jmm/sanitize branch December 3, 2024 00:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run singularity-eos through clang address sanitizer... and fix the HIP segfault! #437

Run singularity-eos through clang address sanitizer... and fix the HIP segfault! #437

Yurlungur commented Nov 29, 2024 •

edited

Loading

rbberger commented Nov 29, 2024 •

edited

Loading

Yurlungur commented Nov 29, 2024

Yurlungur left a comment

Yurlungur Nov 29, 2024

jhp-lanl Dec 2, 2024

Yurlungur Nov 29, 2024

Yurlungur Nov 29, 2024

Yurlungur Nov 29, 2024

Yurlungur Nov 29, 2024

jhp-lanl Dec 2, 2024

Yurlungur Nov 29, 2024

Yurlungur Nov 29, 2024

Yurlungur Nov 29, 2024

Yurlungur Nov 29, 2024

Yurlungur Nov 29, 2024

Yurlungur commented Nov 29, 2024

Yurlungur commented Nov 29, 2024

Yurlungur commented Nov 29, 2024

Yurlungur Nov 29, 2024

Yurlungur Nov 29, 2024

jhp-lanl left a comment

jhp-lanl Dec 2, 2024

Yurlungur commented Dec 3, 2024

Yurlungur commented Dec 3, 2024

		: _a0(a0), _a1(a1), _a2c(a2c), _a2e(a2e), _a3(a3), _b0(b0), _b1(b1), _b2c(b2c),
		_b2e(b2e), _b3(b3), _rho0(rho0) {

Run singularity-eos through clang address sanitizer... and fix the HIP segfault! #437

Run singularity-eos through clang address sanitizer... and fix the HIP segfault! #437

Conversation

Yurlungur commented Nov 29, 2024 • edited Loading

PR Summary

PR Checklist

rbberger commented Nov 29, 2024 • edited Loading

Yurlungur commented Nov 29, 2024

Yurlungur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yurlungur commented Nov 29, 2024

Yurlungur commented Nov 29, 2024

Yurlungur commented Nov 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhp-lanl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yurlungur commented Dec 3, 2024

Yurlungur commented Dec 3, 2024

Yurlungur commented Nov 29, 2024 •

edited

Loading

rbberger commented Nov 29, 2024 •

edited

Loading