Disable MiniSat simplifier after first incremental solve#8851
Disable MiniSat simplifier after first incremental solve#8851tautschnig wants to merge 8 commits intodiffblue:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a hang in CBMC's incremental SAT solving when using MiniSat's SimpSolver. The issue occurs because MiniSat's simplifier runs variable elimination (eliminate()) before every solveLimited/solve call, which can degrade the solver's ability to prove UNSAT during the all-properties verification loop. The fix passes do_simp=false on all calls after the first, allowing the initial full simplification pass but skipping it on subsequent incremental calls.
Changes:
- Added a
solver_was_calledflag tosatcheck_minisat2_basetto track whether the solver has been called before, and useif constexprto passdo_simp=falsetoSimpSolver::solveLimited/solveon subsequent calls - Reordered includes to follow the
.clang-formatpriority ordering and added<type_traits>forstd::is_same_v - Added a regression test (
Array_UF23) that reproduces the hang scenario with--arrays-uf-always --unwind 1 --32
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/solvers/sat/satcheck_minisat2.h | Added solver_was_called boolean member with in-class default initializer |
| src/solvers/sat/satcheck_minisat2.cpp | Used if constexpr to conditionally disable SimpSolver's simplifier after first solve, reordered includes |
| regression/cbmc/Array_UF23/main.c | New regression test C source with VLA array stores and an assertion |
| regression/cbmc/Array_UF23/test.desc | Test descriptor for the regression test |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #8851 +/- ##
===========================================
- Coverage 80.01% 80.01% -0.01%
===========================================
Files 1700 1700
Lines 188338 188347 +9
Branches 73 73
===========================================
+ Hits 150702 150707 +5
- Misses 37636 37640 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Is this a genuine issue in MiniSat, or are we using it wrong? |
|
It's a known limitation of MiniSat's SimpSolver when used incrementally. The SimpSolver was designed primarily for single-shot solving — its MiniSat's API does provide the |
75c8bd0 to
59fef53
Compare
MiniSat's SimpSolver runs variable elimination (eliminate()) before every solveLimited/solve call by default. When used incrementally in the all-properties verification loop, this repeated elimination can degrade the solver's ability to prove UNSAT, causing the CDCL search to hang indefinitely. Fix by passing do_simp=false on all calls after the first. The first call still benefits from the full simplification pass, but subsequent incremental calls skip it, preventing the problematic variable elimination between iterations. This fixes a hang when running: cbmc --arrays-uf-always --unwind 1 --32 Array_UF23/main.c Co-authored-by: Kiro <kiro-agent@users.noreply.github.com>
Add TIMEOUT 1200 to the set_tests_properties call in both copies of the add_test_pl_profile macro (regression/CMakeLists.txt and regression/libcprover-cpp/CMakeLists.txt). This ensures CTest will kill and report as failed any regression test that exceeds 20 minutes, preventing CI jobs from hanging indefinitely on tests that time out. Co-authored-by: Kiro (autonomous agent) <kiro-agent@users.noreply.github.com>
Fork the test command into a new process group and use alarm() to enforce an optional per-test timeout. When the timeout fires, kill the entire process group with SIGKILL and report the test as failed with a TIMEOUT marker. The timeout can be set via -t <secs> on the command line or via the TESTPL_TIMEOUT environment variable (command line takes precedence). Co-authored-by: Kiro <kiro-agent@users.noreply.github.com>
These two test suites regularly exceed the default 1200-second timeout on macOS CI runners. Set a 2700-second (45-minute) timeout for jbmc-symex-driven-lazy-loading-CORE and jbmc-strings-symex-driven-lazy-loading-CORE. Co-authored-by: Kiro <kiro-agent@users.noreply.github.com>
contracts-dfcc-CORE and book-examples-cprover-smt2-CORE exceeded the default 1200-second timeout on macOS 14 ARM runners. Set 3600-second (1-hour) timeouts for these suites. Also bump jbmc symex-driven lazy-loading timeouts from 2700 to 3600 seconds as they still timed out at the previous limit. Co-authored-by: Kiro <kiro-agent@users.noreply.github.com>
Temporarily disable all CI jobs except check-ubuntu-22_04-cmake-gcc-32bit to focus debugging on the timeout issue. Changes to the 32-bit job: - Add timeout-minutes: 45 at the GitHub Actions job level to prevent the 6-hour default timeout from wasting CI resources. - Set TESTPL_TIMEOUT=600 environment variable so test.pl kills any individual test that runs longer than 10 minutes. - Add --timeout 1200 to the ctest command line as a belt-and-suspenders measure alongside the CMakeLists.txt TIMEOUT property. - Add --output-on-failure to ctest for better diagnostic output. The CTest TIMEOUT 1200 property (from CMakeLists.txt) was already present in the last CI run but did not prevent the 6-hour hang, suggesting CTest may not be killing orphaned child processes when it terminates test.pl. The TESTPL_TIMEOUT mechanism should be more reliable as test.pl kills the entire process group (kill -9 -$pid). Co-authored-by: Kiro <kiro-agent@users.noreply.github.com>
The previous unconditional disabling of the simplifier after the first incremental solve caused Multi_Dimensional_Array6 to hang in the 32-bit CI build. The simplifier is beneficial for most workloads; only the Ackermann-style array encoding (--arrays-uf-always) generates the problematic pattern of many incremental calls where variable elimination between solves degrades CDCL performance. Make the behaviour opt-in via set_limit_incremental_simplification() on the MiniSat solver, and activate it from solver_factory when arrays-uf is set to "always". Co-authored-by: Kiro <kiro-agent@users.noreply.github.com>
Re-enable all CI jobs and remove the temporary timeout diagnostics now that the root cause has been identified and fixed. The 32-bit CI hang was caused by Multi_Dimensional_Array6/test.desc timing out because the MiniSat simplifier was unconditionally disabled after the first incremental solve. The fix in the previous commit scopes this behaviour to --arrays-uf-always only. Co-authored-by: Kiro <kiro-agent@users.noreply.github.com>
e60ecaa to
c8ca115
Compare
MiniSat's SimpSolver runs variable elimination (eliminate()) before every solveLimited/solve call by default. When used incrementally in the all-properties verification loop, this repeated elimination can degrade the solver's ability to prove UNSAT, causing the CDCL search to hang indefinitely.
Fix by passing do_simp=false on all calls after the first. The first call still benefits from the full simplification pass, but subsequent incremental calls skip it, preventing the problematic variable elimination between iterations.
This fixes a hang when running:
cbmc --arrays-uf-always --unwind 1 --32 Array_UF23/main.c