`Resizer::canRemoveBuffer` mutates `dont_touch` on a query it answers "no" to

[gadfort]

Describe the bug

I was debugging something else and Claude stumbled over this issue.

Resizer::canRemoveBuffer mutates dont_touch on a query it answers "no" to

canRemoveBuffer is named and used as a predicate, but it strips dont_touch from the buffer and
from both adjacent nets, and downgrades the buffer's FIXED placement status, before the
checks that can reject the removal. When those checks reject, the buffer stays — and the flags are
gone anyway. The user asked OpenROAD not to touch an object; OpenROAD declined to touch it and
un-protected it in the same breath.

Run it

./run.sh                                # diff against expected.log; exit 0 == still reproduces
./run.sh --regen                        # re-record expected.log with your binary
OPENROAD=/path/to/openroad ./run.sh     # pick a binary

Nothing outside this directory is needed. repro.lib and repro.lef are a hand-written two-cell
technology (one buffer, one inverter, ~150 lines total) written for this reproducer — no PDK, no
test/Nangate45, no OpenROAD source tree. The netlist is six lines of Verilog.

What it shows

Two controls establish the intended behaviour; three cases show it breaking. Same design, same
b1, every time.

Case	Removal outcome	Flags after
case0-control.tcl	rejected — remove_buffers sweep form honours dont_touch	intact ✅
case4-override.tcl	succeeds — remove_buffers b1 deliberately overrides dont_touch	n/a, b1 is gone ✅
case1-sdc.tcl	rejected by Sdc::isConstrained on the buffer input pin	dont_touch gone from b1, n1 and n2 ❌
case2-merge.tcl	rejected by dbNet::canMergeNet (a dont_touch sink)	dont_touch gone from b1 — no SDC involved at all ❌
case3-fixed.tcl	rejected by dbNet::canMergeNet (a dont_touch sink)	b1 placement status FIRM → PLACED ❌

Every failing case prints RSZ-97 "Instance b1 cannot be removed …" and RSZ-26 "Removed 0
buffers." — OpenROAD reports that it did nothing, having already discarded user constraints.

case2 is the tightest statement of the bug: eight lines of Tcl, two set_dont_touch calls, no
clock, no SDC, no exception.

"Isn't the override intentional?"

Yes — and that is not what this reports. case4 establishes it: remove_buffers <inst> is
documented (/* don't honor dont touch */) and tested to override dont_touch and remove the
buffer. The in-tree golden src/rsz/test/remove_buffers3.tcl sets dont_touch on b1, runs
remove_buffers b1 b3, and remove_buffers3.ok records "Removed 2 buffers" with b1 absent from
remove_buffers3.defok. That behaviour is deliberate and this reproducer does not challenge it.

The defect is the un-rolled-back override. An override is a trade — the user's flag is spent to
buy the removal. In cases 1-3 the removal never happens and the flag is spent anyway. Nothing
downstream can tell: RSZ-97 says the instance was not removed, so a script that checks the warning
and moves on now runs the rest of the flow with protections silently dropped. There is no command
that reports "your dont_touch was consumed by a query that failed", and no way to restore it
except re-issuing every set_dont_touch after every remove_buffers.

⚠️ A fix therefore does not need to touch remove_buffers3's golden — that test exercises the
success path, which stays as-is.

Mechanism

All line numbers verified on 1a72a68e4f (2026-08-19), openroad 26Q3-1436-g1a72a68e4f.

src/rsz/src/Resizer.cc:2834-2908 — mutations first, decision last:

if (db_inst->isDoNotTouch()) {
  if (honor_dont_touch_fixed) { return false; }
  db_inst->setDoNotTouch(false);                                 // :2855  buffer's flag
}
if (db_inst->isFixed()) {
  if (honor_dont_touch_fixed) { return false; }
  db_inst->setPlacementStatus(odb::dbPlacementStatus::PLACED);   // :2861  FIXED downgraded
}
...
    input_db_net->setDoNotTouch(false);                          // :2879  both nets
    output_db_net->setDoNotTouch(false);                         // :2882
...
if (!sdc->isConstrained(input_pin) && !sdc->isConstrained(output_pin)
    && (removed == nullptr || !sdc->isConstrained(removed))
    && !sdc->isConstrained(buffer)) {                            // :2901-2903  guard runs AFTER
  return db_net_removed == nullptr
         || (db_net_survivor != nullptr
             && db_net_survivor->canMergeNet(db_net_removed));   // :2906  so can this
}
return false;                                                    // ← flags already gone

The honor_dont_touch_fixed = false path is reachable from Tcl and is an expected path, not a
corner. Resizer::removeBuffers passes false for a user-supplied instance list
(Resizer.cc:652, commented /* don't honor dont touch */), and the else arm of that same loop
exists precisely to report the rejection as RSZ-97 (:656-662). A rejection that the code goes
out of its way to log cannot be treated as unreachable.

Scope — this is the only affected entry point. canRemoveBuffer has three other call sites and
all pass true: Resizer.cc:645 (the remove_buffers sweep form), Rebuffer.cc:1752, and
move/UnbufferGenerator.cc:287. remove_buffers <inst> is the whole blast radius, which is what
case0 demonstrates from the outside.

Second defect, same function

dbNet::canMergeNet (src/odb/src/db/dbNet.cpp:2333-2345) opens with

if (isDoNotTouch() || in_net->isDoNotTouch()) { return false; }

on the two nets canRemoveBuffer has just cleared at :2879/:2882. On this path that test can
never fire — it is dead code, and the net-level dont_touch protection it implements is
unreachable from remove_buffers. Only the loop after it (checking dont_touch on instances
attached to the removed net) still does anything, which is what case2 and case3 exercise.

Suggested fix

Split the query from the edit: make canRemoveBuffer const-correct and side-effect-free, and move
the flag clearing into removeBuffer, after the decision to remove is final. removeBufferIfPossible
(Resizer.cc:2825) is already the natural seam — it calls the predicate and then the mutator.

A narrower fix that preserves today's structure: compute the SDC and canMergeNet verdict up front,
and only then clear the flags. That keeps canRemoveBuffer's signature but makes the mutation
conditional on success — and, as a side effect, revives the dead canMergeNet check above.

The second shape is probably what upstream wants: it is a reordering, not a signature change, and it
leaves the intentional override of case4 untouched.

Either way the fix wants a regression test asserting the flags survive a rejected
remove_buffers <inst> (cases 1-3 here are that test, modulo helpers.tcl and a golden .ok) and
registration in both CMake and Bazel.

Files

File
run.sh	driver; runs all five cases, diffs against expected.log
expected.log	recorded buggy output, openroad 26Q3-1436-g1a72a68e4f
case0-control.tcl, case4-override.tcl	the two controls — intended behaviour
case1-sdc.tcl, case2-merge.tcl, case3-fixed.tcl	the three failing cases
repro.v	six-line netlist: in1 → i1 → n1 → b1 → n2 → i2 → out1
repro.lib, repro.lef	hand-written two-cell technology, written for this reproducer

Expected Behavior

If the cell is kept, dont touch needs to restored.

Environment

Head of master

To Reproduce

canremovebuffer-repro.tar.gz

tar xvf canremovebuffer-repro.tar.gz
cd canremovebuffer-repro
./run.sh

Relevant log output

Screenshots

No response

Additional Context

No response