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

Throwing a panic via core::panic reports memory leak #1071

Closed
RalfJung opened this issue Nov 23, 2019 · 8 comments · Fixed by rust-lang/rust#66844
Closed

Throwing a panic via core::panic reports memory leak #1071

RalfJung opened this issue Nov 23, 2019 · 8 comments · Fixed by rust-lang/rust#66844
Labels
A-interpreter Area: affects the core interpreter A-panics Area: affects panics and unwinding C-bug Category: This is a bug.

Comments

@RalfJung
Copy link
Member

RalfJung commented Nov 23, 2019

When a #[should_panic] test throws via core::panic! instead of (the default) std::panic!, we have a memory leak:

Alloc 23790: 00 00 00 00 00 00 00 00 0d 00 00 00 00 00 00 00 35 00 00 00 05 00 00 00 (24 bytes, alignment 8) (stack)
             └───────(23789)───────┘ 
Alloc 23789: 74 65 73 74 73 2f 74 65 73 74 2e 72 73 (13 bytes, alignment 1) (Static)

The 2nd allocation is the string tests/test.rs and 0x35 is 53, the line with the panic statement; this looks like some kind of panic information struct.

The interesting question is, why does a stack allocation not get freed?

Cc @Aaron1011

@RalfJung RalfJung added A-interpreter Area: affects the core interpreter A-panics Area: affects panics and unwinding C-bug Category: This is a bug. labels Nov 23, 2019
@RalfJung
Copy link
Member Author

This can easily be reproduced by adjusting our catch_panic test to use core::panic!.

@RalfJung
Copy link
Member Author

#1073 contains some commented-out tests for this.

@oli-obk
Copy link
Contributor

oli-obk commented Nov 24, 2019

If we create a stack allocation without putting it into a frame's locals list, it will get leaked

@RalfJung
Copy link
Member Author

@bjorn3
Copy link
Member

bjorn3 commented Nov 28, 2019

That should be treated like a const, right?

@RalfJung
Copy link
Member Author

Not sure what you mean? Ideally it should probably be interned but I am not sure what the best way is to achieve that.

@bjorn3
Copy link
Member

bjorn3 commented Nov 28, 2019

I mean it should be treated like const LOC_AT_THIS_POS: &'static Location = ....

@RalfJung
Copy link
Member Author

Yeah that would be achieved by interning the result.

I went with rust-lang/rust#66844 instead as that seemed simpler; I was not sure how to best achieve interning here.

RalfJung added a commit to RalfJung/rust that referenced this issue Nov 29, 2019
…i-obk

Miri: do not consider memory allocated by caller_location leaked

Fixes rust-lang/miri#1071

r? @oli-obk

I am not sure if this is the best approach, but it certainly is the easiest.
bors added a commit that referenced this issue Dec 1, 2019
test more panics

Add some tests for #1071.

Blocked on rust-lang/rust#66844.
bors added a commit that referenced this issue Dec 1, 2019
test more panics

Add some tests for #1071.

Blocked on rust-lang/rust#66844.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-interpreter Area: affects the core interpreter A-panics Area: affects panics and unwinding C-bug Category: This is a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants