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

Improve memory leak identification #1481

Closed
Firstyear opened this issue Jul 17, 2020 · 10 comments
Closed

Improve memory leak identification #1481

Firstyear opened this issue Jul 17, 2020 · 10 comments
Assignees
Labels
A-leaks Area: affects the memory leak checker A-ux Area: This affects the general user experience C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement

Comments

@Firstyear
Copy link
Contributor

Miri is able to determine if memory leaks occur. The output is similar to:

running 10 tests
test collections::bptree::iter::tests::test_bptree2_iter_leafiter_1 ... ok
test collections::bptree::iter::tests::test_bptree2_iter_leafiter_2 ... ok
test collections::bptree::iter::tests::test_bptree2_iter_leafiter_3 ... ok
test collections::bptree::iter::tests::test_bptree2_iter_leafiter_4 ... ok
test collections::bptree::iter::tests::test_bptree2_iter_leafiter_5 ... ok
test collections::bptree_legacy::iter::tests::test_bptree_iter_leafiter_1 ... ok
test collections::bptree_legacy::iter::tests::test_bptree_iter_leafiter_2 ... ok
test collections::bptree_legacy::iter::tests::test_bptree_iter_leafiter_3 ... ok
test collections::bptree_legacy::iter::tests::test_bptree_iter_leafiter_4 ... ok
test collections::bptree_legacy::iter::tests::test_bptree_iter_leafiter_5 ... ok

test result: ok. 10 passed; 0 failed; 0 ignored; 0 measured; 175 filtered out

The following memory was leaked:
alloc876074 (Rust heap, size: 120, align: 8) {
    0x00 │ 07 00 00 00 00 00 00 20 0a 00 00 00 00 00 00 00 │ ....... ........
    0x10 │ 0b 00 00 00 00 00 00 00 0c 00 00 00 00 00 00 00 │ ................
    0x20 │ 0d 00 00 00 00 00 00 00 0e 00 00 00 00 00 00 00 │ ................
    0x30 │ 0f 00 00 00 00 00 00 00 10 00 00 00 00 00 00 00 │ ................
    0x40 │ 0a 00 00 00 00 00 00 00 0b 00 00 00 00 00 00 00 │ ................
    0x50 │ 0c 00 00 00 00 00 00 00 0d 00 00 00 00 00 00 00 │ ................
    0x60 │ 0e 00 00 00 00 00 00 00 0f 00 00 00 00 00 00 00 │ ................
    0x70 │ 10 00 00 00 00 00 00 00                         │ ........
}
... <many more leaks below>

The leaks are not associated to a specific test however. To aid leak isolation, it would be good if there was a way to see which test caused what leak.

Some ideas are potentially to have a test function associated with the leak line like:

alloc876074 (collections::bptree_legacy::iter::tests::test_bptree_iter_leafiter_4) (Rust heap, size: 120, align: 8) {

Another option is to have bisection, so that when warnings or errors are omitted that the warnings can be traced to a minimum set of tests that create those errors.

Thanks!

@oli-obk
Copy link
Contributor

oli-obk commented Jul 17, 2020

We can't really do this in miri, this would need to be a test-suite feature in the official rust test suite.

What we can do is offer functions that you call at the beginning and end of a test and that record the set of allocations that are live and tell you what new allocations still exist at the second call.

@RalfJung
Copy link
Member

We could also (optionally) capture (parts of) the stack trace for each allocation; that could be sufficient here. The performance impact could be really bad though.

@RalfJung RalfJung added C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement A-leaks Area: affects the memory leak checker labels Jul 17, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Jul 17, 2020

cc #17

@Firstyear
Copy link
Contributor Author

I would be quite happy with functions you can call at the start and end of tests to help you identify memory leaks better.

@RalfJung RalfJung added the A-ux Area: This affects the general user experience label Aug 20, 2020
bors bot pushed a commit to bevyengine/bevy that referenced this issue Mar 25, 2022
# 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`
aevyrie pushed a commit to aevyrie/bevy that referenced this issue Jun 7, 2022
# 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`
@RalfJung
Copy link
Member

bevyengine/bevy#4959 contains a very nice snippet that helps identify which test in a test suite is leaking:

cargo --quiet test --lib -- --list | sed 's/: test$//' | xargs -n1 cargo miri test --lib -- --exact

@kryptan
Copy link

kryptan commented Jul 4, 2022

Would it be possible to detect leaks as they happen, that is as soon as the last pointer to the allocation goes out of scope?

@oli-obk
Copy link
Contributor

oli-obk commented Jul 4, 2022

No that's not possible. You could have turned a pointer into a [bool; 64] by taking out its bits individually and shuffling them around. Then when you unshuffle and turn it back into the address, you could dereference it again. Maybe with strict provenance we could do this though? It would be very expensive

Basically a tracing garbage collector, but if you only enable it when you already know a leak is happening, could be ok. Or we add a way to bisect to leak via the basic block counter.

bors added a commit that referenced this issue Jul 21, 2022
[cargo-miri] support nextest

Add the ability to run `cargo miri nextest list` and `cargo miri nextest run`.

[cargo-nextest](https://nexte.st) is a new test runner for Rust maintained mostly by myself. It has several new features, but the most relevant to miri is the fact that it runs [each test in its own process](https://nexte.st/book/how-it-works.html#the-nextest-model). This gives miri users better leak detection (#1481) for free, for example.

See nextest-rs/nextest#181 for discussion, including comments by `@eddyb` and `@RalfJung.`

Future work might be to have miri read [the list of tests](https://docs.rs/nextest-metadata/latest/nextest_metadata/struct.TestListSummary.html) (or [test binaries](https://docs.rs/nextest-metadata/latest/nextest_metadata/struct.BinaryListSummary.html)) generated by `nextest list`. `@eddyb` thinks that might be useful.

I tested `cargo miri nextest run` against smallvec, and it worked great.

Note: Running tests out of archives is currently broken, as the comment in run-test.py explains.
@sunshowers
Copy link
Contributor

FYI miri + nextest supports proper leak detection because it runs each test in its own process. https://nexte.st/book/miri.html has the instructions -- feel free to give it a whirl!

@Firstyear
Copy link
Contributor Author

I think nextest breaks in my environment for a different reason, but that's something I should raise directly.

ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
# 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`
@saethlin saethlin self-assigned this Mar 12, 2023
@oli-obk
Copy link
Contributor

oli-obk commented Apr 23, 2023

Fixed in rust-lang/rust#109061

@oli-obk oli-obk closed this as completed Apr 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-leaks Area: affects the memory leak checker A-ux Area: This affects the general user experience C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement
Projects
None yet
Development

No branches or pull requests

6 participants