Bring main up to stable-backports (carries #148) - #152
Merged
Conversation
pack_muxfs() runs before pack_luts() in both the xc7 and Ultrascale flows. Its constrain_muxf_tree() walks driver.cell across nets with no cell-type check, so a LUT6_2 feeding a MUXF7/8/9 input gets constr_parent/constr_children set on it by the mux-tree constraint walk. split_lut6_2() (previously called from inside pack_luts(), which runs afterwards) then deletes that exact cell via flush_cells() without repairing those pointers, leaving a dangling CellInfo* that the HEAP placer later dereferences -- segfault. Fix by running split_lut6_2() before pack_muxfs()/pack_carries()/ pack_srls() in both flows, so LUT6_2 no longer exists as a type by the time any pass that net-walks into driver cells without a type check gets to run. This closes the whole class rather than adding a type check to constrain_muxf_tree() alone. Also: - flush_cells() now asserts a cell being deleted has no live constr_parent/constr_children links, so a future instance of this class of bug fails loudly at the deletion site instead of segfaulting later, deep in the placer. - constrain_lut6_2_pairs() no longer overwrites constr_parent on a half that was already constrained by an earlier pass (e.g. its own mux-tree constraint) -- found via adversarial testing: the same LUT6_2 being both 5LUT/6LUT-pairable (constant I5) *and* driving a MUXF7 input hit the same class of dangling/desynced-constraint bug through a different code path (constrain_lut6_2_pairs unconditionally overwriting constr_parent), producing a different placer assertion failure. Such a pair is now left unpaired (costs an extra LUT bel, same tradeoff already accepted for the non-constant-I5 case) rather than corrupting constraint state. - split_lut6_2() now propagates the original cell's region constraint to both halves (a region is just a bbox, so this is safe and cheap insurance against silently dropping it). - split_lut6_2() fails loudly (log_error) instead of silently discarding state in two previously-unguarded cases: a LUT6_2 with a pre-existing BEL constraint (no defined rule for which physical half should inherit a single-bel pin), and a split-half name that collides with an existing cell (previously a bare NPNR_ASSERT deep in flush_cells()). Verified against 4 hand-built repro netlists: the original UAF (LUT6_2.O6 -> MUXF7), a paired constant-I5 LUT6_2 (still costs one physical LUT), a paired LUT6_2 whose O6 also drives a MUXF7 input, and the same with O5 driving the MUXF7 (the actual conflicting case for constrain_lut6_2_pairs -- confirmed by reverting the guard and seeing a different placer assertion fire). All four now place and route cleanly. Not fixed here (documented, left out of scope): - Non-constant-I5 LUT6_2 still costs 2 LUT bels instead of 1; the placer's fracturable-LUT model refuses a 5LUT companion whenever the 6LUT uses all 6 inputs (arch_place.cc), so closing this needs an arch-level change, not a packer-level one. - A BEL constraint on a pre-split LUT6_2 has no defined per-half splitting rule; no live producer of this was found in this codebase, so it now fails loudly rather than being silently guessed at. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
split_lut6_2() previously log_error'd on any LUT6_2 carrying a BEL attribute, regardless of whether O5 was actually used. That's a regression versus pre-split behaviour: before split_lut6_2() existed, a BEL-constrained LUT6_2 driving only O6 packed fine (it just aliased straight onto the LUT6 xform rule, keeping its single port name and BEL). Independent review caught this by diffing against the parent commit. A BEL names one physical resource; splitting is only genuinely ambiguous when both O5 and O6 are driven, since then there's no rule for which of the two resulting cells should inherit it. When only one output is used, exactly one half is ever created, so it can just take the BEL directly. Verified: a BEL-constrained, O6-only LUT6_2 now packs/places/routes cleanly (previously hard errored); a BEL-constrained LUT6_2 driving both O5 and O6 still errors, as intended. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Addresses review feedback on openXC7#148: name the BEL-ambiguity check, the split-half name-collision check, the BEL-inherit check, and the two already-constrained checks instead of inlining them at the call site. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
xilinx: fix use-after-free when a LUT6_2 feeds a MUXF7/8/9 tree
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
mainbecame the default branch but sits 4 commits behindstable-backports, so the fix from #148 is currently absent from the default branch — a fresh clone does not have it.mainis a strict ancestor ofstable-backports(git merge-base --is-ancestorpasses, compare API reportsahead_by=0 behind_by=4), so this is a fast-forward. No cherry-picks, no rewritten authorship — the same four objects:Credit is @jasonzeng124's and @hansfbaier's; this PR only moves the branch pointer.
How it arose, so it does not recur
mainwas cut at 00:37 on 2026-08-18; #148 merged intostable-backportsat 02:14, because that PR still targeted the old branch. There are now no open PRs targetingstable-backports— the last two were mine (#151, #120) and I have retargeted them tomain. So this is a one-off gap, not an ongoing leak.Verified before proposing
Built at
4dd779f5and ranlitex-ddr-arty-s7(deephier branch of demo-projects) onxc7s50csga324-1:Seeds 3 and 8 are the two that failed the carry site-exit class before #146, so this also confirms #146 and #148 coexist. FASM differs from the pre-#148 output, which is expected: #148 changes when a
LUT6_2may be packed, so the netlist legitimately changes.What I did not verify: the repo's own
regression/suite — it lives on demo-projects' default branch and my working copy is on the deephier branch. I did not want to claim a gate I had not actually run.@hansfbaier — you said you are focusing on review and merge, so this is meant to be a one-click item. If you would rather fast-forward
maindirectly, or delete it and renamestable-backportsinstead, close this; the branch topology is your call and I did not want to guess.