router: invalidate items freed by PNS::NODE in the clearance cache

This was causing intermittent shove hiccups/freezes as RULE_RESOLVER::GetClearance() could return a bogus clearance value, taking
a cached value for an item that does not exist anymore. If the allocator/stack accidentally reclaimed such item's address - we were getting
very nasty and difficult to reproduce misbehaviours of the shove algorithm. Hopefully this patch fixes this. More info in the comments.

Note to self - I'm still not fully convinced I want PNS::ITEM pointers as the cache key, probably a unique counter would be better here.

pcbnew/router/pns_kicad_iface.cpp

@@ -124,7 +124,7 @@ public:
 
     int ClearanceEpsilon() const override { return m_clearanceEpsilon; }
 
-    void ClearCacheForItem( const PNS::ITEM* aItem ) override;
+    void ClearCacheForItems( std::vector<const PNS::ITEM*>& aItems ) override;
     void ClearCaches() override;
 
 private:
@@ -410,13 +410,30 @@ bool PNS_PCBNEW_RULE_RESOLVER::QueryConstraint( PNS::CONSTRAINT_TYPE aType,
 }
 
 
-void PNS_PCBNEW_RULE_RESOLVER::ClearCacheForItem( const PNS::ITEM* aItem )
+void PNS_PCBNEW_RULE_RESOLVER::ClearCacheForItems( std::vector<const PNS::ITEM*>& aItems )
 {
-    CLEARANCE_CACHE_KEY key = { aItem, nullptr, false };
-    m_clearanceCache.erase( key );
+    int n_pruned = 0;
+    std::set<const PNS::ITEM*> remainingItems( aItems.begin(), aItems.end() );
 
-    key.Flag = true;
-    m_clearanceCache.erase( key );
+/* We need to carefully check both A and B item pointers in the cache against dirty/invalidated
+   items in the set, as the clearance relation is commutative ( CL[a,b] == CL[b,a] ). The code
+   below is a bit ugly, but works in O(n*log(m)) and is run once or twice during ROUTER::Move() call
+   - so I hope it still gets better performance than no cache at all */
+    for( auto it = m_clearanceCache.begin(); it != m_clearanceCache.end(); )
+    {
+        bool dirty = remainingItems.find( it->first.A ) != remainingItems.end();
+        dirty |= remainingItems.find( it->first.B) != remainingItems.end();
+
+        if( dirty )
+        {
+            it = m_clearanceCache.erase( it );
+            n_pruned++;
+        } else
+            it++;
+    }
+#if 0
+    printf("ClearCache : n_pruned %d\n", n_pruned );
+#endif
 }
 
 
@@ -492,7 +509,15 @@ int PNS_PCBNEW_RULE_RESOLVER::Clearance( const PNS::ITEM* aA, const PNS::ITEM* a
     if( aUseClearanceEpsilon && rv > 0 )
         rv = std::max( 0, rv - m_clearanceEpsilon );
 
-    m_clearanceCache[ key ] = rv;
+
+/* It makes no sense to put items that have no owning NODE in the cache - they can be allocated on stack
+   and we can't really invalidate them in the cache when they are destroyed. Probably a better idea would be
+   to use a static unique counter in PNS::ITEM constructor to generate the cache keys. */
+    if( aA && aB && aA->Owner() && aB->Owner() )
+    {
+        m_clearanceCache[ key ] = rv;
+    }
+
     return rv;
 }
 

pcbnew/router/pns_node.cpp

@@ -87,7 +87,7 @@ NODE::~NODE()
 
     m_joints.clear();
 
-    std::vector<ITEM*> toDelete;
+    std::vector<const ITEM*> toDelete;
 
     toDelete.reserve( m_index->Size() );
 
@@ -97,12 +97,17 @@ NODE::~NODE()
             toDelete.push_back( item );
     }
 
-    for( ITEM* item : toDelete )
+    for( const ITEM* item : toDelete )
     {
         wxLogTrace( wxT( "PNS" ), wxT( "del item %p type %s" ), item, item->KindStr().c_str() );
         delete item;
     }
 
+    if( m_ruleResolver )
+    {
+        m_ruleResolver->ClearCacheForItems( toDelete );
+    }
+
     releaseGarbage();
     unlinkParent();
 
@@ -1389,6 +1394,9 @@ void NODE::releaseGarbage()
     if( !isRoot() )
         return;
 
+    std::vector<const ITEM*> cacheCheckItems;
+    cacheCheckItems.reserve( m_garbageItems.size() );
+
     for( ITEM* item : m_garbageItems )
     {
         if( !item->BelongsTo( this ) )
@@ -1396,6 +1404,11 @@ void NODE::releaseGarbage()
     }
 
     m_garbageItems.clear();
+
+    if( m_ruleResolver )
+    {
+        m_ruleResolver->ClearCacheForItems( cacheCheckItems );
+    }
 }
 
 

pcbnew/router/pns_node.h

@@ -155,7 +155,7 @@ public:
 
     virtual wxString NetName( int aNet ) = 0;
 
-    virtual void ClearCacheForItem( const ITEM* aItem ) {}
+    virtual void ClearCacheForItems( std::vector<const PNS::ITEM*>& aItems ) {}
     virtual void ClearCaches() {}
 
     virtual int ClearanceEpsilon() const { return 0; }

pcbnew/router/pns_router.cpp

@@ -103,6 +103,7 @@ void ROUTER::ClearWorld()
 {
     if( m_world )
     {
+        m_world->SetRuleResolver( nullptr );
         m_world->KillChildren();
         m_world.reset();
     }
@@ -716,9 +717,11 @@ void ROUTER::updateView( NODE* aNode, ITEM_SET& aCurrent, bool aDragging )
 
     aNode->GetUpdatedItems( removed, added );
 
+    std::vector<const PNS::ITEM*> cacheCheckItems( added.begin(), added.end() );
+    GetRuleResolver()->ClearCacheForItems( cacheCheckItems );
+
     for( ITEM* item : added )
     {
-        GetRuleResolver()->ClearCacheForItem( item );
         int clearance = GetRuleResolver()->Clearance( item, nullptr );
         m_iface->DisplayItem( item, clearance, aDragging );
     }