Merge pull request #10179 from The-OpenROAD-Project-staging/secure-odb-access-point-cleanup

Fix access point cleanup on instance removal

Author: osamahammad21

src/odb/src/db/dbInst.cpp

@@ -1215,6 +1215,13 @@ bool dbInst::swapMaster(dbMaster* new_master_)
     return false;
   }
 
+  // Clear preferred APs before the ITerms are remapped to the new master.
+  for (const uint32_t iterm_id : inst->iterms_) {
+    dbITerm* iterm = (dbITerm*) block->iterm_tbl_->getPtr(iterm_id);
+    iterm->clearPrefAccessPoints();
+  }
+  inst->pin_access_idx_ = -1;
+
   // remove reference to inst_hdr
   _dbInstHdr* old_inst_hdr
       = block->inst_hdr_hash_.find(((_dbMaster*) old_master_)->id_);
@@ -1501,16 +1508,7 @@ void dbInst::destroy(dbInst* inst_)
     _dbITerm* _iterm = block->iterm_tbl_->getPtr(id);
     dbITerm* iterm = (dbITerm*) _iterm;
     iterm->disconnect();
-    if (inst_->getPinAccessIdx() >= 0) {
-      for (const auto& [pin, aps] : iterm->getAccessPoints()) {
-        for (auto ap : aps) {
-          _dbAccessPoint* _ap = (_dbAccessPoint*) ap;
-          auto [first, last] = std::ranges::remove_if(
-              _ap->iterms_, [id](const auto& id_in) { return id_in == id; });
-          _ap->iterms_.erase(first, last);
-        }
-      }
-    }
+    iterm->clearPrefAccessPoints();
 
     // Notify when pins are deleted (assumption: pins are destroyed only when
     // the related instance is destroyed)

src/odb/src/db/dbMPin.cpp

@@ -3,6 +3,7 @@
 
 #include "dbMPin.h"
 
+#include <cstddef>
 #include <cstdint>
 #include <vector>
 
@@ -16,6 +17,7 @@
 #include "dbMaster.h"
 #include "dbPolygonItr.h"
 #include "dbTable.h"
+#include "dbVector.h"
 #include "odb/db.h"
 #include "odb/dbSet.h"
 #include "odb/geom.h"
@@ -150,11 +152,13 @@ void dbMPin::clearPinAccess(const int pin_access_idx)
 {
   _dbMPin* pin = (_dbMPin*) this;
   _dbBlock* block = (_dbBlock*) getDb()->getChip()->getBlock();
-  if (pin->aps_.size() <= pin_access_idx) {
+  if (pin_access_idx < 0
+      || pin->aps_.size() <= static_cast<std::size_t>(pin_access_idx)) {
     return;
   }
-  const auto aps = pin->aps_[pin_access_idx];
-  for (const auto& ap : aps) {
+  dbVector<dbId<_dbAccessPoint>> aps;
+  aps.swap(pin->aps_[pin_access_idx]);
+  for (const dbId<_dbAccessPoint>& ap : aps) {
     odb::dbAccessPoint::destroy(
         (odb::dbAccessPoint*) block->ap_tbl_->getPtr(ap));
   }

src/odb/test/cpp/TestAccessPoint.cpp

@@ -1,3 +1,4 @@
+#include <cstdint>
 #include <filesystem>
 #include <fstream>
 #include <ios>
@@ -76,5 +77,49 @@ TEST_F(SimpleDbFixture, test_default)
   dbDatabase::destroy(db2);
 }
 
+TEST_F(SimpleDbFixture, SwapMasterClearsAndInvalidatesAccessPoints)
+{
+  createSimpleDB();
+  dbBlock* block = db_->getChip()->getBlock();
+  dbLib* lib = db_->findLib("lib1");
+  ASSERT_NE(lib, nullptr);
+  dbMaster* old_master = db_->findMaster("and2");
+  ASSERT_NE(old_master, nullptr);
+  dbMaster* new_master
+      = createMaster2X1(lib, "and2_x4", 2000, 1000, "a", "b", "o");
+  ASSERT_NE(new_master, nullptr);
+  dbMTerm* old_term = old_master->findMTerm("a");
+  ASSERT_NE(old_term, nullptr);
+  dbMTerm* new_term = new_master->findMTerm("a");
+  ASSERT_NE(new_term, nullptr);
+
+  // Create APs on both masters so a stale pin-access index would be visible.
+  dbMPin* old_pin = dbMPin::create(old_term);
+  ASSERT_NE(old_pin, nullptr);
+  dbAccessPoint* old_ap = dbAccessPoint::create(block, old_pin, 0);
+  ASSERT_NE(old_ap, nullptr);
+  dbMPin* new_pin = dbMPin::create(new_term);
+  ASSERT_NE(new_pin, nullptr);
+  dbAccessPoint* new_ap = dbAccessPoint::create(block, new_pin, 0);
+  ASSERT_NE(new_ap, nullptr);
+
+  dbInst* inst = dbInst::create(block, old_master, "i1");
+  ASSERT_NE(inst, nullptr);
+  dbITerm* iterm = inst->getITerm(old_term);
+  ASSERT_NE(iterm, nullptr);
+  inst->setPinAccessIdx(0);
+  iterm->setAccessPoint(old_pin, old_ap);
+  ASSERT_EQ(iterm->getPrefAccessPoints().size(), 1);
+  ASSERT_EQ(iterm->getAccessPoints().at(old_pin).size(), 1);
+
+  ASSERT_TRUE(inst->swapMaster(new_master));
+  dbITerm* swapped_iterm = inst->findITerm("a");
+  ASSERT_NE(swapped_iterm, nullptr);
+
+  EXPECT_EQ(inst->getPinAccessIdx(), static_cast<uint32_t>(-1));
+  EXPECT_TRUE(swapped_iterm->getPrefAccessPoints().empty());
+  EXPECT_TRUE(swapped_iterm->getAccessPoints().empty());
+}
+
 }  // namespace
 }  // namespace odb