c: fix all memory leaks; add sanitizer checks (deepmodeling#3223)

Fix deepmodeling#3045. All memory leaks have been fixed! --------- Signed-off-by: Jinzhe Zeng <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
njzjz · Feb 4, 2024 · ab2c551 · ab2c551
1 parent 412c812
commit ab2c551
Show file tree

Hide file tree

Showing 7 changed files with 127 additions and 12 deletions.
diff --git a/.github/workflows/test_cc.yml b/.github/workflows/test_cc.yml
@@ -6,6 +6,9 @@ jobs:
   testcc:
     name: Test C++
     runs-on: ubuntu-latest
+    strategy:
+      matrix:
+        check_memleak: [true, false]
     steps:
     - uses: actions/checkout@v4
     - uses: actions/setup-python@v5
@@ -31,6 +34,7 @@ jobs:
         TF_INTER_OP_PARALLELISM_THREADS: 1
         LMP_CXX11_ABI_0: 1
         CMAKE_GENERATOR: Ninja
+        CXXFLAGS: ${{ matrix.check_memleak && '-fsanitize=leak' || '' }}
     # test lammps
     # ASE issue: https://gitlab.com/ase/ase/-/merge_requests/2843
     # TODO: remove ase version when ase has new release
@@ -39,13 +43,15 @@ jobs:
         python -m pip install -e .[cpu,test,lmp] "ase @ https://gitlab.com/ase/ase/-/archive/8c5aa5fd6448c5cfb517a014dccf2b214a9dfa8f/ase-8c5aa5fd6448c5cfb517a014dccf2b214a9dfa8f.tar.gz"
       env:
         DP_BUILD_TESTING: 1
+      if: ${{ !matrix.check_memleak }}
     - run: pytest --cov=deepmd source/lmp/tests
       env:
         OMP_NUM_THREADS: 1
         TF_INTRA_OP_PARALLELISM_THREADS: 1
         TF_INTER_OP_PARALLELISM_THREADS: 1
         LAMMPS_PLUGIN_PATH: ${{ github.workspace }}/dp_test/lib/deepmd_lmp
         LD_LIBRARY_PATH: ${{ github.workspace }}/dp_test/lib
+      if: ${{ !matrix.check_memleak }}
     # test ipi
     - run: pytest --cov=deepmd source/ipi/tests
       env:
@@ -54,6 +60,7 @@ jobs:
         TF_INTER_OP_PARALLELISM_THREADS: 1
         PATH: ${{ github.workspace }}/dp_test/bin:$PATH
         LD_LIBRARY_PATH: ${{ github.workspace }}/dp_test/lib
+      if: ${{ !matrix.check_memleak }}
     - uses: codecov/codecov-action@v3
       with:
         gcov: true

diff --git a/doc/inference/cxx.md b/doc/inference/cxx.md
@@ -62,7 +62,7 @@ int main(){
   free(v);
   free(ae);
   free(av);
-  free(dp);
+  DP_DeleteDeepPot(dp);
 }
 ```
 

diff --git a/examples/infer_water/infer_water.c b/examples/infer_water/infer_water.c
@@ -32,5 +32,5 @@ int main() {
   free(v);
   free(ae);
   free(av);
-  free(dp);
+  DP_DeleteDeepPot(dp);
 }
diff --git a/source/api_c/include/c_api.h b/source/api_c/include/c_api.h
@@ -25,6 +25,13 @@ extern DP_Nlist* DP_NewNlist(int inum_,
                              int* numneigh_,
                              int** firstneigh_);
 
+/**
+ * @brief Delete a neighbor list.
+ *
+ * @param nl Neighbor list to delete.
+ */
+extern void DP_DeleteNlist(DP_Nlist* nl);
+
 /**
  * @brief Check if there is any exceptions throw.
  *
@@ -72,6 +79,13 @@ extern DP_DeepPot* DP_NewDeepPotWithParam2(const char* c_model,
                                            const char* c_file_content,
                                            const int size_file_content);
 
+/**
+ * @brief Delete a Deep Potential.
+ *
+ * @param dp Deep Potential to delete.
+ */
+extern void DP_DeleteDeepPot(DP_DeepPot* dp);
+
 /**
  * @brief Evaluate the energy, force and virial by using a DP. (double version)
  * @attention The number of frames is assumed to be 1.
@@ -491,6 +505,13 @@ extern DP_DeepPotModelDevi* DP_NewDeepPotModelDeviWithParam(
     const int n_file_contents,
     const int* size_file_contents);
 
+/**
+ * @brief Delete a Deep Potential Model Deviation.
+ *
+ * @param dp Deep Potential to delete.
+ */
+extern void DP_DeleteDeepPotModelDevi(DP_DeepPotModelDevi* dp);
+
 /**
  * @brief Evaluate the energy, force and virial by using a DP model deviation
  *with neighbor list. (double version)
@@ -792,6 +813,13 @@ extern DP_DeepTensor* DP_NewDeepTensorWithParam(const char* c_model,
                                                 const int gpu_rank,
                                                 const char* c_name_scope);
 
+/**
+ * @brief Delete a Deep Tensor.
+ *
+ * @param dp Deep Tensor to delete.
+ */
+extern void DP_DeleteDeepTensor(DP_DeepTensor* dt);
+
 /**
  * @brief Evaluate the tensor by using a DP. (double version)
  * @param[in] dt The Deep Tensor to use.
@@ -1094,6 +1122,13 @@ extern DP_DipoleChargeModifier* DP_NewDipoleChargeModifier(const char* c_model);
 extern DP_DipoleChargeModifier* DP_NewDipoleChargeModifierWithParam(
     const char* c_model, const int gpu_rank, const char* c_name_scope);
 
+/**
+ * @brief Delete a Dipole Charge Modifier.
+ *
+ * @param dp Dipole Charge Modifier to delete.
+ */
+extern void DP_DeleteDipoleChargeModifier(DP_DipoleChargeModifier* dcm);
+
 /**
  * @brief Evaluate the force and virial correction by using a dipole charge
  *modifier with the neighbor list. (double version)

diff --git a/source/api_c/include/deepmd.hpp b/source/api_c/include/deepmd.hpp
@@ -522,6 +522,7 @@ struct InputNlist {
         nl(DP_NewNlist(inum_, ilist_, numneigh_, firstneigh_)) {
     DP_CHECK_OK(DP_NlistCheckOK, nl);
   };
+  ~InputNlist() { DP_DeleteNlist(nl); };
   /// @brief C API neighbor list.
   DP_Nlist *nl;
   /// @brief Number of core region atoms
@@ -556,6 +557,8 @@ void inline convert_nlist(InputNlist &to_nlist,
     to_nlist.numneigh[ii] = from_nlist[ii].size();
     to_nlist.firstneigh[ii] = &from_nlist[ii][0];
   }
+  // delete the original nl
+  DP_DeleteNlist(to_nlist.nl);
   to_nlist.nl = DP_NewNlist(to_nlist.inum, to_nlist.ilist, to_nlist.numneigh,
                             to_nlist.firstneigh);
 }
@@ -568,7 +571,7 @@ class DeepPot {
    * @brief DP constructor without initialization.
    **/
   DeepPot() : dp(nullptr){};
-  ~DeepPot(){};
+  ~DeepPot() { DP_DeleteDeepPot(dp); };
   /**
    * @brief DP constructor with initialization.
    * @param[in] model The name of the frozen model file.
@@ -579,7 +582,15 @@ class DeepPot {
           const int &gpu_rank = 0,
           const std::string &file_content = "")
       : dp(nullptr) {
-    init(model, gpu_rank, file_content);
+    try {
+      init(model, gpu_rank, file_content);
+    } catch (...) {
+      // Clean up and rethrow, as the destructor will not be called
+      if (dp) {
+        DP_DeleteDeepPot(dp);
+      }
+      throw;
+    }
   };
   /**
    * @brief Initialize the DP.
@@ -1100,13 +1111,21 @@ class DeepPotModelDevi {
    * @brief DP model deviation constructor without initialization.
    **/
   DeepPotModelDevi() : dp(nullptr){};
-  ~DeepPotModelDevi(){};
+  ~DeepPotModelDevi() { DP_DeleteDeepPotModelDevi(dp); };
   /**
    * @brief DP model deviation constructor with initialization.
    * @param[in] models The names of the frozen model file.
    **/
   DeepPotModelDevi(const std::vector<std::string> &models) : dp(nullptr) {
-    init(models);
+    try {
+      init(models);
+    } catch (...) {
+      // Clean up and rethrow, as the destructor will not be called
+      if (dp) {
+        DP_DeleteDeepPotModelDevi(dp);
+      }
+      throw;
+    }
   };
   /**
    * @brief Initialize the DP model deviation.
@@ -1523,7 +1542,7 @@ class DeepTensor {
    * @brief Deep Tensor constructor without initialization.
    **/
   DeepTensor() : dt(nullptr){};
-  ~DeepTensor(){};
+  ~DeepTensor() { DP_DeleteDeepTensor(dt); };
   /**
    * @brief DeepTensor constructor with initialization.
    * @param[in] model The name of the frozen model file.
@@ -1532,7 +1551,15 @@ class DeepTensor {
              const int &gpu_rank = 0,
              const std::string &name_scope = "")
       : dt(nullptr) {
-    init(model, gpu_rank, name_scope);
+    try {
+      init(model, gpu_rank, name_scope);
+    } catch (...) {
+      // Clean up and rethrow, as the destructor will not be called
+      if (dt) {
+        DP_DeleteDeepTensor(dt);
+      }
+      throw;
+    }
   };
   /**
    * @brief Initialize the DeepTensor.
@@ -1891,7 +1918,7 @@ class DipoleChargeModifier {
    * @brief DipoleChargeModifier constructor without initialization.
    **/
   DipoleChargeModifier() : dcm(nullptr){};
-  ~DipoleChargeModifier(){};
+  ~DipoleChargeModifier() { DP_DeleteDipoleChargeModifier(dcm); };
   /**
    * @brief DipoleChargeModifier constructor with initialization.
    * @param[in] model The name of the frozen model file.
@@ -1902,7 +1929,15 @@ class DipoleChargeModifier {
                        const int &gpu_rank = 0,
                        const std::string &name_scope = "")
       : dcm(nullptr) {
-    init(model, gpu_rank, name_scope);
+    try {
+      init(model, gpu_rank, name_scope);
+    } catch (...) {
+      // Clean up and rethrow, as the destructor will not be called
+      if (dcm) {
+        DP_DeleteDipoleChargeModifier(dcm);
+      }
+      throw;
+    }
   };
   /**
    * @brief Initialize the DipoleChargeModifier.

diff --git a/source/api_c/src/c_api.cc b/source/api_c/src/c_api.cc
@@ -25,6 +25,8 @@ DP_Nlist* DP_NewNlist(int inum_,
             DP_Nlist* new_nl = new DP_Nlist(nl); return new_nl;)
 }
 
+void DP_DeleteNlist(DP_Nlist* nl) { delete nl; }
+
 DP_DeepPot::DP_DeepPot() {}
 DP_DeepPot::DP_DeepPot(deepmd::DeepPot& dp) : dp(dp) {
   dfparam = dp.dim_fparam();
@@ -61,6 +63,8 @@ DP_DeepPot* DP_NewDeepPotWithParam2(const char* c_model,
             DP_DeepPot* new_dp = new DP_DeepPot(dp); return new_dp;)
 }
 
+void DP_DeleteDeepPot(DP_DeepPot* dp) { delete dp; }
+
 DP_DeepPotModelDevi::DP_DeepPotModelDevi() {}
 DP_DeepPotModelDevi::DP_DeepPotModelDevi(deepmd::DeepPotModelDevi& dp)
     : dp(dp) {
@@ -97,6 +101,8 @@ DP_DeepPotModelDevi* DP_NewDeepPotModelDeviWithParam(
             return new_dp;)
 }
 
+void DP_DeleteDeepPotModelDevi(DP_DeepPotModelDevi* dp) { delete dp; }
+
 DP_DeepTensor::DP_DeepTensor() {}
 DP_DeepTensor::DP_DeepTensor(deepmd::DeepTensor& dt) : dt(dt) {}
 
@@ -115,6 +121,8 @@ DP_DeepTensor* DP_NewDeepTensorWithParam(const char* c_model,
             DP_DeepTensor* new_dt = new DP_DeepTensor(dt); return new_dt;)
 }
 
+void DP_DeleteDeepTensor(DP_DeepTensor* dt) { delete dt; }
+
 DP_DipoleChargeModifier::DP_DipoleChargeModifier() {}
 DP_DipoleChargeModifier::DP_DipoleChargeModifier(
     deepmd::DipoleChargeModifier& dcm)
@@ -137,6 +145,8 @@ DP_DipoleChargeModifier* DP_NewDipoleChargeModifierWithParam(
             return new_dcm;)
 }
 
+void DP_DeleteDipoleChargeModifier(DP_DipoleChargeModifier* dcm) { delete dcm; }
+
 }  // extern "C"
 
 template <typename VALUETYPE>

diff --git a/source/api_c/tests/test_deeppot_a.cc b/source/api_c/tests/test_deeppot_a.cc
@@ -86,7 +86,10 @@ class TestInferDeepPotA : public ::testing::Test {
     }
   };
 
-  void TearDown() override { remove("deeppot.pb"); };
+  void TearDown() override {
+    remove("deeppot.pb");
+    DP_DeleteDeepPot(dp);
+  };
 };
 
 TEST_F(TestInferDeepPotA, double_infer) {
@@ -119,6 +122,12 @@ TEST_F(TestInferDeepPotA, double_infer) {
   for (int ii = 0; ii < natoms * 9; ++ii) {
     EXPECT_LT(fabs(atomic_virial[ii] - expected_v[ii]), 1e-10);
   }
+
+  delete ener_;
+  delete[] force_;
+  delete[] virial_;
+  delete[] atomic_ener_;
+  delete[] atomic_virial_;
 }
 
 TEST_F(TestInferDeepPotA, float_infer) {
@@ -151,6 +160,11 @@ TEST_F(TestInferDeepPotA, float_infer) {
   for (int ii = 0; ii < natoms * 9; ++ii) {
     EXPECT_LT(fabs(atomic_virial[ii] - expected_v[ii]), 1e-6);
   }
+  delete ener_;
+  delete[] force_;
+  delete[] virial_;
+  delete[] atomic_ener_;
+  delete[] atomic_virial_;
 }
 
 TEST_F(TestInferDeepPotA, cutoff) {
@@ -253,7 +267,11 @@ class TestInferDeepPotANoPBC : public ::testing::Test {
     }
   };
 
-  void TearDown() override { remove("deeppot.pb"); };
+  void TearDown() override {
+    remove("deeppot.pb");
+
+    DP_DeleteDeepPot(dp);
+  };
 };
 
 TEST_F(TestInferDeepPotANoPBC, double_infer) {
@@ -286,6 +304,11 @@ TEST_F(TestInferDeepPotANoPBC, double_infer) {
   for (int ii = 0; ii < natoms * 9; ++ii) {
     EXPECT_LT(fabs(atomic_virial[ii] - expected_v[ii]), 1e-10);
   }
+  delete ener_;
+  delete[] force_;
+  delete[] virial_;
+  delete[] atomic_ener_;
+  delete[] atomic_virial_;
 }
 
 TEST_F(TestInferDeepPotANoPBC, float_infer) {
@@ -318,4 +341,9 @@ TEST_F(TestInferDeepPotANoPBC, float_infer) {
   for (int ii = 0; ii < natoms * 9; ++ii) {
     EXPECT_LT(fabs(atomic_virial[ii] - expected_v[ii]), 1e-6);
   }
+  delete ener_;
+  delete[] force_;
+  delete[] virial_;
+  delete[] atomic_ener_;
+  delete[] atomic_virial_;
 }