89 lines
4.8 KiB
Markdown
89 lines
4.8 KiB
Markdown
# Differential Review Report — Fold symbolic/assertion changes
|
|
|
|
Date: 2026-06-04
|
|
Scope: unstaged Fold diff covering symbolic path modeling, P-Code path pruning, assertion layer/DBUILD checks, reporting evidence, router/build options, and tests.
|
|
|
|
## Verdict
|
|
|
|
Major review completed with 3 agents plus local verification. Several concrete issues were fixed during review. Four larger design findings remain as follow-up tasks because they require broader modeling changes.
|
|
|
|
## Fixed during review
|
|
|
|
1. **Fold assertions were masked as recoverable TU crashes**
|
|
- Risk: `FOLD_ASSERT`/`FOLD_PRECONDITION` inside source analysis raised `SIGABRT`, and `runClangShardTranslationUnit` could quarantine the TU instead of failing the scan.
|
|
- Fix: `include/fold/core/assert.hpp` now tracks assertion-failure-in-flight; `src/cli/clang_shard_tu.cpp` resets/checks this flag and aborts on Fold assertion failures instead of treating them as ordinary TU signals.
|
|
|
|
2. **`FOLD_DCHECK` test did not match macro semantics**
|
|
- Risk: test assumed only `FOLD_ENABLE_DCHECK` controls `FOLD_DCHECK`, but macro also enables under `!NDEBUG`.
|
|
- Fix: `tests/unit/test_signal_isolation.cpp` now requires `FOLD_ENABLE_DCHECK` for the test build and always expects `SIGABRT`.
|
|
|
|
3. **Debug-only abort conflicted with guarded-fact invalid-subject skip policy**
|
|
- Risk: `GuardedFactStore::add` aborted in test/canary builds for invalid `AccessPath` while release silently skipped.
|
|
- Fix: `src/symbolic/guarded_facts.cpp` now consistently treats invalid subjects as skipped facts, then asserts only scalar/boolean payload invariants for valid facts.
|
|
|
|
4. **P-Code path invariant was debug-only before release dereference**
|
|
- Risk: `path_` was checked with `FOLD_DCHECK` before release dereference.
|
|
- Fix: `src/symbolic/pcode_encoder.cpp` now uses always-on `FOLD_ASSERT` for initialized/boolean P-Code path before feasibility/observer paths.
|
|
|
|
5. **Disabled Rust/Swift frontend options broke compilation**
|
|
- Risk: `-Drust-frontend=false` / `-Dswift-frontend=false` omitted backend sources but `scan_runner.cpp` still instantiated them.
|
|
- Fix: `src/cli/scan_runner.cpp` now gates backend construction with `FOLD_HAVE_RUST` / `FOLD_HAVE_SWIFT` and throws clear disabled-frontend errors.
|
|
|
|
6. **DCHECK canary docs reused another build dir's native file**
|
|
- Risk: `BUILD.md` example pointed `build/dcheck` at `build/release/conan_meson_native.ini`.
|
|
- Fix: docs now use independent `build/dcheck` Conan/Meson files and show `meson configure ... -Denable-dcheck=true` for existing builds.
|
|
|
|
## Remaining findings / follow-up tasks
|
|
|
|
1. **High — P-Code CBRANCH assumes all later ops are fallthrough**
|
|
- Ref: `src/symbolic/pcode_encoder.cpp:437-444`.
|
|
- Scenario: if lifted P-Code is ordered taken-block-first, global `path && !cond` can suppress real hazards in the taken path. Unconditional branch/return boundaries are also not modeled.
|
|
- Follow-up: task #12.
|
|
|
|
2. **Medium — unknown-call havoc does not clear AccessPath-backed facts**
|
|
- Ref: `src/symbolic/encoder.cpp:1038-1069`.
|
|
- Scenario: mutable calls clear member-value cache but not AccessPath ranges/non-null/guarded facts, so facts like `obj.v.size() > 0` may survive `mutate(&obj)`.
|
|
- Follow-up: task #13.
|
|
|
|
3. **Low/Medium — latent/precondition evidence is serialized but not produced by detectors**
|
|
- Refs: `src/report/finding_json.cpp:104-107`, `src/report/sarif.cpp:283-290`.
|
|
- Scenario: real raw-pointer/config-derived precondition findings still appear as ordinary findings until detector emission is wired.
|
|
- Follow-up: task #14.
|
|
|
|
4. **Low — hybrid profile merge drops timing/counter fields**
|
|
- Ref: `src/cli/scan_router.cpp:454-476`.
|
|
- Scenario: hybrid profiles merge detector rows and counts but omit lane timing/prefilter counters, reducing benchmark/profile fidelity.
|
|
- Follow-up: task #15.
|
|
|
|
## Validation run after fixes
|
|
|
|
Passed:
|
|
|
|
```sh
|
|
ninja -C build/release \
|
|
tests/unit/test_signal_isolation tests/unit/test_guarded_facts \
|
|
tests/unit/test_pcode_encoder tests/integration/test_scan_e2e \
|
|
tests/integration/test_subprocess_fanout
|
|
|
|
meson test -C build/release \
|
|
fold:signal_isolation fold:guarded_facts fold:pcode_encoder \
|
|
fold:scan_e2e fold:subprocess_fanout --print-errorlogs
|
|
|
|
git diff --check
|
|
```
|
|
|
|
Result: `5/5 OK`, 0 failures.
|
|
|
|
Also passed no-frontends build smoke:
|
|
|
|
```sh
|
|
meson setup build/review-no-frontends ... \
|
|
-Dbuild-tests=false -Drust-frontend=false -Dswift-frontend=false \
|
|
-Dghidra=disabled -Dcapstone=disabled -Dradare2=disabled \
|
|
-Dbuild-mcp-server=false -Dbuild-lsp-server=false
|
|
ninja -C build/review-no-frontends libfold-core.a
|
|
```
|
|
|
|
## Coverage limits
|
|
|
|
This was a differential review over the current unstaged diff, not a full architectural audit. The largest remaining risks are path-model semantics and fact invalidation, which need dedicated implementation/testing passes.
|