Skip to content

Fix access point cleanup on instance removal #10179

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

Merged
osamahammad21 merged 9 commits into from
Apr 21, 2026
Merged

Fix access point cleanup on instance removal #10179

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9feed74
Fix access point cleanup on instance removal
jhkim-pii Apr 18, 2026
013482b
Address access point cleanup clang-tidy
jhkim-pii Apr 18, 2026
dbae4c1
Fix Bazel layering for access point test
jhkim-pii Apr 18, 2026
5e41e5f
Remove TODO from access point Bazel test
jhkim-pii Apr 18, 2026
20a850f
Clarify access point deduplication range
jhkim-pii Apr 18, 2026
92e85f5
Add ECO test for access point cleanup
jhkim-pii Apr 19, 2026
90d2355
Remove obsolete access point unit tests
jhkim-pii Apr 19, 2026
ea37d99
Clear preferred access points on master swap
jhkim-pii Apr 19, 2026
46ac421
Move access point swapMaster test to odb
jhkim-pii Apr 21, 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
18 changes: 8 additions & 10 deletions src/odb/src/db/dbInst.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Comment on lines +1219 to +1221

@chatgpt-codex-connector chatgpt-codex-connector Bot Apr 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Journal preferred AP cleanup in swapMaster

dbInst::swapMaster() now unconditionally clears each ITerm’s preferred access points before completing the swap, but this mutation is not captured as a journal field update. dbDatabase::undoEco() reverts swaps by calling swapMaster() again, so both the forward and undo paths clear AP metadata and the original preferred AP/back-reference state is never restored. In ECO trial flows that swap then undo (e.g., rejected sizing moves), this leaves the database observably different after undo and can break subsequent pin-access dependent logic.

Useful? React with 👍 / 👎.

@jhkim-pii jhkim-pii Apr 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@maliberty
It looks like ODB journal does not support many physical data structures including dbAccessPoint.
To support the journal, many changes are required (supporting journal just for swapMaster is incomplete. All the network edit operations should be considered).
Is it ok to proceed w/o the ODB journal support?

@maliberty maliberty Apr 20, 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.

Sure. We have only supported journaling on operations we actually undo but adding to the list anything else we need makes sense.

}

@jhkim-pii jhkim-pii Apr 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Change 1. Add preferred AP clean-up before swapping master.

  • I think this is the root-cause fix.
  • Without this, AP has a stale iterm pointer after swapMaster.

inst->pin_access_idx_ = -1;

@osamahammad21 osamahammad21 Apr 20, 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.

Should also invalidate the dbInst's pin_access_idx_ by setting it to -1 or std::numeric_limits<uint32_t>::max()
without this, calling iterm->getAccessPoints would return the invalid set of access points (pre swap).

// remove reference to inst_hdr
_dbInstHdr* old_inst_hdr
= block->inst_hdr_hash_.find(((_dbMaster*) old_master_)->id_);
Expand Down Expand Up @@ -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();

@osamahammad21 osamahammad21 Apr 18, 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.

Although this is a more efficient approach, the outcome theoretically should be exactly the same. The preferred access point of an iterm is always a subset of its total access points which is returned by getAccessPoints

@jhkim-pii jhkim-pii Apr 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah. It is a code reduction by using the existing API.
If you don't want this, I'll revert this change (not a bug fix).

Change 2: Less code. More intuitive to me.

  • This is not the root-cause fix.
  • IMO, this is more like a minor enhancement.

@osamahammad21 osamahammad21 Apr 20, 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.

It's ok. Just wanted to confirm that this is not intended as a fix.


// Notify when pins are deleted (assumption: pins are destroyed only when
// the related instance is destroyed)
Expand Down
10 changes: 7 additions & 3 deletions src/odb/src/db/dbMPin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

#include "dbMPin.h"

#include <cstddef>
#include <cstdint>
#include <vector>

Expand All @@ -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"
Expand Down Expand Up @@ -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;
}
Comment on lines +155 to 158

@osamahammad21 osamahammad21 Apr 18, 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.

I wonder what made you do this change? It is correct, but I wonder if there is any call that uses a negative pin_access_idx which would be more concerning.
Also, I think the more correct approach to make pin_access_idx argument a size_t or unsigned int instead.

@osamahammad21 osamahammad21 Apr 18, 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.

nvm, just found it's ai review triggered.

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));
}
Comment on lines +159 to 164

@osamahammad21 osamahammad21 Apr 18, 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.

Theoretically, there shouldn't be duplicate aps. This looks a bit defensive. I would prefer if such a case exists a crash rather than passing unnoticed.

Suggested change
dbVector<dbId<_dbAccessPoint>> aps;
aps.swap(pin->aps_[pin_access_idx]);
std::ranges::sort(aps);
const auto duplicate_aps = std::ranges::unique(aps);
aps.erase(duplicate_aps.begin(), duplicate_aps.end());
for (const dbId<_dbAccessPoint>& ap : aps) {
odb::dbAccessPoint::destroy(
(odb::dbAccessPoint*) block->ap_tbl_->getPtr(ap));
}
auto& aps = pin->aps_[pin_access_idx];
for (const auto& ap : aps) {
odb::dbAccessPoint::destroy(
(odb::dbAccessPoint*) block->ap_tbl_->getPtr(ap));
}
aps.clear();

@jhkim-pii jhkim-pii Apr 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You're right. The deduplication is too defensive, which should be removed.

Regarding your suggested code, I think it has a vector iterator invalidation risk.

auto& aps = pin->aps_[pin_access_idx];     // Used reference
for (const auto& ap : aps) {
   odb::dbAccessPoint::destroy( ... );       // destroy() updates `aps`, which invalidates the `aps` iteration
}
aps.clear();

To make it safe, a copy should be used: auto aps = pin->aps_[pin_access_idx]; // No &.

Or swap can be used to avoid the copy overhead.

  dbVector<dbId<_dbAccessPoint>> aps;
  aps.swap(pin->aps_[pin_access_idx]);                 // pin->aps_[pin_access_idx] will be empty.
  for (const dbId<_dbAccessPoint>& ap : aps) {
    odb::dbAccessPoint::destroy(
        (odb::dbAccessPoint*) block->ap_tbl_->getPtr(ap));
  }

Change 3: Use swap instead of copy.

  • This is not a root-cause fix.
  • This is just a minor change to avoid the small vector copy overhead.

Expand Down
45 changes: 45 additions & 0 deletions src/odb/test/cpp/TestAccessPoint.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
#include <cstdint>
#include <filesystem>
#include <fstream>
#include <ios>
Expand Down Expand Up @@ -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
Loading