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.

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

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

		@@ -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)) {

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!

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!

chatgpt-codex-connector bot Apr 19, 2026

Choose a reason for hiding this comment

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