Skip to content

279 bug in move assignment of matrices#286

Merged
anyzelman merged 12 commits intodevelopfrom
279-bug-in-move-assignment-of-matrices
Apr 5, 2024
Merged

279 bug in move assignment of matrices#286
anyzelman merged 12 commits intodevelopfrom
279-bug-in-move-assignment-of-matrices

Conversation

@byjtew
Copy link
Collaborator

@byjtew byjtew commented Jan 31, 2024

Closes #279

@byjtew byjtew added the bug Something isn't working label Jan 31, 2024
@byjtew byjtew linked an issue Jan 31, 2024 that may be closed by this pull request
@anyzelman anyzelman added this to the v0.8 milestone Feb 5, 2024
@byjtew byjtew marked this pull request as ready for review February 9, 2024 11:03
Copy link
Member

@anyzelman anyzelman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @byjtew @aleksamilisavljevic could you have a look at the rfc? The first is easy, the second is a little bit tricky to think about-- is the suggestion ok or am i missing something?

@anyzelman
Copy link
Member

anyzelman commented Apr 5, 2024

Concept release notes:

Aleksa uncovered a bug where move-assigning matrices could result in container IDs not explicitly being removed. Since IDs are created on the basis of unique keys and the unique keys are derived from pointers, the following could (and did) happen: on creating a new matrix, a new ID is requested based on the same pointer the retained ID was also created with. This was detected by the internal mechanism keeping track of IDs, and resulted in an exception.

This MR fixes the bug by removing the ID once the container with the original pointer is destroyed via a move. Even if a subsequent allocation then happens to return the same pointer, the internal state is now clean and a new ID is successfully derived. The MR also strengthens the getID unit test to have been able to detect this issue.

The MR also includes a hotfix for an issue detected by our internal CI: on defining full debug tracing (-D_DEBUG), various BSP1D code did not compile-- herewith fixed as well.

Finally, this MR and now again fully related with the issue at hand, improves on _DEBUG tracing in bsp1d/matrix.hpp, and fixes a similar issue for the BSP1D backend: there, prior to this MR, move-assignment would result in never-deleted IDs and a small memory leak. Because of the memory leak, the bug never materialised in the same way it did for the reference backend, though nevertheless with this MR now correctly removes the ID as part of a move-assign and prevents the memory leak.

Thanks to Aleksa Milisavljevic @aleksamilisavljevic for detecting and fixing this bug!

anyzelman
anyzelman previously approved these changes Apr 5, 2024
@anyzelman
Copy link
Member

GitHub CI OK, manual unit tests with LPF and with _DEBUG OK-- will merge. Thanks @aleksamilisavljevic for contributing this one!

@anyzelman anyzelman merged commit 1cdf583 into develop Apr 5, 2024
anyzelman pushed a commit that referenced this pull request Oct 16, 2025
Prior to this MR, the implementation guarantees retention of IDs when subject to copy-assignment. However, under move-assignment, the moved-to container relinquishes its resources, taking over the resources of the moved-from container instead. This implies that the moved-to container takes over the ID of the moved-from container (while the ID, and general state, of the moved-from container becomes undefined). This issue was previously detected and fixed for ALP/GraphBLAS matrices in #279 / #286, which also introduces a unit test for preventing regressions.

This MR introduces a unit test for move-assignment of vectors, which confirmed that ALP prior to this MR had invalid behaviour re IDs when move-assigning vectors, as issue #168 indeed suspected. This MR fixes those issues, and includes two minor additional code style fixes.
GiovaGa added a commit that referenced this pull request Oct 17, 2025
Prior to this MR, the implementation guarantees retention of IDs when subject to copy-assignment. However, under move-assignment, the moved-to container relinquishes its resources, taking over the resources of the moved-from container instead. This implies that the moved-to container takes over the ID of the moved-from container (while the ID, and general state, of the moved-from container becomes undefined). This issue was previously detected and fixed for ALP/GraphBLAS matrices in #279 / #286, which also introduces a unit test for preventing regressions.

This MR introduces a unit test for move-assignment of vectors, which confirmed that ALP prior to this MR had invalid behaviour re IDs when move-assigning vectors, as issue #168 indeed suspected. This MR fixes those issues, and includes two minor additional code style fixes.
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 this pull request may close these issues.

Bug in move assignment of matrices

3 participants