[Merged by Bors] - fix some memory leaks detected by miri by BoxyUwU · Pull Request #4959 · bevyengine/bevy

BoxyUwU · 2022-06-07T19:49:05Z

The first leak:

    #[test]
    fn blob_vec_drop_empty_capacity() {
        let item_layout = Layout::new::<Foo>();
        let drop = drop_ptr::<Foo>;
        let _ = unsafe { BlobVec::new(item_layout, Some(drop), 0) };
    }

this is because we allocate the swap scratch in blobvec regardless of what the capacity is, but we only deallocate if capacity is > 0

The second leak:

    #[test]
    fn panic_while_overwriting_component() {
        let helper = DropTestHelper::new();

        let res = panic::catch_unwind(|| {
            let mut world = World::new();
            world
                .spawn()
                .insert(helper.make_component(true, 0))
                .insert(helper.make_component(false, 1));

            println!("Done inserting! Dropping world...");
        });

        let drop_log = helper.finish(res);

        assert_eq!(
            &*drop_log,
            [
                DropLogItem::Create(0),
                DropLogItem::Create(1),
                DropLogItem::Drop(0),
            ]
        );
    }

this is caused by us not running the drop impl on the to-be-inserted component if the drop impl of the overwritten component panics

managed to figure out where the leaks were by using this 10/10 command

cargo --quiet test --lib -- --list | sed 's/: test$//' | MIRIFLAGS="-Zmiri-disable-isolation" xargs -n1 cargo miri test --lib -- --exact

which runs every test one by one rather than all at once which let miri actually tell me which test had the leak 🙃

TheRawMeatball · 2022-06-07T19:58:36Z

Can we get that command added to somewhere in the docs?

james7132

LGTM on the fix.

alice-i-cecile

Good eye. Nice to see miri catching these problems for us.

bors r+

remove `-Zmiri-ignore-leaks` from CI args and fix leaks that miri detects. The first leak: ```rust #[test] fn blob_vec_drop_empty_capacity() { let item_layout = Layout::new::<Foo>(); let drop = drop_ptr::<Foo>; let _ = unsafe { BlobVec::new(item_layout, Some(drop), 0) }; } ``` this is because we allocate the swap scratch in blobvec regardless of what the capacity is, but we only deallocate if capacity is > 0 The second leak: ```rust #[test] fn panic_while_overwriting_component() { let helper = DropTestHelper::new(); let res = panic::catch_unwind(|| { let mut world = World::new(); world .spawn() .insert(helper.make_component(true, 0)) .insert(helper.make_component(false, 1)); println!("Done inserting! Dropping world..."); }); let drop_log = helper.finish(res); assert_eq!( &*drop_log, [ DropLogItem::Create(0), DropLogItem::Create(1), DropLogItem::Drop(0), ] ); } ``` this is caused by us not running the drop impl on the to-be-inserted component if the drop impl of the overwritten component panics --- managed to figure out where the leaks were by using this 10/10 command ``` cargo --quiet test --lib -- --list | sed 's/: test$//' | MIRIFLAGS="-Zmiri-disable-isolation" xargs -n1 cargo miri test --lib -- --exact ``` which runs every test one by one rather than all at once which let miri actually tell me which test had the leak :upside_down_face:

bors · 2022-06-27T18:14:49Z

Build failed:

miri

alice-i-cecile · 2022-06-27T18:18:36Z

test query::tests::self_conflicting_worldquery - should panic ... ok
warning: thread support is experimental and incomplete: weak memory effects are not emulated.

Looks like there's a warning that should be ignored?

james7132 · 2022-06-30T01:08:25Z

bors retry

remove `-Zmiri-ignore-leaks` from CI args and fix leaks that miri detects. The first leak: ```rust #[test] fn blob_vec_drop_empty_capacity() { let item_layout = Layout::new::<Foo>(); let drop = drop_ptr::<Foo>; let _ = unsafe { BlobVec::new(item_layout, Some(drop), 0) }; } ``` this is because we allocate the swap scratch in blobvec regardless of what the capacity is, but we only deallocate if capacity is > 0 The second leak: ```rust #[test] fn panic_while_overwriting_component() { let helper = DropTestHelper::new(); let res = panic::catch_unwind(|| { let mut world = World::new(); world .spawn() .insert(helper.make_component(true, 0)) .insert(helper.make_component(false, 1)); println!("Done inserting! Dropping world..."); }); let drop_log = helper.finish(res); assert_eq!( &*drop_log, [ DropLogItem::Create(0), DropLogItem::Create(1), DropLogItem::Drop(0), ] ); } ``` this is caused by us not running the drop impl on the to-be-inserted component if the drop impl of the overwritten component panics --- managed to figure out where the leaks were by using this 10/10 command ``` cargo --quiet test --lib -- --list | sed 's/: test$//' | MIRIFLAGS="-Zmiri-disable-isolation" xargs -n1 cargo miri test --lib -- --exact ``` which runs every test one by one rather than all at once which let miri actually tell me which test had the leak :upside_down_face:

bors · 2022-06-30T01:16:37Z

Build failed:

miri

alice-i-cecile · 2022-07-01T21:54:11Z

bors r+

The first leak: ```rust #[test] fn blob_vec_drop_empty_capacity() { let item_layout = Layout::new::<Foo>(); let drop = drop_ptr::<Foo>; let _ = unsafe { BlobVec::new(item_layout, Some(drop), 0) }; } ``` this is because we allocate the swap scratch in blobvec regardless of what the capacity is, but we only deallocate if capacity is > 0 The second leak: ```rust #[test] fn panic_while_overwriting_component() { let helper = DropTestHelper::new(); let res = panic::catch_unwind(|| { let mut world = World::new(); world .spawn() .insert(helper.make_component(true, 0)) .insert(helper.make_component(false, 1)); println!("Done inserting! Dropping world..."); }); let drop_log = helper.finish(res); assert_eq!( &*drop_log, [ DropLogItem::Create(0), DropLogItem::Create(1), DropLogItem::Drop(0), ] ); } ``` this is caused by us not running the drop impl on the to-be-inserted component if the drop impl of the overwritten component panics --- managed to figure out where the leaks were by using this 10/10 command ``` cargo --quiet test --lib -- --list | sed 's/: test$//' | MIRIFLAGS="-Zmiri-disable-isolation" xargs -n1 cargo miri test --lib -- --exact ``` which runs every test one by one rather than all at once which let miri actually tell me which test had the leak :upside_down_face:

bors · 2022-07-01T22:09:32Z

Pull request successfully merged into main.

Build succeeded:

@BoxyUwU

# Objective When `miri` runs in our build system to detect unsoundness, its output can be very unhelpful, as the tests are all run in parallel. ## Solution Add a comment documenting the extremely obvious 10/10 command used by @BoxyUwU in #4959. I've stuck this in the CI file, as it seems like the most obvious place to check when frustrated. I didn't put it in CONTRIBUTING.md because this is an eldritch abomination and will never be useful to new contributors.

The first leak: ```rust #[test] fn blob_vec_drop_empty_capacity() { let item_layout = Layout::new::<Foo>(); let drop = drop_ptr::<Foo>; let _ = unsafe { BlobVec::new(item_layout, Some(drop), 0) }; } ``` this is because we allocate the swap scratch in blobvec regardless of what the capacity is, but we only deallocate if capacity is > 0 The second leak: ```rust #[test] fn panic_while_overwriting_component() { let helper = DropTestHelper::new(); let res = panic::catch_unwind(|| { let mut world = World::new(); world .spawn() .insert(helper.make_component(true, 0)) .insert(helper.make_component(false, 1)); println!("Done inserting! Dropping world..."); }); let drop_log = helper.finish(res); assert_eq!( &*drop_log, [ DropLogItem::Create(0), DropLogItem::Create(1), DropLogItem::Drop(0), ] ); } ``` this is caused by us not running the drop impl on the to-be-inserted component if the drop impl of the overwritten component panics --- managed to figure out where the leaks were by using this 10/10 command ``` cargo --quiet test --lib -- --list | sed 's/: test$//' | MIRIFLAGS="-Zmiri-disable-isolation" xargs -n1 cargo miri test --lib -- --exact ``` which runs every test one by one rather than all at once which let miri actually tell me which test had the leak :upside_down_face:

@BoxyUwU

# Objective When `miri` runs in our build system to detect unsoundness, its output can be very unhelpful, as the tests are all run in parallel. ## Solution Add a comment documenting the extremely obvious 10/10 command used by @BoxyUwU in bevyengine#4959. I've stuck this in the CI file, as it seems like the most obvious place to check when frustrated. I didn't put it in CONTRIBUTING.md because this is an eldritch abomination and will never be useful to new contributors.

The first leak: ```rust #[test] fn blob_vec_drop_empty_capacity() { let item_layout = Layout::new::<Foo>(); let drop = drop_ptr::<Foo>; let _ = unsafe { BlobVec::new(item_layout, Some(drop), 0) }; } ``` this is because we allocate the swap scratch in blobvec regardless of what the capacity is, but we only deallocate if capacity is > 0 The second leak: ```rust #[test] fn panic_while_overwriting_component() { let helper = DropTestHelper::new(); let res = panic::catch_unwind(|| { let mut world = World::new(); world .spawn() .insert(helper.make_component(true, 0)) .insert(helper.make_component(false, 1)); println!("Done inserting! Dropping world..."); }); let drop_log = helper.finish(res); assert_eq!( &*drop_log, [ DropLogItem::Create(0), DropLogItem::Create(1), DropLogItem::Drop(0), ] ); } ``` this is caused by us not running the drop impl on the to-be-inserted component if the drop impl of the overwritten component panics --- managed to figure out where the leaks were by using this 10/10 command ``` cargo --quiet test --lib -- --list | sed 's/: test$//' | MIRIFLAGS="-Zmiri-disable-isolation" xargs -n1 cargo miri test --lib -- --exact ``` which runs every test one by one rather than all at once which let miri actually tell me which test had the leak :upside_down_face:

@BoxyUwU

# Objective When `miri` runs in our build system to detect unsoundness, its output can be very unhelpful, as the tests are all run in parallel. ## Solution Add a comment documenting the extremely obvious 10/10 command used by @BoxyUwU in bevyengine#4959. I've stuck this in the CI file, as it seems like the most obvious place to check when frustrated. I didn't put it in CONTRIBUTING.md because this is an eldritch abomination and will never be useful to new contributors.

The first leak: ```rust #[test] fn blob_vec_drop_empty_capacity() { let item_layout = Layout::new::<Foo>(); let drop = drop_ptr::<Foo>; let _ = unsafe { BlobVec::new(item_layout, Some(drop), 0) }; } ``` this is because we allocate the swap scratch in blobvec regardless of what the capacity is, but we only deallocate if capacity is > 0 The second leak: ```rust #[test] fn panic_while_overwriting_component() { let helper = DropTestHelper::new(); let res = panic::catch_unwind(|| { let mut world = World::new(); world .spawn() .insert(helper.make_component(true, 0)) .insert(helper.make_component(false, 1)); println!("Done inserting! Dropping world..."); }); let drop_log = helper.finish(res); assert_eq!( &*drop_log, [ DropLogItem::Create(0), DropLogItem::Create(1), DropLogItem::Drop(0), ] ); } ``` this is caused by us not running the drop impl on the to-be-inserted component if the drop impl of the overwritten component panics --- managed to figure out where the leaks were by using this 10/10 command ``` cargo --quiet test --lib -- --list | sed 's/: test$//' | MIRIFLAGS="-Zmiri-disable-isolation" xargs -n1 cargo miri test --lib -- --exact ``` which runs every test one by one rather than all at once which let miri actually tell me which test had the leak :upside_down_face:

@BoxyUwU

# Objective When `miri` runs in our build system to detect unsoundness, its output can be very unhelpful, as the tests are all run in parallel. ## Solution Add a comment documenting the extremely obvious 10/10 command used by @BoxyUwU in bevyengine#4959. I've stuck this in the CI file, as it seems like the most obvious place to check when frustrated. I didn't put it in CONTRIBUTING.md because this is an eldritch abomination and will never be useful to new contributors.

# 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 the A-ECS Entities, components, systems, and events label Jun 7, 2022

cart added this to the Bevy 0.8 milestone Jun 7, 2022

james7132 approved these changes Jun 8, 2022

View reviewed changes

RalfJung mentioned this pull request Jun 26, 2022

Improve memory leak identification rust-lang/miri#1481

Closed

alice-i-cecile added the P-Unsound A bug that results in undefined compiler behavior label Jun 27, 2022

alice-i-cecile approved these changes Jun 27, 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 Jun 27, 2022

alice-i-cecile mentioned this pull request Jun 27, 2022

[Merged by Bors] - Add troubleshooting command to miri docs #5116

Closed

alice-i-cecile removed 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 Jul 1, 2022

BoxyUwU added 2 commits July 1, 2022 17:01

Fix leak in blob_vec_drop_empty_capacity test

bcdd93a

Fix leak in panic_while_overwriting_component test

960a7a8

BoxyUwU force-pushed the dont_ignore_leaks branch from 22dcb40 to 960a7a8 Compare July 1, 2022 16:01

BoxyUwU removed the P-Unsound A bug that results in undefined compiler behavior label Jul 1, 2022

readd ignore leaks to CI

aeff15d

BoxyUwU changed the title ~~remove -Zmiri-ignore-leaks from miri CI job~~ fix some memory leaks detected by miri Jul 1, 2022

bors Bot changed the title ~~fix some memory leaks detected by miri~~ [Merged by Bors] - fix some memory leaks detected by miri Jul 1, 2022

bors Bot closed this Jul 1, 2022

joseph-gio mentioned this pull request Aug 9, 2022

CI output for MIRI is unhelpful #5623

Closed

chescock mentioned this pull request Jul 13, 2026

Resolve memory leaks in tests reported by miri #24979

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[Merged by Bors] - fix some memory leaks detected by miri - #4959

[Merged by Bors] - fix some memory leaks detected by miri#4959
BoxyUwU wants to merge 3 commits into
bevyengine:mainfrom
BoxyUwU:dont_ignore_leaks

BoxyUwU commented Jun 7, 2022 •

edited

Loading

Uh oh!

TheRawMeatball commented Jun 7, 2022

Uh oh!

james7132 left a comment

Uh oh!

alice-i-cecile left a comment

Uh oh!

bors Bot commented Jun 27, 2022

Uh oh!

alice-i-cecile commented Jun 27, 2022

Uh oh!

james7132 commented Jun 30, 2022

Uh oh!

bors Bot commented Jun 30, 2022

Uh oh!

alice-i-cecile commented Jul 1, 2022

Uh oh!

bors Bot commented Jul 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Uh oh!

Conversation

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

Uh oh!

Uh oh!

TheRawMeatball commented Jun 7, 2022

Uh oh!

james7132 left a comment

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

bors Bot commented Jun 27, 2022

Uh oh!

alice-i-cecile commented Jun 27, 2022

Uh oh!

james7132 commented Jun 30, 2022

Uh oh!

bors Bot commented Jun 30, 2022

Uh oh!

alice-i-cecile commented Jul 1, 2022

Uh oh!

bors Bot commented Jul 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

BoxyUwU commented Jun 7, 2022 •

edited

Loading