Bug in move assignment of matrices

[aleksamilisavljevic]

Using move assignment on matrices doesn't do a cleanup of a previous id. This results in id collisions.

[byjtew]

Using move assignment on matrices doesn't do a cleanup of a previous id. This results in id collisions.

Which backend?
Can you please also provide a short code to reproduce this bug ?

[aleksamilisavljevic]

Hi, the observed bug is in the reference backend.

Here is the code (unit test) that reproduces it:

	/*
	Test for move assignement id cleanup.
	*/

	grb::Matrix< int > new_one( 10, 10, 10 );
	grb::Matrix< int > new_two( 10, 10, 10 );
	new_two = std::move(new_one);

	std::vector<grb::Matrix< int > > new_vector;

	/*
	Creating multiple new objects to generate potential id collision.
	*/
	for(int i=0;i<1000;i++) {
		new_vector.emplace_back( 10, 10, 10 );
	}

[byjtew]

There is no collision in this example, simply a missing id (=1)

grb::Matrix< int > new_one( 10, 10, 10 );
std::cout << "ID of new_one: " << grb::getID(new_one) << std::endl;
grb::Matrix< int > new_two( 10, 10, 10 );
std::cout << "ID of new_two: " << grb::getID(new_two) << std::endl;
new_two = std::move(new_one);
std::cout << "ID of new_two after move assignement: " << grb::getID(new_two) << std::endl;

std::vector<grb::Matrix< int > > new_vector;
for(int i=0;i<10;i++) {
	new_vector.emplace_back( 10, 10, 10 );
	std::cout << "ID of new_vector[" << i << "]: " << grb::getID(new_vector[i]) << std::endl;
}

Output:

Info: grb::init (reference) called.
ID of new_one: 0
ID of new_two: 1
ID of new_two after move assignement: 0
ID of new_vector[0]: 2
ID of new_vector[1]: 3
ID of new_vector[2]: 4
ID of new_vector[3]: 5
ID of new_vector[4]: 6
ID of new_vector[5]: 7
ID of new_vector[6]: 8
ID of new_vector[7]: 9
ID of new_vector[8]: 10
ID of new_vector[9]: 11
Info: grb::finalize (reference) called.

[aleksamilisavljevic]

The segment of code was from the unit test. However, that segment was run multiple times. That missing id should cause a collision with a lot of iterations or multiple reruns.

This test should be stronger i.e. likelier to reproduce a bug:

	for(int i=0;i<1000;i++) {
		grb::Matrix< int > new_one( 10, 10, 10 );
		grb::Matrix< int > new_two( 10, 10, 10 );
		new_two = std::move(new_one);
	}

[byjtew]

The segment of code was from the unit test. However, that segment was run multiple times. That missing id should cause a collision with a lot of iterations or multiple reruns.

This test should be stronger i.e. likelier to reproduce a bug:

	for(int i=0;i<1000;i++) {
		grb::Matrix< int > new_one( 10, 10, 10 );
		grb::Matrix< int > new_two( 10, 10, 10 );
		new_two = std::move(new_one);
	}

@aleksamilisavljevic

I really can't see where the collision could come from with this example, the highest ID will simply be 2000 ..?

The ID being an unsigned long you would need more than ULONG_MAX iterations to overflow the IDs.

[aleksamilisavljevic]

Does this unit test work for you?

When I run it, it reports the following bug:

DMapper.hpp:97: IndexType grb::utils::DMapper<IndexType>::insert(IndexType) [with IndexType = long unsigned int]: Assertion `it == mapper.end()' failed.
Aborted (core dumped)

If I understand the code well, when creating a new matrix, a pointer is passed to the reference_mapper, which returns the id.

If this id isn't cleared during the move assignment, a collision might occur during one of the following matrix initializations (as it might pass the same pointer to the reference_mapper).

[anyzelman]

I see a branch but I don't see an MR, is that correct (i.e., a fix not yet ready), or am I missing something?

[byjtew]

I see a branch but I don't see an MR, is that correct (i.e., a fix not yet ready), or am I missing something?

Just created it.