From 567ae65e5b43469b67d0296e73fc29a9ba85ab26 Mon Sep 17 00:00:00 2001 From: milisav Date: Fri, 12 Jan 2024 16:21:00 +0100 Subject: [PATCH 01/11] reproduce problem about move assignment within existing test case --- tests/unit/id.cpp | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/unit/id.cpp b/tests/unit/id.cpp index eb2b6daa2..c5949f67e 100644 --- a/tests/unit/id.cpp +++ b/tests/unit/id.cpp @@ -215,6 +215,23 @@ void grb_program2( const struct input &in, struct output &out ) { return; } + /* + 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 > 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 ); + } + } // NOTE: From e6d03c210955bbb533b2b2e4e6732590cd22adab Mon Sep 17 00:00:00 2001 From: Aleksa Milisavljevic Date: Fri, 12 Jan 2024 16:41:40 +0100 Subject: [PATCH 02/11] bugfix for move assignment problem --- include/graphblas/reference/matrix.hpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/include/graphblas/reference/matrix.hpp b/include/graphblas/reference/matrix.hpp index 1d2c0f5a0..0e21a9b90 100644 --- a/include/graphblas/reference/matrix.hpp +++ b/include/graphblas/reference/matrix.hpp @@ -1992,6 +1992,9 @@ namespace grb { /** \internal No implementation notes. */ SelfType& operator=( SelfType &&other ) noexcept { + if( m > 0 && n > 0 && remove_id ) { + internal::reference_mapper.remove( id ); + } moveFromOther( std::forward< SelfType >(other) ); return *this; } From b322c1c4ba7ee29c49f7e68c91f97aacc2a2086c Mon Sep 17 00:00:00 2001 From: Aleksa Milisavljevic Date: Thu, 18 Jan 2024 22:57:34 +0100 Subject: [PATCH 03/11] stronger unit test for the reproduction of move assignemnt bug --- tests/unit/id.cpp | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/tests/unit/id.cpp b/tests/unit/id.cpp index c5949f67e..8342772e5 100644 --- a/tests/unit/id.cpp +++ b/tests/unit/id.cpp @@ -217,19 +217,15 @@ void grb_program2( const struct input &in, struct output &out ) { /* 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 > new_vector; - - /* - Creating multiple new objects to generate potential id collision. + + Creating and performing move assignment on multiple new objects to generate potential id collision. */ for(int i=0;i<1000;i++) { - new_vector.emplace_back( 10, 10, 10 ); + 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); } } From 70dbdda76b7c0bd5073649fcc6e86ab0997f2317 Mon Sep 17 00:00:00 2001 From: Aleksa Milisavljevic Date: Thu, 18 Jan 2024 23:02:29 +0100 Subject: [PATCH 04/11] comment cleanup --- tests/unit/id.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/unit/id.cpp b/tests/unit/id.cpp index 8342772e5..d20ad5c67 100644 --- a/tests/unit/id.cpp +++ b/tests/unit/id.cpp @@ -222,9 +222,7 @@ void grb_program2( const struct input &in, struct output &out ) { */ for(int i=0;i<1000;i++) { 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); } From ccaba4d5425e9a4601abe0fd07f820194d69de9b Mon Sep 17 00:00:00 2001 From: Aleksa Milisavljevic Date: Fri, 9 Feb 2024 11:21:52 +0100 Subject: [PATCH 05/11] code cleanup --- tests/unit/id.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/id.cpp b/tests/unit/id.cpp index d20ad5c67..3de194f35 100644 --- a/tests/unit/id.cpp +++ b/tests/unit/id.cpp @@ -220,7 +220,7 @@ void grb_program2( const struct input &in, struct output &out ) { Creating and performing move assignment on multiple new objects to generate potential id collision. */ - for(int i=0;i<1000;i++) { + 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); From c55bb5427a5b4a85c6543aa08eecedceb6c3b4f2 Mon Sep 17 00:00:00 2001 From: Aleksa Milisavljevic Date: Fri, 9 Feb 2024 11:48:43 +0100 Subject: [PATCH 06/11] comment style --- tests/unit/id.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/unit/id.cpp b/tests/unit/id.cpp index 3de194f35..d169723f4 100644 --- a/tests/unit/id.cpp +++ b/tests/unit/id.cpp @@ -215,11 +215,11 @@ void grb_program2( const struct input &in, struct output &out ) { return; } - /* - Test for move assignement id cleanup. - - Creating and performing move assignment on multiple new objects to generate potential id collision. - */ + /** + * Test for move assignement id cleanup. + * + * Creating and performing move assignment on multiple new objects to generate potential id collision. + */ for( int i = 0; i < 1000; i++ ) { grb::Matrix< int > new_one( 10, 10, 10 ); grb::Matrix< int > new_two( 10, 10, 10 ); From 8d1ff073626c3fe33deb20e1cfa879fdf2aa635b Mon Sep 17 00:00:00 2001 From: "Albert-Jan N. Yzelman" Date: Fri, 5 Apr 2024 14:50:42 +0200 Subject: [PATCH 07/11] Fix same issue for move-construction --- include/graphblas/reference/matrix.hpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/include/graphblas/reference/matrix.hpp b/include/graphblas/reference/matrix.hpp index f00e5736f..1b56931ef 100644 --- a/include/graphblas/reference/matrix.hpp +++ b/include/graphblas/reference/matrix.hpp @@ -2072,6 +2072,9 @@ namespace grb { /** \internal No implementation notes. */ Matrix( SelfType &&other ) noexcept { + if( m > 0 && n > 0 && remove_id ) { + internal::reference_mapper.remove( id ); + } moveFromOther( std::forward< SelfType >(other) ); } From 9608343033c5b6b2bad93d29fc1ae1195a524cdb Mon Sep 17 00:00:00 2001 From: "Albert-Jan N. Yzelman" Date: Fri, 5 Apr 2024 14:50:57 +0200 Subject: [PATCH 08/11] Strengthen unit test further --- tests/unit/id.cpp | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/tests/unit/id.cpp b/tests/unit/id.cpp index d169723f4..070c6780e 100644 --- a/tests/unit/id.cpp +++ b/tests/unit/id.cpp @@ -218,14 +218,32 @@ void grb_program2( const struct input &in, struct output &out ) { /** * Test for move assignement id cleanup. * - * Creating and performing move assignment on multiple new objects to generate potential id collision. + * Creating and performing move assignment on multiple new objects and check + * for collisions. */ for( int i = 0; i < 1000; i++ ) { grb::Matrix< int > new_one( 10, 10, 10 ); grb::Matrix< int > new_two( 10, 10, 10 ); + if( grb::getID( new_one ) == grb::getID( new_two ) ) { + std::cerr << "\t two calls to getID on two new containers return same ID\n"; + rc = grb::FAILED; + return; + } + const size_t old_one_id = grb::getID( new_one ); + const size_t old_two_id = grb::getID( new_two ); new_two = std::move(new_one); + if( grb::getID( new_two ) == old_two_id ) { + std::cerr << "\t detected old ID was retained after a call to std::move\n"; + rc = grb::FAILED; + return; + } + if( grb::getID( new_two ) != old_one_id ) { + std::cerr << "\t detected a new ID after std::move, expected the moved-from " + << "ID to have been retained\n"; + rc = grb::FAILED; + return; + } } - } // NOTE: From 0debdee890031853baddfbcc2ae399e53ed1f8d5 Mon Sep 17 00:00:00 2001 From: "Albert-Jan N. Yzelman" Date: Fri, 5 Apr 2024 15:12:54 +0200 Subject: [PATCH 09/11] Better debug tracing --- include/graphblas/reference/matrix.hpp | 97 ++++++++++++++------------ 1 file changed, 52 insertions(+), 45 deletions(-) diff --git a/include/graphblas/reference/matrix.hpp b/include/graphblas/reference/matrix.hpp index 1b56931ef..9bdead2e8 100644 --- a/include/graphblas/reference/matrix.hpp +++ b/include/graphblas/reference/matrix.hpp @@ -51,6 +51,10 @@ #include "NonzeroWrapper.hpp" #include "forward.hpp" +#ifdef _DEBUG + #define _DEBUG_REFERENCE_MATRIX +#endif + namespace grb { @@ -347,14 +351,14 @@ namespace grb { ColGetter< ColIndexType, rndacc_iterator, populate_ccs > col_getter; if( nz < 1 ) { -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_MATRIX std::cerr << "Attempting to ingest an iterator in end position" << std::endl; #endif return RC::ILLEGAL; } if( num_cols == 0 || num_rows == 0 ) { -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_MATRIX std::cerr << "Attempting to ingest into an empty matrix" << std::endl; #endif return RC::ILLEGAL; @@ -384,7 +388,7 @@ namespace grb { : ccs_col_buffer_size / per_thread_buffer_size ) + 1; -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_MATRIX std::cout << "In populate_storage_parallel with\n" << "\tnz =" << nz << ", " << "bufferlen = " << per_thread_buffer_size << ", " @@ -446,7 +450,7 @@ namespace grb { // continue only if no thread detected an error if( global_rc == SUCCESS ) { -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_MATRIX #pragma omp single { std::cout << "after first step:" << std::endl; @@ -471,7 +475,7 @@ namespace grb { // holds the number of elements in each bucket across all threads: // - prefix_sum_buffer + (num_threads - 1) * per_thread_buffer_size -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_MATRIX #pragma omp single { std::cout << "after second step: " << std::endl; @@ -501,7 +505,7 @@ namespace grb { prefix_sum_buffer[ (num_threads - 1) * per_thread_buffer_size + i - 1 ]; } } -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_MATRIX #pragma omp single { std::cout << "after third step:" << std::endl; @@ -520,7 +524,7 @@ namespace grb { } #pragma omp barrier -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_MATRIX #pragma omp single { std::cout << "after fourth step:" << std::endl; @@ -554,7 +558,7 @@ namespace grb { std::cerr << "error while reading input values" << std::endl; return global_rc; } -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_MATRIX std::cout << "CCS/CRS before sort:" << std::endl; for( size_t s = 0; s < nz; s++ ) { std::cout << s << ": "; @@ -583,7 +587,7 @@ namespace grb { storage.col_start[ i ] = prefix_sum_buffer[ i ]; } } -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_MATRIX std::cout << "\t col_start array already fully sorted after bucket sort:\n"; for( size_t s = 0; s < ccs_col_buffer_size; s++ ) { std::cout << "\t\t" << s << ": " << storage.col_start[ s ] << "\n"; @@ -595,11 +599,11 @@ namespace grb { // In this case, a bucket stores more than one column, and we must sort each // bucket prior to generating a col_start. -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_MATRIX std::cout << "\t sorting buckets\n"; #endif assert( col_values_buffer != nullptr ); -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_MATRIX // fill is not parallel, but so is (not) the printout-- and this is debug // mode only std::fill( storage.col_start, storage.col_start + ccs_col_buffer_size, 0 ); @@ -627,7 +631,7 @@ namespace grb { if( ipsl_max == ipsl_min ) { // the rows are all empty, then done here -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_MATRIX std::cout << "-- thread " << omp_get_thread_num() << ", empty cols fill [" << previous_destination << ", " << max_col << ")" << std::endl; #endif @@ -642,13 +646,13 @@ namespace grb { NZIterator< ValType, RowIndexType, NonzeroIndexType, ColIndexType > end( storage, col_values_buffer, ipsl_max ); std::sort( begin, end ); -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_MATRIX std::cout << "-- thread " << omp_get_thread_num() <<", sort [" << previous_destination << ", " << max_col <<")\n" << ">> max_col= " << max_col << std::endl; #endif // INIT: populate initial value with existing count storage.col_start[ previous_destination ] = ipsl_min; -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_MATRIX std::cout << "thread " << omp_get_thread_num() << ", init write " << ipsl_min << " to pos " << previous_destination << std::endl; #endif @@ -665,7 +669,7 @@ namespace grb { // fill previous columns [previous_destination + 1, current_destination) // if skipped because empty if( previous_destination + 1 <= current_col ) { -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_MATRIX std::cout << "thread " << omp_get_thread_num() << ", write " << count <<" in range [" << previous_destination + 1 << " - " << current_destination << ")" << std::endl; @@ -685,7 +689,7 @@ namespace grb { // otherwise, the next thread will do it in INIT if( current_destination < max_col ) { storage.col_start[ current_destination ] = count; -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_MATRIX std::cout << "thread " << omp_get_thread_num() << ", write " << count << " to pos " << current_destination << std::endl; #endif @@ -699,7 +703,7 @@ namespace grb { // if the columns in [ previous_destination + 1, max_col ) are empty, // write the count also there, since the loop has skipped them if( previous_destination + 1 < max_col ) { -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_MATRIX std::cout << "thread " << omp_get_thread_num() << ", final write " << previous_count << " in range [" << previous_destination + 1 << ", " << max_col << ")" << std::endl; @@ -709,19 +713,19 @@ namespace grb { } } const ColIndexType last_existing_col = col_values_buffer[ nz - 1 ]; -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_MATRIX std::cout << "final offset " << last_existing_col << std::endl; #endif if( last_existing_col + 1 <= num_cols ) { -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_MATRIX std::cout << "final write " << nz << " into [" << last_existing_col + 1 <<", " << num_cols << "]" << std::endl; #endif std::fill( storage.col_start + last_existing_col + 1, storage.col_start + ccs_col_buffer_size, nz ); } -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_MATRIX std::cout << "CRS data after sorting:" << std::endl; for( size_t s = 0; s < nz; s++ ) { std::cout << s << ": "; @@ -878,7 +882,7 @@ namespace grb { // a buffer already exists large enough for fully parallel execution || existing_buf_size >= fully_parallel_buffer_els; -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_MATRIX if( is_fully_parallel ) { std::cout << "fully parallel matrix creation: no extra sorting required" << std::endl; } else { @@ -893,7 +897,7 @@ namespace grb { ? fully_parallel_buffer_size : partial_parallel_buffer_size; if( !internal::ensureReferenceBufsize< unsigned char >( bufferlen_tot ) ) { -#ifndef _DEBUG +#ifndef _DEBUG_REFERENCE_MATRIX std::cerr << "Not enough memory available for populate_storage_parallel buffer" << std::endl; #endif return RC::OUTOFMEM; @@ -1390,7 +1394,7 @@ namespace grb { const size_t rows, const size_t cols, const size_t cap_in ) { -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_MATRIX std::cerr << "\t in Matrix< reference >::initialize...\n" << "\t\t matrix size " << rows << " by " << cols << "\n" << "\t\t requested capacity " << cap_in << "\n"; @@ -1432,7 +1436,7 @@ namespace grb { (cap_in / rows == cols && (cap_in % rows > 0)) || (cap_in / cols == rows && (cap_in % cols > 0)) ) { -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_MATRIX std::cerr << "\t\t Illegal capacity requested\n"; #endif throw std::runtime_error( toString( ILLEGAL ) ); @@ -1491,7 +1495,7 @@ namespace grb { } else if( alloc_ok != SUCCESS ) { throw std::runtime_error( toString( alloc_ok ) ); } -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_MATRIX if( rows > 0 && cols > 0 ) { std::cerr << "\t\t allocations for an " << m << " by " << n << " matrix " << "have successfully completed.\n"; @@ -1504,7 +1508,7 @@ namespace grb { if( id_in != nullptr ) { assert( !remove_id ); id = *id_in; -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_MATRIX std::cerr << "\t\t inherited ID " << id << "\n"; #endif } else { @@ -1513,7 +1517,7 @@ namespace grb { reinterpret_cast< uintptr_t >(alloc[ 0 ]) ); remove_id = true; -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_MATRIX std::cerr << "\t\t assigned new ID " << id << "\n"; #endif } @@ -1765,7 +1769,7 @@ namespace grb { const fwd_iterator &_start, const fwd_iterator &_end ) { -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_MATRIX std::cout << "forward access iterator detected\n"; std::cout << "buildMatrixUnique called with " << cap << " nonzeroes.\n"; std::cout << "buildMatrixUnique: input is\n"; @@ -1809,7 +1813,7 @@ namespace grb { // make counting sort array cumulative for( size_t i = 1; i < m; ++i ) { -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_MATRIX std::cout << "There are " << CRS.col_start[ i ] << " " << "nonzeroes at row " << i << "\n"; #endif @@ -1818,7 +1822,7 @@ namespace grb { // make counting sort array cumulative for( size_t i = 1; i < n; ++i ) { -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_MATRIX std::cout << "There are " << CCS.col_start[ i ] << " " << "nonzeroes at column " << i << "\n"; #endif @@ -1830,20 +1834,20 @@ namespace grb { for( size_t k = 0; it != _end; ++k, ++it ) { const size_t crs_pos = --( CRS.col_start[ it.i() ] ); CRS.recordValue( crs_pos, false, it ); -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_MATRIX std::cout << "Nonzero " << k << ", ( " << it.i() << ", " << it.j() << " ) " << "is stored at CRS position " << static_cast< size_t >( crs_pos ) << ".\n"; #endif const size_t ccs_pos = --( CCS.col_start[ it.j() ] ); CCS.recordValue( ccs_pos, true, it ); -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_MATRIX std::cout << "Nonzero " << k << ", ( " << it.i() << ", " << it.j() << " ) " << "is stored at CCS position " << static_cast< size_t >( ccs_pos ) << ".\n"; #endif } -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_MATRIX for( size_t i = 0; i <= m; ++i ) { std::cout << "row_start[ " << i << " ] = " << CRS.col_start[ i ] << "." << std::endl; @@ -1867,7 +1871,7 @@ namespace grb { const rndacc_iterator &_end, std::random_access_iterator_tag ) { -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_MATRIX std::cout << " rnd access iterator " << '\n'; std::cout << "buildMatrixUnique called with " << cap << " nonzeroes.\n"; std::cout << "buildMatrixUnique: input is\n"; @@ -1915,7 +1919,7 @@ namespace grb { // allocate enough space RC ret = resize( nz ); if( ret != SUCCESS ) { -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_MATRIX std::cerr << "cannot resize the matrix to store the nonzero" << std::endl; #endif return ret; @@ -1924,7 +1928,7 @@ namespace grb { true, ColIndexType, RowIndexType >( n, m, nz, _start, CCS ); if( ret != SUCCESS ) { -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_MATRIX std::cerr << "cannot populate the CRS" << std::endl; #endif clear(); // we resized before, so we need to clean the memory @@ -1934,7 +1938,7 @@ namespace grb { false, RowIndexType, ColIndexType >( m, n, nz, _start, CRS ); if( ret != SUCCESS ) { -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_MATRIX std::cerr << "cannot populate the CCS" << std::endl; #endif clear(); @@ -1985,7 +1989,7 @@ namespace grb { Matrix( const size_t rows, const size_t columns, const size_t nz ) : Matrix() { -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_MATRIX std::cout << "In grb::Matrix constructor (reference, with requested " << "capacity)\n"; #endif @@ -2014,7 +2018,7 @@ namespace grb { Matrix( const size_t rows, const size_t columns ) : Matrix( rows, columns, std::max( rows, columns ) ) { -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_MATRIX std::cout << "In grb::Matrix constructor (reference, default capacity)\n"; #endif } @@ -2038,7 +2042,7 @@ namespace grb { ) : Matrix( other.m, other.n, other.cap ) { -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_MATRIX std::cerr << "In grb::Matrix (reference) copy-constructor\n" << "\t source matrix has " << other.nz << " nonzeroes\n"; #endif @@ -2072,15 +2076,15 @@ namespace grb { /** \internal No implementation notes. */ Matrix( SelfType &&other ) noexcept { - if( m > 0 && n > 0 && remove_id ) { - internal::reference_mapper.remove( id ); - } moveFromOther( std::forward< SelfType >(other) ); } /** \internal No implementation notes. */ SelfType& operator=( SelfType &&other ) noexcept { if( m > 0 && n > 0 && remove_id ) { +#ifdef _DEBUG_REFERENCE_MATRIX + std::cout << "move-assignment: removing ID " << id << "\n"; +#endif internal::reference_mapper.remove( id ); } moveFromOther( std::forward< SelfType >(other) ); @@ -2110,7 +2114,7 @@ namespace grb { * \endparblock */ ~Matrix() { -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_MATRIX std::cerr << "In ~Matrix (reference)\n" << "\t matrix is " << m << " by " << n << "\n" << "\t capacity is " << cap << "\n" @@ -2124,6 +2128,9 @@ namespace grb { } #endif if( m > 0 && n > 0 && remove_id ) { +#ifdef _DEBUG_REFERENCE_MATRIX + std::cout << "destructor: removing ID " << id << "\n"; +#endif internal::reference_mapper.remove( id ); } } @@ -2149,7 +2156,7 @@ namespace grb { RowIndexType, NonzeroIndexType >::template ConstIterator< ActiveDistribution > IteratorType; -#ifdef _DEBUG +#ifdef _DEBUG_REFERENCE_MATRIX std::cout << "In grb::Matrix::cbegin\n"; #endif return IteratorType( CRS, m, n, nz, false, s, P ); From 0dc793831ff971a4821375e923773b9aa86cfcd9 Mon Sep 17 00:00:00 2001 From: "Albert-Jan N. Yzelman" Date: Fri, 5 Apr 2024 17:37:59 +0200 Subject: [PATCH 10/11] Hotfix some more metabugs detected by our internal CI-- all trigger only when compiling with full tracing (i.e. _DEBUG) defined --- include/graphblas/bsp1d/io.hpp | 2 +- include/graphblas/bsp1d/vector.hpp | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/include/graphblas/bsp1d/io.hpp b/include/graphblas/bsp1d/io.hpp index 4767220b6..0639617e0 100644 --- a/include/graphblas/bsp1d/io.hpp +++ b/include/graphblas/bsp1d/io.hpp @@ -1494,7 +1494,7 @@ namespace grb { for( fwd_iterator it = start; it != end; ++it ) { #ifdef _BSP1D_IO_DEBUG std::cout << "\t\t\t process " << data.s << " caches nonzero at " - << *it << "\n"; + << start.i() << ", " << start.j() << "\n"; #endif // sanity check on input if( utils::check_input_coordinates( it, rows, cols ) != SUCCESS ) { diff --git a/include/graphblas/bsp1d/vector.hpp b/include/graphblas/bsp1d/vector.hpp index a97f0ed09..1cbc8c064 100644 --- a/include/graphblas/bsp1d/vector.hpp +++ b/include/graphblas/bsp1d/vector.hpp @@ -881,7 +881,7 @@ namespace grb { RC dense_synchronize( internal::Coordinates< _GRB_BSP1D_BACKEND > &global_coordinates ) const { -#ifndef NDEBUG +#if !defined NDEBUG || defined _DEBUG const auto &data = internal::grb_BSP1D.cload(); #endif assert( data.P > 1 ); @@ -923,7 +923,7 @@ namespace grb { RC array_synchronize( internal::Coordinates< _GRB_BSP1D_BACKEND > &global_coordinates ) const { -#ifndef NDEBUG +#if !defined NDEBUG || defined _DEBUG const auto &data = internal::grb_BSP1D.cload(); #endif assert( data.P > 1 ); From 1d1142bf0646387105fd1a258c3e75ab659b8bc5 Mon Sep 17 00:00:00 2001 From: "Albert-Jan N. Yzelman" Date: Fri, 5 Apr 2024 17:38:23 +0200 Subject: [PATCH 11/11] Fix the same issue for BSP1D -- here, it "only" resulted in memory leaks and never-freed IDs --- include/graphblas/bsp1d/matrix.hpp | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/include/graphblas/bsp1d/matrix.hpp b/include/graphblas/bsp1d/matrix.hpp index f285b638c..e188804d2 100644 --- a/include/graphblas/bsp1d/matrix.hpp +++ b/include/graphblas/bsp1d/matrix.hpp @@ -353,6 +353,9 @@ namespace grb { /** Implements move constructor and assign-from-temporary. */ void moveFromOther( self_type &&other ) { + // make sure we had no pointer and (thus) no ID + assert( _ptr == nullptr ); + // copy fields _id = other._id; _ptr = other._ptr; @@ -507,8 +510,8 @@ namespace grb { << "\t ID is " << _id << "\n"; #endif if( _m > 0 && _n > 0 ) { -#ifdef _DEBUG - std::cerr << "\t removing ID...\n"; +#ifdef _DEBUG_BSP1D_MATRIX + std::cerr << "\t matrix destructor: removing ID...\n"; #endif assert( _ptr != nullptr ); auto &data = internal::grb_BSP1D.load(); @@ -521,6 +524,17 @@ namespace grb { /** Assign-from-temporary. */ self_type& operator=( self_type &&other ) noexcept { + if( _m > 0 && _n > 0 ) { +#ifdef _DEBUG_BSP1D_MATRIX + std::cerr << "\t move-assignment: removing ID...\n"; +#endif + assert( _ptr != nullptr ); + auto &data = internal::grb_BSP1D.load(); + assert( _id != std::numeric_limits< uintptr_t >::max() ); + data.mapper.remove( _id ); + delete [] _ptr; + _ptr = nullptr; + } moveFromOther( std::forward< self_type >(other) ); return *this; }