std: Fix Rc with Gc inside of it

[alexcrichton]

The code in Rc assumes that the inner box is allocated on the global exchange
heap, which is not always true. If T has managed pointers inside of it, it's
allocate on the local heap instead.

This commit reconciles these two modes of T by carefully interacting with the
inner box (with special treatment of its deallocation).

Closes #11532

[thestinger]

I already wrote the code to do this and you didn't want me to include in my pull request...

There's no reason for unique pointers to ever be allocated with local_malloc now. It was added as a form of future-proofing when it was assumed that a garbage collector would require it, as Graydon's did, but that's no longer the plan. It makes no sense to add a bunch of complexity to support something senseless that's going to be replaced very soon. Until a garbage collector is implemented, Gc<T> lacks a use case anyway.

[alexcrichton]

I do not remember you writing any code that looks like this. You had a new NonManaged bound which explicitly disallowed this pattern. I don't recall anything which actually handled the case of pointers that have different structures. You may have alluded to it and talked about it, but you never put such code up for review once we agreed on what to do about weak pointers.

There may not be a reason to use local_malloc, but that's what we're living with today, so we have to deal with it. This is not "a bunch of complexity". This is two helper functions which are specially crafted (and then used everywhere) and then another special function used because the value is dropped before the box. The is the same dances that local_data did a long time ago.

There most certainly is a good reason to add this and it's memory safety! This is entirely memory unsafe if you're using a Gc in an Rc and attempting to get a reference to this. Our options are to add a NonManaged kind which is a huge amount more complexity than a few functions here or to fix it locally (like this).

Depending on how Gc is updated in the future, this will have to stay in sync, but that's the point of regression tests.

[thestinger]

@alexcrichton: I told you several times that I had added the necessary transmutes to support this. I also discovered some take glue issues along the way that are still present here but it's probably not specific to Rc.

There may not be a reason to use local_malloc, but that's what we're living with today, so we have to deal with it.

It wouldn't be hard to remove the code paths I added in the pull request removing the headers from some but not all unique boxes. That's the correct fix here since there's no way these headers are actually going to be used by the garbage collector and it adds a lot of extra code to vec.rs.

There most certainly is a good reason to add this and it's memory safety! This is entirely memory unsafe if you're using a Gc in an Rc and attempting to get a reference to this

Just remove the Gc<T> stub I added or mark it as experimental. It's entirely useless and I only submitted it because Rc<T> used to have strict type bounds. It's going to mislead people into thinking a garbage collector is implemented and they can use it without worrying about reference cycles.

It's also memory-unsafe with the shrink_to_fit method as I reported a while ago and another dozen additional methods that I've kept track of. I could open an issue report for each one... or we could just remove the useless header code paths from librustc and the standard library.

[thestinger]

Can I submit a pull request removing unique pointer/vector headers completely?

[alexcrichton]

You're not discussing the issue at hand any more. You're describing problems with the rest of the compiler. Whether the headers are needed right now or not is irrelevant.

I would personally be ok with removing headers, but you should talk to @pnkfelix first (and not in this bug).

[huonw]

Can I submit a pull request removing unique pointer/vector headers completely?

Will this break the bootstrap? (Since rustc is so @-loving.)

[thestinger]

@huonw: Nope, they're not used. I previously removed all of the headers and then added branches to put them back on managed-unique because Graydon's garbage collector required it.

[thestinger]

@alexcrichton: I don't think it's irrelevant, because it would make this unnecessary and fix the unique vector methods. I really don't think we should add more special cases like the ones in vec.rs to support something that has been obsolete for a year.

[alexcrichton]

Again, you're not discussing the issue at hand, you continue to go on tangents. As far as I know, @pnkfelix is working on the new GC, so he'd know whether we want to keep these headers or not. Please continue this discussion with him.

Yes, I agree that this patch doesn't matter if we don't have headers. No, we're not removing headers right now (pending talks with others). Please continue this discussion about removing headers in a different location (or make a PR to discuss on).

[thestinger]

It's not a tangent. There's no reason to add more special cases if we just remove the cause of these special cases. A workaround for an easily fixable issue that's going to just need to be removed too doesn't make sense to me.

[bill-myers]

This is better than the current situation, but it seems potentially problematic to drop a box with a different size, since the allocator could very well rely on knowing the allocation size on both alloc and free (for instance, larger allocations could be performed by directly calling mmap and munmap, and the size must be passed to munmap and adding an header could waste a whole page).

There is also another issue: tracing RcBox with an hypotetical tracing GC is currently broken, because the GC doesn't know whether the contents are alive or not and thus will either always or never trace them, which are both wrong.

I think what ideally needs to be done is to have a "StrongBox" type that encapsulates the data and strong reference count, stores the data as an [u8, ..sizeof(T)] and manually implements tracing and dropping correctly, knowing that the data is only constructed if the reference count is != 0 (as well as providing try_borrow, try_add_ref, release, etc.)

[alexcrichton]

At this point, I'd rather punt on all "hypothetical GC concerns" because there are a lot of concerns. I agree that dropping something of a different type right is disturbing, but nearly everything in here is dependent on the implementation of the allocator anyway.

Regardless, I'm gonna go ahead and close this in favor of #11535 (although I do like removing all of the unsafe blocks...)

[huonw]

The GC tracing will be fixed by having a user implementable trait to control tracing, similar to Drop.