Add Miri to more crates - #10507
Conversation
github-actions
Bot
added
arrow
Jo2234
left a comment
Jo2234
left a comment
There was a problem hiding this comment.
The expanded crate coverage looks useful. One workflow-trigger gap: could we also include the workspace Cargo.toml and Cargo.lock in the pull_request.paths filter? A root dependency, feature, or resolver change can alter the code built for all of these crates without touching any listed crate directory, so this Miri job would otherwise be skipped for a change that materially affects its test matrix.
|
thanks ! fair point, will add this in |
9e2af8c to
3b884d3
Compare
| } | ||
|
|
||
| #[test] | ||
| #[cfg_attr(miri, ignore)] |
There was a problem hiding this comment.
could we add comments for each ignore to indicate if its due to slowness, unsupported by miri, etc.?
There was a problem hiding this comment.
done, now every ignore has a reason
| let data = ArrayData::builder(data_type) | ||
| .len(len) | ||
| .add_buffer(Buffer::from(vec![0_u8; len * width])) | ||
| .add_buffer(Buffer::from(MutableBuffer::from_len_zeroed(len * width))) |
There was a problem hiding this comment.
is there some miri issue here being fixed?
There was a problem hiding this comment.
yeah i think this is one issue miri found, was getting an error "Misaligned buffers[0] in array of type Int32, offset from expected alignment of 4 by 2"
based on my (limited) understanding vec![0_u8; len * width] only guarantees 1 byte alignment, so it can be misaligned for wider primitives.
| let out: Vec<u8> = writer.into_inner().unwrap(); | ||
|
|
||
| let buffer = Buffer::from_vec(out); | ||
| let buffer = Buffer::from_slice_ref(out); |
There was a problem hiding this comment.
same with these, is there some issue with miri?
There was a problem hiding this comment.
i think this is essentially same issue as above, vec<u8> has byte alignment, but the test later runs zero copy read with require_alignment(true) , so miri reports the buffer as misaligned.
|
looks like miri went from around 20min to a bit over 30min; probably fine for now, can experiment with more partitions in a followup |
1e7cde5 to
078a9eb
Compare
Jefffrey
left a comment
Jefffrey
left a comment
There was a problem hiding this comment.
thanks for the test fixes; they make sense, and since they were caught by existing error handling code (i.e. didnt panic internally) i think it was just an issue with how the tests were written and not a deeper bug in the library 🤔
2240b61 to
75641ab
Compare
|
@Jefffrey yeah agreed. btw, i just did a force push to resolve a conflict. |
|
thanks @ethantang93 & @Jo2234 |
3 participants
Which issue does this PR close?
closes #10471
Rationale for this change
The Miri workflow currently covers only a subset of packages. This adds more miri coverage to detect unsafe code, Parquet crate remains excluded because it is tracked separately in #614.
What changes are included in this PR?
miri.yaml, some crates were missing beforeINSTA_WORKSPACE_ROOTto prevent snapshot tests from invoking unsupported subprocess operations under Mirinote: it takes about 50mins to run miri on my M3 laptop. when this pr gets merge i think it's expected to prolong the CI time in the miri workflow, but we can always add more parallel workers
Are these changes tested?
i ran the following to test locally.
rustup run nightly bash .github/workflows/miri.sh(runs the whole miri suite)cargo testcargo clippy --workspace --all-targets --all-features -- -D warningsgit diff --checkAre there any user-facing changes?
no, only tests
AI disclosure
I used codex to draft this pr after i got the project and miri tests running on my laptop. I have reviewed all the code and have built, tested these changes. I am happy to own follow-ups too.