Fix memory leaks in doctests? #126067
Labels
A-allocators
Area: Custom and system allocators
E-medium
Call for participation: Medium difficulty. Experience needed to fix: Intermediate.
E-mentor
Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion.
T-libs
Relevant to the library team, which will review and decide on the PR/issue.
Comments
rustbot
added
the
needs-triage
|
One (I think somewhat typical) example of a leaky test is this: a |
ChrisDenton
added
A-allocators
|
I've nominated for libs-api to ensure this gets discussed but feel free to un-nominate if a decision is made before a meeting. |
Was that meant as an example of one that takes extra steps to unleak it? Since it calls The Vec::leak doctest is an example that leaks. |
|
Ah, I picked the wrong test indeed, sorry. Here is an example that leaks. |
|
We discussed this in the libs meeting today. We would prefer to have miri check for leaks in doctests and handle the currently leaking tests by either:
|
ChrisDenton
removed
the
I-libs-nominated
That could be done if rustdoc allowed passing flags to Miri... but the could be only for Miri, not rustc, so not sure how to best do this. Alternatively, rust-lang/miri#3670 would help. |
RalfJung
added
E-mentor
workingjubilee
added a commit
to workingjubilee/rustc
that referenced
this issue
Jul 14, 2024
…oc, r=Mark-Simulacrum Remove memory leaks in doctests in `core`, `alloc`, and `std` cc `@RalfJung` rust-lang#126067 rust-lang/miri#3670 Should be no actual *documentation* changes[^1], all added/modified lines in the doctests are hidden with `#`, This PR splits the existing memory leaks in doctests in `core`, `alloc`, and `std` into two general categories: 1. "Non-focused" memory leaks that are incidental to the thing being documented, and/or are easy to remove, i.e. they are only there because preventing the leak would make the doctest less clear and/or concise. - These doctests simply have a comment like `# // Prevent leaks for Miri.` above the added line that removes the memory leak. - [^2]Some of these would perhaps be better as part of the public documentation part of the doctest, to clarify that a memory leak can happen if it is not otherwise mentioned explicitly in the documentation (specifically the ones in `(A)Rc::increment_strong_count(_in)`). 2. "Focused" memory leaks that are intentional and documented, and/or are possibly fragile to remove. - These doctests have a `# // FIXME` comment above the line that removes the memory leak, with a note that once `-Zmiri-disable-leak-check` can be applied at test granularity, these tests should be "un-unleakified" and have `-Zmiri-disable-leak-check` enabled. - Some of these are possibly fragile (e.g. unleaking the result of `Vec::leak`) and thus should definitely not be made part of the documentation. This should be all of the leaks currently in `core` and `alloc`. I only found one leak in `std`, and it was in the first category (excluding the modules `@RalfJung` mentioned in rust-lang#126067 , and reducing the number of iterations of [one test](https://github.com/rust-lang/rust/blob/master/library/std/src/sync/once_lock.rs#L49-L94) from 1000 to 10) [^1]: assuming [^2] is not added [^2]: backlink
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this issue
Jul 14, 2024
Rollup merge of rust-lang#127446 - zachs18:miri-stdlib-leaks-core-alloc, r=Mark-Simulacrum Remove memory leaks in doctests in `core`, `alloc`, and `std` cc `@RalfJung` rust-lang#126067 rust-lang/miri#3670 Should be no actual *documentation* changes[^1], all added/modified lines in the doctests are hidden with `#`, This PR splits the existing memory leaks in doctests in `core`, `alloc`, and `std` into two general categories: 1. "Non-focused" memory leaks that are incidental to the thing being documented, and/or are easy to remove, i.e. they are only there because preventing the leak would make the doctest less clear and/or concise. - These doctests simply have a comment like `# // Prevent leaks for Miri.` above the added line that removes the memory leak. - [^2]Some of these would perhaps be better as part of the public documentation part of the doctest, to clarify that a memory leak can happen if it is not otherwise mentioned explicitly in the documentation (specifically the ones in `(A)Rc::increment_strong_count(_in)`). 2. "Focused" memory leaks that are intentional and documented, and/or are possibly fragile to remove. - These doctests have a `# // FIXME` comment above the line that removes the memory leak, with a note that once `-Zmiri-disable-leak-check` can be applied at test granularity, these tests should be "un-unleakified" and have `-Zmiri-disable-leak-check` enabled. - Some of these are possibly fragile (e.g. unleaking the result of `Vec::leak`) and thus should definitely not be made part of the documentation. This should be all of the leaks currently in `core` and `alloc`. I only found one leak in `std`, and it was in the first category (excluding the modules `@RalfJung` mentioned in rust-lang#126067 , and reducing the number of iterations of [one test](https://github.com/rust-lang/rust/blob/master/library/std/src/sync/once_lock.rs#L49-L94) from 1000 to 10) [^1]: assuming [^2] is not added [^2]: backlink
|
This got fixed by #127446 :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Currently a bunch of our doctests have memory leaks. For that reason, we have to disable Miri's leak checker when running doctests.
To reproduce:
@rust-lang/libs how do you feel about that -- is it okay for the tests to leak, or should we fix that (by adding hidden code that deallocates things again)? Currently we don't have a good way to disable the leak checker on a per-test basis so there is a risk of accidental leaks.
The text was updated successfully, but these errors were encountered: