rsz: fix dangling endpoint_path deref in setup-repair move generators - #11215
rsz: fix dangling endpoint_path deref in setup-repair move generators#11215mguthaus wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a null check for ctx.target.endpoint_path in resolveDriverContext within SizeDownFanoutGenerator.cc to prevent a potential SIGSEGV when the endpoint path is null. There are no review comments, and I have no additional feedback to provide.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b6ce4181f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // the scene/min_max; guard it like the driver pin/vertex (cf. | ||
| // MeasuredVtSwapGenerator) to avoid a SIGSEGV on targets that carry no | ||
| // endpoint path. | ||
| if (ctx.drvr_pin == nullptr || ctx.drvr_vertex == nullptr |
There was a problem hiding this comment.
The reviewed commit has no Signed-off-by: trailer, even though repository policy requires every commit to be created with git commit -s; add the required DCO sign-off before merging.
AGENTS.md reference: AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Maybe it wants my full name? Repushing
9b6ce41 to
b113d0e
Compare
Target::endpoint_path can become stale once an earlier move in a repair sequence commits and perturbs the timing graph. Move generators that then dereference endpoint_path->scene() / ->minMax() (both route through the path's now-invalid tag) crash non-deterministically depending on heap state. Reproduced as a SIGSEGV in repair_setup on asap7/aes with the size_down move inserted mid-sequence: the crash site moved between Path::scene() and Tag::minMax() from run to run, the signature of a dangling pointer rather than a null one. Target already caches `scene` at construction (while endpoint_path is still valid); cache `min_max` the same way and have Target::minMax() prefer the cached value. Route the raw endpoint_path->scene()/minMax() derefs in the Unbuffer, SwapPins and SizeDownFanout generators through the safe activeScene()/minMax() accessors (SizeUp already did). This makes the generators robust to move-sequence reordering, where a move can now run after an earlier commit has invalidated the path. Also guard the genuinely-nullable scenePort() / findLibertyPort() lookups in SizeDownFanoutGenerator, which can return null for multi-Vt / multi-corner libraries (a cell may lack the load port, or the port may not resolve at a given liberty index); such cells are skipped rather than dereferenced. Verified on asap7/aes with size_down inserted into the repair sequence: 16/16 previously-crashing/at-risk seeds now complete cleanly. Signed-off-by: Matthew Guthaus <mrg@ucsc.edu>
b113d0e to
9c52736
Compare
mguthaus
changed the title
2 participants
Problem
repair_setupcan SIGSEGV inside the resizer's move generators. The crash isnon-deterministic: on the same input the faulting frame moved between
sta::Path::scene()andsta::Tag::minMax()from run to run, which is thesignature of a dangling pointer, not a null one.
Root cause:
rsz::Target::endpoint_pathbecomes stale once an earlier move in arepair sequence commits and perturbs the timing graph. Any generator that then
dereferences
endpoint_path->scene()/->minMax()(both route through thepath's now-invalid tag) reads freed/reused memory. It surfaces when a move that
uses
endpoint_pathruns after another move has already committed — e.g. thesize_downmove placed mid-sequence.Fix
Targetalready cachessceneat construction (whileendpoint_pathisvalid). Cache
min_maxthe same way and makeTarget::minMax()prefer thecached value. Default-constructed targets are unaffected (fall back to the old
path lookup), so behaviour is unchanged when the path is valid.
endpoint_path->scene()/minMax()derefs in the Unbuffer,SwapPins and SizeDownFanout generators through the safe
activeScene()/minMax()accessors (SizeUp already did). This makes thegenerators robust to move-sequence reordering.
scenePort()/findLibertyPort()lookups inSizeDownFanoutGenerator— for multi-Vt / multi-corner libraries a cell maylack the load port or its port may not resolve at a given liberty index; such
cells are now skipped rather than dereferenced.
Verification
Reproduced on
asap7/aes(multi-Vt: RVT/LVT/SLVT) withsize_downinserted intothe setup-repair sequence, which previously crashed ~17% of placement seeds.
After the fix, 16/16 previously-crashing / at-risk seeds complete cleanly,
including every seed observed to crash before.