Skip to content

mbff: Move IsValidTray back to mbff.cpp.#10682

Open
Logikable wants to merge 2 commits into
The-OpenROAD-Project:masterfrom
Logikable:dontuse
Open

mbff: Move IsValidTray back to mbff.cpp.#10682
Logikable wants to merge 2 commits into
The-OpenROAD-Project:masterfrom
Logikable:dontuse

Conversation

@Logikable

@Logikable Logikable commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

It was not respecting the resizer's dont_use, instead deferring to the LibertyCell's dont_use.

This issue was introduced in 89f47ff, and this PR reverts only the relevant portion of that commit.
This PR will only build once #10665 is submitted -- it depends on testCell being public.

It was not respecting the resizer's dont_use, instead deferring to the
LibertyCell's dont_use.

Signed-off-by: Sean Luchen <seanluchen@google.com>
@Logikable Logikable requested review from a team as code owners June 17, 2026 21:24
@Logikable Logikable requested review from gudeh and jhkim-pii June 17, 2026 21:24

@gemini-code-assist gemini-code-assist Bot left a comment

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.

Code Review

This pull request refactors the isValidTray function by moving it from dbNetwork to the MBFF class as IsValidTray, updating its usages and associated tests. The review feedback suggests adding a null check for the tray pointer in MBFF::IsValidTray to prevent potential null pointer dereferences.

Comment thread src/gpl/src/mbff.cpp
Comment on lines +2179 to +2181
bool MBFF::IsValidTray(dbInst* tray)
{
const sta::Cell* cell = network_->dbToSta(tray->getMaster());

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.

medium

Defensive programming: tray is a pointer passed to IsValidTray and could potentially be nullptr if the instance creation fails or if an invalid pointer is passed. We should add a null check before dereferencing it to prevent potential segmentation faults.

bool MBFF::IsValidTray(dbInst* tray)
{
  if (tray == nullptr) {
    return false;
  }
  const sta::Cell* cell = network_->dbToSta(tray->getMaster());

Signed-off-by: Sean Luchen <seanluchen@google.com>
@maliberty

Copy link
Copy Markdown
Member

@precisionmoon I feel that using LibertyCell::setDontUse from Resizer::setDontUse, and removing Resizer::dont_use_ is a better solution. It avoids having the "same" information in two places which inevitably leads to problems like this. Agree?

@Logikable

Copy link
Copy Markdown
Contributor Author

We'd prefer having just one dont-use as well.

Does this leave an easy way to do reset_dont_use?

@maliberty

Copy link
Copy Markdown
Member

I suppose it depends on the semantics of reset_dont_use. Does it reset only user defined such or does it include those that come from Liberty itself? If the former rsz would have to keep track of which cells were set but the actual value could stay in sta.

@Logikable

Copy link
Copy Markdown
Contributor Author

It does the former. It shouldn't be too hard to make it work, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants