Skip to content

odb: guard instance module reassignment #10921

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
naveenvenk17 wants to merge 2 commits into
base: master
Choose a base branch
from
Open

odb: guard instance module reassignment #10921

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3962372
odb: guard instance module reassignment
Jul 15, 2026
1f4ff83
odb: update module index when renaming instances
Jul 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 33 additions & 4 deletions src/odb/src/db/dbInst.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,24 @@ class sortITerm
}
};

void _dbInst::setModule(dbId<_dbModule> module)
{
const dbId<_dbModule> previous = module_;
if (previous == module) {
return;
}

module_ = module;
if (previous == 0 || module == 0) {
return;
}
Comment on lines +85 to +87

@maliberty maliberty Jul 15, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why skip the callback?


_dbBlock* block = (_dbBlock*) getOwner();
for (dbBlockCallBackObj* callback : block->callbacks_) {
callback->inDbPostInstParentChange((dbInst*) this);
}
}

void _dbInst::setInstBBox(_dbInst* inst)
{
_dbBlock* block = (_dbBlock*) inst->getOwner();
Expand Down Expand Up @@ -380,9 +398,20 @@ bool dbInst::rename(const char* name)

std::string old_name(inst->name_);
block->inst_hash_.remove(inst);

_dbModule* module = nullptr;
if (inst->getModuleId() != 0) {
module = (_dbModule*) dbModule::getModule((dbBlock*) block,
inst->getModuleId());
module->dbinst_hash_.erase(old_name);
}

free((void*) inst->name_);
inst->name_ = safe_strdup(name);
block->inst_hash_.insert(inst);
if (module != nullptr) {
module->dbinst_hash_[name] = dbId<_dbInst>(inst->getOID());
}

for (dbBlockCallBackObj* cb : block->callbacks_) {
cb->inDbPostInstRename(this, old_name.c_str());
Expand Down Expand Up @@ -866,12 +895,12 @@ dbModule* dbInst::getModule()
{
_dbInst* inst = (_dbInst*) this;

if (inst->module_ == 0) {
if (inst->getModuleId() == 0) {
return nullptr;
}

_dbBlock* block = (_dbBlock*) inst->getOwner();
_dbModule* module = block->module_tbl_->getPtr(inst->module_);
_dbModule* module = block->module_tbl_->getPtr(inst->getModuleId());
return (dbModule*) module;
}

Expand Down Expand Up @@ -1072,7 +1101,7 @@ bool dbInst::isPhysicalOnly()
{
_dbInst* inst = (_dbInst*) this;

return inst->module_ == 0;
return inst->getModuleId() == 0;
}

dbInst* dbInst::getParent()
Expand Down Expand Up @@ -1589,7 +1618,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();
}
Expand Down
18 changes: 11 additions & 7 deletions src/odb/src/db/dbInst.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
namespace odb {

class _dbBox;
class _dbBlock;
class _dbInstHdr;
class _dbHier;
class _dbITerm;
Expand Down Expand Up @@ -72,14 +73,17 @@ 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.

dbId<_dbModule> getModuleId() const { return module_; }
void setModule(dbId<_dbModule> module);

private:
dbId<_dbModule> module_;
friend dbIStream& operator>>(dbIStream& stream, _dbBlock& block);
friend dbIStream& operator>>(dbIStream& stream, _dbInst& inst);
friend dbOStream& operator<<(dbOStream& stream, const _dbInst& inst);

public:
dbId<_dbGroup> group_;
dbId<_dbInst> region_next_;
dbId<_dbInst> module_next_;
Expand Down
24 changes: 11 additions & 13 deletions src/odb/src/db/dbModule.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ void dbModule::addInst(dbInst* inst)
getName());
}

if (_inst->module_ == module->getOID()) {
if (_inst->getModuleId() == module->getOID()) {
return; // already in this module
}

Expand All @@ -240,13 +240,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 = (_inst->getModuleId() != 0);
if (is_reparent) {
dbModule* mod = dbModule::getModule((dbBlock*) block, _inst->module_);
((_dbModule*) mod)->removeInst(inst);
dbModule* mod = dbModule::getModule((dbBlock*) block, _inst->getModuleId());
((_dbModule*) mod)->removeInst(inst, false);
}

_inst->module_ = module->getOID();
module->dbinst_hash_[inst->getName()] = dbId<_dbInst>(_inst->getOID());

if (module->insts_ == 0) {
Expand All @@ -260,20 +259,16 @@ void dbModule::addInst(dbInst* inst)
cur_head->module_prev_ = _inst->getOID();
}

if (is_reparent) {
for (dbBlockCallBackObj* cb : block->callbacks_) {
cb->inDbPostInstParentChange(inst);
}
}
_inst->setModule(module->getOID());
}

void _dbModule::removeInst(dbInst* inst)
void _dbModule::removeInst(dbInst* inst, bool clear_module)
{
_dbModule* module = (_dbModule*) this;
_dbInst* _inst = (_dbInst*) inst;
uint32_t id = _inst->getOID();

if (_inst->module_ != getOID()) {
if (_inst->getModuleId() != getOID()) {
return;
}

Expand All @@ -286,6 +281,7 @@ void _dbModule::removeInst(dbInst* inst)
}

_dbBlock* block = (_dbBlock*) getOwner();
module->dbinst_hash_.erase(inst->getName());
Comment thread
naveenvenk17 marked this conversation as resolved.

if (module->insts_ == id) {
module->insts_ = _inst->module_next_;
Expand All @@ -305,7 +301,9 @@ void _dbModule::removeInst(dbInst* inst)
prev->module_next_ = _inst->module_next_;
}
}
_inst->module_ = 0;
if (clear_module) {
_inst->setModule(dbId<_dbModule>());
}
_inst->module_next_ = 0;
_inst->module_prev_ = 0;
}
Expand Down
3 changes: 1 addition & 2 deletions src/odb/src/db/dbModule.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,7 @@ class _dbModule : public _dbObject
void collectMemInfo(MemInfo& info);
// User Code Begin Methods

// This is only used when destroying an inst
void removeInst(dbInst* inst);
void removeInst(dbInst* inst, bool clear_module = true);

// Copy and uniquify a given module based on current instance
using modBTMap = odb::PtrMap<dbModBTerm, dbModBTerm*>;
Expand Down
58 changes: 58 additions & 0 deletions src/odb/test/cpp/TestModule.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include "gtest/gtest.h"
#include "helper.h"
#include "odb/db.h"
#include "odb/dbBlockCallBackObj.h"
#include "odb/dbSet.h"

namespace odb {
Expand Down Expand Up @@ -235,6 +236,63 @@ class ModuleFixture : public SimpleDbFixture
dbBlock* block_;
};

class ParentChangeCallback : public dbBlockCallBackObj
{
public:
void inDbPostInstParentChange(dbInst* inst) override
{
calls++;
observed_module = inst->getModule();
}

int calls = 0;
dbModule* observed_module = nullptr;
};

TEST_F(ModuleFixture, reparent_updates_index_and_fires_callback)
{
dbModule* top = block_->getTopModule();
dbModule* child = dbModule::create(block_, "child");
dbInst* inst = dbInst::create(block_, lib_->findMaster("and2"), "inst");
ASSERT_NE(inst, nullptr);
ASSERT_EQ(top->findDbInst("inst"), inst);

ParentChangeCallback callback;
callback.addOwner(block_);

child->addInst(inst);
EXPECT_EQ(callback.calls, 1);
EXPECT_EQ(callback.observed_module, child);
EXPECT_EQ(inst->getModule(), child);
EXPECT_EQ(top->findDbInst("inst"), nullptr);
EXPECT_EQ(child->findDbInst("inst"), inst);

child->addInst(inst);
EXPECT_EQ(callback.calls, 1);

top->addInst(inst);
EXPECT_EQ(callback.calls, 2);
EXPECT_EQ(callback.observed_module, top);
EXPECT_EQ(child->findDbInst("inst"), nullptr);
EXPECT_EQ(top->findDbInst("inst"), inst);
}

TEST_F(ModuleFixture, rename_updates_module_index)
{
dbModule* top = block_->getTopModule();
dbInst* inst = dbInst::create(block_, lib_->findMaster("and2"), "old_name");
ASSERT_NE(inst, nullptr);
ASSERT_EQ(top->findDbInst("old_name"), inst);

ASSERT_TRUE(inst->rename("new_name"));
EXPECT_EQ(top->findDbInst("old_name"), nullptr);
EXPECT_EQ(top->findDbInst("new_name"), inst);

dbInst::destroy(inst);
EXPECT_EQ(top->findDbInst("old_name"), nullptr);
EXPECT_EQ(top->findDbInst("new_name"), nullptr);
}

TEST_F(ModuleFixture, test_default)
{
// dbModule::create() Succeed
Expand Down
Loading