[Merged by Bors] - CI runs `cargo miri test -p bevy_ecs` by BoxyUwU · Pull Request #4310 · bevyengine/bevy

BoxyUwU · 2022-03-24T04:38:45Z

Objective

Fixes #1529
Run bevy_ecs in miri

Solution

Don't set thread names when running in miri Setting a thread name on Linux doesn't work with tracked raw pointers rust-lang/miri#1717
Update event-listener to 2.5.2 as previous versions have UB that is detected by miri: event-listener commit
Ignore memory leaks when running in miri as they are impossible to track down Improve memory leak identification rust-lang/miri#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

mockersf · 2022-03-24T06:54:22Z

 [dependencies]
 futures-lite = "1.4.0"
-event-listener = "2.4.0"
+event-listener = "2.5.2"


dependabot should have warned us of this one... 🤔

mockersf · 2022-03-24T06:57:07Z

This adds 7 minutes to CI but is probably worth it

alice-i-cecile · 2022-03-24T07:07:55Z

This adds 7 minutes to CI but is probably worth it

@mockersf down the line, can we change this to only run on PRs that modify bevy_ecs?

Also as someone ~~prone to~~ who touches code that could produce UB, I totally agree.

mockersf · 2022-03-24T07:11:22Z

It should be added to Bors in https://github.com/bevyengine/bevy/blob/main/.github/bors.toml

@mockersf down the line, can we change this to only run on PRs that modify bevy_ecs?

Yes, but I'm not sure how Bors handles a job that only run on some of the PR...

Co-authored-by: François <mockersf@gmail.com>

cart · 2022-03-24T23:32:18Z

Can you push something that introduces UB so we can prove this will fail CI if we "regress"?

cart · 2022-03-24T23:33:06Z

(or if you've already tested this case on CI, can you send me a link?)

BoxyUwU · 2022-03-25T00:08:07Z

https://github.com/bevyengine/bevy/runs/5684860671?check_suite_focus=true UB found by miri example

cart

Brilliant. LGTM!

cart · 2022-03-25T00:18:27Z

bors r+

# Objective Fixes #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`

bors · 2022-03-25T00:47:30Z

Pull request successfully merged into main.

Build succeeded:

RalfJung · 2022-03-26T21:38:31Z

+            ~/.cargo/registry/index/
+            ~/.cargo/registry/cache/
+            ~/.cargo/git/db/
+            target/


This seems to not cache ~/.cargo/{.crates2.json,.crates.toml} so I assume it will still rebuild xargo each time? I am not sure how caching works for GHA though.

the list of paths to cache is from https://github.com/actions/cache/blob/main/examples.md#rust---cargo... maybe it's not complete?

# 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`

# Objective Make it possible to run miri with memory leak detection enabled. It is enabled by default, so it reports errors whenever I run it locally. And running it in CI should help prevent any new leaks from being introduced. See bevyengine#4310 and bevyengine#4959 for previous attempts to enable this. Note that miri has added backtraces to leaked allocations since then (rust-lang/rust#109061), so leaks are much easier to diagnose. ## Solution The first leak: When trying to drop non-send data on the wrong thread, the allocation for the value was being leaked. We can't drop the value from the wrong thread, but we could still deallocate its storage. Rework the thread check in `NonSendData::drop` so that it still deallocates, setting `present = false` to ensure the value's `drop` is not called. Change the `panic!` to a `warn!` so that we can have the check work the same way during unwinding. The second leak: The tests for `Interned` intentionally leak data! So we can't exactly "fix" them, but I'd like miri to stop failing on them. Then I realized that miri does not report leaks for our use of `Interned` for things like `ScheduleLabel`. That's because those values are stored in a `static`, so are still reachable. So, add a `static` to each of the tests with intentional leaks and store the leaked values there. miri will consider those values reachable, and not report any leaks. --------- Co-authored-by: Daniel Skates <zeophlite@gmail.com>

BoxyUwU added 3 commits March 24, 2022 03:30

update UB dep

d2acfa8

do not set thread name in miri

0a78960

make test less big

84b9aa8

BoxyUwU changed the title ~~Run bevy_ecs in miri~~ (wip) Run bevy_ecs in miri Mar 24, 2022

github-actions Bot added the S-Needs-Triage This issue needs to be labelled label Mar 24, 2022

BoxyUwU force-pushed the bevy_ecs_miri branch from 12366b3 to 99a517b Compare March 24, 2022 05:21

BoxyUwU changed the title ~~(wip) Run bevy_ecs in miri~~ CI runs cargo miri test -p bevy_ecs Mar 24, 2022

BoxyUwU added A-ECS Entities, components, systems, and events C-Dependencies A change to the crates that Bevy depends on and removed S-Needs-Triage This issue needs to be labelled labels Mar 24, 2022

BoxyUwU requested a review from mockersf March 24, 2022 05:38

Run in CI

8035644

BoxyUwU force-pushed the bevy_ecs_miri branch from 99a517b to 8035644 Compare March 24, 2022 05:43

alice-i-cecile reviewed Mar 24, 2022

View reviewed changes

Comment thread .github/workflows/ci.yml

mockersf reviewed Mar 24, 2022

View reviewed changes

Comment thread crates/bevy_ecs/src/lib.rs Outdated

mockersf reviewed Mar 24, 2022

View reviewed changes

Comment thread crates/bevy_tasks/src/task_pool.rs Outdated

BoxyUwU and others added 5 commits March 24, 2022 13:12

make comment a TODO

daedf38

Co-authored-by: François <mockersf@gmail.com>

squish code together

6e3f611

Co-authored-by: François <mockersf@gmail.com>

comments

a0bd9bb

bors dot toml

f52f30e

SOMEONE 👀

ba6d95a

BoxyUwU closed this Mar 24, 2022

BoxyUwU reopened this Mar 24, 2022

james7132 approved these changes Mar 24, 2022

View reviewed changes

Comment thread .github/workflows/ci.yml Outdated

alice-i-cecile approved these changes Mar 24, 2022

View reviewed changes

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Mar 24, 2022

cart reviewed Mar 24, 2022

View reviewed changes

Comment thread crates/bevy_tasks/src/task_pool.rs Outdated

reviews

fb3b8cf

remove ub

f0df0ed

cart approved these changes Mar 25, 2022

View reviewed changes

alice-i-cecile mentioned this pull request Mar 25, 2022

[Merged by Bors] - ParamSet for conflicting SystemParam:s #2765

Closed

bors Bot changed the title ~~CI runs cargo miri test -p bevy_ecs~~ [Merged by Bors] - CI runs cargo miri test -p bevy_ecs Mar 25, 2022

bors Bot closed this Mar 25, 2022

RalfJung reviewed Mar 26, 2022

View reviewed changes

chescock mentioned this pull request Jul 13, 2026

Resolve memory leaks in tests reported by miri #24979

Merged

Uh oh!

Uh oh!

Conversation

BoxyUwU commented Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Uh oh!

Uh oh!

Uh oh!

mockersf Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mockersf commented Mar 24, 2022

Uh oh!

alice-i-cecile commented Mar 24, 2022

Uh oh!

mockersf commented Mar 24, 2022

Uh oh!

Uh oh!

cart commented Mar 24, 2022

Uh oh!

cart commented Mar 24, 2022

Uh oh!

Uh oh!

BoxyUwU commented Mar 25, 2022

Uh oh!

cart left a comment

Choose a reason for hiding this comment

Uh oh!

cart commented Mar 25, 2022

Uh oh!

bors Bot commented Mar 25, 2022

Uh oh!

RalfJung Mar 26, 2022

Choose a reason for hiding this comment

Uh oh!

mockersf Mar 26, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

BoxyUwU commented Mar 24, 2022 •

edited

Loading

mockersf Mar 24, 2022 •

edited

Loading