Merge pull request #1286 from LLNL/bugfix/kweiss/lsan-fixes

Fixes memory leaks in axom

RELEASE-NOTES.md

@@ -66,6 +66,7 @@ The Axom project release numbers follow [Semantic Versioning](http://semver.org/
 - Fixed a bug when loading Sidre groups with attributes that already exist
 - Fixed `std::locale` error when when compiling `src/axom/core/utilities/System.cpp` using nvcc
 - Include `cstdint` for higher gcc version support (e.g. gcc-13)
+- Fixed several memory leaks in `axom::Array`, `quest::Shaping` and `sidre::MFEMSidreDataCollection`
 
 ## [Version 0.8.1] - Release date 2023-08-16
 

src/axom/core/Array.hpp

@@ -257,23 +257,24 @@ class Array : public ArrayBase<T, DIM, Array<T, DIM, SPACE>>
   {
     if(this != &other)
     {
+      this->clear();
       static_cast<ArrayBase<T, DIM, Array<T, DIM, SPACE>>&>(*this) = other;
       m_allocator_id = other.m_allocator_id;
       m_resize_ratio = other.m_resize_ratio;
-      initialize(other.size(), other.capacity());
-      // Use fill_range to ensure that copy constructors are invoked for each
-      // element.
+      setCapacity(other.capacity());
+      // Use fill_range to ensure that copy constructors are invoked for each element
       MemorySpace srcSpace = SPACE;
       if(srcSpace == MemorySpace::Dynamic)
       {
         srcSpace = axom::detail::getAllocatorSpace(other.m_allocator_id);
       }
       OpHelper::fill_range(m_data,
                            0,
-                           m_num_elements,
+                           other.size(),
                            m_allocator_id,
                            other.data(),
                            srcSpace);
+      updateNumElements(other.size());
     }
 
     return *this;
@@ -286,6 +287,7 @@ class Array : public ArrayBase<T, DIM, Array<T, DIM, SPACE>>
   {
     if(this != &other)
     {
+      this->clear();
       if(m_data != nullptr)
       {
         axom::deallocate(m_data);
@@ -1473,15 +1475,15 @@ inline void Array<T, DIM, SPACE>::initialize_from_other(
 #endif
     m_allocator_id = axom::detail::getAllocatorID<SPACE>();
   }
-  initialize(num_elements, num_elements);
-  // Use fill_range to ensure that copy constructors are invoked for each
-  // element.
+  this->setCapacity(num_elements);
+  // Use fill_range to ensure that copy constructors are invoked for each element
   OpHelper::fill_range(m_data,
                        0,
-                       m_num_elements,
+                       num_elements,
                        m_allocator_id,
                        other_data,
                        other_data_space);
+  this->updateNumElements(num_elements);
 }
 
 //------------------------------------------------------------------------------

src/axom/core/numerics/Matrix.hpp

@@ -4,15 +4,15 @@
 // SPDX-License-Identifier: (BSD-3-Clause)
 
 #include "axom/config.hpp"
-#include "axom/core/utilities/Utilities.hpp"  // for utilities::swap()
-#include "axom/core/memory_management.hpp"    // for alloc(), free()
+#include "axom/core/utilities/Utilities.hpp"
+#include "axom/core/memory_management.hpp"
 
 #include "axom/fmt.hpp"
 
 // C/C++ includes
-#include <cassert>   // for assert()
-#include <cstring>   // for memcpy()
-#include <iostream>  // for std::ostream
+#include <cassert>
+#include <cstring>
+#include <iostream>
 
 #ifndef AXOM_MATRIX_HPP_
   #define AXOM_MATRIX_HPP_
@@ -39,6 +39,25 @@ class Matrix;
 template <typename T>
 std::ostream& operator<<(std::ostream& os, const Matrix<T>& A);
 
+/*!
+ * \brief Checks if two matrices are component-wise equal
+ *
+ * \param [in] lhs first matrix
+ * \param [in] rhs second matrix
+ * \return status true if lhs==rhs, otherwise, false.
+ */
+template <typename T>
+bool operator==(const Matrix<T>& lhs, const Matrix<T>& rhs);
+
+/*!
+ * \brief Checks if two matrices are not component-wise equal
+ *
+ * \param [in] lhs first matrix
+ * \param [in] rhs second matrix
+ * \return status true if lhs!=rhs, otherwise, false.
+ */
+template <typename T>
+bool operator!=(const Matrix<T>& lhs, const Matrix<T>& rhs);
 /// @}
 
 /// \name Matrix Operators
@@ -949,6 +968,37 @@ AXOM_HOST_DEVICE void Matrix<T>::clear()
 // FREE METHODS
 //-----------------------------------------------------------------------------
 
+template <typename T>
+bool operator==(const Matrix<T>& lhs, const Matrix<T>& rhs)
+{
+  const int R = lhs.getNumRows();
+  const int C = lhs.getNumColumns();
+
+  if(R != rhs.getNumRows() || C != rhs.getNumColumns())
+  {
+    return false;
+  }
+
+  using axom::utilities::isNearlyEqual;
+  constexpr double EPS = 1e-12;
+
+  for(int i = 0; i < R * C; ++i)
+  {
+    if(!isNearlyEqual(lhs.data()[i], rhs.data()[i], EPS))
+    {
+      return false;
+    }
+  }
+
+  return true;
+}
+
+template <typename T>
+bool operator!=(const Matrix<T>& lhs, const Matrix<T>& rhs)
+{
+  return !(lhs == rhs);
+}
+
 //-----------------------------------------------------------------------------
 template <typename T>
 Matrix<T> lower_triangular(const Matrix<T>& A, bool unit_diagonal)

src/axom/core/tests/core_array.hpp

@@ -2359,3 +2359,103 @@ TEST(core_array, reserve_nontrivial_reloc_um)
   }
 }
 #endif
+
+// Regression test for a memory leak in axom::Array's copy/move/initialization
+template <typename T1, typename T2>
+auto make_arr_data(int SZ1, int SZ2)
+  -> std::pair<axom::Array<axom::Array<T1>>, axom::Array<axom::Array<T2>>>
+{
+  using Arr1 = axom::Array<axom::Array<T1>>;
+  using Arr2 = axom::Array<axom::Array<T2>>;
+
+  T1 val1 {};
+  T2 val2 {};
+
+  Arr1 arr1(SZ1, 2 * SZ1);
+  for(int i = 0; i < SZ1; ++i)
+  {
+    arr1[i].resize(i + 10);
+    arr1[i].shrink();
+    arr1[i].push_back(val1);
+  }
+
+  Arr2 arr2(SZ2, 2 * SZ2);
+  for(int i = 0; i < SZ2; ++i)
+  {
+    arr2[i].resize(i + 10);
+    arr2[i].shrink();
+    arr2[i].push_back(val2);
+  }
+  arr2.shrink();
+
+  return {std::move(arr1), std::move(arr2)};
+}
+
+TEST(core_array, regression_array_move)
+{
+  using T1 = int;
+  using Arr1 = axom::Array<axom::Array<T1>>;
+
+  using T2 = NonTriviallyRelocatable;
+  using Arr2 = axom::Array<axom::Array<T2>>;
+
+  using PArrArr = std::pair<Arr1, Arr2>;
+
+  constexpr int SZ1 = 20;
+  constexpr int SZ2 = 30;
+
+  {
+    PArrArr pr;
+    pr = make_arr_data<T1, T2>(SZ1, SZ2);
+
+    EXPECT_EQ(SZ1, pr.first.size());
+    EXPECT_EQ(2 * SZ1, pr.first.capacity());
+
+    EXPECT_EQ(SZ2, pr.second.size());
+    EXPECT_EQ(SZ2, pr.second.capacity());
+  }
+
+  {
+    auto pr = make_arr_data<T1, T2>(SZ1, SZ2);
+
+    EXPECT_EQ(SZ1, pr.first.size());
+    EXPECT_EQ(2 * SZ1, pr.first.capacity());
+
+    EXPECT_EQ(SZ2, pr.second.size());
+    EXPECT_EQ(SZ2, pr.second.capacity());
+  }
+
+  {
+    auto pr = make_arr_data<T1, T2>(SZ1, SZ2);
+    EXPECT_EQ(SZ1, pr.first.size());
+    EXPECT_EQ(2 * SZ1, pr.first.capacity());
+    EXPECT_EQ(SZ2, pr.second.size());
+    EXPECT_EQ(SZ2, pr.second.capacity());
+
+    pr = make_arr_data<T1, T2>(SZ2, SZ1);
+    EXPECT_EQ(SZ2, pr.first.size());
+    EXPECT_EQ(2 * SZ2, pr.first.capacity());
+    EXPECT_EQ(SZ1, pr.second.size());
+    EXPECT_EQ(SZ1, pr.second.capacity());
+  }
+
+  // lots of copy and move assignments and constructions
+  {
+    auto pr = make_arr_data<T1, T2>(SZ1, SZ2);
+    pr = make_arr_data<T1, T2>(5, 7);
+
+    auto pr2 = std::move(pr);
+    auto pr3 = pr2;
+
+    pr2 = make_arr_data<T1, T2>(13, 17);
+
+    auto pr4 {make_arr_data<T1, T2>(33, 44)};
+
+    auto pr5(std::move(pr4));
+
+    EXPECT_EQ(33, pr5.first.size());
+    EXPECT_EQ(66, pr5.first.capacity());
+    EXPECT_EQ(44, pr5.second.size());
+    EXPECT_EQ(44, pr5.second.capacity());
+  }
+}

src/axom/core/tests/core_map.hpp

@@ -396,7 +396,11 @@ TEST(core_map, bucket_return_value)
 
     EXPECT_EQ(0, bucket.get_size());
     EXPECT_EQ(length, bucket.get_capacity());
+
+    // Note: Bucket's not intended to be used on its own; explicitly free the memory
+    axom::deallocate(bucket.m_list);
   }
+
   {
     const int length = 5;
     auto fn = [](Key i) { return i * i; };
@@ -411,6 +415,9 @@ TEST(core_map, bucket_return_value)
       EXPECT_EQ(i, node.key);
       EXPECT_EQ(fn(i), node.value);
     }
+
+    // Note: Bucket's not intended to be used on its own; explicitly free the memory
+    axom::deallocate(bucket.m_list);
   }
 
   {
@@ -427,6 +434,9 @@ TEST(core_map, bucket_return_value)
       EXPECT_EQ(i, node.key);
       EXPECT_EQ(fn(i), node.value);
     }
+
+    // Note: Bucket's not intended to be used on its own; explicitly free the memory
+    axom::deallocate(bucket.m_list);
   }
 }
 
@@ -468,4 +478,4 @@ TEST(core_map, hashmap_return_value)
       }
     }
   }
-}
+}

src/axom/core/tests/numerics_matrix.hpp

@@ -246,6 +246,85 @@ TEST(numerics_matrix, is_square)
   EXPECT_TRUE(A.isSquare());
 }
 
+//------------------------------------------------------------------------------
+TEST(numerics_matrix, equality)
+{
+  using Mtx = axom::numerics::Matrix<double>;
+  constexpr double val = 2.2;
+
+  {
+    Mtx a, b;
+    EXPECT_EQ(a, b);
+    EXPECT_EQ(b, a);
+  }
+
+  {
+    Mtx a, b(5, 7);
+    EXPECT_NE(a, b);
+
+    EXPECT_NE(b, a);
+  }
+
+  {
+    Mtx a(5, 7, val), b(5, 7, val);
+    EXPECT_EQ(a, b);
+    EXPECT_EQ(b, a);
+
+    a(3, 4) = 1;
+    EXPECT_NE(a, b);
+    EXPECT_NE(b, a);
+  }
+
+  {
+    Mtx a = Mtx::identity(3);
+    Mtx b = Mtx::identity(4);
+    EXPECT_NE(a, b);
+    EXPECT_NE(b, a);
+  }
+
+  {
+    Mtx a = Mtx::identity(3);
+    Mtx b = Mtx::identity(3);
+    EXPECT_EQ(a, b);
+    EXPECT_EQ(b, a);
+  }
+
+  {
+    Mtx a = Mtx::zeros(3, 3);
+    Mtx b = Mtx::ones(3, 3);
+    EXPECT_NE(a, b);
+    EXPECT_NE(b, a);
+  }
+
+  {
+    Mtx a = Mtx::identity(3);
+    Mtx b = Mtx::zeros(3, 3);
+    b(0, 0) = 1.;
+    b(1, 1) = 1.;
+    b(2, 2) = 1.;
+
+    EXPECT_EQ(a, b);
+    EXPECT_EQ(b, a);
+  }
+
+  {
+    Mtx a(7, 5);
+    Mtx b(5, 7);
+    EXPECT_NE(a, b);
+    EXPECT_NE(b, a);
+
+    a(3, 4) = val;
+    b(4, 3) = val;
+    EXPECT_NE(a, b);
+    EXPECT_NE(b, a);
+
+    Mtx a_tr(5, 7);
+    axom::numerics::matrix_transpose(a, a_tr);
+    EXPECT_EQ(a_tr, b);
+    EXPECT_EQ(b, a_tr);
+  }
+}
+
 //------------------------------------------------------------------------------
 TEST(numerics_matrix, random_access_operators)
 {

src/axom/inlet/VariantKey.hpp

@@ -126,7 +126,7 @@ class VariantKey
 
   // Integer and string keys
   // With only two possible types a union is overkill
-  int m_int;
+  int m_int {};
   std::string m_string;
 
   // Active key type