Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 20 commits into from
Feb 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .github/workflows/test_cc.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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:
Expand All @@ -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
Expand Down
2 changes: 1 addition & 1 deletion doc/inference/cxx.md
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ int main(){
free(v);
free(ae);
free(av);
free(dp);
DP_DeleteDeepPot(dp);
}
```

Expand Down
2 changes: 1 addition & 1 deletion examples/infer_water/infer_water.c
Original file line number Diff line number Diff line change
Expand Up @@ -32,5 +32,5 @@ int main() {
free(v);
free(ae);
free(av);
free(dp);
DP_DeleteDeepPot(dp);
}
35 changes: 35 additions & 0 deletions source/api_c/include/c_api.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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)
Expand Down
51 changes: 43 additions & 8 deletions source/api_c/include/deepmd.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);
}
Expand All @@ -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.
Expand All @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand All @@ -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.
Expand Down Expand Up @@ -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.
Expand All @@ -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.
Expand Down
10 changes: 10 additions & 0 deletions source/api_c/src/c_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {}

Expand All @@ -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)
Expand All @@ -137,6 +145,8 @@ DP_DipoleChargeModifier* DP_NewDipoleChargeModifierWithParam(
return new_dcm;)
}

void DP_DeleteDipoleChargeModifier(DP_DipoleChargeModifier* dcm) { delete dcm; }

} // extern "C"

template <typename VALUETYPE>
Expand Down
32 changes: 30 additions & 2 deletions source/api_c/tests/test_deeppot_a.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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_;
}
Loading