Fix access point cleanup on instance removal by openroad-ci · Pull Request #10179 · The-OpenROAD-Project/OpenROAD

openroad-ci · 2026-04-18T09:02:31Z

Summary

Found odb crash during new Resizer architecture testing.
Clear ITerm preferred access point back-references when destroying an instance.
Clear and de-duplicate MPin pin-access entries before destroying access points.
Add ODB tests that cover stale AP/ITerm references and repeated pin-access cleanup.

Problem

ORFS nangate45/gcd crashed in 5_1_grt while repair_timing triggered parasitic updates, incremental global routing, and DRT dirty pin-access database updates.
The crash was reproduced with FLOW_VARIANT=legacy_mt and RSZ_POLICY=legacy_mt. The same 4_cts.odb/.sdc input passed with RSZ_POLICY=legacy, which ruled out an already-corrupt input database.
The minimal failing move sequence for legacy_mt was sizeup,buffer, so the multi-threaded legacy repair flow exposed stale OpenDB access point bookkeeping during repeated ECO pin-access updates.

Symptom:

repair_timing -setup_margin 0 -hold_margin 0 -repair_tns 100 -verbose
[INFO RSZ-0100] Repair move sequence: UnbufferMove SizeUpMove SwapPinsMove BufferMove CloneMove SplitLoadMove
...
openroad: .../src/odb/src/db/dbTable.inc:47:
T* odb::dbTable<T, page_size>::getPtr(odb::dbId<T>) const [with T = odb::_dbITerm; ...]:
Assertion `p->offset_in_bytes_ & kAllocBit' failed.
Signal 6 received

Call stack:

odb::dbMPin::clearPinAccess(int)
odb::dbMaster::clearPinAccess(int)
drt::io::Writer::updateDbAccessPoints(odb::dbBlock*, odb::dbTech*)
drt::io::Writer::updateDb(odb::dbDatabase*, drt::RouterConfiguration*, bool, bool)
drt::TritonRoute::updateDirtyPAData()
utl::CallBackHandler::triggerOnPinAccessUpdateRequired()
grt::GlobalRouter::updateDirtyRoutes(bool)
grt::IncrementalGRoute::updateRoutes(bool)
est::EstimateParasitics::updateParasitics()
...
rsz::Resizer::repairSetup(...)
rsz::repair_setup(...)

Root-cause

OpenDB access point cleanup did not fully maintain the bidirectional references among MPin pin-access slots, ITerm preferred access points, and AccessPoint ITerm back-references.

dbMPin::clearPinAccess() destroyed the dbAccessPoint objects stored in pin->aps_[pin_access_idx], but it did not clear the MPin AP id vector for that pin-access slot. A later update of the same pin-access index could revisit stale AP ids and try to destroy already-destroyed access points.
dbInst::destroy() tried to remove AP back-references before ITerm deletion, but it used iterm->getAccessPoints(). That API derives APs from the instance's current pin-access index, so it can miss preferred APs stored in the ITerm aps_ map for other pin-access indices.
When such an ITerm was destroyed, stale ITerm ids could remain in _dbAccessPoint::iterms_. Later, dbAccessPoint::destroy() iterated _ap->iterms_ and dereferenced an already-destroyed ITerm through block->iterm_tbl_->getPtr(), hitting the OpenDB allocation assert.

Solution

In dbMPin::clearPinAccess(), move the AP ids out of pin->aps_[pin_access_idx] before destroying the AP objects. This leaves the MPin slot empty immediately, prevents stale id reuse on repeated cleanup, and avoids iterator invalidation while dbAccessPoint::destroy() updates MPin bookkeeping.
Sort and de-duplicate the copied AP id list before destruction so duplicated AP bookkeeping cannot cause a double destroy.
In dbInst::destroy(), replace the current-pin-access-index cleanup with iterm->clearPrefAccessPoints(). That method walks the ITerm's preferred AP map directly and removes this ITerm id from every referenced AP back-reference list before the ITerm object is destroyed.
Add targeted TestAccessPoint regression coverage for idempotent MPin pin-access cleanup, duplicate AP ids in an MPin pin-access slot, and stale AP back-reference removal when an instance is destroyed.

Testing

cmake --build build --target TestAccessPoint -j4
./build/src/odb/test/cpp/TestAccessPoint --gtest_filter='*clear_mpin_access_points_is_idempotent:*clear_mpin_access_points_removes_duplicate_references:*destroy_inst_removes_access_point_back_references:*test_default'
bazel test //src/odb/test/cpp:TestAccessPoint

The new stale back-reference test fails on origin/master HEAD a00a5750166b before the fix and passes with this branch.

Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

jhkim-pii · 2026-04-18T09:04:05Z

I found this issue during new Resizer architecture testing.

@eder-matheus I added you as a reviewer because I found that you edited the related codes a week ago.
Please add an appropriate review if you are not the right person. Thank you in advance.

gemini-code-assist

Code Review

This pull request refactors access point management in dbInst and dbMPin to ensure idempotency and correctly handle duplicate references. Key changes include updating dbMPin::clearPinAccess to sort and deduplicate access points before destruction and replacing manual cleanup in dbInst with a centralized method call. New unit tests were added to verify these improvements. The review feedback suggests hardening the index check in dbMPin::clearPinAccess against negative values and using structured bindings with std::ranges::unique to enhance code readability.

gemini-code-assist · 2026-04-18T09:05:03Z

@@ -153,8 +155,11 @@ void dbMPin::clearPinAccess(const int pin_access_idx)
  if (pin->aps_.size() <= pin_access_idx) {


The comparison between pin->aps_.size() (which is unsigned) and pin_access_idx (which is a signed int) can lead to unexpected behavior if pin_access_idx is negative. While it currently works "by accident" for -1 (as it promotes to a large unsigned value), it is safer and clearer to explicitly check for negative values or change the parameter type to uint32_t to match dbInst::getPinAccessIdx().

Suggested change

if (pin->aps_.size() <= pin_access_idx) {

if (pin_access_idx < 0 || pin->aps_.size() <= static_cast<uint32_t>(pin_access_idx)) {

gemini-code-assist · 2026-04-18T09:05:03Z

+  dbVector<dbId<_dbAccessPoint>> aps;
+  aps.swap(pin->aps_[pin_access_idx]);
+  std::ranges::sort(aps);
+  aps.erase(std::ranges::unique(aps).begin(), aps.end());


Using structured bindings with std::ranges::unique would improve readability by avoiding the explicit call to .begin() on the returned subrange.

auto [unique_end, original_end] = std::ranges::unique(aps); aps.erase(unique_end, original_end);

github-actions · 2026-04-18T09:07:14Z

clang-tidy review says "All clean, LGTM! 👍"

Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

github-actions · 2026-04-18T09:28:27Z

clang-tidy review says "All clean, LGTM! 👍"

maliberty · 2026-04-18T14:17:03Z

    srcs = [
        "TestAccessPoint.cpp",
    ],
+    features = ["-layering_check"],


The test added private include statements that Bazel disallows by default.

#include "../../src/db/dbAccessPoint.h" #include "../../src/db/dbMPin.h"

-layering_check is added to avoid the Bazel issue.

maliberty · 2026-04-18T14:21:30Z

@codex review

maliberty · 2026-04-18T14:21:47Z

Why do we have duplicate APs in the first place?

chatgpt-codex-connector · 2026-04-18T14:26:24Z

Codex Review: Didn't find any major issues. Swish!

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

osamahammad21

The only effective change of this PR is clearing the MPin's access points at the end of dbMPin::clearPinAccess
However, the other changes and the test case seem to be defensive, which I generally don't prefer.

dbInst::destroy() tried to remove AP back-references before ITerm deletion, but it used iterm->getAccessPoints(). That API derives APs from the instance's current pin-access index, so it can miss preferred APs stored in the ITerm aps_ map for other pin-access indices.

What do you mean by other pin-access indices? Each instance has one and only one pin access index, so its iterms.

Sort and de-duplicate the copied AP id list before destruction so duplicated AP bookkeeping cannot cause a double destroy.

There should never be duplicate APs. If there is, then this needs to be addressed.

osamahammad21 · 2026-04-18T14:44:30Z

+  if (pin_access_idx < 0
+      || pin->aps_.size() <= static_cast<std::size_t>(pin_access_idx)) {
    return;
  }


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.

nvm, just found it's ai review triggered.

osamahammad21 · 2026-04-18T15:07:47Z

-        }
-      }
-    }
+    iterm->clearPrefAccessPoints();


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

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.

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

osamahammad21 · 2026-04-18T15:15:56Z

+  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));
  }


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();

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.

osamahammad21 · 2026-04-18T16:35:47Z

The only effective change of this PR is clearing the MPin's access points at the end of dbMPin::clearPinAccess However, the other changes and the test case seem to be defensive, which I generally don't prefer.

dbInst::destroy() tried to remove AP back-references before ITerm deletion, but it used iterm->getAccessPoints(). That API derives APs from the instance's current pin-access index, so it can miss preferred APs stored in the ITerm aps_ map for other pin-access indices.

What do you mean by other pin-access indices? Each instance has one and only one pin access index, so its iterms.

Sort and de-duplicate the copied AP id list before destruction so duplicated AP bookkeeping cannot cause a double destroy.

There should never be duplicate APs. If there is, then this needs to be addressed.

@jhkim-pii Actually after checking the code, I found that erasing the mpin's aps list should be done by dbAccessPoint::destroy. I'll try to reproduce and investigate further.

jhkim-pii · 2026-04-19T05:48:34Z

Why do we have duplicate APs in the first place?

There should be no duplicate as Osama mentioned. The deduplication code is redundant.

Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

jhkim-pii · 2026-04-19T12:46:48Z

@codex review

chatgpt-codex-connector · 2026-04-19T12:46:52Z

To use Codex here, create a Codex account and connect to github.

jhkim-pii · 2026-04-19T12:52:33Z

+
+  EXPECT_EQ(block_->findInst("buf1"), nullptr);
+  EXPECT_TRUE(ap_impl->iterms_.empty());
+}


Made a new test case that catches the problematic sequence.

Execute pin access

Do swapMaster (BUF_X1 -> BUF_X4)

The preferred AP of the old iterm BUF_X1/A is not cleared properly. The AP has a stale pointer to the old iterm BUF_X1/A.

Remove the buffer

When the buffer is destroyed, the APs related to the buffer instance should be deleted.

But during the AP deletion, the AP's stale iterm pointer (BUF_X1/A) is accessed --> assert fail.

github-actions · 2026-04-19T12:53:34Z

clang-tidy review says "All clean, LGTM! 👍"

jhkim-pii · 2026-04-19T12:54:18Z

+  for (const uint32_t iterm_id : inst->iterms_) {
+    dbITerm* iterm = (dbITerm*) block->iterm_tbl_->getPtr(iterm_id);
+    iterm->clearPrefAccessPoints();
+  }


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.

jhkim-pii · 2026-04-19T13:02:02Z

@osamahammad21 My previous understanding about the problem was wrong.
I removed the previous C++ unit tests and added a new test case that captures the problematic sequence. Please refer to my comment at the new test case code.

Problematic sequence

Execute pin access
Do swapMaster (BUF_X1 -> BUF_X4)

The preferred AP of the old iterm BUF_X1/A is not cleared properly. The AP has a stale pointer to the old iterm BUF_X1/A.

Remove the buffer

When the buffer is destroyed, the APs related to the buffer instance should be deleted.
But during the AP deletion, the AP's stale iterm pointer (BUF_X1/A) is accessed --> assert fail.

This PR has three changes.

Change 1. Root-cause fix - Add preferred AP clean-up in dbInst::swapMaster().
Change 2. Minor - iterm->clearPrefAccessPoints() in dbInst::destroy().
Change 3. Minor - aps.swap(pin->aps_[pin_access_idx]) in dbMPin::clearPinAccess()

maliberty · 2026-04-19T15:44:51Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ea37d99fa5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-04-19T15:52:16Z

+  for (const uint32_t iterm_id : inst->iterms_) {
+    dbITerm* iterm = (dbITerm*) block->iterm_tbl_->getPtr(iterm_id);
+    iterm->clearPrefAccessPoints();


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 👍 / 👎.

@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?

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

osamahammad21

Nice catch, The PR makes much more sense to me now, and it looks like it's fixing a real bug. That said, I don't think TestBufferRemoval is an effective test case here. It's essentially only testing odb functionality and doesn't depend on rsz or drt. If the point of the test is to validate the correctness of the swapMaster API and its effect on access points (db's), it should live in odb's test suite.

osamahammad21 · 2026-04-20T14:25:45Z

+    dbITerm* iterm = (dbITerm*) block->iterm_tbl_->getPtr(iterm_id);
+    iterm->clearPrefAccessPoints();
+  }
+


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).

osamahammad21 · 2026-04-20T14:26:25Z

-        }
-      }
-    }
+    iterm->clearPrefAccessPoints();


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

Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

github-actions · 2026-04-21T11:25:01Z

clang-tidy review says "All clean, LGTM! 👍"

jhkim-pii · 2026-04-21T12:07:56Z

@osamahammad21 Thank you for your detailed review!

osamahammad21 · 2026-04-21T12:37:52Z

@osamahammad21 Thank you for your detailed review!

@jhkim-pii You're welcome :)

jhkim-pii added 4 commits April 18, 2026 12:23

Fix access point cleanup on instance removal

9feed74

Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

Address access point cleanup clang-tidy

013482b

Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

Fix Bazel layering for access point test

dbae4c1

Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

Remove TODO from access point Bazel test

5e41e5f

Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

openroad-ci assigned jhkim-pii Apr 18, 2026

github-actions Bot added the size/S label Apr 18, 2026

jhkim-pii requested review from eder-matheus and maliberty April 18, 2026 09:03

gemini-code-assist Bot reviewed Apr 18, 2026

View reviewed changes

Clarify access point deduplication range

20a850f

Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

maliberty reviewed Apr 18, 2026

View reviewed changes

maliberty requested a review from osamahammad21 April 18, 2026 14:22

osamahammad21 requested changes Apr 18, 2026

View reviewed changes

jhkim-pii added 3 commits April 19, 2026 20:17

Add ECO test for access point cleanup

92e85f5

Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

Remove obsolete access point unit tests

90d2355

Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

Clear preferred access points on master swap

ea37d99

Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

github-actions Bot added size/M and removed size/S labels Apr 19, 2026

jhkim-pii reviewed Apr 19, 2026

View reviewed changes

jhkim-pii requested a review from osamahammad21 April 19, 2026 13:02

chatgpt-codex-connector Bot reviewed Apr 19, 2026

View reviewed changes

osamahammad21 requested changes Apr 20, 2026

View reviewed changes

Move access point swapMaster test to odb

46ac421

Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

github-actions Bot added size/S and removed size/M labels Apr 21, 2026

osamahammad21 approved these changes Apr 21, 2026

View reviewed changes

osamahammad21 merged commit 5f65c48 into The-OpenROAD-Project:master Apr 21, 2026
16 checks passed

openroad-ci deleted the secure-odb-access-point-cleanup branch April 21, 2026 12:38

		@@ -153,8 +155,11 @@ void dbMPin::clearPinAccess(const int pin_access_idx)
		if (pin->aps_.size() <= pin_access_idx) {

	if (pin->aps_.size() <= pin_access_idx) {
	if (pin_access_idx < 0 \|\| pin->aps_.size() <= static_cast<uint32_t>(pin_access_idx)) {

Uh oh!

Conversation

openroad-ci commented Apr 18, 2026

Summary

Problem

Root-cause

Solution

Testing

Uh oh!

jhkim-pii commented Apr 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Apr 18, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Apr 18, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Apr 18, 2026

Uh oh!

github-actions Bot commented Apr 18, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maliberty commented Apr 18, 2026

Uh oh!

maliberty commented Apr 18, 2026

Uh oh!

chatgpt-codex-connector Bot commented Apr 18, 2026

Uh oh!

osamahammad21 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhkim-pii Apr 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhkim-pii Apr 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

osamahammad21 commented Apr 18, 2026

Uh oh!

jhkim-pii commented Apr 19, 2026

Uh oh!

jhkim-pii commented Apr 19, 2026

Uh oh!

chatgpt-codex-connector Bot commented Apr 19, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Apr 19, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhkim-pii commented Apr 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maliberty commented Apr 19, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

jhkim-pii commented Apr 18, 2026 •

edited

Loading

jhkim-pii Apr 19, 2026 •

edited

Loading

jhkim-pii Apr 19, 2026 •

edited

Loading

jhkim-pii commented Apr 19, 2026 •

edited

Loading