From 24863f815ae0786edfe7dfbad474fc43cc598dd6 Mon Sep 17 00:00:00 2001 From: Naveen Venkat Date: Sun, 5 Jul 2026 12:45:57 +0000 Subject: [PATCH] odb: encapsulate inst module assignment Keep _dbInst::module_ private and route runtime reparenting through a setter that fires inDbPostInstParentChange. Limit quiet assignments to serialization and module internals, and cover callback/no-op behavior. Fixes #10282 Signed-off-by: Naveen Venkat --- src/odb/src/db/dbBlock.cpp | 21 +++++++++++++++++--- src/odb/src/db/dbInst.cpp | 28 ++++++++++++++++++++------ src/odb/src/db/dbInst.h | 20 ++++++++++++------- src/odb/src/db/dbModule.cpp | 19 +++++++++--------- src/odb/test/cpp/CallBack.h | 7 +++++++ src/odb/test/cpp/TestCallBacks.cpp | 32 ++++++++++++++++++++++++++++++ 6 files changed, 102 insertions(+), 25 deletions(-) diff --git a/src/odb/src/db/dbBlock.cpp b/src/odb/src/db/dbBlock.cpp index 5074840edec..e49b862f861 100644 --- a/src/odb/src/db/dbBlock.cpp +++ b/src/odb/src/db/dbBlock.cpp @@ -871,7 +871,7 @@ dbOStream& operator<<(dbOStream& stream, const _dbBlock& block) * @param block The database block containing the objects. * @param table The internal database table storing the object implementations. * @param module_field Member pointer to the field storing the parent module ID - * (e.g., &_dbInst::module_ or &_dbModInst::parent_). + * (e.g., &_dbModInst::parent_). * @param hash_field Member pointer to the hash map member in _dbModule where * the object ID should be stored. */ @@ -896,6 +896,22 @@ static void rebuildModuleHash( } } +static void rebuildDbInstModuleHash(_dbBlock& block) +{ + dbSet items((dbBlock*) &block, block.inst_tbl_); + for (dbInst* obj : items) { + _dbInst* _obj = (_dbInst*) obj; + dbId<_dbModule> mid = _obj->getModuleId(); + if (mid == 0) { + mid = block.top_module_; + } + _dbModule* module = block.module_tbl_->getPtr(mid); + if (module && _obj->name_) { + module->dbinst_hash_[_obj->name_] = _obj->getId(); + } + } +} + dbIStream& operator>>(dbIStream& stream, _dbBlock& block) { _dbDatabase* db = block.getImpl()->getDatabase(); @@ -989,8 +1005,7 @@ dbIStream& operator>>(dbIStream& stream, _dbBlock& block) } if (db->isSchema(kSchemaDbRemoveHash)) { // Construct dbinst_hash_ - rebuildModuleHash( - block, block.inst_tbl_, &_dbInst::module_, &_dbModule::dbinst_hash_); + rebuildDbInstModuleHash(block); } if (db->isSchema(kSchemaBlockOwnsScanInsts)) { stream >> *block.scan_inst_tbl_; diff --git a/src/odb/src/db/dbInst.cpp b/src/odb/src/db/dbInst.cpp index 0aecdd4be95..0bc84ddfc0a 100644 --- a/src/odb/src/db/dbInst.cpp +++ b/src/odb/src/db/dbInst.cpp @@ -87,6 +87,19 @@ void _dbInst::setInstBBox(_dbInst* inst) block->add_rect(box->shape_.rect); } +void _dbInst::setModule(dbId<_dbModule> module) +{ + if (module_ == module) { + return; + } + + module_ = module; + _dbBlock* block = (_dbBlock*) getOwner(); + for (dbBlockCallBackObj* cb : block->callbacks_) { + cb->inDbPostInstParentChange((dbInst*) this); + } +} + _dbInst::_dbInst(_dbDatabase*) { flags_.orient = dbOrientType::R0; @@ -157,7 +170,7 @@ dbOStream& operator<<(dbOStream& stream, const _dbInst& inst) stream << inst.inst_hdr_; stream << inst.bbox_; stream << inst.region_; - stream << inst.module_; + stream << inst.getModuleId(); stream << inst.group_; stream << inst.region_next_; stream << inst.module_next_; @@ -185,7 +198,9 @@ dbIStream& operator>>(dbIStream& stream, _dbInst& inst) stream >> inst.inst_hdr_; stream >> inst.bbox_; stream >> inst.region_; - stream >> inst.module_; + dbId<_dbModule> module; + stream >> module; + inst.setModuleQuiet(module); stream >> inst.group_; stream >> inst.region_next_; stream >> inst.module_next_; @@ -866,12 +881,13 @@ dbModule* dbInst::getModule() { _dbInst* inst = (_dbInst*) this; - if (inst->module_ == 0) { + const dbId<_dbModule> module_id = inst->getModuleId(); + if (module_id == 0) { return nullptr; } _dbBlock* block = (_dbBlock*) inst->getOwner(); - _dbModule* module = block->module_tbl_->getPtr(inst->module_); + _dbModule* module = block->module_tbl_->getPtr(module_id); return (dbModule*) module; } @@ -1072,7 +1088,7 @@ bool dbInst::isPhysicalOnly() { _dbInst* inst = (_dbInst*) this; - return inst->module_ == 0; + return inst->getModuleId() == 0; } dbInst* dbInst::getParent() @@ -1589,7 +1605,7 @@ void dbInst::destroy(dbInst* inst_) block->journal_->pushParam(inst->x_); block->journal_->pushParam(inst->y_); block->journal_->pushParam(inst->group_); - block->journal_->pushParam(inst->module_); + block->journal_->pushParam(inst->getModuleId()); block->journal_->pushParam(inst->region_); block->journal_->endAction(); } diff --git a/src/odb/src/db/dbInst.h b/src/odb/src/db/dbInst.h index b0e3d5a2920..5b15b9e78fc 100644 --- a/src/odb/src/db/dbInst.h +++ b/src/odb/src/db/dbInst.h @@ -24,6 +24,7 @@ class _dbGroup; class _dbChipBump; class _dbChipRegion; class dbInst; +class dbModule; class dbIStream; class dbOStream; @@ -63,6 +64,9 @@ class _dbInst : public _dbObject void collectMemInfo(MemInfo& info); static void setInstBBox(_dbInst* inst); + dbId<_dbModule> getModuleId() const { return module_; } + void setModule(dbId<_dbModule> module); + _dbInstFlags flags_; char* name_; int x_; @@ -72,14 +76,16 @@ class _dbInst : public _dbObject dbId<_dbInstHdr> inst_hdr_; dbId<_dbBox> bbox_; dbId<_dbRegion> region_; - // WARNING: re-parenting an existing instance changes its full SDC - // path. dbModule::addInst() fires inDbPostInstParentChange in that - // case so downstream caches (e.g., dbSdcNetwork's path-to-instance - // map) stay consistent. The callback is suppressed on the initial - // assignment during dbInst::create -- that path's accounting belongs - // to inDbInstCreate. Prefer dbModule::addInst() over assigning this - // field directly. + + private: + friend class dbModule; + friend class _dbModule; + friend dbIStream& operator>>(dbIStream& stream, _dbInst& inst); + dbId<_dbModule> module_; + void setModuleQuiet(dbId<_dbModule> module) { module_ = module; } + + public: dbId<_dbGroup> group_; dbId<_dbInst> region_next_; dbId<_dbInst> module_next_; diff --git a/src/odb/src/db/dbModule.cpp b/src/odb/src/db/dbModule.cpp index 0b12c9ffd22..638a6e07293 100644 --- a/src/odb/src/db/dbModule.cpp +++ b/src/odb/src/db/dbModule.cpp @@ -224,7 +224,8 @@ void dbModule::addInst(dbInst* inst) getName()); } - if (_inst->module_ == module->getOID()) { + const dbId<_dbModule> old_module_id = _inst->getModuleId(); + if (old_module_id == module->getOID()) { return; // already in this module } @@ -240,13 +241,12 @@ void dbModule::addInst(dbInst* inst) // initial assignment during dbInst::create (module_ is unset). Only // the former should fire inDbPostInstParentChange -- otherwise every // create would falsely trigger downstream subtree-invalidation paths. - const bool is_reparent = (_inst->module_ != 0); + const bool is_reparent = (old_module_id != 0); if (is_reparent) { - dbModule* mod = dbModule::getModule((dbBlock*) block, _inst->module_); + dbModule* mod = dbModule::getModule((dbBlock*) block, old_module_id); ((_dbModule*) mod)->removeInst(inst); } - _inst->module_ = module->getOID(); module->dbinst_hash_[inst->getName()] = dbId<_dbInst>(_inst->getOID()); if (module->insts_ == 0) { @@ -261,9 +261,10 @@ void dbModule::addInst(dbInst* inst) } if (is_reparent) { - for (dbBlockCallBackObj* cb : block->callbacks_) { - cb->inDbPostInstParentChange(inst); - } + _inst->setModule(module->getOID()); + } else { + // Initial dbInst::create ownership is reported through inDbInstCreate. + _inst->setModuleQuiet(module->getOID()); } } @@ -273,7 +274,7 @@ void _dbModule::removeInst(dbInst* inst) _dbInst* _inst = (_dbInst*) inst; uint32_t id = _inst->getOID(); - if (_inst->module_ != getOID()) { + if (_inst->getModuleId() != getOID()) { return; } @@ -305,7 +306,7 @@ void _dbModule::removeInst(dbInst* inst) prev->module_next_ = _inst->module_next_; } } - _inst->module_ = 0; + _inst->setModuleQuiet(0); _inst->module_next_ = 0; _inst->module_prev_ = 0; } diff --git a/src/odb/test/cpp/CallBack.h b/src/odb/test/cpp/CallBack.h index 9de634c651a..315933e8768 100644 --- a/src/odb/test/cpp/CallBack.h +++ b/src/odb/test/cpp/CallBack.h @@ -38,6 +38,13 @@ class CallBack : public dbBlockCallBackObj events.push_back("Destroy inst " + inst->getName()); } } + void inDbPostInstParentChange(dbInst* inst) override + { + if (!_pause) { + events.push_back("Change parent of inst " + inst->getName() + " to " + + inst->getModule()->getName()); + } + } void inDbInstSwapMasterBefore(dbInst* inst, dbMaster* master) override { if (!_pause) { diff --git a/src/odb/test/cpp/TestCallBacks.cpp b/src/odb/test/cpp/TestCallBacks.cpp index ed87ad459dd..e27155c225d 100644 --- a/src/odb/test/cpp/TestCallBacks.cpp +++ b/src/odb/test/cpp/TestCallBacks.cpp @@ -86,6 +86,38 @@ TEST_F(CallbackFixture, test_inst_and_iterm) EXPECT_EQ(cb_.events[3], "Destroy inst i1"); } +TEST_F(CallbackFixture, test_inst_parent_change) +{ + createSimpleDB(); + dbBlock* block = db_->getChip()->getBlock(); + cb_.addOwner(block); + + dbInst* inst = dbInst::create(block, db_->findMaster("and2"), "i1"); + EXPECT_EQ(cb_.events.size(), 4); + cb_.clearEvents(); + + dbModule* parent = dbModule::create(block, "parent"); + parent->addInst(inst); + ASSERT_EQ(cb_.events.size(), 1); + EXPECT_EQ(cb_.events[0], "Change parent of inst i1 to parent"); + cb_.clearEvents(); + + parent->addInst(inst); + EXPECT_TRUE(cb_.events.empty()); + + dbModule* top = block->getTopModule(); + top->addInst(inst); + ASSERT_EQ(cb_.events.size(), 1); + EXPECT_EQ(cb_.events[0], + "Change parent of inst i1 to " + std::string(top->getName())); + cb_.clearEvents(); + + top->addInst(inst); + EXPECT_TRUE(cb_.events.empty()); + + dbInst::destroy(inst); +} + TEST_F(CallbackFixture, test_net) { createSimpleDB();