syn: minimal Bazel repro for SIGSEGV in syn::map_combinationals (post-#10473)#10492
syn: minimal Bazel repro for SIGSEGV in syn::map_combinationals (post-#10473)#10492oharboe wants to merge 4 commits into
Conversation
Adds a manual-tagged Bazel sh_test that drives the openroad binary through the smallest input that triggers the post-ABC mapper crash introduced in The-OpenROAD-Project#10473: a 2-input AND gate. Any RTL with at least one AND node in the post-ABC AIG SIGSEGVs immediately after the "[INFO SYN-0023] abc_roundtrip: after ABC: 1 ANDs ..." log line, inside syn::map_combinationals. The test is tagged manual so it does not turn CI red while the bug is unfixed. Run it explicitly with: bazelisk test //src/syn/test:synthesize_minimal_and_sigsegv_test Once the mapper bug is fixed, remove the "manual" tag and the test becomes a permanent regression guard: it passes iff `synthesize` completes and prints "OK". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
![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 introduces an end-to-end reproduction test case for a SIGSEGV bug in the syn::map_combinationals component, including a new Bazel sh_test target, a shell wrapper script, a minimal Verilog design, and a Tcl script. A review comment correctly identified that the shell wrapper script is missing a command to change the working directory to the workspace root, which is necessary for the Tcl script to resolve relative paths correctly. A code suggestion was provided to address this issue.
|
clang-tidy review says "All clean, LGTM! 👍" |
- run_openroad_repro.sh: cd into the runfiles workspace root before exec'ing openroad and use relative paths to the binary and Tcl script, so that the openroad process inherits the workspace root as its cwd. The previous version relied on sh_test's default cwd for the Tcl-side relative paths to resolve; making the cd explicit matches the header comment and is robust to changes in the sh_test default cwd convention. - synthesize_minimal_and.tcl: tclfmt-applied indentation fix on the sv_elaborate continuation line (4 -> 2 spaces), unblocks the Tclint CI check. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a regression test to reproduce a SIGSEGV in the syn::map_combinationals component. The test includes a new sh_test target in the Bazel build file, a shell script wrapper for executing the openroad binary, a minimal SystemVerilog design with an AND gate, and a Tcl script to drive the synthesis process. The test is currently marked as manual as it is expected to fail until the underlying bug is resolved. I have no feedback to provide.
|
clang-tidy review says "All clean, LGTM! 👍" |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a minimal end-to-end reproduction test for a SIGSEGV bug in the syn::map_combinationals component, consisting of a new sh_test target, a bash wrapper script, a minimal SystemVerilog design, and a Tcl script. Feedback suggests optimizing the test execution by specifying a small size, improving the bash script's reliability with pipefail, and using more robust methods for detecting the Bazel workspace root.
Three gemini-flagged items applied:
- BUILD: tag the sh_test with size = "small" so Bazel can schedule
it with the right resource budget. The test is a 1-AND repro, well
under the "small" 60s limit.
- run_openroad_repro.sh: switch from "set -eu" to "set -euo pipefail"
so a failure in any stage of a future pipeline also fails the
script.
- run_openroad_repro.sh: drop the manual cd into
${TEST_SRCDIR}/${TEST_WORKSPACE:-_main}. Bazel's sh_test already
sets cwd to the runfiles workspace root, so the cd was redundant
and the brittle "_main" fallback (which a previous review had
pointed out) is no longer needed at all. The script now just
execs openroad with "./" relative paths.
Test still reproduces the SIGSEGV after the SYN-0023 line as
expected (regression guard is intact).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
|
/gemini review |
|
clang-tidy review says "All clean, LGTM! 👍" |
There was a problem hiding this comment.
Code Review
This pull request introduces a regression test suite to reproduce a SIGSEGV in the syn::map_combinationals component, including a new Bazel sh_test target, a shell wrapper, a minimal SystemVerilog design, and a Tcl script. Feedback was provided to remove a redundant "./" prefix from the Tcl script path argument in the shell wrapper to ensure more robust path resolution.
Addresses PR The-OpenROAD-Project#10492 review feedback. The "./" was only needed on the openroad binary path (so exec resolves it relative to cwd instead of PATH-searching); the Tcl path is consumed by openroad's own open(), which handles relative-to-cwd paths fine on its own and which would mishandle a "./absolute/path" if a future change ever passed an absolute rlocationpath here. A brief comment now spells out the asymmetry so the next reader does not "consistency"-fix the binary side back to PATH-search semantics. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
|
/gemini review |
|
clang-tidy review says "All clean, LGTM! 👍" |
There was a problem hiding this comment.
Code Review
This pull request adds a regression test suite to reproduce a SIGSEGV in the syn::map_combinationals component. The changes include a new Bazel sh_test target, a shell script wrapper for the openroad binary, a minimal SystemVerilog design, and a Tcl script that triggers the crash. The test is tagged as manual to prevent CI failures while the bug remains unfixed. I have no feedback to provide.
|
Are tests of this form helpful? I'm thinking they would be a bit more complicated as things come along and that the least complicated can be pruned as integration tests come online. I find that unit-tests shine during bringup and then integration tests provide most of the testing value later on. |
|
@oharboe We are missing unit tests which would have flagged this. I'm adding those. That being said one integration test which does the "elaborate + synthesize" end to end is helpful. I think it's worth making it more complicated than a single AND operation. |
|
I'd like to see bazel being used to build ORFS tests, we could make designs non-manual(i.e. always built with bazelisk test ...) and then enable whatever we want tested without touching .github/ .yaml files. |
2 participants
Summary
Adds a Bazel
sh_testat//src/syn/test:synthesize_minimal_and_sigsegv_testthat reproduces a SIGSEGV introduced by #10473 (the integrated synth merge,
edcc2e8d92).module synthesize_minimal_and (input wire a, input wire b, output wire q); assign q = a & b; endmodule).openroadbinary crashes withSignal 11immediately after the[INFO SYN-0023] abc_roundtrip: after ABC: 1 ANDs, 2 CIs, 1 COslog line, insidesyn::map_combinationals.The crash is not in
abc_roundtrip.synthesize -reduce_name_loss(which skips both
abc_roundtripcalls) also crashes, with noSYN-0023line before the SIGSEGV. The bug is downstream of ABC roundtrip in the new
post-bitblast mapper.
Bisection (all ASAP7, single-Vt RVT)
q = a)mapCombinationals: doneq = ~a)mapCombinationals: doneSYN-0027 no dbMaster(not the crash)Threshold: any design with
>= 1AND node post-ABC triggers the crash.Stack (
-c opt, symbols stripped)Why the test is
tags = ["manual"]The test asserts the post-fix behavior:
synthesizeruns to completion andprints
OK. Today it SIGSEGVs, so the test fails. To keep CI green whilethe underlying bug is unfixed, the test is tagged
manualand excludedfrom default
bazelisk test //...runs. Run it explicitly with:Once the mapper bug is fixed, remove the
manualtag and the test becomesa permanent regression guard.
Bazel-only by design
The repro is intentionally registered only in Bazel (
src/syn/test/BUILD),not in
src/syn/test/CMakeLists.txt. The CMakeor_integration_testshelper does not currently expose
WILL_FAILsemantics, so registering aknown-failing test there would just break
ctestfor everyone. Once thebug is fixed and the test passes, CMake registration can be added in a
follow-up.
Test plan
bazelisk test //src/syn/test:synthesize_minimal_and_sigsegv_testreproducesSignal 11afterSYN-0023(seetest.logexcerpt above)bazelisk test //...is unaffected (test is taggedmanual)ctestis unaffected🤖 Generated with Claude Code