Cranelift verifier: run in CI, using an SMT query caching setup.#13929
Cranelift verifier: run in CI, using an SMT query caching setup.#13929cfallin wants to merge 10 commits into
Conversation
Implement a file-based cache for SMT solver queries in the Cranelift verifier, enabling fast re-verification without re-invoking the solver. The cache is keyed on a hash of the full SMT-LIB transcript for a query, and provides the response (unsat or sat + the model). It can be configured in two ways: - Read-write cache: use the result on hit, invoke the solver and populate the cache on a miss. Meant for local usage. - Read-only enforcing cache: use results on a hit, or fail verification on a miss. This is meant to run in CI. The intended workflow is to *check in the cache* into git; this enables CI to run with cached responses rather than re-invoke the solver on every CI run. This sidesteps the issue of long-running queries to the solver; we can reverify our verification on every commit. One might reasonably ask: what is the TCB and what is the ground truth against which we're verifying, if the repo contains effectively a cache of "yes, it's verified"? The point is that we re-run the full tool against the current source; this reduces "verify Cranelift" to "evaluate these SMT queries". We then *trust* that our checked-in SMT query results are valid, but only that and nothing more. If we ever change the source in a way that causes a new instruction lowering path to exist, the tool will attempt to verify this against the cache and (presumably) fail. The idea is then that the local developer does the verification, and checks in the "SMT query witness"; we either trust that, for regular contributors, or we re-run the verification ourselves, for anonymous contributors. We also have the option of re-regenerating the cache (i.e., re-running verification from scratch) whenever we want.
Also add scripts to generate and verify against the cache: `update-cache.sh` and `verify-cache.sh`.
Subscribe to Label ActionDetailsThis issue or pull request has been labeled: "cranelift", "isle"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
avanhatt
left a comment
There was a problem hiding this comment.
Woo!
Have a few initial comments. Could you also add a description (similar to the PR description) to cranelift/isle/veri/README.md?
Will also test locally and report back.
| echo "" | ||
| echo "Cranelift verification failed (cache miss or verification error)." | ||
| echo "" | ||
| echo "This may be because you made a change to the backends and there are" |
There was a problem hiding this comment.
To be more specific: "the ISLE files for the backends".
This does bring to mind one slight risk here: someone could change the Rust implementation of an external extractor/constructor and not have any indication that they need to change the spec. As it stands, this would still require a manual change to the corresponding spec anyway, but just a case to look out for.
| fn cache_verdict_to_verify_report( | ||
| &self, | ||
| verdict: CacheVerdict, | ||
| _model: Option<crate::cache::CacheModel>, |
There was a problem hiding this comment.
What's the reasoning for this unused argument?
| "key_sha256": "00cb4ad542944b4a6e6b8e027264a46b97b71ca8773532d8dc8ae5e4fc42da6b", | ||
| "short_key": "00cb4ad54294", |
|
How will outdated files be GCed? |
| submodules: true | ||
| - uses: ./.github/actions/install-rust | ||
| - run: ./cranelift/isle/veri/verify-cache.sh cranelift/isle/veri/configs/aarch64-fast.args | ||
| - run: ./cranelift/isle/veri/verify-cache.sh cranelift/isle/veri/configs/aarch64.args |
There was a problem hiding this comment.
This is currently superset of aarch64-fast.args, so we shouldn't need to run both
|
Personally I think infrastructure-wise it would be best to invest in some storage for this cache outside of the git-tracked state of this repo. I mentioned this in the cranelift meeting earlier today but I'm concerned about adding another ~1000 generated files to the repo, the weight of the generated files (77M right now locally), churn (up to +77M of diff if hashes change, huge PRs to sift through, etc), and the required changes to handle contributors PRs (vets are already a pain enough as-is I'd rather not add a second vector of "you can't land this until a maintainer pushes a thing to your branch"). I don't know what the best alternative would be, though, as I'm not entirely sure what the experience will be running this (e.g. how frequent, how difficult, etc). One thing I'd like to preserve though is to ideally continue to have contribution of new rules by contributors be pretty low friction -- if adding a new optimization/lowering rule (e.g. #13640) is blocked until verification is 100% working we'll probably basically not get outside contributions to the backend would be my guess. For CI we could use a github-actions cache, but that's not easily downloadable by users to verify it themselves. One possible idea is a separate git repo pulled in as a submodule, but that doesn't really help with download size. Another possible option is a separate repo but manually managed (not via submodules) but instead by the verification script/etc. That could then be auto-updated in CI and/or by us. Before brainstorming too much I'd like to get agreement on "yes this is in-repo" or "no, this is not in-repo" first. I'd personally like to push this to be outside, but if you'd like to push to instead put this inside I'd like to work through that first. |
|
Thanks for your perspective on this! A few thoughts (some repeated from this morning's meeting, but for purposes of wider visibility in the discussion):
|
|
Also to @alexcrichton 's point about rule additions by new contributors: if the rule is truly outside of the scope of the verification's "default excludes" (e.g., what we think is most likely to cause CVEs), it's a ~1 line addition for the contributor to mark it as such and then quiet any cache miss/failure. For example, it looks like the PR you link would pass just by tagging the new term as |
|
Thanks for working on this! It should be a massive usability improvement. I have not done a full review, but I wanted to raise some high-level questions first. Generic SMT Caching Wrapper. This design tightly integrates the cache with the verifier. When I imagined building this feature, what I had in mind was an SMT-proxy binary that would wrap an SMT solver call and add a caching layer. Ideally this would be transparent to the verifier, only requiring a change to the command-line it uses for the backend solver. Theoretically, it would be a reusable component that could slot into any SMT-backed project (though I doubt we'd actually want to go that far). I think this might actually be doable. The challenge at the moment is solver statefulness: the wrapper would need to proxy stdin and understand push/pop/check-sat etc. A variant of this approach is to modify the verifier to render the full SMT query instead of using the solver interactively and dynamically branching on the applicability-check result. I think this may be doable too, though I'd need to think it through a bit more. The advantage here is your SMT wrapper becomes somewhat trivial, it's essentially a hash of the full input SMT file and returns a cached response. So I think the possible alternatives might be a generic SMT caching proxy that's either (a) stateful and handles interactive SMT queries or (b) stateless and handles only full SMT files. Cache Location and Size. I tend to agree with @alexcrichton that ideally a cache of this size wouldn't be in the main git repository. It seems that a separate git repository for the cache would avoid the downsides of adding it to the main repository whilst also being usable for new contributors? However, this may be less of an issue if we could address the size. Is most of it coming from Could we drop the SMT2 text field and save a load of cache space? Churn. It might be a good idea for us to run some hypothetical workflows and confirm the cache helps in the way we expect. Maybe you have already? For example:
I think it should all work this way, but it would be good to validate, since the usability of the cache depends on it. |
There was a problem hiding this comment.
Can we add the following to .gitattributes in the root of the repo?
cranelift/isle/veri/cache/*.json -diff
That will hide their diffs in github and in the git cli
| /// Full SHA-256 hex digest of the SMT2 text + solver backend. | ||
| pub key_sha256: String, | ||
| /// Short key (first 12 hex chars) used for file naming. | ||
| pub short_key: String, |
There was a problem hiding this comment.
| pub short_key: String, | |
| #[serde(skip)] | |
| pub short_key: String, |
| fn send_and_capture(&mut self, expr: SExpr) -> Result<()> { | ||
| let text = self.smt.display(expr).to_string(); | ||
| self.smt2_transcript.push(text); | ||
| self.smt.raw_send(expr)?; | ||
| // Receive and discard the response (typically "success" for non-query | ||
| // commands like set-logic, push, pop, assert, declare-*). | ||
| let resp = self.smt.raw_recv()?; | ||
| self.smt2_transcript | ||
| .push(format!(";; response: {}", self.smt.display(resp))); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
I think all these custom methods to push to the transcript could use the existing https://docs.rs/easy-smt/latest/easy_smt/struct.ContextBuilder.html#method.replay_file functionality instead.
|
+1 for keeping the cache in tree. FWIW, we can hide diffs via #13929 (comment) |
|
Including my Claude interactive review session summary below (collapsed) in case its useful. You'll see there the two points I already raised in #13929 (comment)
It reports a few other claims that I haven't independently confirmed but we should look into:
Robot ReviewReview: wasmtime PR #13929 — "Cranelift verifier: run in CI, using an SMT query caching setup"
This document records the review and the findings verified empirically against a local checkout at the PR head. All claims marked [verified] were reproduced by running the tool; [reasoned] are from code reading only. 1. What the PR doesAdds a file-based cache of SMT solver query results to the verifier:
Non-cache source changed (the rest of the 713 files are cache JSON):
2. Overall assessmentThe core idea is sound and valuable: reduce "verify Cranelift" to "evaluate a fixed set of SMT queries," commit the witnesses, and let CI re-run the tool while trusting the results. Three things stand out, in priority order:
Plus the deep coupling into Q1 — Separating the cache from the verifierWhere the coupling actually lives.
Option (a) — module/crate in Option (b) — transparent SMT-proxy binary. The genuinely reusable primitive and the ideal end-state. easy-smt already talks to the solver as a subprocess over an SMT-LIB text pipe; The "render the query script" middle path (recommended for this PR). Don't capture the transcript as a side effect of sending; render the query text deterministically from
Decisive benefit of rendering over the current capture: because the key is computed from Sync cost (rendered text must match what's sent): keep each renderer tiny and local to its Recommendation: Design B — it answers "how does the control flow adapt" (it doesn't; each solver call gets a cache wrapper), fixes §4.2, makes "no solver in CI" real, and is the migration path to the reusable proxy (b). Design A is the fallback if exactly one entry per task is preferred. Pure (a) alone leaves the real coupling in place. Q2 — Storing the cache in the repoMeasured [verified] on the checked-in cache (
Decisive fact: Options, most impactful first:
Churn / GC (bjorn3's "how will outdated files be GCed?"): Where else could it live? The in-repo-witness choice is deliberate and good — self-contained, fork-friendly, offline, no external trust boundary or infra — and worth keeping, provided it's verdict-only + single-file (hundreds of KB). Move out-of-repo only if that size ever becomes objectionable (it won't). For completeness: git-LFS (adds an LFS dep, complicates forks/CI, still "in repo"); 3. Empirical verification (receipts)Local checkout at 3.1
|
| Config | Type instantiations | Distinct keys | Intra-run Hits (collisions) |
|---|---|---|---|
| x64-iadd-base-case | 69 | 69 | 0 |
| aarch64-fast | 463 | 460 | 3 (~0.6%) |
Verification is dispatched with par_iter() (runner.rs:688), so colliding tasks can run concurrently.
4. Findings
4.1 CI will fail / "no solver in CI" is inaccurate — high [verified]
The solver is spawned and the entire prelude+encode phase round-tripped through it before the cache is consulted (runner.rs:1194-1210); the CI job installs no solver (§3.2, §3.3). When isle_veri_check runs on run-full it will fail at solver spawn. The cache only skips check-sat reasoning, not the solver process. Fix: either install a solver in the job (contradicts the narrative and keeps encode overhead), or — better — compute the key from Conditions without the solver (Q1 render approach) so a full-hit run spawns nothing.
4.2 Cache key is the encoding, not the query — high / latent unsoundness [reasoned + partially verified]
The PR says the key is "a hash of the full SMT-LIB transcript for a query." The implementation keys on baseline = solver.take_baseline_transcript() taken right after encode() (runner.rs:1206); encode() only emits per-expression declare-const/(assert (= eN …)). The actual queries — applicability (assert (and assumptions))(check-sat) and VC (assert (not (=> assumptions assertions)))(check-sat) — live in the phase transcript, which is discarded (runner.rs:1304 let _ = solver.take_phase_transcript();). The assumptions/assertions partition over conditions.exprs never appears in the key. So the cache maps encoding → verdict and trusts that the encoding determines the answer — not enforced anywhere. §3.4 shows distinct tasks already collide on the baseline key at ~0.6%; today they happen to share verdicts, so no wrong answer, but the silent-false-"verified" channel is live. Fix: key on the full query (Q1 Design A/B).
4.3 Storage: each entry stores unused full SMT text — high (footprint) [verified]
See Q2. smt2_text (~97% of each file, 76 MB total) is never read on lookup. Drop it → ~200× smaller.
4.4 Smaller issues
--cache-mode offincoherent — medium [verified]. Reachableunreachable!panic at cache.rs:266;check()has noOffbranch so it serves hits (cache.rs:228);store()writes inOff(onlyReadOnlyEnforcingearly-returns, cache.rs:197). Documented as "No caching." Either removeOff(redundant with omitting--cache-dir) or make it truly bypass and not panic.- Dead counterexample-model machinery — medium [reasoned].
store_cache_entryalways writesmodel: None(runner.rs:1150);cache_verdict_to_verify_reportignores_model(runner.rs:1128, avanhatt's comment).CacheModel/model/valuesare never populated or consumed; a cachedFailuresilently loses its counterexample. Wire it end-to-end or remove it ("no phantom features"). - Temp-file race under rayon — low [verified precondition].
store()uses a fixed{short_key}.json.tmp(cache.rs:207); §3.4 shows same-key collisions andpar_iter()runs them concurrently. Only affects read-write generation (neverReadOnlyEnforcing— no writes), and a torn file is caught as a parse error → miss → self-heals. Usetempfile::NamedTempFile::new_in(dir)(already a dep). - Silent swallow of store errors + dead error stat — low [reasoned]. Three
let _ = self.store_cache_entry(...)sites (runner.rs:1230/1277/1310);inc_errorsis#[allow(dead_code)](cache.rs:102) so "Errors" always prints 0. Wire failures intoinc_errors+log::warn!. - Redundant stored fields — low [reasoned].
short_key,solver_version(bjorn3). Drop. - Responses baked into transcript/key — low [reasoned].
;; response: successlines (solver.rs:139) couple the key to solver output. Strip. - Duplicated methods — trivial [verified].
take_baseline_transcriptandtake_phase_transcriptare byte-identical (solver.rs:166 / 174). Collapse. CacheStore::openpanics — trivial [reasoned].panic!on mkdir failure (cache.rs:132) while the rest of the API returnsResult. Make fallible.- CI redundancy — trivial [verified].
aarch64.args=aarch64-fast.args∪slow, so running bothverify-cache.shinvocations duplicates work (avanhatt). - README not updated (avanhatt).
- No
.gitattributesfor the cache dir — 703 JSON files pollute language stats, diffs, blame. Moot if adopting one-file storage (Q2.3). - No GC/prune (bjorn3) — see Q2.
|
@mmcloughlin thanks for this; I will note however that our AI tool policy states that one should not post the direct outputs of LLMs without verifying, so "here is the robot's review that I haven't verified" is pretty directly not the sort of thing that we've decided is helpful. I know you mean well and you're not a drive-by contributor like most folks who run astray of that; just thought I should mention to set/remind norms we've decided on. Thanks! (Thanks to all for the input above -- will refine+synthesize and post another comment about cache location and CI integration in a bit) |
|
@cfallin, @avanhatt, @mmcloughlin, and I chatted a bit about this as well and I wanted to write down what we concluded (correct me if I'm wrong). The current thinking is that we'll model the primitive here as: there's a script which takes an input cache directory, runs full verification using that, then generates a new directory of all cache entries needed. This will take a bit of time for uncached queries but will unconditionally run full verification. This script will land first in this PR not hooked up to CI. Next we'll hook this up to CI, notably creating a github actions artifact which is the output directory. This'll thread its way through our CI processes to get published in the |
Ah, apologies for that. My mistake for not being aware of the AI policy. I was trying to be reasonable by explicitly declaring and hiding the AI output in a Points 1 was my idea, I verified point 2. While not quite verified, I had thought through points 3 and 4 enough to think they were worth raising. Seeing the PR approval also made me think it might be worth raising potential issues sooner. |
- Two-sided cache: a source and a destination. We still have read-only-enforcing and read-write modes, but now in read-write mode, we can produce a new cache that effectively solves the GC problem -- it retains only the entries that are necessary for the current run. This is a primitive that we can use in CI to ensure that our cache does not grow unboundedly. - Ensure that we don't start up the SMT solver and feed it the problem while we collect the transcript before the cache probe; this way, in read-only mode when we run purely out of the cache, the solver need not be installed. - Rework the driver script for the above with three modes: `cache-only`, `rebuild-cache`, and local (no arg).
…; move caching out of `veri`.
Implement a file-based cache for SMT solver queries in the Cranelift verifier, enabling fast re-verification without re-invoking the solver.
The cache is keyed on a hash of the full SMT-LIB transcript for a query, and provides the response (unsat or sat + the model). It can be configured in two ways:
Read-write cache: use the result on hit, invoke the solver and populate the cache on a miss. Meant for local usage.
Read-only enforcing cache: use results on a hit, or fail verification on a miss. This is meant to run in CI.
The intended workflow is to check in the cache into git; this enables CI to run with cached responses rather than re-invoke the solver on every CI run. This sidesteps the issue of long-running queries to the solver; we can reverify our verification on every commit.
One might reasonably ask: what is the TCB and what is the ground truth against which we're verifying, if the repo contains effectively a cache of "yes, it's verified"?
The point is that we re-run the full tool against the current source; this reduces "verify Cranelift" to "evaluate these SMT queries". We then trust that our checked-in SMT query results are valid, but only that and nothing more. If we ever change the source in a way that causes a new instruction lowering path to exist, the tool will attempt to verify this against the cache and (presumably) fail.
The idea is then that the local developer does the verification, and checks in the "SMT query witness"; we either trust that, for regular contributors, or we re-run the verification ourselves, for anonymous contributors. We also have the option of re-regenerating the cache (i.e., re-running verification from scratch) whenever we want.
This commits all the query results that we should need to verify what is in-tree, also, and adds the job to CI. There is still some compute in generation of the queries, but locally (on my 16-core Ryzen 9950X) the full AArch64 backend verification completes in 27 seconds.