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

Add Box type #141

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add Box type #141

wants to merge 2 commits into from

Conversation

faern
Copy link
Contributor

@faern faern commented May 2, 2020

Adds a loom version of std::boxed::Box. To make it possible/easier to write loom tests that detect memory leaks if the library under test usually uses Box.

However, box is magical, so a normal crate can't replicate its API to 100%. A normal box can be dereferenced so the box is consumed and the value moved back to the stack. Normal crates can't implement this, so I added a Box::into_value(self) -> T method instead.

As you can see, there is one test that is marked with #[ignore]. It's a test that checks if loom correctly catches a memory leak. And it does, but it causes an illegal instruction and the process dies. So #[should_panic] is not enough to capture the panic here sadly. Not sure if it's possible to work around this currently? I guess loom first has to not cause illegal instructions when it detects a memory leak.

@faern
Copy link
Contributor Author

faern commented Jun 19, 2020

Ping. Do you think this type belongs in loom? Loom tries to make it possible to track heap allocations, so it feels quite natural that it would have the most fundamental heap allocation primitive in the standard library.

Loom already has Arc, so Box feels quite natural. Both of those can be used extensively with the into_raw and from_raw. Making it very easy to accidentally leak memory or reference already freed memory. As a result, being able to check that with Loom would be great.

Or, since Loom exposes the raw allocation API, do you think any program/library that uses Loom should always just use the alloc API directly internally and never use Box?

@hawkw hawkw requested a review from carllerche June 19, 2020 16:09
src/boxed.rs Outdated Show resolved Hide resolved
@faern
Copy link
Contributor Author

faern commented Jul 14, 2022

Do you want Box in loom at all? I have left this PR forgotten for a long time now. I still copy this hack-box type into crates where I need Boxes and I want to test the crate with loom. I suppose there are a bunch of improvements and fixes needed before this can get merged. But I won't do those unless the type is going to get merged at some point.

@faern
Copy link
Contributor Author

faern commented Jul 14, 2022

There are a bunch of stuff that this Box is not doing properly. For example handling zero sized types. It looks like it could be a bunch of non-trivial work to make that work as expected.

@Darksonn
Copy link
Contributor

I see that I have reviewed this PR before, but I have no memory of doing so. Regardless, I would be happy to see a Box type in loom so that we may employ its leak-checking. In fact, I recall releasing a version of Tokio with a memory leak that having a loom Box would have detected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants