Allow types containing references to have finalizers

[jacob-hughes]

Previously FSA prevented Gc<T>s from having a finalizer if T (directly or indirectly) contained a reference. This made retro-fitting GC to large libraries difficult. It also, somewhat surprisingly, completely prevents finalizable nested Gc<dyn Trait>s (due to the underlying vptr being a &'static reference).

This commit loosens this restriction, allowing FSA to interrogate drop method bodies (much the same as for Send + Sync + FinalizerSafe) looking for unsound uses of references.

This change is based on the following observation: a reference inside of T's drop method is safe to dereference provided it never originates from T or one of T's fields. For example:

fn drop(&mut self) {
    let a = 123;

    let r1 = &1; // const with 'static lifetime
    let r2 = &a; // stack-local
    let r3 = &*(unsafe { ... }); // unsafe code, all bets are off.

    let r4: &u64 = self.a;
    let r5: &u64 = self.b.a;
    let r6: &u64 = self.c[0]; // c: [&u64; 2]
}

In this example, r1, r2, and r3 are allowed by FSA: r1 and r2 are sound because their referent will always be valid when the finalizer is called. r3 uses unsafe code, so FSA gets out of the way here, as it is up to the user to ensure that this is sound themselves.

r4, r5, and r6 on the other hand are not allowed by FSA. This is because they are references which are obtained through field or array index projections into T. Such projections are nearly always unsound, so we always prevent these.

This change means that we can remove the ReferenceFree trait, since we now always check for references inside a drop method. As with our relaxed FSA rules for FinalizerSafe and friends -- if for any reason we can't see into a type, we err on the side of caution and emit an error.

[ltratt]

What does this do if I have something like:

fn drop(&mut self) {
  let x = self.f();
}

fn f(&self) -> &T {
  &self.blah
}

?

[jacob-hughes]

This is not allowed as of this PR because we can't look through self.f() and see what's going on inside. However, the part two PR allows FSA to look through function calls. In which case your example would be allowed or denied based on whether self.blah is a valid projection.

[jacob-hughes]

I've pushed a couple of commits so that this gives us the desired behaviour. The trick is in 7bd00ae, where we negatively impl FinalizerSafe for references, so that we get the viral properties for them inside FSA just like for Send and Sync. I've also tested this on our various benchmark suite and the behaviour is as expected.

[jacob-hughes]

If you're happy, this is ready to squash.

[ltratt]

Please squash.

[jacob-hughes]

Squashed

[jacob-hughes]

Fixed here 61a644a

[ltratt]

Please squash.

[jacob-hughes]

Squashed

[jacob-hughes]

Fixed here 75bf832

[ltratt]

Please squash.

[jacob-hughes]

Squashed

[jacob-hughes]

We can give this one another kick once softdevteam/yksom#232 lands.

[ltratt]

Please (a) rebase against master (c) remove the exit 0 from .buildbot.sh.

[jacob-hughes]

Not sure I understand? There's no need to rebase this, it should just merge now that softdevteam/yksom#232 has landed. I'll raise a separate PR in yksom to remove the exit 0.

And was there supposed to be a (b)? ;)

[ltratt]

Oops! (b) was "remove exit 0 from .buildbot.sh. You'll have to rebase to be able to do that; if you don't, this PR will merge without any meaningful CI!

[jacob-hughes]

This is the Alloy PR though, there is no exit 0 in .buildbot.sh, that was added to yksom. If we revert the exit 0 in yksom now before this lands, then it won't build again. The steps as I see them are:

a) merge this.
b) raise a yksom PR which reverts exit 0 in its CI.

Unless I'm clearly missing something?

[ltratt]

Oops -- I misread the other one. Sorry!