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

Double free when making copies of a distributed tree #368

Closed
dalg24 opened this issue Sep 11, 2020 · 5 comments
Closed

Double free when making copies of a distributed tree #368

dalg24 opened this issue Sep 11, 2020 · 5 comments
Assignees
Labels
bug Something isn't working

Comments

@dalg24
Copy link
Contributor

dalg24 commented Sep 11, 2020

This occurred to me when reviewing ECP-copa/Cabana#295
The implicitly-declared copy constructor and copy assignment operator are problematic because they make copies of the communicator data member and when an object gets out of scope MPI_Comm_free gets called on it.

~DistributedSearchTree() { MPI_Comm_free(&_comm); }

@dalg24 dalg24 added the bug Something isn't working label Sep 11, 2020
@aprokop
Copy link
Contributor

aprokop commented Sep 11, 2020

This is not the only problem from copying communicators. If a user copies a distributed tree, then the MPI calls and requests from different trees may overlap, potentially resulting in undefined behavior and/or deadlocks.

The fix is straightforward: copy constructor and copy assignment operators must duplicate the communicator.

@masterleinad
Copy link
Collaborator

Time for https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-special-member-functions.html?

@aprokop
Copy link
Contributor

aprokop commented Sep 11, 2020

Time for https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-special-member-functions.html?

Good suggestion, but maybe overkill for now.

@masterleinad
Copy link
Collaborator

Why overkill? Isn't thinking about copy/move semantics worth it?

@aprokop
Copy link
Contributor

aprokop commented Sep 11, 2020

@masterleinad Probably not. I would rather not get distracted by possibly multiple places to get spotted by the tool at the moment. But it could be interesting to just post the results of it here just to see where we are at.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants