Update SMT-based ISLE verifier#13550
Conversation
Co-authored-by: Michael McLoughlin <mmcloughlin@gmail.com>
Co-authored-by: Michael McLoughlin <mmcloughlin@gmail.com>
Co-authored-by: Michael McLoughlin <mmcloughlin@gmail.com>
Co-authored-by: Michael McLoughlin <mmcloughlin@gmail.com>
Co-authored-by: Michael McLoughlin <mmcloughlin@gmail.com>
There was a problem hiding this comment.
Ah yes, thanks for spotting!
cfallin
left a comment
There was a problem hiding this comment.
This largely looks good! I skimmed over any spec/model/attr forms in the diffs of ISLE, and the cranelift/isle/veri tree in general; reviewed more carefully the bits that were touched in Cranelift and the ISLE compiler proper. Some logistical comments but nothing really substantial. Thanks for all the work in getting this in shape for upstreaming!
| } | ||
|
|
||
| fn add_binding(&mut self, id: BindingId, binding: &Binding) -> Result<()> { | ||
| dbg_depth!("add_binding"); |
There was a problem hiding this comment.
Does this debug code need to stay in-tree? (It looks like it's trying to help find very deeply nested invocations? In any case I'd rather not have the ad-hoc thread-local magic here if we don't need this check permanently)
There was a problem hiding this comment.
Oops, did not mean to include, removed!
Subscribe to Label ActionDetailsThis issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:meta", "isle"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
|
@avanhatt I'm taking a look at the cargo-vets now and it looks like this PR bumps a bunch of dep versions in ways that I hope are not necessary -- the vet backlog is now
Which is unfortunately probably infeasible for me to do. Could you (i) revert |
|
Thanks for your last commit -- looks like the dep bumps and vet burden are real; still 425163 LoC. Digging into one particular bump ( Would you mind doing a little dep gardening to see if we can get the diff to our deps a bit smaller? |
An earlier incidental `cargo update` bumped ~hundreds of transitive deps (tracing, clap, cc, ...), inflating the cargo-vet backlog to ~425k LoC. Reset Cargo.lock to main's versions and re-resolve, so the only additions are the veri crates' genuine deps (num-bigint, the pest family, easy-smt). The vet backlog drops to ~51k LoC, and everything else stays at main's vetted versions.
Done and pushed. I see 7 unvetted dependencies in Let us know if you see the same, and the backlog is manageable. |
|
A note on the vets here -- we already exempt dependencies exclusively related to fuzzing from vetting and I think it would be reasonable to take a similar stance here. Specifically if you apply this diff: diff --git a/supply-chain/config.toml b/supply-chain/config.toml
index 16d0abfb19..925447fe98 100644
--- a/supply-chain/config.toml
+++ b/supply-chain/config.toml
@@ -64,6 +64,12 @@ audit-as-crates-io = true
[policy.cranelift-isle]
audit-as-crates-io = true
+[policy.cranelift-isle-veri]
+criteria = []
+
+[policy.cranelift-isle-veri-aslp]
+criteria = []
+
[policy.cranelift-jit]
audit-as-crates-io = truethe |
…ecs. (#104) The deeply-nested parsing requiring a larger stack in `build.rs` probably also slows down the Cranelift build, which we also don't want. On further thought the root issue is actually that we're trying to parse these specs at all during an ordinary Cranelift build (verification is a separate run). This commit excludes the relevant ISLE sources entirely during normal builds, re-enabling them when the `spec` Cargo feature (set by the veri-related crates) is enabled.
cfallin
left a comment
There was a problem hiding this comment.
With all of the tweaks and now a green CI run, this looks ready to merge.
Thanks again @avanhatt, @mmcloughlin and others for all the work on this! Looking forward to followup work to get this in CI by default (I have some tasks for this). It's a great addition to our correctness story.
05e7921
Update the core ISLE verifier for formal, SMT-based checking of instruction lowering rules. This brings the implementation up-to-date with our work described at OOPSLA 2025, rather than the prior ASPLOS 2024 implementation.
The core improvements are:
veri chainto be automatically composed/inlined. This reduces the specification burden substantially. See the paper for details.aarch64backend, most machine instMinstspecifications are automatically derived from the authoritative ARM ASL reference, with our work building on the ASLp project to derive more targeted specifications for this domain.