199 lines
8.1 KiB
Markdown
199 lines
8.1 KiB
Markdown
# Fold-on-fold static-analysis pass (2026-05-23)
|
|
|
|
Output of running fold against its own source tree:
|
|
|
|
```
|
|
fold --quiet scan \
|
|
--compile-db build/release/compile_commands.json \
|
|
--detectors clone,dead-statement,equiv \
|
|
--clone-type 3 --clone-similarity 0.8 \
|
|
--processes 4 \
|
|
--format json --output docs/internal/perf-audit/fold-on-fold-2026-05-23.json
|
|
```
|
|
|
|
Wall: ~9.7 min over 134 TUs.
|
|
|
|
**Totals:** 639 findings — 487 clone, 152 dead-statement, 0 equiv.
|
|
|
|
The equiv-detector slot caught nothing because equiv only fires on
|
|
function pairs whose signatures match exactly AND that pass z3 input/
|
|
output equivalence; the recently scout-rule-extracted helpers don't
|
|
collide on signature.
|
|
|
|
## Top duplication clusters worth refactoring
|
|
|
|
### `mixHash` across the security detectors
|
|
|
|
Sites: `src/detect/sign_conversion.cpp:154`, `div_by_zero.cpp:*`,
|
|
`int_truncation.cpp:*`, `int_overflow.cpp:*`, `null_deref.cpp:*`.
|
|
|
|
5+ identical copies of the FNV-1a-style mix function inside per-
|
|
detector anonymous namespaces. **Refactor target:** lift to a single
|
|
helper in `src/ir/normalize.cpp` (already has `mix64`) or
|
|
`src/symbolic/theory.hpp`. Each detector loses ~6 lines.
|
|
|
|
### `supportsParam` and `collectConjuncts`
|
|
|
|
Multiple anonymous-namespace copies across detectors. The Phase-B v2
|
|
migration moved some helpers but missed these. **Refactor target:**
|
|
extract to `src/symbolic/z3_helpers.cpp` (sister to
|
|
`z3_model_support.{hpp,cpp}` that already exists).
|
|
|
|
### `onFunctionEnd` boilerplate
|
|
|
|
`DivByZeroObserver::onFunctionEnd`, `IntOverflowObserver::onFunctionEnd`,
|
|
`OOBObserver::onFunctionEnd`, etc. all do the same `solver_ = nullptr;
|
|
current_fn_ = nullptr;` cleanup. **Refactor target:** a small mixin
|
|
helper or a base-class `SafetyObserverBase` providing the default
|
|
implementation.
|
|
|
|
### `ir::*::valid` / `operator bool` / `operator==` boilerplate
|
|
|
|
`include/fold/ir/call.hpp:23-29` and friends — `Call`, `MemberRef`,
|
|
`Symbol`, `Expr` all spell out their own identical `valid()`,
|
|
`operator bool`, `operator==`, `operator!=`, `asClangX()`. Five copies
|
|
of each. **Refactor target:** common CRTP base or just a macro; the
|
|
opaque-handle shape repeats deliberately but the accessor boilerplate
|
|
shouldn't.
|
|
|
|
### Test-fixture duplication
|
|
|
|
`tests/unit/test_oob_access.cpp:*` (60 hits), `test_null_deref.cpp:*`
|
|
(51 hits), `test_int_truncation.cpp:*` (7 hits) all show massive
|
|
cross-cluster matches. **Judgment call:** these are parameterised
|
|
detector tests — the duplication is the *point* of fixture coverage.
|
|
Not a refactor target. Could be reframed as Catch2 GENERATE() tables
|
|
if test count keeps growing, but that's stylistic.
|
|
|
|
## Dead-statement findings (152 → 16 after fix)
|
|
|
|
**Initial finding:** 152 dead-statement reports. Spot-checks of the
|
|
top files (incremental_cache.cpp, call_graph.cpp, def_use.cpp,
|
|
r2_backend.cpp, static_ptr_asymmetry.cpp) all showed the same
|
|
shape: the flagged statement was inside a range-for body. Plain
|
|
`for (int x : v)` was fine; `for (const auto& [k, v] : map)` was
|
|
not.
|
|
|
|
**Root cause:** clang's CFG for a range-for whose loop variable
|
|
is a structured binding leaves the body block without an incoming
|
|
edge from the loop's condition check. The reachability walk in
|
|
`DeadStatementVisitor` then marks every body statement
|
|
unreachable. Reproducer in 7 lines of C++.
|
|
|
|
**Fix:** commit `8895d50` adds a parent-map walk; if the anchor's
|
|
ancestor chain contains a `CXXForRangeStmt`, suppress the finding.
|
|
Regression test `dead-statement: range-for + structured binding
|
|
body is reachable` pins it.
|
|
|
|
**Result:** 152 → 16 findings on fold's own source (90% reduction).
|
|
|
|
### Remaining 16 are all "defensive return after exhaustive enum switch"
|
|
|
|
The 16 surviving findings are the same idiom across 13 files:
|
|
|
|
```cpp
|
|
const char* findingKindName(FindingKind k) {
|
|
switch (k) {
|
|
case FindingKind::Clone: return "clone";
|
|
// ... every enum value covered ...
|
|
case FindingKind::FuzzCrash: return "fuzz-crash";
|
|
}
|
|
return "unknown"; // ← flagged
|
|
}
|
|
```
|
|
|
|
When every enum case returns (no default), clang's CFG does not
|
|
insert a synthetic edge to the post-switch block, so the trailing
|
|
`return "unknown"` is genuinely unreachable. The detector is
|
|
*technically correct*; the engineer wrote the return *defensively*
|
|
in case a future enum addition forgets a case.
|
|
|
|
Three resolutions worth considering:
|
|
|
|
1. **Leave as documented exception.** The defensive return is
|
|
graceful — returns a placeholder rather than UB. Cost is a few
|
|
FPs on tight C++ codebases that use the same idiom.
|
|
2. **Replace with `__builtin_unreachable()` / `std::unreachable()`
|
|
at fold's own sites.** Removes the dead code, signals intent,
|
|
improves codegen. Cost: UB at runtime if exhaustiveness breaks
|
|
later.
|
|
3. **Add a second filter to dead_statement.cpp** for trailing
|
|
statements after exhaustive enum switches. Cost: complex AST
|
|
analysis (find the surrounding switch, verify it covers every
|
|
enum case, etc.) and risks suppressing real dead code.
|
|
|
|
Recommendation: **(1)** is fine for fold-as-a-tool; the 16
|
|
remaining findings are a known false-positive idiom worth
|
|
documenting in user-facing release notes. Sites in fold's own
|
|
source: finding.cpp (3), theory.cpp (5), ir_expr_clang.cpp (1),
|
|
corpus_writer.cpp (1), harness_emitter.cpp (2), clone.cpp (1),
|
|
triage.cpp (1), int_overflow.cpp (1), test_cross_tu_equiv.cpp (1).
|
|
|
|
## What this confirms vs. denies
|
|
|
|
**Confirms:**
|
|
- The 5 security detectors share substantial duplicated mechanics
|
|
(mixHash, supportsParam, onFunctionEnd, hashQuery cache shape).
|
|
Hypothesised in the perf-audit plan; visible in the data.
|
|
- The ir::* opaque types have unnecessary per-type boilerplate.
|
|
|
|
**Denies:**
|
|
- equiv-detector finding nothing means the prior scout-rule
|
|
extractions DID move shared logic out — the per-detector
|
|
remaining duplication is in small helpers, not whole functions.
|
|
|
|
## Pointer
|
|
|
|
Full JSON: `docs/internal/perf-audit/fold-on-fold-2026-05-23.json` (639
|
|
findings, ~50 KB). Profile: `fold-on-fold-2026-05-23-profile.json`.
|
|
|
|
## 2026-05-23 late-session pass (after audit + cache + composite-walker)
|
|
|
|
| Rule | Count |
|
|
|---|---|
|
|
| fold.clone | 81 |
|
|
| fold.dead-statement | 14 |
|
|
| fold.equiv | 1 |
|
|
| fold.null-deref | 22 (new; not in earlier passes) |
|
|
|
|
The 22 null-deref findings on fold itself are **mostly the same
|
|
shape**: a parameter is null-checked early via a compound
|
|
condition (e.g. `if (!rd || rd->isImplicit()) return true;`),
|
|
then the parameter is passed to a helper function that
|
|
dereferences it. The encoder doesn't propagate the
|
|
`!rd → false` arm of the `||` into a non-null fact on the
|
|
post-early-return path; downstream the helper sees an unknown
|
|
pointer and flags the deref.
|
|
|
|
Example: `src/detect/class_dup.cpp:186-193`. Parameter `rd` is
|
|
null-checked at line 186 (`if (!rd || ...)`), passed to
|
|
`eligibleMethodCount(rd)` at line 193 which derefs param 0.
|
|
|
|
### What this surfaces
|
|
|
|
The compound-guard pattern (`if (!x || other) return;`) is real
|
|
and worth recognising syntactically. `cfg_helpers::nullCheckVar`
|
|
now handles `NULLCHECK || other` and `NULLCHECK && other`
|
|
(symmetric AND case for `if (x != nullptr && other)` shapes).
|
|
Three unit tests under `[m22-extension]` pin the contract. The
|
|
existing SMT path predicate already handled most in-function
|
|
derefs after these guards; the syntactic flag is mainly defense
|
|
in depth + faster precondition discharge at call sites.
|
|
|
|
The 22 null-deref findings on fold itself are NOT eliminated by
|
|
that change. They come from a separate gap: fold analyzes static
|
|
file-local helpers as standalone entry points, so
|
|
`static int methods(const R* r) { return r->v; }` emits a
|
|
nullable-deref finding on `r->v` regardless of whether the only
|
|
callers in the TU prove `r` non-null. Two routes to fix that:
|
|
|
|
1. Skip standalone analysis for file-static helpers whose every
|
|
call site discharges the parameter non-null precondition.
|
|
2. Derive a stronger required-precondition summary from the
|
|
callee body and suppress the deref-site finding when every
|
|
call site satisfies it.
|
|
|
|
Both are detector-side work, larger scope, filed for a future
|
|
null-deref precision refresh. Tight well-typed codebases tend to
|
|
produce a long tail of these context-sensitive parameter-deref
|
|
FPs — they are domain-invariant, not signal.
|