loom icon indicating copy to clipboard operation
loom copied to clipboard

Add Box type

Open faern opened this issue 5 years ago • 4 comments
trafficstars

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 avatar May 02 '20 13:05 faern

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?

faern avatar Jun 19 '20 11:06 faern

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 avatar Jul 14 '22 10:07 faern

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.

faern avatar Jul 14 '22 11:07 faern

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.

Darksonn avatar Jul 14 '22 13:07 Darksonn