From 4c4bbdc8f3778de7d6a0b85cb1f6bd29c5c22edc Mon Sep 17 00:00:00 2001 From: Jon Evans Date: Sun, 6 Aug 2023 00:56:51 -0400 Subject: [PATCH] Stricter API for LIB_TABLE Prevent nickname map or row parent getting out of sync Fixes https://gitlab.com/kicad/code/kicad/-/issues/15315 --- common/lib_table_base.cpp | 154 ++++++++++++++++++----- eeschema/dialogs/panel_sym_lib_table.cpp | 25 +--- eeschema/symbol_lib_table.cpp | 10 +- include/lib_table_base.h | 113 +++++------------ pcbnew/dialogs/panel_fp_lib_table.cpp | 8 +- pcbnew/footprint_info_impl.cpp | 6 - 6 files changed, 168 insertions(+), 148 deletions(-) diff --git a/common/lib_table_base.cpp b/common/lib_table_base.cpp index 14b11b9115..c96332d7d3 100644 --- a/common/lib_table_base.cpp +++ b/common/lib_table_base.cpp @@ -127,6 +127,13 @@ LIB_TABLE::~LIB_TABLE() } +void LIB_TABLE::Clear() +{ + m_rows.clear(); + m_rowsMap.clear(); +} + + bool LIB_TABLE::IsEmpty( bool aIncludeFallback ) { if( !aIncludeFallback || !m_fallBack ) @@ -191,31 +198,26 @@ LIB_TABLE_ROW* LIB_TABLE::findRow( const wxString& aNickName, bool aCheckIfEnabl do { - cur->ensureIndex(); + std::shared_lock lock( cur->m_mutex, std::try_to_lock ); - std::shared_lock lock( cur->m_nickIndexMutex ); + if( !cur->m_rowsMap.count( aNickName ) ) + continue; - for( const std::pair& entry : cur->m_nickIndex ) - { - if( entry.first == aNickName ) - { - row = &cur->m_rows[entry.second]; + row = &*cur->m_rowsMap.at( aNickName ); - if( !aCheckIfEnabled || row->GetIsEnabled() ) - return row; - } - } + if( !aCheckIfEnabled || row->GetIsEnabled() ) + return row; // Repeat, this time looking for names that were "fixed" by legacy versions because // the old eeschema file format didn't support spaces in tokens. - for( const std::pair& entry : cur->m_nickIndex ) + for( const std::pair& entry : cur->m_rowsMap ) { wxString legacyLibName = entry.first; legacyLibName.Replace( " ", "_" ); if( legacyLibName == aNickName ) { - row = &cur->m_rows[entry.second]; + row = &*entry.second; if( !aCheckIfEnabled || row->GetIsEnabled() ) return row; @@ -235,9 +237,7 @@ const LIB_TABLE_ROW* LIB_TABLE::FindRowByURI( const wxString& aURI ) do { - cur->ensureIndex(); - - for( unsigned i = 0; i < cur->m_rows.size(); i++ ) + for( unsigned i = 0; i < cur->m_rows.size(); i++ ) { wxString tmp = cur->m_rows[i].GetFullURI( true ); @@ -276,10 +276,10 @@ std::vector LIB_TABLE::GetLogicalLibs() do { - for( LIB_TABLE_ROWS_CITER it = cur->m_rows.begin(); it!=cur->m_rows.end(); ++it ) + for( const LIB_TABLE_ROW& row : cur->m_rows ) { - if( it->GetIsEnabled() ) - unique.insert( it->GetNickName() ); + if( row.GetIsEnabled() ) + unique.insert( row.GetNickName() ); } } while( ( cur = cur->m_fallBack ) != nullptr ); @@ -303,28 +303,120 @@ std::vector LIB_TABLE::GetLogicalLibs() bool LIB_TABLE::InsertRow( LIB_TABLE_ROW* aRow, bool doReplace ) { - ensureIndex(); + std::lock_guard lock( m_mutex ); - std::lock_guard lock( m_nickIndexMutex ); + auto it = m_rowsMap.find( aRow->GetNickName() ); - INDEX_CITER it = m_nickIndex.find( aRow->GetNickName() ); + if( it != m_rowsMap.end() ) + { + if( !doReplace ) + return false; - aRow->SetParent( this ); - - if( it == m_nickIndex.end() ) + m_rows.replace( it->second, aRow ); + } + else { m_rows.push_back( aRow ); - m_nickIndex.insert( INDEX_VALUE( aRow->GetNickName(), m_rows.size() - 1 ) ); - return true; } - if( doReplace ) + aRow->SetParent( this ); + reindex(); + return true; +} + + +bool LIB_TABLE::RemoveRow( const LIB_TABLE_ROW* aRow ) +{ + std::lock_guard lock( m_mutex ); + + bool found = false; + auto it = m_rowsMap.find( aRow->GetNickName() ); + + if( it != m_rowsMap.end() ) { - m_rows.replace( it->second, aRow ); - return true; + if( &*it->second == aRow ) + { + found = true; + m_rows.erase( it->second ); + } } - return false; + if( !found ) + { + // Bookkeeping got messed up... + for( size_t i = m_rows.size() - 1; i >= 0; --i ) + { + if( &m_rows[i] == aRow ) + { + m_rows.erase( m_rows.begin() + i ); + found = true; + break; + } + } + } + + if( found ) + reindex(); + + return found; +} + + +bool LIB_TABLE::ReplaceRow( size_t aIndex, LIB_TABLE_ROW* aRow ) +{ + std::lock_guard lock( m_mutex ); + + if( aIndex >= m_rows.size() ) + return false; + + m_rowsMap.erase( m_rows[aIndex].GetNickName() ); + + m_rows.replace( aIndex, aRow ); + reindex(); + return true; +} + + +bool LIB_TABLE::ChangeRowOrder( size_t aIndex, int aOffset ) +{ + std::lock_guard lock( m_mutex ); + + if( aIndex >= m_rows.size() ) + return false; + + int newPos = static_cast( aIndex ) + aOffset; + + if( newPos < 0 || newPos > static_cast( m_rows.size() ) - 1 ) + return false; + + auto element = m_rows.release( m_rows.begin() + aIndex ); + + m_rows.insert( m_rows.begin() + newPos, element.release() ); + reindex(); + + return true; +} + + +void LIB_TABLE::TransferRows( LIB_TABLE_ROWS& aRowsList ) +{ + std::lock_guard lock( m_mutex ); + + m_rows.transfer( m_rows.end(), aRowsList.begin(), aRowsList.end(), aRowsList ); + + reindex(); +} + + +void LIB_TABLE::reindex() +{ + m_rowsMap.clear(); + + for( LIB_TABLE_ROWS_ITER it = m_rows.begin(); it != m_rows.end(); ++it ) + { + it->SetParent( this ); + m_rowsMap[it->GetNickName()] = it; + } } diff --git a/eeschema/dialogs/panel_sym_lib_table.cpp b/eeschema/dialogs/panel_sym_lib_table.cpp index b188a575c9..f44c2b5046 100644 --- a/eeschema/dialogs/panel_sym_lib_table.cpp +++ b/eeschema/dialogs/panel_sym_lib_table.cpp @@ -200,7 +200,7 @@ protected: tbl->AppendRows( tmp_tbl.GetCount() - tbl->GetNumberRows() ); for( unsigned i = 0; i < tmp_tbl.GetCount(); ++i ) - tbl->m_rows.replace( i, tmp_tbl.At( i ).clone() ); + tbl->ReplaceRow( i, tmp_tbl.At( i ).clone() ); } m_grid->AutoSizeColumns( false ); @@ -494,9 +494,6 @@ bool PANEL_SYM_LIB_TABLE::verifyTables() { SYMBOL_LIB_TABLE_ROW& row = dynamic_cast( table->At( r ) ); - if( !row.GetParent() ) - row.SetParent( table ); - if( !row.GetIsEnabled() ) continue; @@ -712,11 +709,7 @@ void PANEL_SYM_LIB_TABLE::moveUpHandler( wxCommandEvent& event ) // @todo: add multiple selection moves. if( curRow >= 1 ) { - boost::ptr_vector< LIB_TABLE_ROW >::auto_type move_me = - tbl->m_rows.release( tbl->m_rows.begin() + curRow ); - - --curRow; - tbl->m_rows.insert( tbl->m_rows.begin() + curRow, move_me.release() ); + tbl->ChangeRowOrder( curRow--, -1 ); if( tbl->GetView() ) { @@ -742,11 +735,7 @@ void PANEL_SYM_LIB_TABLE::moveDownHandler( wxCommandEvent& event ) // @todo: add multiple selection moves. if( unsigned( curRow + 1 ) < tbl->m_rows.size() ) { - boost::ptr_vector< LIB_TABLE_ROW >::auto_type move_me = - tbl->m_rows.release( tbl->m_rows.begin() + curRow ); - - ++curRow; - tbl->m_rows.insert( tbl->m_rows.begin() + curRow, move_me.release() ); + tbl->ChangeRowOrder( curRow++, 1 ); if( tbl->GetView() ) { @@ -934,9 +923,7 @@ bool PANEL_SYM_LIB_TABLE::TransferDataFromWindow() m_parent->m_GlobalTableChanged = true; m_globalTable->Clear(); - m_globalTable->m_rows.transfer( m_globalTable->m_rows.end(), global_model()->m_rows.begin(), - global_model()->m_rows.end(), global_model()->m_rows ); - m_globalTable->reindex(); + m_globalTable->TransferRows( global_model()->m_rows ); } if( project_model() && *project_model() != *m_projectTable ) @@ -944,9 +931,7 @@ bool PANEL_SYM_LIB_TABLE::TransferDataFromWindow() m_parent->m_ProjectTableChanged = true; m_projectTable->Clear(); - m_projectTable->m_rows.transfer( m_projectTable->m_rows.end(), project_model()->m_rows.begin(), - project_model()->m_rows.end(), project_model()->m_rows ); - m_projectTable->reindex(); + m_projectTable->TransferRows( project_model()->m_rows ); } return true; diff --git a/eeschema/symbol_lib_table.cpp b/eeschema/symbol_lib_table.cpp index 7d7e4f3c6c..0367609fe0 100644 --- a/eeschema/symbol_lib_table.cpp +++ b/eeschema/symbol_lib_table.cpp @@ -296,10 +296,8 @@ void SYMBOL_LIB_TABLE::Format( OUTPUTFORMATTER* aOutput, int aIndentLevel ) cons aOutput->Print( aIndentLevel, "(sym_lib_table\n" ); aOutput->Print( aIndentLevel + 1, "(version %d)\n", m_version ); - for( LIB_TABLE_ROWS_CITER it = m_rows.begin(); it != m_rows.end(); ++it ) - { - it->Format( aOutput, aIndentLevel+1 ); - } + for( const LIB_TABLE_ROW& row : m_rows ) + row.Format( aOutput, aIndentLevel + 1 ); aOutput->Print( aIndentLevel, ")\n" ); } @@ -374,7 +372,9 @@ void SYMBOL_LIB_TABLE::LoadSymbolLib( std::vector& aSymbolList, const wxString& aNickname, bool aPowerSymbolsOnly ) { SYMBOL_LIB_TABLE_ROW* row = FindRow( aNickname, true ); - wxCHECK( row && row->plugin, /* void */ ); + + if( !row || !row->plugin ) + return; std::lock_guard lock( row->GetMutex() ); diff --git a/include/lib_table_base.h b/include/lib_table_base.h index b103ca6fa1..9e80994b65 100644 --- a/include/lib_table_base.h +++ b/include/lib_table_base.h @@ -335,13 +335,7 @@ public: virtual ~LIB_TABLE(); /// Delete all rows. - void Clear() - { - std::lock_guard lock( m_nickIndexMutex ); - - m_rows.clear(); - m_nickIndex.clear(); - } + void Clear(); /** * Compares this table against another. @@ -451,23 +445,30 @@ public: bool InsertRow( LIB_TABLE_ROW* aRow, bool doReplace = false ); /** - * Removes a row from the table. + * Removes a row from the table and frees the pointer * @param aRow is the row to remove * @return true if the row was found (and removed) */ - bool RemoveRow( const LIB_TABLE_ROW* aRow ) - { - for( auto iter = m_rows.begin(); iter != m_rows.end(); ++iter ) - { - if( *iter == *aRow ) - { - m_rows.erase( iter, iter + 1 ); - reindex(); - return true; - } - } - return false; - } + bool RemoveRow( const LIB_TABLE_ROW* aRow ); + + /** + * Replaces the Nth row with the given new row + * @return true if successful + */ + bool ReplaceRow( size_t aIndex, LIB_TABLE_ROW* aRow ); + + /** + * Moves a row within the table + * @param aIndex is the current index of the row to move + * @param aOffset is the number of positions to move it by in the table + * @return true if the move resulted in a change + */ + bool ChangeRowOrder( size_t aIndex, int aOffset ); + + /** + * Takes ownership of another list of rows; the original list will be freed + */ + void TransferRows( LIB_TABLE_ROWS& aRowsList ); /** * @return a #LIB_TABLE_ROW pointer if \a aURI is found in this table or in any chained @@ -528,18 +529,6 @@ public: return m_version; } - /** - * While this is an encapsulation leak, calling it before threaded loads *may* prevent - * some Sentry crashes we're seeing (KICAD-4S). - */ - void EnsureIndex() - { - ensureIndex(); - - if( m_fallBack ) - m_fallBack->EnsureIndex(); - } - protected: /** * Return a #LIB_TABLE_ROW if \a aNickname is found in this table or in any chained @@ -559,60 +548,24 @@ protected: */ bool migrate(); - /** - * Rebuilds the m_nickIndex - * - * @param aForce is to avoid rebuilding the index multiple times because multiple threads hit ensureIndex - * at the same time - */ - void reindex() - { - std::lock_guard lock( m_nickIndexMutex ); - - m_nickIndex.clear(); - - for( LIB_TABLE_ROWS_ITER it = m_rows.begin(); it != m_rows.end(); ++it ) - m_nickIndex.insert( INDEX_VALUE( it->GetNickName(), it - m_rows.begin() ) ); - } - - void ensureIndex() - { - // The dialog lib table editor may not maintain the nickIndex. - // Lazy indexing may be required. To handle lazy indexing, we must enforce - // that "nickIndex" is either empty or accurate, but never inaccurate. - { - std::shared_lock lock( m_nickIndexMutex ); - - if( m_nickIndex.size() ) - return; - } - - reindex(); - } - -private: - friend class PANEL_FP_LIB_TABLE; - friend class LIB_TABLE_GRID; + void reindex(); protected: - LIB_TABLE_ROWS m_rows; - - /// this is a non-owning index into the LIB_TABLE_ROWS table - typedef std::map INDEX; // "int" is std::vector array index - typedef INDEX::iterator INDEX_ITER; - typedef INDEX::const_iterator INDEX_CITER; - typedef INDEX::value_type INDEX_VALUE; - - /// this particular key is the nickName within each row. - INDEX m_nickIndex; - LIB_TABLE* m_fallBack; /// Versioning to handle importing old tables mutable int m_version; - /// Mutex to protect access to the nickIndex variable - mutable std::shared_mutex m_nickIndexMutex; + /// Owning set of rows. + // TODO: This should really be private; but the lib table grids re-use it + // (without using m_rowsMap). + LIB_TABLE_ROWS m_rows; + + /// this is a non-owning index into the LIB_TABLE_ROWS table + std::map m_rowsMap; + + /// Mutex to protect access to the rows vector + mutable std::shared_mutex m_mutex; }; #endif // _LIB_TABLE_BASE_H_ diff --git a/pcbnew/dialogs/panel_fp_lib_table.cpp b/pcbnew/dialogs/panel_fp_lib_table.cpp index 82ae15fbfd..778255b358 100644 --- a/pcbnew/dialogs/panel_fp_lib_table.cpp +++ b/pcbnew/dialogs/panel_fp_lib_table.cpp @@ -973,9 +973,7 @@ bool PANEL_FP_LIB_TABLE::TransferDataFromWindow() m_parent->m_GlobalTableChanged = true; m_global->Clear(); - m_global->m_rows.transfer( m_global->m_rows.end(), global_model()->m_rows.begin(), - global_model()->m_rows.end(), global_model()->m_rows ); - m_global->reindex(); + m_global->TransferRows( global_model()->m_rows ); } if( project_model() && *project_model() != *m_project ) @@ -983,9 +981,7 @@ bool PANEL_FP_LIB_TABLE::TransferDataFromWindow() m_parent->m_ProjectTableChanged = true; m_project->Clear(); - m_project->m_rows.transfer( m_project->m_rows.end(), project_model()->m_rows.begin(), - project_model()->m_rows.end(), project_model()->m_rows ); - m_project->reindex(); + m_project->TransferRows( project_model()->m_rows ); } return true; diff --git a/pcbnew/footprint_info_impl.cpp b/pcbnew/footprint_info_impl.cpp index baea001e82..f7a6d9fdc4 100644 --- a/pcbnew/footprint_info_impl.cpp +++ b/pcbnew/footprint_info_impl.cpp @@ -268,12 +268,6 @@ void FOOTPRINT_LIST_IMPL::loadFootprints() return 1; }; - // While the private ensureIndex() is supposedly thread-safe, and having a public call is - // a bit of an encapsulation leak, we have at least one non-reproducible Sentry issue - // (KICAD-4S) that *might* be prevented by doing the EnsureIndex() before spooling up the - // multi-threaded part. - m_lib_table->EnsureIndex(); - for( size_t ii = 0; ii < num_elements; ++ii ) returns[ii] = tp.submit( fp_thread );