From e985e46107c103e703bc9ff355af3b5cebfcc415 Mon Sep 17 00:00:00 2001 From: Minju Kim Date: Tue, 21 Apr 2026 15:36:23 +0900 Subject: [PATCH 01/14] rsz: use prev_arc input port instead of prevPath pin for driver input The driver cell's input LibertyPort on a timing path is read from drvr_path->prevPath()->pin() in SizeUpMove, SwapPinsMove, UnbufferMove and RecoverPower::downsizeDrvr. When PathExpanded collapses intermediate cell pins, that pin can belong to a different instance than the driver, yielding a port name that does not exist on the driver's swappable cells. upsizeCell/downsizeCell then call LibertyCell::findLibertyPort with that name, which returns nullptr, and the subsequent capacitance() dereference crashes with SIGSEGV. Read the driver's input port from drvr_path->prevArc()->from() instead. The timing arc is carried on the path itself and is immune to PathExpanded pin collapsing. Also bail out early when prevArc is null (generated clock source or latch D->Q roots). See issue #10210. Signed-off-by: Minju Kim --- src/rsz/src/RecoverPower.cc | 9 +++++---- src/rsz/src/SizeUpMove.cc | 12 ++++++------ src/rsz/src/SwapPinsMove.cc | 8 +++++--- src/rsz/src/UnbufferMove.cc | 5 ++++- 4 files changed, 20 insertions(+), 14 deletions(-) diff --git a/src/rsz/src/RecoverPower.cc b/src/rsz/src/RecoverPower.cc index bc5c3326a3b..d181482f822 100644 --- a/src/rsz/src/RecoverPower.cc +++ b/src/rsz/src/RecoverPower.cc @@ -328,10 +328,8 @@ 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 (!resizer_->dontTouch(drvr)) { float prev_drive = 0.0; if (drvr_index >= 2) { @@ -344,6 +342,9 @@ bool RecoverPower::downsizeDrvr(const sta::Path* drvr_path, prev_drive = prev_drvr_port->driveResistance(); } } + if (in_port == nullptr) { + return false; + } const sta::LibertyPort* drvr_port = network_->libertyPort(drvr_pin); const sta::LibertyCell* downsize = downsizeCell(in_port, drvr_port, diff --git a/src/rsz/src/SizeUpMove.cc b/src/rsz/src/SizeUpMove.cc index ed05ce9ff6b..f796ae06db9 100644 --- a/src/rsz/src/SizeUpMove.cc +++ b/src/rsz/src/SizeUpMove.cc @@ -48,26 +48,26 @@ 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; 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; } + if (in_port == nullptr) { + return false; + } 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); diff --git a/src/rsz/src/SwapPinsMove.cc b/src/rsz/src/SwapPinsMove.cc index 3c648ce0f14..19f856ea35f 100644 --- a/src/rsz/src/SwapPinsMove.cc +++ b/src/rsz/src/SwapPinsMove.cc @@ -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; diff --git a/src/rsz/src/UnbufferMove.cc b/src/rsz/src/UnbufferMove.cc index b8bc03bd709..ec560d8dcc7 100644 --- a/src/rsz/src/UnbufferMove.cc +++ b/src/rsz/src/UnbufferMove.cc @@ -99,7 +99,10 @@ 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; + sta::Pin* drvr_input_pin + = in_lib_port ? network_->findPin(drvr, in_lib_port) : nullptr; sta::Path* prev_drvr_path = drvr_path->prevPath()->prevPath(); sta::Pin* prev_drvr_pin = prev_drvr_path ? prev_drvr_path->pin(sta_) : nullptr; From 69f0889f6e188527cbd7d361f0c9d33600e4b572 Mon Sep 17 00:00:00 2001 From: Minju Kim Date: Tue, 21 Apr 2026 17:28:49 +0900 Subject: [PATCH 02/14] rsz: add missing sta/TimingArc.hh include Fixes clang-tidy misc-include-cleaner warning: these files call drvr_path->prevArc(sta_)->from() and use sta::TimingArc directly, so the header must be included directly rather than via transitive include. Signed-off-by: Minju Kim --- src/rsz/src/SizeUpMove.cc | 1 + src/rsz/src/UnbufferMove.cc | 1 + 2 files changed, 2 insertions(+) diff --git a/src/rsz/src/SizeUpMove.cc b/src/rsz/src/SizeUpMove.cc index f796ae06db9..93cbf519ae0 100644 --- a/src/rsz/src/SizeUpMove.cc +++ b/src/rsz/src/SizeUpMove.cc @@ -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 { diff --git a/src/rsz/src/UnbufferMove.cc b/src/rsz/src/UnbufferMove.cc index ec560d8dcc7..b7713fa20bd 100644 --- a/src/rsz/src/UnbufferMove.cc +++ b/src/rsz/src/UnbufferMove.cc @@ -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" From 2ef3a8ac9575ee970085970530f5c3cdf09b34a4 Mon Sep 17 00:00:00 2001 From: Minju Kim Date: Wed, 22 Apr 2026 14:15:07 +0900 Subject: [PATCH 03/14] rsz: hoist null in_port check before prevPath()->prevPath() chain When drvr_path->prevArc() returns null (rare genclk source / latch D->Q), prevPath() is also null and dereferencing it in the subsequent prevPath()->prevPath() call would SIGSEGV. Hoist the null-check on the resolved input LibertyPort (or LibertyPort+pin in UnbufferMove) to immediately after it is computed, before any further path traversal. Also matches the pattern already used in SwapPinsMove and avoids wasted prev_drive / prev_drvr work on the reject path. In UnbufferMove this also keeps drvr_input_pin non-null on the happy path, so estimatedSlackOK correctly skips the buffer instance being removed when scanning the driving net's fanouts. Addresses PR review feedback on #10225 (issue #10210). Signed-off-by: Minju Kim --- src/rsz/src/RecoverPower.cc | 6 +++--- src/rsz/src/SizeUpMove.cc | 7 ++++--- src/rsz/src/UnbufferMove.cc | 6 ++++-- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/src/rsz/src/RecoverPower.cc b/src/rsz/src/RecoverPower.cc index d181482f822..a626af6d317 100644 --- a/src/rsz/src/RecoverPower.cc +++ b/src/rsz/src/RecoverPower.cc @@ -330,6 +330,9 @@ bool RecoverPower::downsizeDrvr(const sta::Path* drvr_path, drvr_pin, drvr_path->scene(sta_), drvr_path->minMax(sta_)); 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) { @@ -342,9 +345,6 @@ bool RecoverPower::downsizeDrvr(const sta::Path* drvr_path, prev_drive = prev_drvr_port->driveResistance(); } } - if (in_port == nullptr) { - return false; - } const sta::LibertyPort* drvr_port = network_->libertyPort(drvr_pin); const sta::LibertyCell* downsize = downsizeCell(in_port, drvr_port, diff --git a/src/rsz/src/SizeUpMove.cc b/src/rsz/src/SizeUpMove.cc index 93cbf519ae0..15899cd5f09 100644 --- a/src/rsz/src/SizeUpMove.cc +++ b/src/rsz/src/SizeUpMove.cc @@ -51,6 +51,10 @@ bool SizeUpMove::doMove(const sta::Path* drvr_path, float setup_slack_margin) 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; @@ -63,9 +67,6 @@ bool SizeUpMove::doMove(const sta::Path* drvr_path, float setup_slack_margin) } } - if (in_port == nullptr) { - return false; - } 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); diff --git a/src/rsz/src/UnbufferMove.cc b/src/rsz/src/UnbufferMove.cc index b7713fa20bd..6baaa26ac1f 100644 --- a/src/rsz/src/UnbufferMove.cc +++ b/src/rsz/src/UnbufferMove.cc @@ -102,8 +102,10 @@ bool UnbufferMove::doMove(const sta::Path* drvr_path, float setup_slack_margin) sta::Vertex* drvr_vertex = graph_->pinDrvrVertex(drvr_pin); const sta::TimingArc* in_arc = drvr_path->prevArc(sta_); const sta::LibertyPort* in_lib_port = in_arc ? in_arc->from() : nullptr; - sta::Pin* drvr_input_pin - = in_lib_port ? network_->findPin(drvr, in_lib_port) : nullptr; + if (in_lib_port == nullptr) { + return false; + } + sta::Pin* drvr_input_pin = network_->findPin(drvr, in_lib_port); sta::Path* prev_drvr_path = drvr_path->prevPath()->prevPath(); sta::Pin* prev_drvr_pin = prev_drvr_path ? prev_drvr_path->pin(sta_) : nullptr; From 9add37fcaceacabe2354b6cd28b0b28b219790ad Mon Sep 17 00:00:00 2001 From: Minju Kim Date: Wed, 22 Apr 2026 14:50:06 +0900 Subject: [PATCH 04/14] rsz: add repair_setup_swap_sizeup_unbuffer regression test Covers the -sequence "swap,sizeup,unbuffer" path that triggered the SIGSEGV in issue #10210. Exercises SwapPinsMove, SizeUpMove, and UnbufferMove, each of which now resolves the driver input port via prev_arc->from() instead of prevPath()->pin(). Registered for both CMake and Bazel. Signed-off-by: Minju Kim --- src/rsz/test/BUILD | 1 + src/rsz/test/CMakeLists.txt | 1 + .../test/repair_setup_swap_sizeup_unbuffer.ok | 92 +++++++++++++++++++ .../repair_setup_swap_sizeup_unbuffer.tcl | 22 +++++ 4 files changed, 116 insertions(+) create mode 100644 src/rsz/test/repair_setup_swap_sizeup_unbuffer.ok create mode 100644 src/rsz/test/repair_setup_swap_sizeup_unbuffer.tcl diff --git a/src/rsz/test/BUILD b/src/rsz/test/BUILD index 957c0139272..e2554eeb4ea 100644 --- a/src/rsz/test/BUILD +++ b/src/rsz/test/BUILD @@ -123,6 +123,7 @@ TESTS = [ "repair_setup9_hier", "repair_setup_sizedown", "repair_setup_sizeup_match", + "repair_setup_swap_sizeup_unbuffer", "repair_setup_undo1", "repair_setup_undo2", "repair_slew1", diff --git a/src/rsz/test/CMakeLists.txt b/src/rsz/test/CMakeLists.txt index 185f8bb1534..e52068f99ad 100644 --- a/src/rsz/test/CMakeLists.txt +++ b/src/rsz/test/CMakeLists.txt @@ -86,6 +86,7 @@ or_integration_tests( repair_hold15 repair_setup_sizedown repair_setup_sizeup_match + repair_setup_swap_sizeup_unbuffer repair_setup1 repair_setup2 repair_setup3 diff --git a/src/rsz/test/repair_setup_swap_sizeup_unbuffer.ok b/src/rsz/test/repair_setup_swap_sizeup_unbuffer.ok new file mode 100644 index 00000000000..db1e14e65fb --- /dev/null +++ b/src/rsz/test/repair_setup_swap_sizeup_unbuffer.ok @@ -0,0 +1,92 @@ +[INFO ODB-0227] LEF file: Nangate45/Nangate45.lef, created 22 layers, 27 vias, 135 library cells +[INFO ODB-0128] Design: reg1 +[INFO ODB-0130] Created 1 pins. +[INFO ODB-0131] Created 14 components and 70 component-terminals. +[INFO ODB-0132] Created 2 special nets and 28 connections. +[INFO ODB-0133] Created 9 nets and 28 connections. +Startpoint: r1 (rising edge-triggered flip-flop clocked by clk) +Endpoint: r2 (rising edge-triggered flip-flop clocked by clk) +Path Group: clk +Path Type: max + + Delay Time Description +----------------------------------------------------------- + 0.000 0.000 clock clk (rise edge) + 0.000 0.000 clock network delay (ideal) + 0.000 0.000 ^ r1/CK (DFF_X1) + 0.191 0.191 ^ r1/Q (DFF_X1) + 0.017 0.208 ^ u1/A (BUF_X8) + 0.029 0.238 ^ u1/Z (BUF_X8) + 0.002 0.239 ^ u2/A (BUF_X1) + 0.038 0.278 ^ u2/Z (BUF_X1) + 0.002 0.280 ^ u3/A (BUF_X1) + 0.042 0.322 ^ u3/Z (BUF_X1) + 0.002 0.323 ^ u4/A (BUF_X1) + 0.042 0.365 ^ u4/Z (BUF_X1) + 0.002 0.367 ^ u5/A (BUF_X1) + 0.116 0.483 ^ u5/Z (BUF_X1) + 0.049 0.532 ^ r2/D (DFF_X1) + 0.532 data arrival time + + 0.350 0.350 clock clk (rise edge) + 0.000 0.350 clock network delay (ideal) + 0.000 0.350 clock reconvergence pessimism + 0.350 ^ r2/CK (DFF_X1) + -0.048 0.302 library setup time + 0.302 data required time +----------------------------------------------------------- + 0.302 data required time + -0.532 data arrival time +----------------------------------------------------------- + -0.230 slack (VIOLATED) + + +[INFO RSZ-0100] Repair move sequence: SwapPinsMove SizeUpMove UnbufferMove +[INFO RSZ-0094] Found 1 endpoints with setup violations. +[INFO RSZ-0099] Repairing 1 out of 1 (100.00%) violating endpoints... + Iter | Removed | Resized | Inserted | Cloned | Pin | Area | WNS | StTNS | EnTNS | Viol | Worst + | Buffers | Gates | Buffers | Gates | Swaps | | | | | Endpts | St/EnPt +------------------------------------------------------------------------------------------------------------------------------ + 0* | 0 | 0 | 0 | 0 | 0 | +0.0% | -0.230 | -0.2 | -0.2 | 1 | r2/D + 21* | 0 | 14 | 0 | 0 | 0 | +30.0% | -0.027 | -0.0 | -0.0 | 1 | r2/D + final | 0 | 14 | 0 | 0 | 0 | +30.0% | -0.027 | -0.0 | -0.0 | 1 | r2/D +------------------------------------------------------------------------------------------------------------------------------ +[INFO RSZ-0051] Resized 14 instances: 14 up, 0 up match, 0 down, 0 VT +[WARNING RSZ-0062] Unable to repair all setup violations. +Startpoint: r1 (rising edge-triggered flip-flop clocked by clk) +Endpoint: r2 (rising edge-triggered flip-flop clocked by clk) +Path Group: clk +Path Type: max + + Delay Time Description +----------------------------------------------------------- + 0.000 0.000 clock clk (rise edge) + 0.000 0.000 clock network delay (ideal) + 0.000 0.000 ^ r1/CK (DFF_X2) + 0.152 0.152 ^ r1/Q (DFF_X2) + 0.017 0.169 ^ u1/A (BUF_X8) + 0.029 0.198 ^ u1/Z (BUF_X8) + 0.004 0.202 ^ u2/A (BUF_X8) + 0.021 0.222 ^ u2/Z (BUF_X8) + 0.004 0.226 ^ u3/A (BUF_X8) + 0.020 0.246 ^ u3/Z (BUF_X8) + 0.004 0.250 ^ u4/A (BUF_X8) + 0.021 0.271 ^ u4/Z (BUF_X8) + 0.005 0.276 ^ u5/A (BUF_X16) + 0.022 0.298 ^ u5/Z (BUF_X16) + 0.038 0.336 ^ r2/D (DFF_X1) + 0.336 data arrival time + + 0.350 0.350 clock clk (rise edge) + 0.000 0.350 clock network delay (ideal) + 0.000 0.350 clock reconvergence pessimism + 0.350 ^ r2/CK (DFF_X1) + -0.042 0.308 library setup time + 0.308 data required time +----------------------------------------------------------- + 0.308 data required time + -0.336 data arrival time +----------------------------------------------------------- + -0.027 slack (VIOLATED) + + diff --git a/src/rsz/test/repair_setup_swap_sizeup_unbuffer.tcl b/src/rsz/test/repair_setup_swap_sizeup_unbuffer.tcl new file mode 100644 index 00000000000..07291887323 --- /dev/null +++ b/src/rsz/test/repair_setup_swap_sizeup_unbuffer.tcl @@ -0,0 +1,22 @@ +# Exercises SwapPinsMove/SizeUpMove/UnbufferMove through repair_timing -sequence. +# Regression guard for issue #10210: the three moves resolve the driver's +# input LibertyPort via prev_arc->from() (stable) rather than +# prevPath()->pin() (which could point into reallocated Vertex::paths_). +source "helpers.tcl" +if { ![info exists repair_args] } { + set repair_args {} +} +read_liberty Nangate45/Nangate45_typ.lib +read_lef Nangate45/Nangate45.lef +read_def repair_setup_sizedown.def +create_clock -period 0.35 clk +set_load 1.0 [all_outputs] + +source Nangate45/Nangate45.rc +set_wire_rc -layer metal3 +estimate_parasitics -placement +report_checks -fields input -digits 3 + +set_dont_use [get_lib_cells CLKBUF*] +repair_timing -setup -sequence "swap,sizeup,unbuffer" {*}$repair_args +report_checks -fields input -digits 3 From 556e68c7d5210a5ca4289dc1eac1d640bbbd68a5 Mon Sep 17 00:00:00 2001 From: Minju Kim Date: Wed, 22 Apr 2026 16:28:57 +0900 Subject: [PATCH 05/14] rsz: remove repair_setup_swap_sizeup_unbuffer regression test Reverts the test added for issue #10210. The UAF in prevPath()->pin() requires specific post-CTS path-enumeration and allocator timing that no minimal public testcase (Nangate45 small, ASAP7 gcd 3-tier VT, ASAP7 aes_place.def at 22k components) reproduces under ASAN. The Tcl test therefore exercises the code path but does not actually guard the regression -- reverting it would still pass. Verification of the fix is via the original customer post-CTS ODB under ASAN. Keeping a non-guarding test file adds noise without signal. Signed-off-by: Minju Kim --- src/rsz/test/BUILD | 1 - src/rsz/test/CMakeLists.txt | 1 - .../test/repair_setup_swap_sizeup_unbuffer.ok | 92 ------------------- .../repair_setup_swap_sizeup_unbuffer.tcl | 22 ----- 4 files changed, 116 deletions(-) delete mode 100644 src/rsz/test/repair_setup_swap_sizeup_unbuffer.ok delete mode 100644 src/rsz/test/repair_setup_swap_sizeup_unbuffer.tcl diff --git a/src/rsz/test/BUILD b/src/rsz/test/BUILD index e2554eeb4ea..957c0139272 100644 --- a/src/rsz/test/BUILD +++ b/src/rsz/test/BUILD @@ -123,7 +123,6 @@ TESTS = [ "repair_setup9_hier", "repair_setup_sizedown", "repair_setup_sizeup_match", - "repair_setup_swap_sizeup_unbuffer", "repair_setup_undo1", "repair_setup_undo2", "repair_slew1", diff --git a/src/rsz/test/CMakeLists.txt b/src/rsz/test/CMakeLists.txt index e52068f99ad..185f8bb1534 100644 --- a/src/rsz/test/CMakeLists.txt +++ b/src/rsz/test/CMakeLists.txt @@ -86,7 +86,6 @@ or_integration_tests( repair_hold15 repair_setup_sizedown repair_setup_sizeup_match - repair_setup_swap_sizeup_unbuffer repair_setup1 repair_setup2 repair_setup3 diff --git a/src/rsz/test/repair_setup_swap_sizeup_unbuffer.ok b/src/rsz/test/repair_setup_swap_sizeup_unbuffer.ok deleted file mode 100644 index db1e14e65fb..00000000000 --- a/src/rsz/test/repair_setup_swap_sizeup_unbuffer.ok +++ /dev/null @@ -1,92 +0,0 @@ -[INFO ODB-0227] LEF file: Nangate45/Nangate45.lef, created 22 layers, 27 vias, 135 library cells -[INFO ODB-0128] Design: reg1 -[INFO ODB-0130] Created 1 pins. -[INFO ODB-0131] Created 14 components and 70 component-terminals. -[INFO ODB-0132] Created 2 special nets and 28 connections. -[INFO ODB-0133] Created 9 nets and 28 connections. -Startpoint: r1 (rising edge-triggered flip-flop clocked by clk) -Endpoint: r2 (rising edge-triggered flip-flop clocked by clk) -Path Group: clk -Path Type: max - - Delay Time Description ------------------------------------------------------------ - 0.000 0.000 clock clk (rise edge) - 0.000 0.000 clock network delay (ideal) - 0.000 0.000 ^ r1/CK (DFF_X1) - 0.191 0.191 ^ r1/Q (DFF_X1) - 0.017 0.208 ^ u1/A (BUF_X8) - 0.029 0.238 ^ u1/Z (BUF_X8) - 0.002 0.239 ^ u2/A (BUF_X1) - 0.038 0.278 ^ u2/Z (BUF_X1) - 0.002 0.280 ^ u3/A (BUF_X1) - 0.042 0.322 ^ u3/Z (BUF_X1) - 0.002 0.323 ^ u4/A (BUF_X1) - 0.042 0.365 ^ u4/Z (BUF_X1) - 0.002 0.367 ^ u5/A (BUF_X1) - 0.116 0.483 ^ u5/Z (BUF_X1) - 0.049 0.532 ^ r2/D (DFF_X1) - 0.532 data arrival time - - 0.350 0.350 clock clk (rise edge) - 0.000 0.350 clock network delay (ideal) - 0.000 0.350 clock reconvergence pessimism - 0.350 ^ r2/CK (DFF_X1) - -0.048 0.302 library setup time - 0.302 data required time ------------------------------------------------------------ - 0.302 data required time - -0.532 data arrival time ------------------------------------------------------------ - -0.230 slack (VIOLATED) - - -[INFO RSZ-0100] Repair move sequence: SwapPinsMove SizeUpMove UnbufferMove -[INFO RSZ-0094] Found 1 endpoints with setup violations. -[INFO RSZ-0099] Repairing 1 out of 1 (100.00%) violating endpoints... - Iter | Removed | Resized | Inserted | Cloned | Pin | Area | WNS | StTNS | EnTNS | Viol | Worst - | Buffers | Gates | Buffers | Gates | Swaps | | | | | Endpts | St/EnPt ------------------------------------------------------------------------------------------------------------------------------- - 0* | 0 | 0 | 0 | 0 | 0 | +0.0% | -0.230 | -0.2 | -0.2 | 1 | r2/D - 21* | 0 | 14 | 0 | 0 | 0 | +30.0% | -0.027 | -0.0 | -0.0 | 1 | r2/D - final | 0 | 14 | 0 | 0 | 0 | +30.0% | -0.027 | -0.0 | -0.0 | 1 | r2/D ------------------------------------------------------------------------------------------------------------------------------- -[INFO RSZ-0051] Resized 14 instances: 14 up, 0 up match, 0 down, 0 VT -[WARNING RSZ-0062] Unable to repair all setup violations. -Startpoint: r1 (rising edge-triggered flip-flop clocked by clk) -Endpoint: r2 (rising edge-triggered flip-flop clocked by clk) -Path Group: clk -Path Type: max - - Delay Time Description ------------------------------------------------------------ - 0.000 0.000 clock clk (rise edge) - 0.000 0.000 clock network delay (ideal) - 0.000 0.000 ^ r1/CK (DFF_X2) - 0.152 0.152 ^ r1/Q (DFF_X2) - 0.017 0.169 ^ u1/A (BUF_X8) - 0.029 0.198 ^ u1/Z (BUF_X8) - 0.004 0.202 ^ u2/A (BUF_X8) - 0.021 0.222 ^ u2/Z (BUF_X8) - 0.004 0.226 ^ u3/A (BUF_X8) - 0.020 0.246 ^ u3/Z (BUF_X8) - 0.004 0.250 ^ u4/A (BUF_X8) - 0.021 0.271 ^ u4/Z (BUF_X8) - 0.005 0.276 ^ u5/A (BUF_X16) - 0.022 0.298 ^ u5/Z (BUF_X16) - 0.038 0.336 ^ r2/D (DFF_X1) - 0.336 data arrival time - - 0.350 0.350 clock clk (rise edge) - 0.000 0.350 clock network delay (ideal) - 0.000 0.350 clock reconvergence pessimism - 0.350 ^ r2/CK (DFF_X1) - -0.042 0.308 library setup time - 0.308 data required time ------------------------------------------------------------ - 0.308 data required time - -0.336 data arrival time ------------------------------------------------------------ - -0.027 slack (VIOLATED) - - diff --git a/src/rsz/test/repair_setup_swap_sizeup_unbuffer.tcl b/src/rsz/test/repair_setup_swap_sizeup_unbuffer.tcl deleted file mode 100644 index 07291887323..00000000000 --- a/src/rsz/test/repair_setup_swap_sizeup_unbuffer.tcl +++ /dev/null @@ -1,22 +0,0 @@ -# Exercises SwapPinsMove/SizeUpMove/UnbufferMove through repair_timing -sequence. -# Regression guard for issue #10210: the three moves resolve the driver's -# input LibertyPort via prev_arc->from() (stable) rather than -# prevPath()->pin() (which could point into reallocated Vertex::paths_). -source "helpers.tcl" -if { ![info exists repair_args] } { - set repair_args {} -} -read_liberty Nangate45/Nangate45_typ.lib -read_lef Nangate45/Nangate45.lef -read_def repair_setup_sizedown.def -create_clock -period 0.35 clk -set_load 1.0 [all_outputs] - -source Nangate45/Nangate45.rc -set_wire_rc -layer metal3 -estimate_parasitics -placement -report_checks -fields input -digits 3 - -set_dont_use [get_lib_cells CLKBUF*] -repair_timing -setup -sequence "swap,sizeup,unbuffer" {*}$repair_args -report_checks -fields input -digits 3 From 178fce5991ffd37ca411f7ba3ac3ea4b458d6983 Mon Sep 17 00:00:00 2001 From: Minju Kim Date: Wed, 22 Apr 2026 17:05:38 +0900 Subject: [PATCH 06/14] rsz: add cpp tests for issue #10210 fix (equivalence + UAF proof + stability) Three regression tests pin the #10210 fix in SizeUpMove / SwapPinsMove / UnbufferMove / RecoverPower. The fix replaces drvr_path->prevPath()->pin()->libertyPort() (pre-fix) and expanded->path(drvr_index - 1)->pin() (b984a8d7e8 era) with drvr_path->prevArc()->from(), which reads prev_edge_id_/prev_arc_idx_ off the Path object itself -- no raw prev_path_ dereference, no expanded-vector indirection. 1. PrevArcFromMatchesDriverInputPort: equivalence test. On the BufRemTest buffer chain, prevArc()->from() resolves to the driver's own input LibertyPort, and agrees with the pre-fix prevPath()->pin()->libertyPort() form on non-stale paths. 2. PrevArcFromStableAcrossCellReplace: stability test. After Sta::replaceCell + updateTiming perturbs the chain, a fresh drvr_path query at b3/Z still returns the same LibertyPort (b3's A port) via prevArc()->from(), both in absolute terms and relative to the pre-perturbation result. Confirms the fix is semantically stable across the topology change that causes the UAF; does not dereference any stale pointer. 3. ExpandedPathPointerGoesStaleAfterCellReplace: deterministic UAF proof. After Sta::replaceCell + updateTiming triggers Search::setVertexArrivals -> Vertex::makePaths (delete[] paths_; new Path[N]), the Path pointer previously captured inside a PathExpanded vector lies outside the vertex's live paths_ range. Detection uses pointer-address comparison only (never dereferences the stale pointer, so it is ASAN-safe); when the allocator happens to reuse the same base the test skips. On glibc with Nangate45 it deterministically exercises the staleness path. The test observes three OpenSTA implementation details (PathExpanded storing raw Path* pointers, Vertex::makePaths doing delete[]/new[], Vertex::paths() returning the array base). If OpenSTA refactors any of these -- e.g., arena/pool allocation with stable addresses, in-place grow, or PathExpanded value-copying -- the test will skip rather than fail, which is an acceptable signal that the UAF shape no longer applies. The equivalence test remains the semantic guard regardless. Signed-off-by: Minju Kim --- src/rsz/test/cpp/TestBufferRemoval.cc | 222 ++++++++++++++++++++++++++ 1 file changed, 222 insertions(+) diff --git a/src/rsz/test/cpp/TestBufferRemoval.cc b/src/rsz/test/cpp/TestBufferRemoval.cc index b5e1cb914f2..27670be501d 100644 --- a/src/rsz/test/cpp/TestBufferRemoval.cc +++ b/src/rsz/test/cpp/TestBufferRemoval.cc @@ -24,9 +24,12 @@ #include "sta/Graph.hh" #include "sta/Liberty.hh" #include "sta/NetworkClass.hh" +#include "sta/Path.hh" +#include "sta/PathExpanded.hh" #include "sta/Search.hh" #include "sta/SearchClass.hh" #include "sta/Sta.hh" +#include "sta/TimingArc.hh" #include "stt/SteinerTreeBuilder.h" #include "tst/fixture.h" #include "tst/nangate45_fixture.h" @@ -167,4 +170,223 @@ TEST_F(BufRemTest, SlackImproves) EXPECT_EQ(restoredArrival, origArrival); } +// Regression guard for #10210: rsz moves (SizeUpMove / SwapPinsMove / +// UnbufferMove / RecoverPower) read the driver cell's input LibertyPort from +// drvr_path->prevPath()->pin()->libertyPort(). The fix switches to +// drvr_path->prevArc()->from(), which is carried on the Path itself via +// prev_edge_id_/prev_arc_idx_ and therefore does not dereference the +// prev_path_ raw pointer. This test pins three properties the fix relies on: +// 1. prevArc()->from() resolves to the driver's own input LibertyPort. +// 2. It agrees with the pre-fix prevPath()->pin()->libertyPort() form on +// the current master code path (equivalence, so the fix is not a +// behavioral change in the non-stale case). +// 3. prev_path_ on a driver-output path lands on the driver's own input +// pin -- same instance -- which is the master invariant the fix +// preserves without dereferencing the raw pointer. +TEST_F(BufRemTest, PrevArcFromMatchesDriverInputPort) +{ + sta::Graph* graph = sta_->ensureGraph(); + sta::Network* network = sta_->network(); + sta_->updateTiming(true); + + odb::dbInst* b3_db = block_->findInst("b3"); + ASSERT_NE(b3_db, nullptr); + sta::Instance* b3 = db_network_->dbToSta(b3_db); + sta::Pin* b3_z = network->findPin(b3, "Z"); + sta::Pin* b3_a = network->findPin(b3, "A"); + ASSERT_NE(b3_z, nullptr); + ASSERT_NE(b3_a, nullptr); + + sta::Vertex* b3_z_vertex = graph->pinDrvrVertex(b3_z); + ASSERT_NE(b3_z_vertex, nullptr); + + sta::Path* drvr_path + = sta_->vertexWorstArrivalPath(b3_z_vertex, sta::MinMax::max()); + ASSERT_NE(drvr_path, nullptr); + ASSERT_FALSE(drvr_path->isNull()); + + // Property 1: the fix's form resolves to the driver's input LibertyPort. + const sta::TimingArc* in_arc = drvr_path->prevArc(sta_.get()); + ASSERT_NE(in_arc, nullptr); + sta::LibertyPort* port_via_arc = in_arc->from(); + EXPECT_EQ(port_via_arc, network->libertyPort(b3_a)); + + // Property 2: equivalence with the pre-fix form on non-stale paths. + sta::Path* prev_path = drvr_path->prevPath(); + ASSERT_NE(prev_path, nullptr); + sta::Pin* prev_pin = prev_path->pin(sta_.get()); + ASSERT_NE(prev_pin, nullptr); + EXPECT_EQ(port_via_arc, network->libertyPort(prev_pin)); + + // Property 3: prev_path_ on a driver-output path lands on the driver's + // own input pin (same instance). The historical bug in #10210 was + // against the expanded.path(drvr_index - 1) form (forward direction in + // the expanded array), which could land on a downstream load pin. The + // prevArc form is safe regardless of that invariant holding. + EXPECT_EQ(network->instance(prev_pin), b3); +} + +// Companion to ExpandedPathPointerGoesStaleAfterCellReplace: verifies that +// the fix's prevArc()->from() resolution still returns the correct driver +// input port after the same realloc that leaves PathExpanded snapshots +// stale. This does not dereference any stale pointer -- it re-queries a +// fresh Path from the STA after the perturbation, then checks that +// prevArc()->from() matches the LibertyPort of the driver's input pin both +// before and after. Because b3's cell is unchanged, the two LibertyPort +// pointers must be identical (Liberty objects have library lifetime). +TEST_F(BufRemTest, PrevArcFromStableAcrossCellReplace) +{ + sta::Graph* graph = sta_->ensureGraph(); + sta::Network* network = sta_->network(); + sta_->updateTiming(true); + + odb::dbInst* b3_db = block_->findInst("b3"); + ASSERT_NE(b3_db, nullptr); + sta::Instance* b3 = db_network_->dbToSta(b3_db); + sta::Pin* b3_z = network->findPin(b3, "Z"); + sta::Pin* b3_a = network->findPin(b3, "A"); + ASSERT_NE(b3_z, nullptr); + ASSERT_NE(b3_a, nullptr); + sta::Vertex* b3_z_vertex = graph->pinDrvrVertex(b3_z); + ASSERT_NE(b3_z_vertex, nullptr); + + sta::LibertyPort* expected_port = network->libertyPort(b3_a); + ASSERT_NE(expected_port, nullptr); + + // Baseline: fresh path, fresh prevArc()->from(). + sta::Path* drvr_path_before + = sta_->vertexWorstArrivalPath(b3_z_vertex, sta::MinMax::max()); + ASSERT_NE(drvr_path_before, nullptr); + const sta::TimingArc* arc_before = drvr_path_before->prevArc(sta_.get()); + ASSERT_NE(arc_before, nullptr); + sta::LibertyPort* port_before = arc_before->from(); + EXPECT_EQ(port_before, expected_port); + + // Trigger the same realloc the staleness test uses. + odb::dbInst* b1_db = block_->findInst("b1"); + ASSERT_NE(b1_db, nullptr); + sta::Instance* b1 = db_network_->dbToSta(b1_db); + sta::LibertyCell* buf_x8 = network->findLibertyCell("BUF_X8"); + ASSERT_NE(buf_x8, nullptr); + sta_->replaceCell(b1, buf_x8); + sta_->updateTiming(true); + + // After realloc: fresh path (drvr_path_before may be stale and is + // intentionally not touched here). The b3 vertex may have a new paths_ + // array, but its internal A->Z arc is unchanged, so prevArc()->from() + // must still resolve to b3's A port. + sta::Path* drvr_path_after + = sta_->vertexWorstArrivalPath(b3_z_vertex, sta::MinMax::max()); + ASSERT_NE(drvr_path_after, nullptr); + const sta::TimingArc* arc_after = drvr_path_after->prevArc(sta_.get()); + ASSERT_NE(arc_after, nullptr); + sta::LibertyPort* port_after = arc_after->from(); + + EXPECT_EQ(port_after, expected_port); + EXPECT_EQ(port_after, port_before); +} + +// Demonstrates the root cause of issue #10210: PathExpanded captures Path* +// pointers at construction time, but the underlying Vertex::paths_ arrays +// (from Graph.cc) are delete[]'d and reallocated whenever a cell replace or +// buffer removal forces Search::setVertexArrivals to rebuild with a different +// tag_group. After such a reallocation the Path pointers stored inside the +// expanded vector dangle -- expanded.path(k) returns the same stored address, +// but that address is outside the vertex's live paths array, so dereferencing +// it reads either freed memory or a different Vertex's reused Path slot. +// +// The fix drops dependence on expanded.path(drvr_index - 1) / prevPath()->pin() +// and resolves the driver's input port from drvr_path->prevArc()->from(), +// which reads prev_edge_id_/prev_arc_idx_ off the Path object itself -- no +// expanded indirection, no prev_path_ dereference. +// +// This test detects the stale state via pointer address comparison (never +// dereferences the stale pointer, so it is ASAN-safe). When the Nangate45 +// allocator happens to hand the new paths_ array the same base as the freed +// one, the test cannot distinguish stale vs live and is skipped. +// +// The test observes three OpenSTA implementation details: +// (1) PathExpanded::paths_ stores raw Path* pointers (not copies). +// (2) Vertex::makePaths does `delete[] paths_; new Path[N]` on tag_group +// change in Search::setVertexArrivals. +// (3) Vertex::paths() returns the base address of the current paths_ array. +// If OpenSTA refactors any of these (e.g., arena/pool allocation with stable +// addresses, in-place grow, PathExpanded value-copying), the test will skip +// rather than fail -- that is an acceptable signal that the UAF shape no +// longer applies. The equivalence test above remains the semantic guard. +TEST_F(BufRemTest, ExpandedPathPointerGoesStaleAfterCellReplace) +{ + sta::Graph* graph = sta_->ensureGraph(); + sta::Network* network = sta_->network(); + sta_->updateTiming(true); + + // Endpoint path at the block output "out1". + odb::dbBTerm* out_bterm = block_->findBTerm("out1"); + ASSERT_NE(out_bterm, nullptr); + sta::Pin* out_pin = db_network_->dbToSta(out_bterm); + sta::Vertex* out_vertex = graph->pinLoadVertex(out_pin); + ASSERT_NE(out_vertex, nullptr); + sta::Path* endpoint_path + = sta_->vertexWorstArrivalPath(out_vertex, sta::MinMax::max()); + ASSERT_NE(endpoint_path, nullptr); + + sta::PathExpanded expanded(endpoint_path, sta_.get()); + ASSERT_GT(expanded.size(), 2u); + + // Pick an intermediate path. It points into some Vertex v's paths_ + // array. After we perturb the design, v's paths_ may be freed and + // reallocated; the pointer we hold in `expanded` is the snapshot. + const size_t target_index = expanded.size() / 2; + const sta::Path* snapshot = expanded.path(target_index); + ASSERT_NE(snapshot, nullptr); + sta::Vertex* v = snapshot->vertex(sta_.get()); + ASSERT_NE(v, nullptr); + + // Snapshot the base of v's paths_ array before perturbation. + sta::Path* v_paths_before = v->paths(); + ASSERT_NE(v_paths_before, nullptr); + + // Trigger Search::setVertexArrivals to rebuild v's paths_ array by + // replacing the upstream-most buffer. This invalidates arrivals along + // the whole chain; the next incremental STA update reallocates paths_. + odb::dbInst* b1_db = block_->findInst("b1"); + ASSERT_NE(b1_db, nullptr); + sta::Instance* b1 = db_network_->dbToSta(b1_db); + sta::LibertyCell* buf_x8 = network->findLibertyCell("BUF_X8"); + ASSERT_NE(buf_x8, nullptr); + sta_->replaceCell(b1, buf_x8); + sta_->updateTiming(true); + + sta::Path* v_paths_after = v->paths(); + + if (v_paths_after == v_paths_before) { + GTEST_SKIP() + << "Allocator reused the same base for v->paths() -- test cannot " + "prove staleness by pointer comparison without dereferencing " + "freed memory (which is UB). Run under a different allocator " + "or with a forced-different-address hook to exercise this case."; + } + + // v's paths_ was reallocated to a different address. The snapshot + // pointer we captured before -- which is what expanded.path(k) still + // returns -- is outside v's live paths array, i.e. it dangles. + // + // pathCount on a reallocated vertex is bounded by TagGroup size; we + // don't have that accessor here, but a loose upper-bound of 256 Path + // slots suffices: if the snapshot is inside [v_paths_after, + // v_paths_after + 256) it might alias, and we skip. Otherwise it is + // provably outside the live range. + const uintptr_t snap_addr = reinterpret_cast(snapshot); + const uintptr_t base_after = reinterpret_cast(v_paths_after); + const uintptr_t max_live = base_after + 256 * sizeof(sta::Path); + if (snap_addr >= base_after && snap_addr < max_live) { + GTEST_SKIP() << "Snapshot pointer aliases into the new paths range; " + "cannot prove staleness without UB dereference."; + } + EXPECT_TRUE(snap_addr < base_after || snap_addr >= max_live) + << "Stale pointer demonstrated: expanded.path(" << target_index + << ") = " << snapshot << " is outside v->paths() = [" << v_paths_after + << ", " << reinterpret_cast(max_live) << ")"; +} + } // namespace rsz From fc5e2583a54fd59bf35f701742f06f5964ca9626 Mon Sep 17 00:00:00 2001 From: Minju Kim Date: Thu, 23 Apr 2026 14:32:48 +0900 Subject: [PATCH 07/14] rsz: fix clang-tidy include-cleaner and int-to-ptr warnings Add / for size_t / uintptr_t used in the UAF test, and drop the int-to-pointer cast in the EXPECT_TRUE stream message by printing the live-range as 'base + N bytes' instead of a synthesized void*. Signed-off-by: Minju Kim --- src/rsz/test/cpp/TestBufferRemoval.cc | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/rsz/test/cpp/TestBufferRemoval.cc b/src/rsz/test/cpp/TestBufferRemoval.cc index 27670be501d..a2966803b91 100644 --- a/src/rsz/test/cpp/TestBufferRemoval.cc +++ b/src/rsz/test/cpp/TestBufferRemoval.cc @@ -3,6 +3,8 @@ #include +#include +#include #include #include #include @@ -378,15 +380,16 @@ TEST_F(BufRemTest, ExpandedPathPointerGoesStaleAfterCellReplace) // provably outside the live range. const uintptr_t snap_addr = reinterpret_cast(snapshot); const uintptr_t base_after = reinterpret_cast(v_paths_after); - const uintptr_t max_live = base_after + 256 * sizeof(sta::Path); + const std::size_t live_bytes = 256 * sizeof(sta::Path); + const uintptr_t max_live = base_after + live_bytes; if (snap_addr >= base_after && snap_addr < max_live) { GTEST_SKIP() << "Snapshot pointer aliases into the new paths range; " "cannot prove staleness without UB dereference."; } EXPECT_TRUE(snap_addr < base_after || snap_addr >= max_live) << "Stale pointer demonstrated: expanded.path(" << target_index - << ") = " << snapshot << " is outside v->paths() = [" << v_paths_after - << ", " << reinterpret_cast(max_live) << ")"; + << ") = " << snapshot << " is outside v->paths() base " << v_paths_after + << " + " << live_bytes << "B"; } } // namespace rsz From b07d0544eb921ee6c46944ba113fdd57d618b92e Mon Sep 17 00:00:00 2001 From: Minju Kim Date: Thu, 23 Apr 2026 20:47:11 +0900 Subject: [PATCH 08/14] rsz: add TestPathExpandedStaleness for #10210 stale Path* regression Replaces the abandoned pointer-equality UAF test that was part of TestBufferRemoval.cc with a standalone gtest exercising the actual crash path: 1. Capture drvr_path at nd1/ZN. 2. Delete upstream b1 + updateTiming -> free b1/inv1 Path[] slots. 3. Add a fresh cone + clk2 + updateTiming -> recycle those slots. 4. Assert drvr_path->prevPath()->pin() decodes to a stale pin (Phase-2 BTerm, not nd1's real input A1/A2). 5. Call SizeUpMove::doMove. With the fix reverted the call SIGSEGVs on the stale prev pointer. With the fix applied (prev_arc->from()) the stale deref is avoided and doMove cleanly early-returns. Registered in both CMake and Bazel. Signed-off-by: Minju Kim --- src/rsz/test/BUILD | 28 +++ src/rsz/test/cpp/CMakeLists.txt | 29 +++ src/rsz/test/cpp/TestBufferRemoval.cc | 225 ------------------ src/rsz/test/cpp/TestPathExpandedStaleness.cc | 203 ++++++++++++++++ 4 files changed, 260 insertions(+), 225 deletions(-) create mode 100644 src/rsz/test/cpp/TestPathExpandedStaleness.cc diff --git a/src/rsz/test/BUILD b/src/rsz/test/BUILD index 957c0139272..7c723bac468 100644 --- a/src/rsz/test/BUILD +++ b/src/rsz/test/BUILD @@ -265,6 +265,34 @@ test_suite( tests = [":TestBufRem1"], ) +cc_test( + name = "TestPathExpandedStaleness", + srcs = [ + "cpp/TestPathExpandedStaleness.cc", + ], + data = [ + "Nangate45/Nangate45.lef", + "Nangate45/Nangate45_typ.lib", + ], + deps = [ + "//src/ant", + "//src/dbSta", + "//src/dbSta:dbNetwork", + "//src/dpl", + "//src/est", + "//src/grt", + "//src/odb", + "//src/rsz", + "//src/sta:opensta_lib", + "//src/stt", + "//src/tst", + "//src/tst:nangate45_fixture", + "//src/utl", + "@googletest//:gtest", + "@googletest//:gtest_main", + ], +) + cc_test( name = "rsz_buffer_removal", srcs = [ diff --git a/src/rsz/test/cpp/CMakeLists.txt b/src/rsz/test/cpp/CMakeLists.txt index 75900b054ac..b56796dcb72 100644 --- a/src/rsz/test/cpp/CMakeLists.txt +++ b/src/rsz/test/cpp/CMakeLists.txt @@ -28,6 +28,35 @@ gtest_discover_tests(TestBufRem1 add_dependencies(build_and_test TestBufRem1 ) +add_executable(TestPathExpandedStaleness TestPathExpandedStaleness.cc) +target_link_libraries(TestPathExpandedStaleness + OpenSTA + GTest::gtest + GTest::gtest_main + GTest::gmock + dbSta_lib + utl_lib + rsz_lib + grt_lib + dpl_lib + stt_lib + odb + tst + ${TCL_LIBRARY} +) + +target_include_directories(TestPathExpandedStaleness + PRIVATE + ${PROJECT_SOURCE_DIR}/src/rsz/src +) + +gtest_discover_tests(TestPathExpandedStaleness + WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/.. +) + +add_dependencies(build_and_test TestPathExpandedStaleness +) + add_executable(TestBufferRemoval2 TestBufferRemoval2.cc) target_link_libraries(TestBufferRemoval2 OpenSTA diff --git a/src/rsz/test/cpp/TestBufferRemoval.cc b/src/rsz/test/cpp/TestBufferRemoval.cc index a2966803b91..b5e1cb914f2 100644 --- a/src/rsz/test/cpp/TestBufferRemoval.cc +++ b/src/rsz/test/cpp/TestBufferRemoval.cc @@ -3,8 +3,6 @@ #include -#include -#include #include #include #include @@ -26,12 +24,9 @@ #include "sta/Graph.hh" #include "sta/Liberty.hh" #include "sta/NetworkClass.hh" -#include "sta/Path.hh" -#include "sta/PathExpanded.hh" #include "sta/Search.hh" #include "sta/SearchClass.hh" #include "sta/Sta.hh" -#include "sta/TimingArc.hh" #include "stt/SteinerTreeBuilder.h" #include "tst/fixture.h" #include "tst/nangate45_fixture.h" @@ -172,224 +167,4 @@ TEST_F(BufRemTest, SlackImproves) EXPECT_EQ(restoredArrival, origArrival); } -// Regression guard for #10210: rsz moves (SizeUpMove / SwapPinsMove / -// UnbufferMove / RecoverPower) read the driver cell's input LibertyPort from -// drvr_path->prevPath()->pin()->libertyPort(). The fix switches to -// drvr_path->prevArc()->from(), which is carried on the Path itself via -// prev_edge_id_/prev_arc_idx_ and therefore does not dereference the -// prev_path_ raw pointer. This test pins three properties the fix relies on: -// 1. prevArc()->from() resolves to the driver's own input LibertyPort. -// 2. It agrees with the pre-fix prevPath()->pin()->libertyPort() form on -// the current master code path (equivalence, so the fix is not a -// behavioral change in the non-stale case). -// 3. prev_path_ on a driver-output path lands on the driver's own input -// pin -- same instance -- which is the master invariant the fix -// preserves without dereferencing the raw pointer. -TEST_F(BufRemTest, PrevArcFromMatchesDriverInputPort) -{ - sta::Graph* graph = sta_->ensureGraph(); - sta::Network* network = sta_->network(); - sta_->updateTiming(true); - - odb::dbInst* b3_db = block_->findInst("b3"); - ASSERT_NE(b3_db, nullptr); - sta::Instance* b3 = db_network_->dbToSta(b3_db); - sta::Pin* b3_z = network->findPin(b3, "Z"); - sta::Pin* b3_a = network->findPin(b3, "A"); - ASSERT_NE(b3_z, nullptr); - ASSERT_NE(b3_a, nullptr); - - sta::Vertex* b3_z_vertex = graph->pinDrvrVertex(b3_z); - ASSERT_NE(b3_z_vertex, nullptr); - - sta::Path* drvr_path - = sta_->vertexWorstArrivalPath(b3_z_vertex, sta::MinMax::max()); - ASSERT_NE(drvr_path, nullptr); - ASSERT_FALSE(drvr_path->isNull()); - - // Property 1: the fix's form resolves to the driver's input LibertyPort. - const sta::TimingArc* in_arc = drvr_path->prevArc(sta_.get()); - ASSERT_NE(in_arc, nullptr); - sta::LibertyPort* port_via_arc = in_arc->from(); - EXPECT_EQ(port_via_arc, network->libertyPort(b3_a)); - - // Property 2: equivalence with the pre-fix form on non-stale paths. - sta::Path* prev_path = drvr_path->prevPath(); - ASSERT_NE(prev_path, nullptr); - sta::Pin* prev_pin = prev_path->pin(sta_.get()); - ASSERT_NE(prev_pin, nullptr); - EXPECT_EQ(port_via_arc, network->libertyPort(prev_pin)); - - // Property 3: prev_path_ on a driver-output path lands on the driver's - // own input pin (same instance). The historical bug in #10210 was - // against the expanded.path(drvr_index - 1) form (forward direction in - // the expanded array), which could land on a downstream load pin. The - // prevArc form is safe regardless of that invariant holding. - EXPECT_EQ(network->instance(prev_pin), b3); -} - -// Companion to ExpandedPathPointerGoesStaleAfterCellReplace: verifies that -// the fix's prevArc()->from() resolution still returns the correct driver -// input port after the same realloc that leaves PathExpanded snapshots -// stale. This does not dereference any stale pointer -- it re-queries a -// fresh Path from the STA after the perturbation, then checks that -// prevArc()->from() matches the LibertyPort of the driver's input pin both -// before and after. Because b3's cell is unchanged, the two LibertyPort -// pointers must be identical (Liberty objects have library lifetime). -TEST_F(BufRemTest, PrevArcFromStableAcrossCellReplace) -{ - sta::Graph* graph = sta_->ensureGraph(); - sta::Network* network = sta_->network(); - sta_->updateTiming(true); - - odb::dbInst* b3_db = block_->findInst("b3"); - ASSERT_NE(b3_db, nullptr); - sta::Instance* b3 = db_network_->dbToSta(b3_db); - sta::Pin* b3_z = network->findPin(b3, "Z"); - sta::Pin* b3_a = network->findPin(b3, "A"); - ASSERT_NE(b3_z, nullptr); - ASSERT_NE(b3_a, nullptr); - sta::Vertex* b3_z_vertex = graph->pinDrvrVertex(b3_z); - ASSERT_NE(b3_z_vertex, nullptr); - - sta::LibertyPort* expected_port = network->libertyPort(b3_a); - ASSERT_NE(expected_port, nullptr); - - // Baseline: fresh path, fresh prevArc()->from(). - sta::Path* drvr_path_before - = sta_->vertexWorstArrivalPath(b3_z_vertex, sta::MinMax::max()); - ASSERT_NE(drvr_path_before, nullptr); - const sta::TimingArc* arc_before = drvr_path_before->prevArc(sta_.get()); - ASSERT_NE(arc_before, nullptr); - sta::LibertyPort* port_before = arc_before->from(); - EXPECT_EQ(port_before, expected_port); - - // Trigger the same realloc the staleness test uses. - odb::dbInst* b1_db = block_->findInst("b1"); - ASSERT_NE(b1_db, nullptr); - sta::Instance* b1 = db_network_->dbToSta(b1_db); - sta::LibertyCell* buf_x8 = network->findLibertyCell("BUF_X8"); - ASSERT_NE(buf_x8, nullptr); - sta_->replaceCell(b1, buf_x8); - sta_->updateTiming(true); - - // After realloc: fresh path (drvr_path_before may be stale and is - // intentionally not touched here). The b3 vertex may have a new paths_ - // array, but its internal A->Z arc is unchanged, so prevArc()->from() - // must still resolve to b3's A port. - sta::Path* drvr_path_after - = sta_->vertexWorstArrivalPath(b3_z_vertex, sta::MinMax::max()); - ASSERT_NE(drvr_path_after, nullptr); - const sta::TimingArc* arc_after = drvr_path_after->prevArc(sta_.get()); - ASSERT_NE(arc_after, nullptr); - sta::LibertyPort* port_after = arc_after->from(); - - EXPECT_EQ(port_after, expected_port); - EXPECT_EQ(port_after, port_before); -} - -// Demonstrates the root cause of issue #10210: PathExpanded captures Path* -// pointers at construction time, but the underlying Vertex::paths_ arrays -// (from Graph.cc) are delete[]'d and reallocated whenever a cell replace or -// buffer removal forces Search::setVertexArrivals to rebuild with a different -// tag_group. After such a reallocation the Path pointers stored inside the -// expanded vector dangle -- expanded.path(k) returns the same stored address, -// but that address is outside the vertex's live paths array, so dereferencing -// it reads either freed memory or a different Vertex's reused Path slot. -// -// The fix drops dependence on expanded.path(drvr_index - 1) / prevPath()->pin() -// and resolves the driver's input port from drvr_path->prevArc()->from(), -// which reads prev_edge_id_/prev_arc_idx_ off the Path object itself -- no -// expanded indirection, no prev_path_ dereference. -// -// This test detects the stale state via pointer address comparison (never -// dereferences the stale pointer, so it is ASAN-safe). When the Nangate45 -// allocator happens to hand the new paths_ array the same base as the freed -// one, the test cannot distinguish stale vs live and is skipped. -// -// The test observes three OpenSTA implementation details: -// (1) PathExpanded::paths_ stores raw Path* pointers (not copies). -// (2) Vertex::makePaths does `delete[] paths_; new Path[N]` on tag_group -// change in Search::setVertexArrivals. -// (3) Vertex::paths() returns the base address of the current paths_ array. -// If OpenSTA refactors any of these (e.g., arena/pool allocation with stable -// addresses, in-place grow, PathExpanded value-copying), the test will skip -// rather than fail -- that is an acceptable signal that the UAF shape no -// longer applies. The equivalence test above remains the semantic guard. -TEST_F(BufRemTest, ExpandedPathPointerGoesStaleAfterCellReplace) -{ - sta::Graph* graph = sta_->ensureGraph(); - sta::Network* network = sta_->network(); - sta_->updateTiming(true); - - // Endpoint path at the block output "out1". - odb::dbBTerm* out_bterm = block_->findBTerm("out1"); - ASSERT_NE(out_bterm, nullptr); - sta::Pin* out_pin = db_network_->dbToSta(out_bterm); - sta::Vertex* out_vertex = graph->pinLoadVertex(out_pin); - ASSERT_NE(out_vertex, nullptr); - sta::Path* endpoint_path - = sta_->vertexWorstArrivalPath(out_vertex, sta::MinMax::max()); - ASSERT_NE(endpoint_path, nullptr); - - sta::PathExpanded expanded(endpoint_path, sta_.get()); - ASSERT_GT(expanded.size(), 2u); - - // Pick an intermediate path. It points into some Vertex v's paths_ - // array. After we perturb the design, v's paths_ may be freed and - // reallocated; the pointer we hold in `expanded` is the snapshot. - const size_t target_index = expanded.size() / 2; - const sta::Path* snapshot = expanded.path(target_index); - ASSERT_NE(snapshot, nullptr); - sta::Vertex* v = snapshot->vertex(sta_.get()); - ASSERT_NE(v, nullptr); - - // Snapshot the base of v's paths_ array before perturbation. - sta::Path* v_paths_before = v->paths(); - ASSERT_NE(v_paths_before, nullptr); - - // Trigger Search::setVertexArrivals to rebuild v's paths_ array by - // replacing the upstream-most buffer. This invalidates arrivals along - // the whole chain; the next incremental STA update reallocates paths_. - odb::dbInst* b1_db = block_->findInst("b1"); - ASSERT_NE(b1_db, nullptr); - sta::Instance* b1 = db_network_->dbToSta(b1_db); - sta::LibertyCell* buf_x8 = network->findLibertyCell("BUF_X8"); - ASSERT_NE(buf_x8, nullptr); - sta_->replaceCell(b1, buf_x8); - sta_->updateTiming(true); - - sta::Path* v_paths_after = v->paths(); - - if (v_paths_after == v_paths_before) { - GTEST_SKIP() - << "Allocator reused the same base for v->paths() -- test cannot " - "prove staleness by pointer comparison without dereferencing " - "freed memory (which is UB). Run under a different allocator " - "or with a forced-different-address hook to exercise this case."; - } - - // v's paths_ was reallocated to a different address. The snapshot - // pointer we captured before -- which is what expanded.path(k) still - // returns -- is outside v's live paths array, i.e. it dangles. - // - // pathCount on a reallocated vertex is bounded by TagGroup size; we - // don't have that accessor here, but a loose upper-bound of 256 Path - // slots suffices: if the snapshot is inside [v_paths_after, - // v_paths_after + 256) it might alias, and we skip. Otherwise it is - // provably outside the live range. - const uintptr_t snap_addr = reinterpret_cast(snapshot); - const uintptr_t base_after = reinterpret_cast(v_paths_after); - const std::size_t live_bytes = 256 * sizeof(sta::Path); - const uintptr_t max_live = base_after + live_bytes; - if (snap_addr >= base_after && snap_addr < max_live) { - GTEST_SKIP() << "Snapshot pointer aliases into the new paths range; " - "cannot prove staleness without UB dereference."; - } - EXPECT_TRUE(snap_addr < base_after || snap_addr >= max_live) - << "Stale pointer demonstrated: expanded.path(" << target_index - << ") = " << snapshot << " is outside v->paths() base " << v_paths_after - << " + " << live_bytes << "B"; -} - } // namespace rsz diff --git a/src/rsz/test/cpp/TestPathExpandedStaleness.cc b/src/rsz/test/cpp/TestPathExpandedStaleness.cc new file mode 100644 index 00000000000..de36735a9f6 --- /dev/null +++ b/src/rsz/test/cpp/TestPathExpandedStaleness.cc @@ -0,0 +1,203 @@ +// SPDX-License-Identifier: BSD-3-Clause +// Copyright (c) 2026-2026, The OpenROAD Authors + +#include "SizeUpMove.hh" +#include "ant/AntennaChecker.hh" +#include "db_sta/dbNetwork.hh" +#include "db_sta/dbSta.hh" +#include "dpl/Opendp.h" +#include "est/EstimateParasitics.h" +#include "grt/GlobalRouter.h" +#include "gtest/gtest.h" +#include "odb/db.h" +#include "rsz/Resizer.hh" +#include "sta/Graph.hh" +#include "sta/NetworkClass.hh" +#include "sta/Path.hh" +#include "sta/Sdc.hh" +#include "sta/Sta.hh" +#include "stt/SteinerTreeBuilder.h" +#include "tst/nangate45_fixture.h" +#include "utl/ServiceRegistry.h" + +namespace rsz { + +static const std::string prefix("_main/src/rsz/test/"); + +// in1 -> b1(BUF) -> inv1(INV) -> nd1(NAND2) -> out1 +class StalePathTest : public tst::Nangate45Fixture +{ + protected: + StalePathTest() + : stt_(db_.get(), &logger_), + service_registry_(&logger_), + dp_(db_.get(), &logger_), + ant_(db_.get(), &logger_), + grt_(&logger_, + &service_registry_, + &stt_, + db_.get(), + sta_.get(), + &ant_, + &dp_), + ep_(&logger_, &service_registry_, db_.get(), sta_.get(), &stt_, &grt_), + resizer_(&logger_, db_.get(), sta_.get(), &stt_, &grt_, &dp_, &ep_) + { + readLiberty(prefix + "Nangate45/Nangate45_typ.lib"); + db_network_ = sta_->getDbNetwork(); + db_network_->setBlock(block_); + sta_->postReadDef(block_); + + const char* layer = "metal1"; + makeBTerm(block_, + "in1", + {.bpins = {{.layer_name = layer, .rect = {0, 0, 10, 10}}}}); + makeBTerm(block_, + "in2", + {.bpins = {{.layer_name = layer, .rect = {0, 100, 10, 110}}}}); + makeBTerm( + block_, + "out1", + {.io_type = odb::dbIoType::OUTPUT, + .bpins = {{.layer_name = layer, .rect = {990, 990, 1000, 1000}}}}); + + makeInst(block_, + db_->findMaster("BUF_X1"), + "b1", + {.location = {100, 100}, + .status = odb::dbPlacementStatus::PLACED, + .iterms = {{.net_name = "in1", .term_name = "A"}, + {.net_name = "n0", .term_name = "Z"}}}); + makeInst(block_, + db_->findMaster("INV_X1"), + "inv1", + {.location = {200, 100}, + .status = odb::dbPlacementStatus::PLACED, + .iterms = {{.net_name = "n0", .term_name = "A"}, + {.net_name = "n1", .term_name = "ZN"}}}); + makeInst(block_, + db_->findMaster("NAND2_X1"), + "nd1", + {.location = {300, 150}, + .status = odb::dbPlacementStatus::PLACED, + .iterms = {{.net_name = "n1", .term_name = "A1"}, + {.net_name = "in2", .term_name = "A2"}, + {.net_name = "out1", .term_name = "ZN"}}}); + + makeClockOn("clk", block_->findBTerm("in1")); + sta::Sdc* sdc = sta_->cmdSdc(); + sta_->setOutputDelay(db_network_->dbToSta(block_->findBTerm("out1")), + sta::RiseFallBoth::riseFall(), + sdc->findClock("clk"), + sta::RiseFall::rise(), + nullptr, + false, + false, + sta::MinMaxAll::all(), + false, + 0.0f, + sdc); + + resizer_.initBlock(); + db_->setLogger(&logger_); + sta_->updateTiming(true); + } + + // sta::Sdc::makeClock takes ownership of the PinSet* and FloatSeq* and + // frees them in Clock::setPins / ~Clock, so the raw `new` is not a leak. + void makeClockOn(const char* clk_name, odb::dbBTerm* bt) + { + auto* pins = new sta::PinSet(sta_->network()); + pins->insert(db_network_->dbToSta(bt)); + auto* waveform = new sta::FloatSeq{0.0f, 0.1f}; + sta_->makeClock(clk_name, pins, false, 0.2f, waveform, "", sta_->cmdMode()); + } + + stt::SteinerTreeBuilder stt_; + utl::ServiceRegistry service_registry_; + dpl::Opendp dp_; + ant::AntennaChecker ant_; + grt::GlobalRouter grt_; + est::EstimateParasitics ep_; + rsz::Resizer resizer_; + sta::dbNetwork* db_network_{nullptr}; +}; + +// Regression for #10210 (stale Path* dereference). +// +// Flow: +// 1. Capture drvr_path at nd1/ZN (terminal logic cell, stays alive). +// 2. Delete upstream b1 + updateTiming -> free b1/inv1 Path[] slots. +// 3. Add new cone + clk2 + updateTiming -> recycle freed slots. +// 4. Assert drvr_path->prevPath()->pin() now decodes to a stale pin +// (not nd1's real input). +// 5. Call SizeUpMove::doMove: pre-fix derefs stale prev -> SIGSEGV; +// post-fix reads prev_arc (liberty-stable) -> safe early-return. +TEST_F(StalePathTest, SizeUpMoveSurvivesStalePathAfterUpdateTiming) +{ + sta::Network* network = sta_->network(); + sta::Graph* graph = sta_->ensureGraph(); + + // 1. Capture drvr_path at nd1/ZN. + sta::Instance* nd1 = db_network_->dbToSta(block_->findInst("nd1")); + sta::Path* drvr_path = sta_->vertexWorstArrivalPath( + graph->pinDrvrVertex(network->findPin(nd1, "ZN")), sta::MinMax::max()); + ASSERT_NE(drvr_path, nullptr); + ASSERT_EQ(network->pathName(drvr_path->pin(sta_.get())), "nd1/ZN"); + + // 2. Free upstream Path[] slots. + sta_->deleteInstance(db_network_->dbToSta(block_->findInst("b1"))); + sta_->updateTiming(true); + + // 3. Recycle slots via a fresh cone + clock. + auto* new_bt = makeBTerm( + block_, + "in3", + {.bpins = {{.layer_name = "metal1", .rect = {0, 300, 10, 310}}}}); + makeBTerm(block_, + "in4", + {.bpins = {{.layer_name = "metal1", .rect = {0, 400, 10, 410}}}}); + odb::dbNet::create(block_, "nbuf"); + odb::dbNet::create(block_, "ninv"); + odb::dbNet::create(block_, "nfan"); + makeInst(block_, + db_->findMaster("BUF_X1"), + "bnew", + {.location = {400, 300}, + .status = odb::dbPlacementStatus::PLACED, + .iterms = {{.net_name = "in3", .term_name = "A"}, + {.net_name = "nbuf", .term_name = "Z"}}}); + makeInst(block_, + db_->findMaster("INV_X1"), + "inew", + {.location = {450, 300}, + .status = odb::dbPlacementStatus::PLACED, + .iterms = {{.net_name = "nbuf", .term_name = "A"}, + {.net_name = "ninv", .term_name = "ZN"}}}); + makeInst(block_, + db_->findMaster("NAND2_X1"), + "ndnew", + {.location = {500, 300}, + .status = odb::dbPlacementStatus::PLACED, + .iterms = {{.net_name = "ninv", .term_name = "A1"}, + {.net_name = "in4", .term_name = "A2"}, + {.net_name = "nfan", .term_name = "ZN"}}}); + makeClockOn("clk2", new_bt); + sta_->updateTiming(true); + + // 4. Drvr still decodes to nd1/ZN (nd1 survived), but prev slot was + // recycled: prev_pin is not one of nd1's real inputs (nd1/A1, nd1/A2). + ASSERT_EQ(network->pathName(drvr_path->pin(sta_.get())), "nd1/ZN"); + const sta::Path* prev = drvr_path->prevPath(); + ASSERT_NE(prev, nullptr); + const std::string prev_pin = network->pathName(prev->pin(sta_.get())); + EXPECT_TRUE(prev_pin != "nd1/A1" && prev_pin != "nd1/A2") + << "prev slot should be recycled; got prev_pin=" << prev_pin; + + // 5. Pre-fix SIGSEGV on stale prev; post-fix safe early-return. + rsz::SizeUpMove size_up_move(&resizer_); + size_up_move.init(); + (void) size_up_move.doMove(drvr_path, 0.0f); +} + +} // namespace rsz From ff573e4961ece590f8ba5b4e1b0976c1686fcecc Mon Sep 17 00:00:00 2001 From: Minju Kim Date: Thu, 23 Apr 2026 20:56:05 +0900 Subject: [PATCH 09/14] rsz: tighten stale-pointer assertion + fix include-cleaner warnings Add direct stale-pointer signature check: snapshot drvr_path->prevPath() address and its decoded pin name BEFORE the free+recycle phases, then assert after-mutation that the pointer address is unchanged while the decoded pin differs -- raw pointer not refreshed by STA's incremental update, but underlying slot was freed and reused. Also fix misc-include-cleaner warnings on , odb/dbTypes.h, and sta/SdcClass.hh. Signed-off-by: Minju Kim --- src/rsz/test/cpp/TestPathExpandedStaleness.cc | 34 +++++++++++++------ 1 file changed, 23 insertions(+), 11 deletions(-) diff --git a/src/rsz/test/cpp/TestPathExpandedStaleness.cc b/src/rsz/test/cpp/TestPathExpandedStaleness.cc index de36735a9f6..38654bb22e6 100644 --- a/src/rsz/test/cpp/TestPathExpandedStaleness.cc +++ b/src/rsz/test/cpp/TestPathExpandedStaleness.cc @@ -1,6 +1,8 @@ // SPDX-License-Identifier: BSD-3-Clause // Copyright (c) 2026-2026, The OpenROAD Authors +#include + #include "SizeUpMove.hh" #include "ant/AntennaChecker.hh" #include "db_sta/dbNetwork.hh" @@ -10,11 +12,13 @@ #include "grt/GlobalRouter.h" #include "gtest/gtest.h" #include "odb/db.h" +#include "odb/dbTypes.h" #include "rsz/Resizer.hh" #include "sta/Graph.hh" #include "sta/NetworkClass.hh" #include "sta/Path.hh" #include "sta/Sdc.hh" +#include "sta/SdcClass.hh" #include "sta/Sta.hh" #include "stt/SteinerTreeBuilder.h" #include "tst/nangate45_fixture.h" @@ -126,11 +130,12 @@ class StalePathTest : public tst::Nangate45Fixture // Regression for #10210 (stale Path* dereference). // // Flow: -// 1. Capture drvr_path at nd1/ZN (terminal logic cell, stays alive). +// 1. Capture drvr_path at nd1/ZN and snapshot prevPath() pointer + pin. // 2. Delete upstream b1 + updateTiming -> free b1/inv1 Path[] slots. // 3. Add new cone + clk2 + updateTiming -> recycle freed slots. -// 4. Assert drvr_path->prevPath()->pin() now decodes to a stale pin -// (not nd1's real input). +// 4. Assert stale-pointer signature: prevPath() returns the SAME raw +// pointer as before but pin() now decodes to different data (slot +// was freed + reused without STA refreshing prev_path_). // 5. Call SizeUpMove::doMove: pre-fix derefs stale prev -> SIGSEGV; // post-fix reads prev_arc (liberty-stable) -> safe early-return. TEST_F(StalePathTest, SizeUpMoveSurvivesStalePathAfterUpdateTiming) @@ -138,12 +143,16 @@ TEST_F(StalePathTest, SizeUpMoveSurvivesStalePathAfterUpdateTiming) sta::Network* network = sta_->network(); sta::Graph* graph = sta_->ensureGraph(); - // 1. Capture drvr_path at nd1/ZN. + // 1. Capture drvr_path at nd1/ZN and snapshot prev identity. sta::Instance* nd1 = db_network_->dbToSta(block_->findInst("nd1")); sta::Path* drvr_path = sta_->vertexWorstArrivalPath( graph->pinDrvrVertex(network->findPin(nd1, "ZN")), sta::MinMax::max()); ASSERT_NE(drvr_path, nullptr); ASSERT_EQ(network->pathName(drvr_path->pin(sta_.get())), "nd1/ZN"); + const sta::Path* prev_before = drvr_path->prevPath(); + ASSERT_NE(prev_before, nullptr); + const std::string prev_pin_before + = network->pathName(prev_before->pin(sta_.get())); // 2. Free upstream Path[] slots. sta_->deleteInstance(db_network_->dbToSta(block_->findInst("b1"))); @@ -185,14 +194,17 @@ TEST_F(StalePathTest, SizeUpMoveSurvivesStalePathAfterUpdateTiming) makeClockOn("clk2", new_bt); sta_->updateTiming(true); - // 4. Drvr still decodes to nd1/ZN (nd1 survived), but prev slot was - // recycled: prev_pin is not one of nd1's real inputs (nd1/A1, nd1/A2). + // 4. Stale-pointer signature: same raw prev pointer, different decode. ASSERT_EQ(network->pathName(drvr_path->pin(sta_.get())), "nd1/ZN"); - const sta::Path* prev = drvr_path->prevPath(); - ASSERT_NE(prev, nullptr); - const std::string prev_pin = network->pathName(prev->pin(sta_.get())); - EXPECT_TRUE(prev_pin != "nd1/A1" && prev_pin != "nd1/A2") - << "prev slot should be recycled; got prev_pin=" << prev_pin; + const sta::Path* prev_after = drvr_path->prevPath(); + ASSERT_NE(prev_after, nullptr); + EXPECT_EQ(prev_after, prev_before) + << "stale raw pointer: prev_path_ should be unchanged in address"; + const std::string prev_pin_after + = network->pathName(prev_after->pin(sta_.get())); + EXPECT_NE(prev_pin_after, prev_pin_before) + << "but slot content should differ after free+reuse. before=" + << prev_pin_before << " after=" << prev_pin_after; // 5. Pre-fix SIGSEGV on stale prev; post-fix safe early-return. rsz::SizeUpMove size_up_move(&resizer_); From 262c3128269e31cddd7cfe7e2b39ab7b3ef0aae9 Mon Sep 17 00:00:00 2001 From: Minju Kim Date: Thu, 23 Apr 2026 21:39:10 +0900 Subject: [PATCH 10/14] rsz: fix TestPathExpandedStaleness Bazel build + allocator portability Bazel CI failed to compile the test because `SizeUpMove.hh` is a private rsz impl header and the Bazel cc_test did not have `src/rsz/src` on the include path (CMake's target_include_directories covered it, so local CMake build was green). Add the same `copts = ["-Isrc/rsz/src"]` + `features = ["-layering_check"]` pattern used by //src/web/test targets that reach into their module's internal headers. With the test now building under Bazel the strict assertion `drvr_path->pin() == "nd1/ZN"` started failing in sandbox (Bazel runs under a different allocator: the drvr Path[] slot itself got recycled with `ndnew/ZN` content, while CMake/glibc keeps the drvr slot and only recycles prev). Either form is legitimate stale-slot evidence the fix has to tolerate. Relax step 4 to accept both: assert "at least one slot got recycled (drvr or prev)", and only require the tight same- address-different-decode signature when the drvr slot was preserved. Also fix misc-include-cleaner warning on `std::fprintf` (``). Signed-off-by: Minju Kim --- src/rsz/test/BUILD | 2 + src/rsz/test/cpp/TestPathExpandedStaleness.cc | 43 +++++++++++++++---- 2 files changed, 36 insertions(+), 9 deletions(-) diff --git a/src/rsz/test/BUILD b/src/rsz/test/BUILD index 7c723bac468..b7629c79ecf 100644 --- a/src/rsz/test/BUILD +++ b/src/rsz/test/BUILD @@ -270,10 +270,12 @@ cc_test( srcs = [ "cpp/TestPathExpandedStaleness.cc", ], + copts = ["-Isrc/rsz/src"], data = [ "Nangate45/Nangate45.lef", "Nangate45/Nangate45_typ.lib", ], + features = ["-layering_check"], deps = [ "//src/ant", "//src/dbSta", diff --git a/src/rsz/test/cpp/TestPathExpandedStaleness.cc b/src/rsz/test/cpp/TestPathExpandedStaleness.cc index 38654bb22e6..5928b464620 100644 --- a/src/rsz/test/cpp/TestPathExpandedStaleness.cc +++ b/src/rsz/test/cpp/TestPathExpandedStaleness.cc @@ -1,6 +1,7 @@ // SPDX-License-Identifier: BSD-3-Clause // Copyright (c) 2026-2026, The OpenROAD Authors +#include #include #include "SizeUpMove.hh" @@ -194,17 +195,41 @@ TEST_F(StalePathTest, SizeUpMoveSurvivesStalePathAfterUpdateTiming) makeClockOn("clk2", new_bt); sta_->updateTiming(true); - // 4. Stale-pointer signature: same raw prev pointer, different decode. - ASSERT_EQ(network->pathName(drvr_path->pin(sta_.get())), "nd1/ZN"); + // 4. Staleness evidence: at least one of the captured Path's slots has + // been recycled. The precise form depends on the allocator (system + // libc vs. jemalloc/tcmalloc etc.): + // (a) drvr Path slot itself recycled -> drvr pin decodes to something + // other than nd1/ZN, or + // (b) drvr slot preserved but prev slot recycled -> prev pin decodes + // to a pin that is not one of nd1's real inputs (nd1/A1, nd1/A2). + // (b) is the strict stale-pointer signature (raw pointer address + // unchanged, content changed); we assert it tightly when drvr survives. + const std::string drvr_pin_after + = network->pathName(drvr_path->pin(sta_.get())); const sta::Path* prev_after = drvr_path->prevPath(); - ASSERT_NE(prev_after, nullptr); - EXPECT_EQ(prev_after, prev_before) - << "stale raw pointer: prev_path_ should be unchanged in address"; const std::string prev_pin_after - = network->pathName(prev_after->pin(sta_.get())); - EXPECT_NE(prev_pin_after, prev_pin_before) - << "but slot content should differ after free+reuse. before=" - << prev_pin_before << " after=" << prev_pin_after; + = prev_after ? network->pathName(prev_after->pin(sta_.get())) + : std::string(""); + const bool drvr_recycled = drvr_pin_after != "nd1/ZN"; + const bool prev_recycled = prev_after != nullptr && prev_pin_after != "nd1/A1" + && prev_pin_after != "nd1/A2"; + EXPECT_TRUE(drvr_recycled || prev_recycled) + << "expected slot reuse to be demonstrable; drvr=" << drvr_pin_after + << " prev=" << prev_pin_after; + if (!drvr_recycled && prev_after != nullptr) { + EXPECT_EQ(prev_after, prev_before) + << "stale-pointer signature: prev_path_ address unchanged"; + EXPECT_NE(prev_pin_after, prev_pin_before) + << "but slot content should differ after free+reuse. before=" + << prev_pin_before << " after=" << prev_pin_after; + } + std::fprintf(stderr, + "DBG stale: drvr=%s prev_addr=%p prev_before=%s " + "prev_after=%s\n", + drvr_pin_after.c_str(), + (const void*) prev_after, + prev_pin_before.c_str(), + prev_pin_after.c_str()); // 5. Pre-fix SIGSEGV on stale prev; post-fix safe early-return. rsz::SizeUpMove size_up_move(&resizer_); From 79634096075f3226867f4a54169bab6c272c43b6 Mon Sep 17 00:00:00 2001 From: Minju Kim Date: Fri, 24 Apr 2026 11:43:50 +0900 Subject: [PATCH 11/14] rsz: simplify stale-Path regression test + generalize naming Slim the #10210 regression down to the essential claim -- that the raw prev_path_ pointer survives free+reuse of its underlying slot: - Shrink the Phase-3 recycle trigger from a 3-stage BUF/INV/NAND2 cone + 2 extra BTerms + 3 nets to a single fresh BUF + clock. Both glibc (CMake) and the Bazel sandbox allocator still reliably recycle the freed slot with just this one allocation. - Drop the SizeUpMove::doMove invocation and its diagnostic fprintf. The staleness itself is the regression the fix addresses; the crash-on-deref follow-up is no longer needed for the assertion to land. Also drops the #include of the private "SizeUpMove.hh" header and the Bazel copts/features hack that enabled it. - Rename file/fixture/test to topology-based names so future tests sharing the same small BUF->INV->NAND2+side_input circuit can just add a TEST_F alongside this one: TestPathExpandedStaleness.cc -> TestBufInvNand2.cc class StalePathTest -> class BufInvNand2Test SizeUpMoveSurvivesStale... -> StalePrevPathAfterUpdateTiming Net effect: -42 LOC, clearer file/test scoping, no internal-header dependency in Bazel. CMake + Bazel builds both pass, clang-tidy clean. Signed-off-by: Minju Kim --- src/rsz/test/BUILD | 6 +- src/rsz/test/cpp/CMakeLists.txt | 13 +--- ...xpandedStaleness.cc => TestBufInvNand2.cc} | 73 +++++-------------- 3 files changed, 25 insertions(+), 67 deletions(-) rename src/rsz/test/cpp/{TestPathExpandedStaleness.cc => TestBufInvNand2.cc} (71%) diff --git a/src/rsz/test/BUILD b/src/rsz/test/BUILD index b7629c79ecf..699bcc92075 100644 --- a/src/rsz/test/BUILD +++ b/src/rsz/test/BUILD @@ -266,16 +266,14 @@ test_suite( ) cc_test( - name = "TestPathExpandedStaleness", + name = "TestBufInvNand2", srcs = [ - "cpp/TestPathExpandedStaleness.cc", + "cpp/TestBufInvNand2.cc", ], - copts = ["-Isrc/rsz/src"], data = [ "Nangate45/Nangate45.lef", "Nangate45/Nangate45_typ.lib", ], - features = ["-layering_check"], deps = [ "//src/ant", "//src/dbSta", diff --git a/src/rsz/test/cpp/CMakeLists.txt b/src/rsz/test/cpp/CMakeLists.txt index b56796dcb72..b214799dab5 100644 --- a/src/rsz/test/cpp/CMakeLists.txt +++ b/src/rsz/test/cpp/CMakeLists.txt @@ -28,8 +28,8 @@ gtest_discover_tests(TestBufRem1 add_dependencies(build_and_test TestBufRem1 ) -add_executable(TestPathExpandedStaleness TestPathExpandedStaleness.cc) -target_link_libraries(TestPathExpandedStaleness +add_executable(TestBufInvNand2 TestBufInvNand2.cc) +target_link_libraries(TestBufInvNand2 OpenSTA GTest::gtest GTest::gtest_main @@ -45,16 +45,11 @@ target_link_libraries(TestPathExpandedStaleness ${TCL_LIBRARY} ) -target_include_directories(TestPathExpandedStaleness - PRIVATE - ${PROJECT_SOURCE_DIR}/src/rsz/src -) - -gtest_discover_tests(TestPathExpandedStaleness +gtest_discover_tests(TestBufInvNand2 WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/.. ) -add_dependencies(build_and_test TestPathExpandedStaleness +add_dependencies(build_and_test TestBufInvNand2 ) add_executable(TestBufferRemoval2 TestBufferRemoval2.cc) diff --git a/src/rsz/test/cpp/TestPathExpandedStaleness.cc b/src/rsz/test/cpp/TestBufInvNand2.cc similarity index 71% rename from src/rsz/test/cpp/TestPathExpandedStaleness.cc rename to src/rsz/test/cpp/TestBufInvNand2.cc index 5928b464620..c5d772197e1 100644 --- a/src/rsz/test/cpp/TestPathExpandedStaleness.cc +++ b/src/rsz/test/cpp/TestBufInvNand2.cc @@ -1,10 +1,8 @@ // SPDX-License-Identifier: BSD-3-Clause // Copyright (c) 2026-2026, The OpenROAD Authors -#include #include -#include "SizeUpMove.hh" #include "ant/AntennaChecker.hh" #include "db_sta/dbNetwork.hh" #include "db_sta/dbSta.hh" @@ -30,10 +28,10 @@ namespace rsz { static const std::string prefix("_main/src/rsz/test/"); // in1 -> b1(BUF) -> inv1(INV) -> nd1(NAND2) -> out1 -class StalePathTest : public tst::Nangate45Fixture +class BufInvNand2Test : public tst::Nangate45Fixture { protected: - StalePathTest() + BufInvNand2Test() : stt_(db_.get(), &logger_), service_registry_(&logger_), dp_(db_.get(), &logger_), @@ -133,13 +131,13 @@ class StalePathTest : public tst::Nangate45Fixture // Flow: // 1. Capture drvr_path at nd1/ZN and snapshot prevPath() pointer + pin. // 2. Delete upstream b1 + updateTiming -> free b1/inv1 Path[] slots. -// 3. Add new cone + clk2 + updateTiming -> recycle freed slots. -// 4. Assert stale-pointer signature: prevPath() returns the SAME raw -// pointer as before but pin() now decodes to different data (slot -// was freed + reused without STA refreshing prev_path_). -// 5. Call SizeUpMove::doMove: pre-fix derefs stale prev -> SIGSEGV; -// post-fix reads prev_arc (liberty-stable) -> safe early-return. -TEST_F(StalePathTest, SizeUpMoveSurvivesStalePathAfterUpdateTiming) +// 3. Add a fresh BUF + clock + updateTiming -> recycle freed slots. +// 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. When the drvr slot itself is preserved, also assert +// the strict stale-pointer signature (same raw address, different +// content). +TEST_F(BufInvNand2Test, StalePrevPathAfterUpdateTiming) { sta::Network* network = sta_->network(); sta::Graph* graph = sta_->ensureGraph(); @@ -159,16 +157,11 @@ TEST_F(StalePathTest, SizeUpMoveSurvivesStalePathAfterUpdateTiming) sta_->deleteInstance(db_network_->dbToSta(block_->findInst("b1"))); sta_->updateTiming(true); - // 3. Recycle slots via a fresh cone + clock. + // 3. Recycle freed slots via a single fresh BUF driven by a new clock. auto* new_bt = makeBTerm( block_, "in3", {.bpins = {{.layer_name = "metal1", .rect = {0, 300, 10, 310}}}}); - makeBTerm(block_, - "in4", - {.bpins = {{.layer_name = "metal1", .rect = {0, 400, 10, 410}}}}); - odb::dbNet::create(block_, "nbuf"); - odb::dbNet::create(block_, "ninv"); odb::dbNet::create(block_, "nfan"); makeInst(block_, db_->findMaster("BUF_X1"), @@ -176,34 +169,18 @@ TEST_F(StalePathTest, SizeUpMoveSurvivesStalePathAfterUpdateTiming) {.location = {400, 300}, .status = odb::dbPlacementStatus::PLACED, .iterms = {{.net_name = "in3", .term_name = "A"}, - {.net_name = "nbuf", .term_name = "Z"}}}); - makeInst(block_, - db_->findMaster("INV_X1"), - "inew", - {.location = {450, 300}, - .status = odb::dbPlacementStatus::PLACED, - .iterms = {{.net_name = "nbuf", .term_name = "A"}, - {.net_name = "ninv", .term_name = "ZN"}}}); - makeInst(block_, - db_->findMaster("NAND2_X1"), - "ndnew", - {.location = {500, 300}, - .status = odb::dbPlacementStatus::PLACED, - .iterms = {{.net_name = "ninv", .term_name = "A1"}, - {.net_name = "in4", .term_name = "A2"}, - {.net_name = "nfan", .term_name = "ZN"}}}); + {.net_name = "nfan", .term_name = "Z"}}}); makeClockOn("clk2", new_bt); sta_->updateTiming(true); - // 4. Staleness evidence: at least one of the captured Path's slots has - // been recycled. The precise form depends on the allocator (system - // libc vs. jemalloc/tcmalloc etc.): - // (a) drvr Path slot itself recycled -> drvr pin decodes to something - // other than nd1/ZN, or - // (b) drvr slot preserved but prev slot recycled -> prev pin decodes - // to a pin that is not one of nd1's real inputs (nd1/A1, nd1/A2). - // (b) is the strict stale-pointer signature (raw pointer address - // unchanged, content changed); we assert it tightly when drvr survives. + // 4. Staleness evidence. Allocator behaviour decides which slot lands + // on the recycled memory: + // (a) drvr slot preserved + prev slot recycled (glibc in our CI) -- + // strict stale-pointer signature: same raw prev address, but + // pin() decodes to content from an unrelated instance. + // (b) drvr slot itself recycled (other allocators) -- drvr pin decodes + // to something other than nd1/ZN. + // Either case proves the captured raw Path* outlived the slot. const std::string drvr_pin_after = network->pathName(drvr_path->pin(sta_.get())); const sta::Path* prev_after = drvr_path->prevPath(); @@ -223,18 +200,6 @@ TEST_F(StalePathTest, SizeUpMoveSurvivesStalePathAfterUpdateTiming) << "but slot content should differ after free+reuse. before=" << prev_pin_before << " after=" << prev_pin_after; } - std::fprintf(stderr, - "DBG stale: drvr=%s prev_addr=%p prev_before=%s " - "prev_after=%s\n", - drvr_pin_after.c_str(), - (const void*) prev_after, - prev_pin_before.c_str(), - prev_pin_after.c_str()); - - // 5. Pre-fix SIGSEGV on stale prev; post-fix safe early-return. - rsz::SizeUpMove size_up_move(&resizer_); - size_up_move.init(); - (void) size_up_move.doMove(drvr_path, 0.0f); } } // namespace rsz From 79e8cb343520aac2de0283f33e532d05dbf19732 Mon Sep 17 00:00:00 2001 From: Minju Kim Date: Fri, 24 Apr 2026 13:25:28 +0900 Subject: [PATCH 12/14] dbSta: move stale-Path regression from rsz and switch to verilog+IntegratedFixture Signed-off-by: Minju Kim --- src/dbSta/test/BUILD | 1 + src/dbSta/test/cpp/TestDbSta.cc | 88 ++++++++++ src/dbSta/test/cpp/TestDbSta_StalePath.v | 18 ++ src/rsz/test/BUILD | 28 ---- src/rsz/test/cpp/CMakeLists.txt | 24 --- src/rsz/test/cpp/TestBufInvNand2.cc | 205 ----------------------- 6 files changed, 107 insertions(+), 257 deletions(-) create mode 100644 src/dbSta/test/cpp/TestDbSta_StalePath.v delete mode 100644 src/rsz/test/cpp/TestBufInvNand2.cc diff --git a/src/dbSta/test/BUILD b/src/dbSta/test/BUILD index 20444a82e8c..fd106d6d157 100644 --- a/src/dbSta/test/BUILD +++ b/src/dbSta/test/BUILD @@ -293,6 +293,7 @@ cc_test( "Nangate45/Nangate45.lef", "Nangate45/Nangate45_typ.lib", "cpp/TestDbSta_0.v", + "cpp/TestDbSta_StalePath.v", ], deps = [ "//src/dbSta", diff --git a/src/dbSta/test/cpp/TestDbSta.cc b/src/dbSta/test/cpp/TestDbSta.cc index defb66ca337..eb91bc55918 100644 --- a/src/dbSta/test/cpp/TestDbSta.cc +++ b/src/dbSta/test/cpp/TestDbSta.cc @@ -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 { @@ -97,4 +102,87 @@ TEST_F(TestDbSta, TestHierarchyConnectivity) ASSERT_EQ(bterm_clk->getITerm(), nullptr); } +// Regression for #10210 (stale Path* dereference in rsz). +// +// Topology (TestDbSta_StalePath.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. +// 2. Delete upstream b1 + updateTiming -> free b1/inv1 Path[] slots. +// 3. Add a fresh BUF + clock + updateTiming -> recycle freed slots. +// 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. When the drvr slot itself is preserved, also assert +// the strict stale-pointer signature (same raw address, different +// content). +TEST_F(TestDbSta, StalePrevPathAfterUpdateTiming) +{ + readVerilogAndSetup("TestDbSta_StalePath.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* prev_before = drvr_path->prevPath(); + ASSERT_NE(prev_before, nullptr); + const std::string prev_pin_before + = network->pathName(prev_before->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. Allocator behaviour decides which slot lands + // on the recycled memory: + // (a) drvr slot preserved + prev slot recycled (glibc in our CI) -- + // strict stale-pointer signature: same raw prev address, but + // pin() decodes to content from an unrelated instance. + // (b) drvr slot itself recycled (other allocators) -- drvr pin decodes + // to something other than nd1/ZN. + // Either case proves the captured raw Path* outlived the slot. + const std::string drvr_pin_after + = network->pathName(drvr_path->pin(sta_.get())); + const Path* prev_after = drvr_path->prevPath(); + const std::string prev_pin_after + = prev_after ? network->pathName(prev_after->pin(sta_.get())) + : std::string(""); + const bool drvr_recycled = drvr_pin_after != "nd1/ZN"; + const bool prev_recycled = prev_after != nullptr && prev_pin_after != "nd1/A1" + && prev_pin_after != "nd1/A2"; + EXPECT_TRUE(drvr_recycled || prev_recycled) + << "expected slot reuse to be demonstrable; drvr=" << drvr_pin_after + << " prev=" << prev_pin_after; + if (!drvr_recycled && prev_after != nullptr) { + EXPECT_EQ(prev_after, prev_before) + << "stale-pointer signature: prev_path_ address unchanged"; + EXPECT_NE(prev_pin_after, prev_pin_before) + << "but slot content should differ after free+reuse. before=" + << prev_pin_before << " after=" << prev_pin_after; + } +} + } // namespace sta diff --git a/src/dbSta/test/cpp/TestDbSta_StalePath.v b/src/dbSta/test/cpp/TestDbSta_StalePath.v new file mode 100644 index 00000000000..078c5e90667 --- /dev/null +++ b/src/dbSta/test/cpp/TestDbSta_StalePath.v @@ -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 diff --git a/src/rsz/test/BUILD b/src/rsz/test/BUILD index 699bcc92075..957c0139272 100644 --- a/src/rsz/test/BUILD +++ b/src/rsz/test/BUILD @@ -265,34 +265,6 @@ test_suite( tests = [":TestBufRem1"], ) -cc_test( - name = "TestBufInvNand2", - srcs = [ - "cpp/TestBufInvNand2.cc", - ], - data = [ - "Nangate45/Nangate45.lef", - "Nangate45/Nangate45_typ.lib", - ], - deps = [ - "//src/ant", - "//src/dbSta", - "//src/dbSta:dbNetwork", - "//src/dpl", - "//src/est", - "//src/grt", - "//src/odb", - "//src/rsz", - "//src/sta:opensta_lib", - "//src/stt", - "//src/tst", - "//src/tst:nangate45_fixture", - "//src/utl", - "@googletest//:gtest", - "@googletest//:gtest_main", - ], -) - cc_test( name = "rsz_buffer_removal", srcs = [ diff --git a/src/rsz/test/cpp/CMakeLists.txt b/src/rsz/test/cpp/CMakeLists.txt index b214799dab5..75900b054ac 100644 --- a/src/rsz/test/cpp/CMakeLists.txt +++ b/src/rsz/test/cpp/CMakeLists.txt @@ -28,30 +28,6 @@ gtest_discover_tests(TestBufRem1 add_dependencies(build_and_test TestBufRem1 ) -add_executable(TestBufInvNand2 TestBufInvNand2.cc) -target_link_libraries(TestBufInvNand2 - OpenSTA - GTest::gtest - GTest::gtest_main - GTest::gmock - dbSta_lib - utl_lib - rsz_lib - grt_lib - dpl_lib - stt_lib - odb - tst - ${TCL_LIBRARY} -) - -gtest_discover_tests(TestBufInvNand2 - WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/.. -) - -add_dependencies(build_and_test TestBufInvNand2 -) - add_executable(TestBufferRemoval2 TestBufferRemoval2.cc) target_link_libraries(TestBufferRemoval2 OpenSTA diff --git a/src/rsz/test/cpp/TestBufInvNand2.cc b/src/rsz/test/cpp/TestBufInvNand2.cc deleted file mode 100644 index c5d772197e1..00000000000 --- a/src/rsz/test/cpp/TestBufInvNand2.cc +++ /dev/null @@ -1,205 +0,0 @@ -// SPDX-License-Identifier: BSD-3-Clause -// Copyright (c) 2026-2026, The OpenROAD Authors - -#include - -#include "ant/AntennaChecker.hh" -#include "db_sta/dbNetwork.hh" -#include "db_sta/dbSta.hh" -#include "dpl/Opendp.h" -#include "est/EstimateParasitics.h" -#include "grt/GlobalRouter.h" -#include "gtest/gtest.h" -#include "odb/db.h" -#include "odb/dbTypes.h" -#include "rsz/Resizer.hh" -#include "sta/Graph.hh" -#include "sta/NetworkClass.hh" -#include "sta/Path.hh" -#include "sta/Sdc.hh" -#include "sta/SdcClass.hh" -#include "sta/Sta.hh" -#include "stt/SteinerTreeBuilder.h" -#include "tst/nangate45_fixture.h" -#include "utl/ServiceRegistry.h" - -namespace rsz { - -static const std::string prefix("_main/src/rsz/test/"); - -// in1 -> b1(BUF) -> inv1(INV) -> nd1(NAND2) -> out1 -class BufInvNand2Test : public tst::Nangate45Fixture -{ - protected: - BufInvNand2Test() - : stt_(db_.get(), &logger_), - service_registry_(&logger_), - dp_(db_.get(), &logger_), - ant_(db_.get(), &logger_), - grt_(&logger_, - &service_registry_, - &stt_, - db_.get(), - sta_.get(), - &ant_, - &dp_), - ep_(&logger_, &service_registry_, db_.get(), sta_.get(), &stt_, &grt_), - resizer_(&logger_, db_.get(), sta_.get(), &stt_, &grt_, &dp_, &ep_) - { - readLiberty(prefix + "Nangate45/Nangate45_typ.lib"); - db_network_ = sta_->getDbNetwork(); - db_network_->setBlock(block_); - sta_->postReadDef(block_); - - const char* layer = "metal1"; - makeBTerm(block_, - "in1", - {.bpins = {{.layer_name = layer, .rect = {0, 0, 10, 10}}}}); - makeBTerm(block_, - "in2", - {.bpins = {{.layer_name = layer, .rect = {0, 100, 10, 110}}}}); - makeBTerm( - block_, - "out1", - {.io_type = odb::dbIoType::OUTPUT, - .bpins = {{.layer_name = layer, .rect = {990, 990, 1000, 1000}}}}); - - makeInst(block_, - db_->findMaster("BUF_X1"), - "b1", - {.location = {100, 100}, - .status = odb::dbPlacementStatus::PLACED, - .iterms = {{.net_name = "in1", .term_name = "A"}, - {.net_name = "n0", .term_name = "Z"}}}); - makeInst(block_, - db_->findMaster("INV_X1"), - "inv1", - {.location = {200, 100}, - .status = odb::dbPlacementStatus::PLACED, - .iterms = {{.net_name = "n0", .term_name = "A"}, - {.net_name = "n1", .term_name = "ZN"}}}); - makeInst(block_, - db_->findMaster("NAND2_X1"), - "nd1", - {.location = {300, 150}, - .status = odb::dbPlacementStatus::PLACED, - .iterms = {{.net_name = "n1", .term_name = "A1"}, - {.net_name = "in2", .term_name = "A2"}, - {.net_name = "out1", .term_name = "ZN"}}}); - - makeClockOn("clk", block_->findBTerm("in1")); - sta::Sdc* sdc = sta_->cmdSdc(); - sta_->setOutputDelay(db_network_->dbToSta(block_->findBTerm("out1")), - sta::RiseFallBoth::riseFall(), - sdc->findClock("clk"), - sta::RiseFall::rise(), - nullptr, - false, - false, - sta::MinMaxAll::all(), - false, - 0.0f, - sdc); - - resizer_.initBlock(); - db_->setLogger(&logger_); - sta_->updateTiming(true); - } - - // sta::Sdc::makeClock takes ownership of the PinSet* and FloatSeq* and - // frees them in Clock::setPins / ~Clock, so the raw `new` is not a leak. - void makeClockOn(const char* clk_name, odb::dbBTerm* bt) - { - auto* pins = new sta::PinSet(sta_->network()); - pins->insert(db_network_->dbToSta(bt)); - auto* waveform = new sta::FloatSeq{0.0f, 0.1f}; - sta_->makeClock(clk_name, pins, false, 0.2f, waveform, "", sta_->cmdMode()); - } - - stt::SteinerTreeBuilder stt_; - utl::ServiceRegistry service_registry_; - dpl::Opendp dp_; - ant::AntennaChecker ant_; - grt::GlobalRouter grt_; - est::EstimateParasitics ep_; - rsz::Resizer resizer_; - sta::dbNetwork* db_network_{nullptr}; -}; - -// Regression for #10210 (stale Path* dereference). -// -// Flow: -// 1. Capture drvr_path at nd1/ZN and snapshot prevPath() pointer + pin. -// 2. Delete upstream b1 + updateTiming -> free b1/inv1 Path[] slots. -// 3. Add a fresh BUF + clock + updateTiming -> recycle freed slots. -// 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. When the drvr slot itself is preserved, also assert -// the strict stale-pointer signature (same raw address, different -// content). -TEST_F(BufInvNand2Test, StalePrevPathAfterUpdateTiming) -{ - sta::Network* network = sta_->network(); - sta::Graph* graph = sta_->ensureGraph(); - - // 1. Capture drvr_path at nd1/ZN and snapshot prev identity. - sta::Instance* nd1 = db_network_->dbToSta(block_->findInst("nd1")); - sta::Path* drvr_path = sta_->vertexWorstArrivalPath( - graph->pinDrvrVertex(network->findPin(nd1, "ZN")), sta::MinMax::max()); - ASSERT_NE(drvr_path, nullptr); - ASSERT_EQ(network->pathName(drvr_path->pin(sta_.get())), "nd1/ZN"); - const sta::Path* prev_before = drvr_path->prevPath(); - ASSERT_NE(prev_before, nullptr); - const std::string prev_pin_before - = network->pathName(prev_before->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. - auto* new_bt = makeBTerm( - block_, - "in3", - {.bpins = {{.layer_name = "metal1", .rect = {0, 300, 10, 310}}}}); - odb::dbNet::create(block_, "nfan"); - makeInst(block_, - db_->findMaster("BUF_X1"), - "bnew", - {.location = {400, 300}, - .status = odb::dbPlacementStatus::PLACED, - .iterms = {{.net_name = "in3", .term_name = "A"}, - {.net_name = "nfan", .term_name = "Z"}}}); - makeClockOn("clk2", new_bt); - sta_->updateTiming(true); - - // 4. Staleness evidence. Allocator behaviour decides which slot lands - // on the recycled memory: - // (a) drvr slot preserved + prev slot recycled (glibc in our CI) -- - // strict stale-pointer signature: same raw prev address, but - // pin() decodes to content from an unrelated instance. - // (b) drvr slot itself recycled (other allocators) -- drvr pin decodes - // to something other than nd1/ZN. - // Either case proves the captured raw Path* outlived the slot. - const std::string drvr_pin_after - = network->pathName(drvr_path->pin(sta_.get())); - const sta::Path* prev_after = drvr_path->prevPath(); - const std::string prev_pin_after - = prev_after ? network->pathName(prev_after->pin(sta_.get())) - : std::string(""); - const bool drvr_recycled = drvr_pin_after != "nd1/ZN"; - const bool prev_recycled = prev_after != nullptr && prev_pin_after != "nd1/A1" - && prev_pin_after != "nd1/A2"; - EXPECT_TRUE(drvr_recycled || prev_recycled) - << "expected slot reuse to be demonstrable; drvr=" << drvr_pin_after - << " prev=" << prev_pin_after; - if (!drvr_recycled && prev_after != nullptr) { - EXPECT_EQ(prev_after, prev_before) - << "stale-pointer signature: prev_path_ address unchanged"; - EXPECT_NE(prev_pin_after, prev_pin_before) - << "but slot content should differ after free+reuse. before=" - << prev_pin_before << " after=" << prev_pin_after; - } -} - -} // namespace rsz From 8ad26897b327c934c9c1d146b051f908d2e5df29 Mon Sep 17 00:00:00 2001 From: Minju Kim Date: Fri, 24 Apr 2026 13:50:13 +0900 Subject: [PATCH 13/14] dbSta: simplify StalePrevPathAfterUpdateTiming assertions Collapse the dual allocator-branch assertion (drvr_recycled vs prev_recycled) into a single strict stale-pointer signature: same prev_path_ address, different decoded pin name. Rename pre/post snapshot vars for clarity and drop the unused drvr_pin_after local. Signed-off-by: Minju Kim --- src/dbSta/test/cpp/TestDbSta.cc | 55 +++++++++++---------------------- 1 file changed, 18 insertions(+), 37 deletions(-) diff --git a/src/dbSta/test/cpp/TestDbSta.cc b/src/dbSta/test/cpp/TestDbSta.cc index eb91bc55918..2aca23462c7 100644 --- a/src/dbSta/test/cpp/TestDbSta.cc +++ b/src/dbSta/test/cpp/TestDbSta.cc @@ -109,14 +109,12 @@ TEST_F(TestDbSta, TestHierarchyConnectivity) // nd1/A2 <- in2 // // Flow: -// 1. Capture drvr_path at nd1/ZN and snapshot prevPath() pointer + pin. -// 2. Delete upstream b1 + updateTiming -> free b1/inv1 Path[] slots. -// 3. Add a fresh BUF + clock + updateTiming -> recycle freed slots. +// 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. When the drvr slot itself is preserved, also assert -// the strict stale-pointer signature (same raw address, different -// content). +// real input. TEST_F(TestDbSta, StalePrevPathAfterUpdateTiming) { readVerilogAndSetup("TestDbSta_StalePath.v"); @@ -130,10 +128,9 @@ TEST_F(TestDbSta, StalePrevPathAfterUpdateTiming) MinMax::max()); ASSERT_NE(drvr_path, nullptr); ASSERT_EQ(network->pathName(drvr_path->pin(sta_.get())), "nd1/ZN"); - const Path* prev_before = drvr_path->prevPath(); - ASSERT_NE(prev_before, nullptr); - const std::string prev_pin_before - = network->pathName(prev_before->pin(sta_.get())); + 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"))); @@ -156,33 +153,17 @@ TEST_F(TestDbSta, StalePrevPathAfterUpdateTiming) "clk2", clk2_pins, false, 0.2f, clk2_waveform, "", sta_->cmdMode()); sta_->updateTiming(true); - // 4. Staleness evidence. Allocator behaviour decides which slot lands - // on the recycled memory: - // (a) drvr slot preserved + prev slot recycled (glibc in our CI) -- - // strict stale-pointer signature: same raw prev address, but - // pin() decodes to content from an unrelated instance. - // (b) drvr slot itself recycled (other allocators) -- drvr pin decodes - // to something other than nd1/ZN. - // Either case proves the captured raw Path* outlived the slot. - const std::string drvr_pin_after - = network->pathName(drvr_path->pin(sta_.get())); - const Path* prev_after = drvr_path->prevPath(); - const std::string prev_pin_after - = prev_after ? network->pathName(prev_after->pin(sta_.get())) - : std::string(""); - const bool drvr_recycled = drvr_pin_after != "nd1/ZN"; - const bool prev_recycled = prev_after != nullptr && prev_pin_after != "nd1/A1" - && prev_pin_after != "nd1/A2"; - EXPECT_TRUE(drvr_recycled || prev_recycled) - << "expected slot reuse to be demonstrable; drvr=" << drvr_pin_after - << " prev=" << prev_pin_after; - if (!drvr_recycled && prev_after != nullptr) { - EXPECT_EQ(prev_after, prev_before) - << "stale-pointer signature: prev_path_ address unchanged"; - EXPECT_NE(prev_pin_after, prev_pin_before) - << "but slot content should differ after free+reuse. before=" - << prev_pin_before << " after=" << prev_pin_after; - } + // 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(""); + + 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 From 5a31bf909d4675a8d0a6544f9dcde82bbf9b3105 Mon Sep 17 00:00:00 2001 From: Minju Kim Date: Fri, 24 Apr 2026 14:01:29 +0900 Subject: [PATCH 14/14] dbSta: rename StalePrevPath test and derive verilog filename from gtest info Tighten the test-name <-> verilog-file coupling so future stale-path variants can just drop a new TestDbSta_.v alongside a new TEST_F without touching the data list: use gtest's current_test_info() at runtime to build the filename, matching the pattern already used in src/rsz/test/cpp/TestInsertBuffer.cpp. Test renamed StalePrevPathAfterUpdateTiming -> StalePrevPath (the "AfterUpdateTiming" qualifier was redundant with the flow header). Verilog renamed TestDbSta_StalePath.v -> TestDbSta_StalePrevPath.v to match. Signed-off-by: Minju Kim --- src/dbSta/test/BUILD | 2 +- src/dbSta/test/cpp/TestDbSta.cc | 9 ++++++--- .../{TestDbSta_StalePath.v => TestDbSta_StalePrevPath.v} | 0 3 files changed, 7 insertions(+), 4 deletions(-) rename src/dbSta/test/cpp/{TestDbSta_StalePath.v => TestDbSta_StalePrevPath.v} (100%) diff --git a/src/dbSta/test/BUILD b/src/dbSta/test/BUILD index fd106d6d157..333b78daec7 100644 --- a/src/dbSta/test/BUILD +++ b/src/dbSta/test/BUILD @@ -293,7 +293,7 @@ cc_test( "Nangate45/Nangate45.lef", "Nangate45/Nangate45_typ.lib", "cpp/TestDbSta_0.v", - "cpp/TestDbSta_StalePath.v", + "cpp/TestDbSta_StalePrevPath.v", ], deps = [ "//src/dbSta", diff --git a/src/dbSta/test/cpp/TestDbSta.cc b/src/dbSta/test/cpp/TestDbSta.cc index 2aca23462c7..eef8d5404dc 100644 --- a/src/dbSta/test/cpp/TestDbSta.cc +++ b/src/dbSta/test/cpp/TestDbSta.cc @@ -104,7 +104,7 @@ TEST_F(TestDbSta, TestHierarchyConnectivity) // Regression for #10210 (stale Path* dereference in rsz). // -// Topology (TestDbSta_StalePath.v): +// Topology (TestDbSta_StalePrevPath.v): // clk -> b1(BUF) -> inv1(INV) -> nd1(NAND2) -> out1 // nd1/A2 <- in2 // @@ -115,9 +115,12 @@ TEST_F(TestDbSta, TestHierarchyConnectivity) // 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, StalePrevPathAfterUpdateTiming) +TEST_F(TestDbSta, StalePrevPath) { - readVerilogAndSetup("TestDbSta_StalePath.v"); + 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(); diff --git a/src/dbSta/test/cpp/TestDbSta_StalePath.v b/src/dbSta/test/cpp/TestDbSta_StalePrevPath.v similarity index 100% rename from src/dbSta/test/cpp/TestDbSta_StalePath.v rename to src/dbSta/test/cpp/TestDbSta_StalePrevPath.v