From 1e33928b964dafcff651b2b5a744fdb758eaa8d6 Mon Sep 17 00:00:00 2001 From: Jon Evans Date: Mon, 18 Jan 2021 12:24:07 -0500 Subject: [PATCH] Fix issues with zone filling connectivity locking Two issues found with the locking system used to prevent access to stale connectivity data during the zone fill process: 1) a std::mutex has undefined behavior if you try to use it to guard against access from the same thread. Because of the use of wx event loops (and coroutines) it is entirely possible, and in some situations inevitable, that the same thread will try to redraw the ratsnest in the middle of zone refilling. 2) The mutex was only guarding the ZONE_FILLER::Fill method, but the callers of that method also do connectivity updates as part of the COMMIT::Push. Redrawing the ratsnest after the Fill but before the Push will result in stale connectivity pointers to zone filled areas. Fixed (1) by switching to a trivial spinlock implementation. Spinlocks would generally not be desirable if the contention for the connectivity data crossed thread boundaries, but at the moment I believe it's guaranteed that the reads and writes to connectivity that are guarded by this lock happen from the main UI thread. The writes are also quite rare compared to reads, and reads are generally fast, so I'm not really worried about the UI thread spinning for any real amount of time. Fixed (2) by moving the locking location up to the call sites of ZONE_FILLER::Fill. This issue was quite difficult to reproduce, but I found a fairly reliable way: It only happens (for me) on Windows, MSYS2 build, with wxWidgets 3.0 It also only happens if I restrict PcbNew to use 2 CPU cores. With those conditions, I can reproduce the issue described in #6471 by repeatedly editing a zone properties and changing its net. The crash is especially easy to trigger if you press some keys (such as 'e' for edit) while the progress dialog is displayed. It's easiest to do this in a debug build as the slower KiCad is running, the bigger the window is to trigger this bug. Fixes https://gitlab.com/kicad/code/kicad/-/issues/6471 Fixes https://gitlab.com/kicad/code/kicad/-/issues/7048 --- include/core/spinlock.h | 60 +++++++++++++++++++++++++ pcbnew/connectivity/connectivity_data.h | 5 ++- pcbnew/edit_zone_helpers.cpp | 2 + pcbnew/ratsnest/ratsnest_viewitem.cpp | 2 +- pcbnew/tools/zone_create_helper.cpp | 9 +++- pcbnew/tools/zone_filler_tool.cpp | 4 ++ pcbnew/zone_filler.cpp | 4 -- pcbnew/zone_filler.h | 7 +++ 8 files changed, 85 insertions(+), 8 deletions(-) create mode 100644 include/core/spinlock.h diff --git a/include/core/spinlock.h b/include/core/spinlock.h new file mode 100644 index 0000000000..6228e5d9ce --- /dev/null +++ b/include/core/spinlock.h @@ -0,0 +1,60 @@ +/* +* This program source code file is part of KiCad, a free EDA CAD application. +* +* Copyright (C) 2021 KiCad Developers, see AUTHORS.txt for contributors. +* +* This program is free software: you can redistribute it and/or modify it +* under the terms of the GNU General Public License as published by the +* Free Software Foundation, either version 3 of the License, or (at your +* option) any later version. +* +* This program is distributed in the hope that it will be useful, but +* WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +* General Public License for more details. +* +* You should have received a copy of the GNU General Public License along +* with this program. If not, see . +*/ + +#ifndef __KICAD_SPINLOCK_H +#define __KICAD_SPINLOCK_H + +#include + + +/** + * A trivial spinlock implementation with no optimization. Don't use if congestion is expected! + */ +class KISPINLOCK +{ +public: + KISPINLOCK() : + m_lock( false ) + {} + + void lock() + { + while( m_lock.exchange( true, std::memory_order_acquire ) ); + } + + bool try_lock() + { + return !m_lock.exchange( true, std::memory_order_acquire ); + } + + void unlock() + { + m_lock.store( false, std::memory_order_release ); + } + + bool test() + { + return m_lock.load( std::memory_order_acquire ); + } + +private: + std::atomic m_lock; +}; + +#endif diff --git a/pcbnew/connectivity/connectivity_data.h b/pcbnew/connectivity/connectivity_data.h index 8cc03a8358..a59f21486f 100644 --- a/pcbnew/connectivity/connectivity_data.h +++ b/pcbnew/connectivity/connectivity_data.h @@ -28,6 +28,7 @@ #define __CONNECTIVITY_DATA_H #include +#include #include #include @@ -261,7 +262,7 @@ public: return m_connAlgo; } - std::mutex& GetLock() + KISPINLOCK& GetLock() { return m_lock; } @@ -307,7 +308,7 @@ private: bool m_skipRatsnest = false; - std::mutex m_lock; + KISPINLOCK m_lock; /// Map of netcode -> netclass the net is a member of; used for ratsnest painting std::map m_netclassMap; diff --git a/pcbnew/edit_zone_helpers.cpp b/pcbnew/edit_zone_helpers.cpp index 2465ff6537..482b34fe9b 100644 --- a/pcbnew/edit_zone_helpers.cpp +++ b/pcbnew/edit_zone_helpers.cpp @@ -125,6 +125,8 @@ void PCB_EDIT_FRAME::Edit_Zone_Params( ZONE* aZone ) commit.Stage( pickedList ); + std::lock_guard lock( GetBoard()->GetConnectivity()->GetLock() ); + if( zones_to_refill.size() ) { ZONE_FILLER filler( GetBoard(), &commit ); diff --git a/pcbnew/ratsnest/ratsnest_viewitem.cpp b/pcbnew/ratsnest/ratsnest_viewitem.cpp index 480dc4c3fc..83c4a59bd5 100644 --- a/pcbnew/ratsnest/ratsnest_viewitem.cpp +++ b/pcbnew/ratsnest/ratsnest_viewitem.cpp @@ -63,7 +63,7 @@ const BOX2I RATSNEST_VIEWITEM::ViewBBox() const void RATSNEST_VIEWITEM::ViewDraw( int aLayer, KIGFX::VIEW* aView ) const { - std::unique_lock lock( m_data->GetLock(), std::try_to_lock ); + std::unique_lock lock( m_data->GetLock(), std::try_to_lock ); if( !lock ) return; diff --git a/pcbnew/tools/zone_create_helper.cpp b/pcbnew/tools/zone_create_helper.cpp index cec1e6a5b0..cde19faf26 100644 --- a/pcbnew/tools/zone_create_helper.cpp +++ b/pcbnew/tools/zone_create_helper.cpp @@ -21,6 +21,8 @@ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA */ +#include +#include #include #include #include @@ -168,6 +170,8 @@ void ZONE_CREATE_HELPER::performZoneCutout( ZONE& aZone, ZONE& aCutout ) ZONE_FILLER filler( board, &commit ); + std::lock_guard lock( board->GetConnectivity()->GetLock() ); + if( !filler.Fill( newZones ) ) { commit.Revert(); @@ -207,9 +211,12 @@ void ZONE_CREATE_HELPER::commitZone( std::unique_ptr aZone ) aZone->HatchBorder(); bCommit.Add( aZone.get() ); + BOARD* board = m_tool.getModel(); + std::lock_guard lock( board->GetConnectivity()->GetLock() ); + if( !m_params.m_keepout ) { - ZONE_FILLER filler( m_tool.getModel(), &bCommit ); + ZONE_FILLER filler( board, &bCommit ); std::vector toFill = { aZone.get() }; if( !filler.Fill( toFill ) ) diff --git a/pcbnew/tools/zone_filler_tool.cpp b/pcbnew/tools/zone_filler_tool.cpp index 99da06d7f0..2a8128e35a 100644 --- a/pcbnew/tools/zone_filler_tool.cpp +++ b/pcbnew/tools/zone_filler_tool.cpp @@ -128,6 +128,8 @@ void ZONE_FILLER_TOOL::FillAllZones( wxWindow* aCaller, PROGRESS_REPORTER* aRepo else filler.InstallNewProgressReporter( aCaller, _( "Fill All Zones" ), 3 ); + std::lock_guard lock( board()->GetConnectivity()->GetLock() ); + if( filler.Fill( toFill ) ) { commit.Push( _( "Fill Zone(s)" ), false ); @@ -171,6 +173,8 @@ int ZONE_FILLER_TOOL::ZoneFill( const TOOL_EVENT& aEvent ) ZONE_FILLER filler( board(), &commit ); filler.InstallNewProgressReporter( frame(), _( "Fill Zone" ), 4 ); + std::lock_guard lock( board()->GetConnectivity()->GetLock() ); + if( filler.Fill( toFill ) ) commit.Push( _( "Fill Zone(s)" ), false ); else diff --git a/pcbnew/zone_filler.cpp b/pcbnew/zone_filler.cpp index 8e123ae159..2063e7ab4e 100644 --- a/pcbnew/zone_filler.cpp +++ b/pcbnew/zone_filler.cpp @@ -88,10 +88,6 @@ bool ZONE_FILLER::Fill( std::vector& aZones, bool aCheck, wxWindow* aPare std::vector islandsList; std::shared_ptr connectivity = m_board->GetConnectivity(); - std::unique_lock lock( connectivity->GetLock(), std::try_to_lock ); - - if( !lock ) - return false; // Rebuild just in case. This really needs to be reliable. connectivity->Clear(); diff --git a/pcbnew/zone_filler.h b/pcbnew/zone_filler.h index 80dd840d5c..8fa0641418 100644 --- a/pcbnew/zone_filler.h +++ b/pcbnew/zone_filler.h @@ -44,6 +44,13 @@ public: void SetProgressReporter( PROGRESS_REPORTER* aReporter ); void InstallNewProgressReporter( wxWindow* aParent, const wxString& aTitle, int aNumPhases ); + + /** + * Fills the given list of zones. Invalidates connectivity - it is up to the caller to obtain + * a lock on the connectivity data before calling Fill to prevent access to stale data by other + * coroutines (for example, ratsnest redraw). This will generally be required if a UI-based + * progress reporter has been installed. + */ bool Fill( std::vector& aZones, bool aCheck = false, wxWindow* aParent = nullptr ); bool IsDebug() const { return m_debugZoneFiller; }