haskal
/
kicad
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Stricter API for LIB_TABLE 

Prevent nickname map or row parent getting out of sync

Fixes https://gitlab.com/kicad/code/kicad/-/issues/15315
This commit is contained in:
Jon Evans 2023-08-06 00:56:51 -04:00
parent 6f2574d0d2
commit 4c4bbdc8f3
6 changed files with 168 additions and 148 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

154
common/lib_table_base.cpp
View File

@ -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<std::shared_mutex> lock( cur->m_mutex, std::try_to_lock );
std::shared_lock<std::shared_mutex> lock( cur->m_nickIndexMutex );
if( !cur->m_rowsMap.count( aNickName ) )
continue;
for( const std::pair<const wxString, int>& 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<const wxString, int>& entry : cur->m_nickIndex )
for( const std::pair<const wxString, LIB_TABLE_ROWS_ITER>& 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<wxString> 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<wxString> LIB_TABLE::GetLogicalLibs()
bool LIB_TABLE::InsertRow( LIB_TABLE_ROW* aRow, bool doReplace )
{
ensureIndex();
std::lock_guard<std::shared_mutex> lock( m_mutex );
std::lock_guard<std::shared_mutex> 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<std::shared_mutex> 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<std::shared_mutex> 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<std::shared_mutex> lock( m_mutex );
if( aIndex >= m_rows.size() )
return false;
int newPos = static_cast<int>( aIndex ) + aOffset;
if( newPos < 0 || newPos > static_cast<int>( 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<std::shared_mutex> 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;
}
}

25
eeschema/dialogs/panel_sym_lib_table.cpp
View File

@ -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<SYMBOL_LIB_TABLE_ROW&>( 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;

10
eeschema/symbol_lib_table.cpp
View File

@ -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<LIB_SYMBOL*>& 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<std::mutex> lock( row->GetMutex() );

113
include/lib_table_base.h
View File

@ -335,13 +335,7 @@ public:
virtual ~LIB_TABLE();
/// Delete all rows.
void Clear()
{
std::lock_guard<std::shared_mutex> 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<std::shared_mutex> 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<std::shared_mutex> 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<wxString,int> 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<wxString, LIB_TABLE_ROWS_ITER> m_rowsMap;
/// Mutex to protect access to the rows vector
mutable std::shared_mutex m_mutex;
};
#endif // _LIB_TABLE_BASE_H_

8
pcbnew/dialogs/panel_fp_lib_table.cpp
View File

@ -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;

6
pcbnew/footprint_info_impl.cpp
View File

@ -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 );