Add support for trait objects in make_collectable

[jacob-hughes]

This is a surprisingly small PR but with very big implications (hence the large block comment).

This is a draft PR because I'm not sure if the benefit of allowing the NoFinalize optimisation on types containing trait objects is worth the risk that it introduces. I'd like to know your thoughts before merging.

[jacob-hughes]

The block comment in compiler/rustc_codegen_llvm/src/intrinsic_util.rs describes the issue. I'm happy to elaborate if it isn't clear.

[ltratt]

In other words, every concrete value used as a trait object must have the same memory layout

This sounds like it's a non-starter to me?

But, perhaps, we can solve this by benchmarking yksom with and without this change? If it doesn't make much difference we can forget about it. If it does make a difference then we might have some thinking to do!

[jacob-hughes]

But, perhaps, we can solve this by benchmarking yksom with and without this change? If it doesn't make much difference we can forget about it. If it does make a difference then we might have some thinking to do!

This change is motivated by yksom. Almost all VM objects in yksom are allocated with the type Gc<&dyn Obj> (actually it's a ThinObj using natrob, but that's not important). If we blanket ban the NoFinalize optimisation for all trait objects, we will see pretty much no benefit in yksom.

However, not all hope is lost. There's a couple of observations I've noticed in yksom that mean this isn't the end of the world:

1. Most &dyn Obj trait objects point to other GC objects in the SOM object graph, so those will behave as expected
2. Some of these &dyn Obj trait objects point to other GC objects, but through intermediate non-collectable allocations, i.e. the concrete type MethodsArray has a field store: UnsafeCell<Vec<Gc<&dyn Obj>>>. These are the tricky ones. When we're doing this optimisation, we only know that it is a &dyn Obj, we don't know anything about the fields in MethodsArray. A solution I can think of to this is to make sure that the Vec<T, A> in MethodsArray uses A = GcAllocator. This would be a yksom specific change though, and is a bit of a leaky abstraction.

[ltratt]

@jacob-hughes what's the plan with this PR now?

[jacob-hughes]

Closing this because in favour of a new PR because in its current form I think it's too dangerous