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

Support running miri in ci #534

Merged
merged 6 commits into from
Apr 2, 2024
Merged

Support running miri in ci #534

merged 6 commits into from
Apr 2, 2024

Conversation

ryoqun
Copy link
Collaborator

@ryoqun ryoqun commented Apr 2, 2024

Problem

my unsafe is getting disputed (#129). :)

Summary of Changes

after playing a bit, it turned out miri can detect many aliasing violations at runtime, which cann't be caught at compile-time.

So, to argue for my unsafes, let's sneak in miri support into the ci...

That said, I have yet another reason to use miri in my task queue. so, making using miri easy shouln't be wasted effort.

cc: @alessandrod @apfitzge

@ryoqun ryoqun requested a review from yihau April 2, 2024 08:29
@ryoqun ryoqun marked this pull request as ready for review April 2, 2024 08:29
ci/test-miri.sh Outdated

source ci/rust-version.sh nightly

rustup component add miri --toolchain "$rust_nightly"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe, need to move to Dockerfile...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that said, running this multiple times is harmless:

$ rustup component add miri --toolchain nightly-2024-01-05-x86_64-unknown-linux-gnu
info: component 'miri' for target 'x86_64-unknown-linux-gnu' is up to date
$ echo $?
0

so, i can move this line as a follow-up pr. once after mighty @yihau did update our rust dockers. :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah. should put into Dockerfile. could we have the Dockerfile changes in this PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, done: 2c370a6

ci/test-miri.sh Outdated

source ci/rust-version.sh nightly

rustup component add miri --toolchain "$rust_nightly"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah. should put into Dockerfile. could we have the Dockerfile changes in this PR?

rustup component add miri --toolchain "$rust_nightly"

# miri is very slow; so only run very few of selective tests!
cargo "+${rust_nightly}" miri test -p solana-program -- hash:: account_info::
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does the #[cfg_attr(miri)] work for this purpose? (I'm not quite understand how it select tests. try to steal your brain 😂)

Copy link
Collaborator Author

@ryoqun ryoqun Apr 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that would need to add #[cfg_attr(miri, ignore)] to almost all tests...

@ryoqun ryoqun requested a review from yihau April 2, 2024 08:49
Copy link
Member

@yihau yihau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated the docker image and miri is green ✅

@yihau yihau added the automerge automerge Merge this Pull Request automatically once CI passes label Apr 2, 2024
@ryoqun ryoqun merged commit 798cb56 into anza-xyz:master Apr 2, 2024
22 of 24 checks passed
@brooksprumo
Copy link

Wow, nicely done! I remember when I first started on the code base, one thing I tried to do initially was get miri running. I failed, so glad to see you succeed here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge automerge Merge this Pull Request automatically once CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants