Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add miri to CI #464

Merged
merged 4 commits into from
Mar 11, 2024
Merged

Add miri to CI #464

merged 4 commits into from
Mar 11, 2024

Conversation

DropDemBits
Copy link
Contributor

Fixes #463

Copy link

netlify bot commented Nov 9, 2023

Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit 407d6bc
🔍 Latest deploy log https://app.netlify.com/sites/salsa-rs/deploys/654f14640be34c00087d8ebb

`-Zmiri-disable-isolation` can't be used since `insta` eventually ends up
calling `pipe2`, which currently doesn't have a shim in miri.
@nikomatsakis
Copy link
Member

This is awesome! I see we are getting CI failures though :(

I don't know enough about miri to know if these are known limitations or what -- I imagine maybe we just want to run this on a subset of our test suite?

@DropDemBits
Copy link
Contributor Author

miri does have support for shimming out to some foreign functions, though not for pipe2 currently. This is an issue for any tests using the insta crate, which eventually ends up calling pipe2.

Thankfully, it's isolated to the older version of salsa, as salsa-2022 doesn't use the insta crate for tests. It's been a bit but I think that's what I did to test out the placement new changes in miri.

The unfortunate thing is that we'd need to list out every package that we'd want to run miri on, but I think that's an acceptable compromise for now.

@DropDemBits

This comment was marked as outdated.

These don't need to disable isolation so the `-Zmiri-isolation-error` flag can be removed.
@DropDemBits
Copy link
Contributor Author

DropDemBits commented Nov 11, 2023

For now, I made it so that miri only runs on the salsa-2022 packages, but since there is preexisting UB it'll still fail Edit: was from an outdated package, see next comment.

@DropDemBits
Copy link
Contributor Author

Whoops, turns out locally I had an outdated Cargo.lock file with an older arc-swap. 😅
Though I think we should bump arc-swap to 1.6.0 since 1.5.1 and earlier are known to have UB (see vorner/arc-swap#86).

@DropDemBits
Copy link
Contributor Author

Added a commit bumping the arc-swap version, though I can split it into a separate PR if need be.

1.6.0 fixes some UB in previous versions of `arc-swap`.
@nikomatsakis
Copy link
Member

bors r+

@nikomatsakis
Copy link
Member

Thanks, and I appreciate your patience a second time! Things got crazy there.

@nikomatsakis nikomatsakis added this pull request to the merge queue Mar 11, 2024
Merged via the queue into salsa-rs:master with commit 4a13020 Mar 11, 2024
8 checks passed
@DropDemBits DropDemBits deleted the miri-ci branch March 11, 2024 16:25
@DropDemBits
Copy link
Contributor Author

It's easy to be patient when I'm busy with college again 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add miri to CI
2 participants