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.

Sign up for GitHub

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

Jump to bottom

Run bevy_ecs in miri on CI #1529

Closed
bjorn3 opened this issue Feb 27, 2021 · 6 comments
Closed

Run bevy_ecs in miri on CI #1529

bjorn3 opened this issue Feb 27, 2021 · 6 comments
Labels
A-Build-System Related to build systems or continuous integration A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change

Comments

@bjorn3
Copy link
Contributor

bjorn3 commented Feb 27, 2021 •
edited
Loading

What problem does this solve or what need does it fill?

Bevy_ecs contains a lot of unsafe code. Unsafe code in Bevy has been proven to be unsound on multiple occasions.

What solution would you like?

Run the bevy_ecs test suite in miri to detect UB. Preferably miri would also manually be run on #1525 before it is merged.

What alternative(s) have you considered?

Remove all unsafe code.
Will regress performance by a lot.
Formally verify all unsafe code.
Way too much work.
Manually verify all unsafe code.
Takes a lot of time and will miss some unsoundness.
@bjorn3 bjorn3 added A-Build-System Related to build systems or continuous integration C-Code-Quality A section of code that is hard to understand or change A-ECS Entities, components, systems, and events labels Feb 27, 2021
@cart
Copy link
Member

cart commented Feb 27, 2021

Makes sense to me! Given that miri is pretty slow and the change frequency of bevy_ecs internals is also low, I'm thinking this should probably be a periodic test (daily?). I ran miri prior to creating #1525. I'll do it again before merging.

On main (and #1525), the parallel executor's async-channel usage fails with:
image

A few days ago I also ran miri on the async-channel crate's tests, which passed. So it's unclear if this is a bevy-call-context issue that exposes a problem in async-channel, a bevy issue, or a miri bug (the error says the rule is experimental).

Additionally, if the test above is commented out, miri has an internal compiler error on another test (which seems to be an indicator that bevy's Future code can't be tested by miri yet):
image

@bjorn3
Copy link
Contributor Author

bjorn3 commented Feb 27, 2021

Could you report an issue on rust-lang/rust for the ICE?

@cart
Copy link
Member

cart commented Feb 27, 2021

Sure thing!

@cart
Copy link
Member

cart commented Feb 27, 2021

Looks like a dupe of rust-lang/miri#1038

@NathanSWard
Copy link
Contributor

I would love to try and get miri integrated with CI, however we do seem to be blocked by rust-lang/miri#1038.
(At least for bevy_ecs)

@alice-i-cecile alice-i-cecile added the S-Blocked This cannot move forward until something else changes label Dec 12, 2021
@RalfJung
Copy link
Contributor

rust-lang/miri#1038 is fixed now, btw. :)
Once rust-lang/rust#95144 lands, this will be in the next nightly.

@james7132 james7132 removed the S-Blocked This cannot move forward until something else changes label Mar 21, 2022
@BoxyUwU BoxyUwU mentioned this issue Mar 24, 2022
Closed
@bors bors bot closed this as completed in 28ba87e Mar 25, 2022
aevyrie pushed a commit to aevyrie/bevy that referenced this issue Jun 7, 2022
@BoxyUwU @aevyrie
CI runs cargo miri test -p bevy_ecs (bevyengine#4310)
0113c29 
# Objective

Fixes bevyengine#1529
Run bevy_ecs in miri

## Solution

- Don't set thread names when running in miri rust-lang/miri/issues/1717
- Update `event-listener` to `2.5.2` as previous versions have UB that is detected by miri: [event-listener commit](smol-rs/event-listener@1fa31c5)
- Ignore memory leaks when running in miri as they are impossible to track down rust-lang/miri/issues/1481
- Make `table_add_remove_many` test less "many" because miri is really quite slow :)
- Make CI run `RUSTFLAGS="-Zrandomize-layout" MIRIFLAGS="-Zmiri-ignore-leaks -Zmiri-tag-raw-pointers -Zmiri-disable-isolation" cargo +nightly miri test -p bevy_ecs`
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
@BoxyUwU @ItsDoot
CI runs cargo miri test -p bevy_ecs (bevyengine#4310)
d0b738d 
# Objective

Fixes bevyengine#1529
Run bevy_ecs in miri

## Solution

- Don't set thread names when running in miri rust-lang/miri/issues/1717
- Update `event-listener` to `2.5.2` as previous versions have UB that is detected by miri: [event-listener commit](smol-rs/event-listener@1fa31c5)
- Ignore memory leaks when running in miri as they are impossible to track down rust-lang/miri/issues/1481
- Make `table_add_remove_many` test less "many" because miri is really quite slow :)
- Make CI run `RUSTFLAGS="-Zrandomize-layout" MIRIFLAGS="-Zmiri-ignore-leaks -Zmiri-tag-raw-pointers -Zmiri-disable-isolation" cargo +nightly miri test -p bevy_ecs`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-Build-System Related to build systems or continuous integration A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Merged by Bors] - CI runs cargo miri test -p bevy_ecs BoxyUwU/bevy
6 participants
@RalfJung @cart @james7132 @alice-i-cecile @bjorn3 @NathanSWard