Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions src/dbSta/test/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,7 @@ cc_test(
"Nangate45/Nangate45.lef",
"Nangate45/Nangate45_typ.lib",
"cpp/TestDbSta_0.v",
"cpp/TestDbSta_StalePrevPath.v",
],
deps = [
"//src/dbSta",
Expand Down
72 changes: 72 additions & 0 deletions src/dbSta/test/cpp/TestDbSta.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,12 @@
#include "db_sta/dbNetwork.hh"
#include "gtest/gtest.h"
#include "odb/db.h"
#include "odb/dbTypes.h"
#include "sta/Graph.hh"
#include "sta/NetworkClass.hh"
#include "sta/Path.hh"
#include "sta/SdcClass.hh"
#include "sta/Sta.hh"
#include "tst/IntegratedFixture.h"

namespace sta {
Expand Down Expand Up @@ -97,4 +102,71 @@ TEST_F(TestDbSta, TestHierarchyConnectivity)
ASSERT_EQ(bterm_clk->getITerm(), nullptr);
}

// Regression for #10210 (stale Path* dereference in rsz).
//
// Topology (TestDbSta_StalePrevPath.v):
// clk -> b1(BUF) -> inv1(INV) -> nd1(NAND2) -> out1
// nd1/A2 <- in2
//
// Flow:
// 1. Capture drvr_path at nd1/ZN and snapshot prevPath() pointer + pin name
// 2. Delete upstream b1 + updateTiming -> free
// 3. Add a fresh BUF + clock + updateTiming -> recycle
// 4. Assert the captured Path's prev slot has been recycled: pin()
// decodes to data that belongs to a different instance than nd1's
// real input.
TEST_F(TestDbSta, StalePrevPath)
{
const auto* test_info = testing::UnitTest::GetInstance()->current_test_info();
const std::string test_name
= std::string(test_info->test_suite_name()) + "_" + test_info->name();
readVerilogAndSetup(test_name + ".v");
sta_->updateTiming(true);

Network* network = sta_->network();

Instance* nd1 = db_network_->dbToSta(block_->findInst("nd1"));
Path* drvr_path = sta_->vertexWorstArrivalPath(
sta_->ensureGraph()->pinDrvrVertex(network->findPin(nd1, "ZN")),
MinMax::max());
ASSERT_NE(drvr_path, nullptr);
ASSERT_EQ(network->pathName(drvr_path->pin(sta_.get())), "nd1/ZN");
const Path* pre_addr = drvr_path->prevPath();
ASSERT_NE(pre_addr, nullptr);
const std::string pre_pin_name = network->pathName(pre_addr->pin(sta_.get()));

// 2. Free upstream Path[] slots.
sta_->deleteInstance(db_network_->dbToSta(block_->findInst("b1")));
sta_->updateTiming(true);

// 3. Recycle freed slots via a single fresh BUF driven by a new clock.
odb::dbNet* in3_net = odb::dbNet::create(block_, "in3");
odb::dbBTerm* new_bt = odb::dbBTerm::create(in3_net, "in3");
new_bt->setIoType(odb::dbIoType::INPUT);
odb::dbNet* nfan_net = odb::dbNet::create(block_, "nfan");
odb::dbInst* bnew
= odb::dbInst::create(block_, db_->findMaster("BUF_X1"), "bnew");
bnew->findITerm("A")->connect(in3_net);
bnew->findITerm("Z")->connect(nfan_net);

PinSet clk2_pins(db_network_);
clk2_pins.insert(db_network_->dbToSta(new_bt));
FloatSeq clk2_waveform = {0.0f, 0.1f};
sta_->makeClock(
"clk2", clk2_pins, false, 0.2f, clk2_waveform, "", sta_->cmdMode());
sta_->updateTiming(true);

// 4. Staleness evidence. Pointer address is same but pin name has changed.
const Path* post_addr = drvr_path->prevPath();
const std::string post_pin_name
= post_addr ? network->pathName(post_addr->pin(sta_.get()))
: std::string("<null>");

EXPECT_EQ(pre_addr, post_addr)
<< "stale-pointer signature: prev_path_ address unchanged";
EXPECT_NE(pre_pin_name, post_pin_name)
<< "but slot content should differ after free+reuse. before="
<< pre_pin_name << " after=" << post_pin_name;
}

} // namespace sta
18 changes: 18 additions & 0 deletions src/dbSta/test/cpp/TestDbSta_StalePrevPath.v
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
module top (clk,
in2,
out1);
input clk;
input in2;
output out1;

wire n0;
wire n1;

BUF_X1 b1 (.A(clk),
.Z(n0));
INV_X1 inv1 (.A(n0),
.ZN(n1));
NAND2_X1 nd1 (.A1(n1),
.A2(in2),
.ZN(out1));
endmodule
9 changes: 5 additions & 4 deletions src/rsz/src/RecoverPower.cc
Original file line number Diff line number Diff line change
Expand Up @@ -328,10 +328,11 @@ bool RecoverPower::downsizeDrvr(const sta::Path* drvr_path,
sta::Instance* drvr = network_->instance(drvr_pin);
const float load_cap = graph_delay_calc_->loadCap(
drvr_pin, drvr_path->scene(sta_), drvr_path->minMax(sta_));
const int in_index = drvr_index - 1;
const sta::Path* in_path = expanded->path(in_index);
const sta::Pin* in_pin = in_path->pin(sta_);
const sta::LibertyPort* in_port = network_->libertyPort(in_pin);
const sta::TimingArc* in_arc = drvr_path->prevArc(sta_);
const sta::LibertyPort* in_port = in_arc ? in_arc->from() : nullptr;
if (in_port == nullptr) {
return false;
}
if (!resizer_->dontTouch(drvr)) {
float prev_drive = 0.0;
if (drvr_index >= 2) {
Expand Down
14 changes: 8 additions & 6 deletions src/rsz/src/SizeUpMove.cc

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.

early-return on in_arc == nullptr in all 4 sites (genclk source / latch
D->Q roots) means those paths are now skipped by size/swap/unbuffer/recover-power
instead of using whatever the old prevPath()->pin() returned. Any QoR delta on
designs with generated clocks or latch-based pipelines

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.

No expected QoR delta.

in_arc == nullptr only fires at genuine path startpoints with no upstream arc. Latch D→Q has a real TimingArcprevArc() returns it, unaffected. Clock-source BTerms are filtered earlier by isTopLevelPort/isLogicStdCell. Generated-clock source pins either go through a divider cell (has input→output arc) or are BTerms (filtered).

At those true startpoints the old code wasn't doing useful work anyway — prevPath()->pin() was either null (crash) or an unrelated pin feeding the wrong port into upsizeCell (crash or luck-based sizing). New code just skips cleanly.

Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include "sta/Liberty.hh"
#include "sta/NetworkClass.hh"
#include "sta/Path.hh"
#include "sta/TimingArc.hh"
#include "utl/Logger.h"

namespace rsz {
Expand Down Expand Up @@ -48,26 +49,27 @@ bool SizeUpMove::doMove(const sta::Path* drvr_path, float setup_slack_margin)
return false;
}

sta::Pin* drvr_input_pin = drvr_path->prevPath()->pin(sta_);
const sta::TimingArc* in_arc = drvr_path->prevArc(sta_);
sta::LibertyPort* in_port = in_arc ? in_arc->from() : nullptr;
if (in_port == nullptr) {
return false;
}

sta::Path* prev_drvr_path = drvr_path->prevPath()->prevPath();
sta::Pin* prev_drvr_pin
= prev_drvr_path ? prev_drvr_path->pin(sta_) : nullptr;

float prev_drive;
float prev_drive = 0.0;
if (prev_drvr_pin) {
prev_drive = 0.0;
sta::LibertyPort* prev_drvr_port = network_->libertyPort(prev_drvr_pin);
if (prev_drvr_port) {
prev_drive = prev_drvr_port->driveResistance();
}
} else {
prev_drive = 0.0;
}

sta::Scene* scene = drvr_path->scene(sta_);
const sta::MinMax* min_max = drvr_path->minMax(sta_);
const float load_cap = graph_delay_calc_->loadCap(drvr_pin, scene, min_max);
sta::LibertyPort* in_port = network_->libertyPort(drvr_input_pin);
sta::LibertyPort* drvr_port = network_->libertyPort(drvr_pin);
sta::LibertyCell* upsize
= upsizeCell(in_port, drvr_port, load_cap, prev_drive, scene, min_max);
Expand Down
8 changes: 5 additions & 3 deletions src/rsz/src/SwapPinsMove.cc
Original file line number Diff line number Diff line change
Expand Up @@ -95,10 +95,12 @@ bool SwapPinsMove::doMove(const sta::Path* drvr_path, float setup_slack_margin)
sta::Scene* scene = drvr_path->scene(sta_);
const sta::MinMax* min_max = drvr_path->minMax(sta_);
const float load_cap = graph_delay_calc_->loadCap(drvr_pin, scene, min_max);
sta::Pin* drvr_input_pin = drvr_path->prevPath()->pin(sta_);

// We get the driver port and the cell for that port.
sta::LibertyPort* input_port = network_->libertyPort(drvr_input_pin);
const sta::TimingArc* in_arc = drvr_path->prevArc(sta_);
sta::LibertyPort* input_port = in_arc ? in_arc->from() : nullptr;
if (input_port == nullptr) {
return false;
}
sta::LibertyPort* swap_port = input_port;
LibertyPortVec ports;

Expand Down
8 changes: 7 additions & 1 deletion src/rsz/src/UnbufferMove.cc
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include "sta/Mode.hh"
#include "sta/NetworkClass.hh"
#include "sta/Path.hh"
#include "sta/TimingArc.hh"
#include "sta/Transition.hh"
#include "utl/Logger.h"

Expand Down Expand Up @@ -99,7 +100,12 @@ bool UnbufferMove::doMove(const sta::Path* drvr_path, float setup_slack_margin)

// Don't remove buffer if new max fanout violations are created
sta::Vertex* drvr_vertex = graph_->pinDrvrVertex(drvr_pin);
sta::Pin* drvr_input_pin = drvr_path->prevPath()->pin(sta_);
const sta::TimingArc* in_arc = drvr_path->prevArc(sta_);
const sta::LibertyPort* in_lib_port = in_arc ? in_arc->from() : nullptr;
if (in_lib_port == nullptr) {
return false;
}
sta::Pin* drvr_input_pin = network_->findPin(drvr, in_lib_port);

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.

behavior change: drvr_input_pin used to come from prevPath()->pin() and is now network_->findPin(drvr, in_lib_port) - always a pin on the driver instance. This flows into BaseMove.cc:413 (side_input_pin == params.driver_input_pin skip). This now refers to a different thing than before - intentional? Wondering if it changes the skip behavior on side-fanouts where the old code compared against the wrong pin.

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.

Yes, intentional.

driver_input_pin, as the name suggests, is meant to be the driver cell's input pin on the current path.

The old prevPath()->pin() happened to return that in stable STA state, but not robustly (Please check my latest comment)

  • In PathExpanded cases where cell-internal pins get collapsed, prev is a load pin on a different instance — the == driver_input_pin comparison never matches the real driver input, and may spuriously skip an unrelated fanout.

network_->findPin(drvr, in_arc->from()) is "the input port of the arc ending at drvr" resolved against the current driver instance. Correct pin by construction, regardless of PathExpanded quirks or allocator behavior. The skip now does what the variable name promises — so side-fanout iteration in repair_timing should be strictly more correct after this change.

sta::Path* prev_drvr_path = drvr_path->prevPath()->prevPath();
Comment thread
maliberty marked this conversation as resolved.
sta::Pin* prev_drvr_pin
= prev_drvr_path ? prev_drvr_path->pin(sta_) : nullptr;
Expand Down
Loading