Skip to content

Commit

Permalink
Perform FSA on a GC entry point's return type
Browse files Browse the repository at this point in the history
Previously, when we encountered a GC FSA entry-point (e.g. `Gc::new`) we
performed FSA on the argument type of that entry point. When `Gc::new`
was our only entry point, this worked fine, because it would always
resolve to the `T` in `Gc<T>`.

However, with the addition the of `Gc::from` conversion trait entry
point, we can end up being overly conservative with FSA for no good
reason. Consider the following:

```rust
    let x: Gc<HasRef> = Gc::from(Box::new(HasRef::default()));
```

Where the argument type to this entry-point is `Box<HasRef>`, but the
actual constructed `Gc` type is `Gc<HasRef>`. Using our previous
approach, we would have to perform FSA on `Box<HasRef>`, even though the
`Box` is never used in the context of GC. On large codebases, this lead
to FSA rejecting sound `Gc<T>`s.

This commit therefore tweaks FSA to check the return type of an
entry-point, which should always be some `Gc<T>` (we assert! this,
because if it isn't, something has gone very wrong.)
  • Loading branch information
jacob-hughes committed Nov 11, 2024
1 parent 8752468 commit bf6e042
Show file tree
Hide file tree
Showing 5 changed files with 124 additions and 28 deletions.
8 changes: 8 additions & 0 deletions compiler/rustc_middle/src/ty/sty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2025,6 +2025,14 @@ impl<'tcx> Ty<'tcx> {
}
}

/// Panics if called on any type other than `Gc<T>`.
pub fn gced_ty(self, tcx: TyCtxt<'tcx>) -> Ty<'tcx> {
match self.kind() {
Adt(_, args) if self.is_gc(tcx) => args.type_at(0),
_ => bug!("`gced_ty` is called on non-GC type {:?}", self),
}
}

/// A scalar type is one that denotes an atomic datum, with no sub-components.
/// (A RawPtr is scalar because it represents a non-managed pointer, so its
/// contents are abstract to rustc.)
Expand Down
56 changes: 29 additions & 27 deletions compiler/rustc_mir_transform/src/check_finalizers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@ impl<'tcx> MirPass<'tcx> for CheckFinalizers {
continue;
};

let ret_ty = fn_ty.fn_sig(tcx).output().skip_binder();

// The following is a gross hack for performance reasons!
//
// Calls in MIR which are trait method invocations point to the DefId
Expand All @@ -112,23 +114,24 @@ impl<'tcx> MirPass<'tcx> for CheckFinalizers {
// resolve fn calls to their precise instance when they actually are some kind
// of `Gc` constructor (we still check for the attribute later on to make sure
// though!).
if !in_std_lib(tcx, *fn_did) || !fn_ty.fn_sig(tcx).output().skip_binder().is_gc(tcx) {
continue;
}
let mono_fn_did = ty::Instance::resolve(tcx, param_env, *fn_did, substs)
.unwrap()
.unwrap()
.def
.def_id();
if !tcx.has_attr(mono_fn_did, sym::rustc_fsa_entry_point) {
// Skip over any call that's not marked #[rustc_fsa_entry_point]
if !in_std_lib(tcx, *fn_did)
|| !ret_ty.is_gc(tcx)
|| ty::Instance::expect_resolve(tcx, param_env, *fn_did, substs)
.def
.get_attrs(tcx, sym::rustc_fsa_entry_point)
.next()
.is_none()
{
continue;
}

assert_eq!(args.len(), 1);
let arg_ty = args[0].node.ty(body, tcx);
FSAEntryPointCtxt::new(source_info.span, args[0].span, arg_ty, tcx, param_env)
.check_drop_glue();
FSAEntryPointCtxt::new(
source_info.span,
args[0].span,
ret_ty.gced_ty(tcx),
tcx,
param_env,
)
.check_drop_glue();
}
}
}
Expand All @@ -140,9 +143,8 @@ struct FSAEntryPointCtxt<'tcx> {
fn_span: Span,
/// Span of the argument to the entry point.
arg_span: Span,
/// Type of the arg to the entry point. This could be deduced from the field above but it is
/// inconvenient.
arg_ty: Ty<'tcx>,
/// Type of the GC'd value created by the entry point.
value_ty: Ty<'tcx>,
tcx: TyCtxt<'tcx>,
param_env: ParamEnv<'tcx>,
}
Expand All @@ -151,29 +153,29 @@ impl<'tcx> FSAEntryPointCtxt<'tcx> {
fn new(
fn_span: Span,
arg_span: Span,
arg_ty: Ty<'tcx>,
value_ty: Ty<'tcx>,
tcx: TyCtxt<'tcx>,
param_env: ParamEnv<'tcx>,
) -> Self {
Self { fn_span, arg_span, arg_ty, tcx, param_env }
Self { fn_span, arg_span, value_ty, tcx, param_env }
}

fn check_drop_glue(&self) {
if !self.arg_ty.needs_finalizer(self.tcx, self.param_env)
|| self.arg_ty.is_finalize_unchecked(self.tcx)
if !self.value_ty.needs_finalizer(self.tcx, self.param_env)
|| self.value_ty.is_finalize_unchecked(self.tcx)
{
return;
}

if self.arg_ty.is_send(self.tcx, self.param_env)
&& self.arg_ty.is_sync(self.tcx, self.param_env)
&& self.arg_ty.is_finalizer_safe(self.tcx, self.param_env)
if self.value_ty.is_send(self.tcx, self.param_env)
&& self.value_ty.is_sync(self.tcx, self.param_env)
&& self.value_ty.is_finalizer_safe(self.tcx, self.param_env)
{
return;
}

let mut errors = Vec::new();
let mut tys = vec![self.arg_ty];
let mut tys = vec![self.value_ty];

loop {
let Some(ty) = tys.pop() else {
Expand Down Expand Up @@ -383,7 +385,7 @@ impl<'tcx> FSAEntryPointCtxt<'tcx> {
}
err.span_label(
self.fn_span,
format!("caused by trying to construct a `Gc<{}>` here.", self.arg_ty),
format!("caused by trying to construct a `Gc<{}>` here.", self.value_ty),
);
err.emit();
}
Expand Down
2 changes: 1 addition & 1 deletion library/std/src/gc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -683,7 +683,7 @@ impl<T> From<T> for Gc<T> {
/// ```
#[cfg_attr(not(bootstrap), rustc_fsa_entry_point)]
fn from(t: T) -> Self {
Gc::new(t)
unsafe { Gc::new_internal(t) }
}
}

Expand Down
27 changes: 27 additions & 0 deletions tests/ui/static/gc/fsa/gc_from.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
#![feature(gc)]
#![feature(negative_impls)]
#![allow(dead_code)]
include!{"./auxiliary/types.rs"}

impl<'a> Drop for HasRef<'a> {
fn drop(&mut self) {
use_val(self.a); // should fail
}
}

fn main() {
let _: Gc<HasRef> = Gc::from(HasRef::default());
//~^ ERROR: The drop method for `HasRef<'_>` cannot be safely finalized.
let _: Gc<HasRef> = Gc::from(Box::new(HasRef::default()));
//~^ ERROR: The drop method for `HasRef<'_>` cannot be safely finalized.
let _: Gc<[HasRef]> = Gc::from(vec![HasRef::default()]);
//~^ ERROR: The drop method for `HasRef<'_>` cannot be safely finalized.
let _: Gc<[HasRef]> = Gc::from(vec![HasRef::default()].into_boxed_slice());
//~^ ERROR: The drop method for `HasRef<'_>` cannot be safely finalized.

// The following should all pass.
let _: Gc<u8> = Gc::from(1);
let _: Gc<u8> = Gc::from(Box::new(1));
let _: Gc<[u8]> = Gc::from(vec![1, 2, 3]);
let _: Gc<[u8]> = Gc::from(vec![1, 2, 3].into_boxed_slice());
}
59 changes: 59 additions & 0 deletions tests/ui/static/gc/fsa/gc_from.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
error: The drop method for `HasRef<'_>` cannot be safely finalized.
--> $DIR/gc_from.rs:13:34
|
LL | use_val(self.a); // should fail
| ------
| |
| a finalizer cannot safely dereference this `&u64`
| because it might not live long enough.
...
LL | let _: Gc<HasRef> = Gc::from(HasRef::default());
| ---------^^^^^^^^^^^^^^^^^- caused by trying to construct a `Gc<HasRef<'_>>` here.
|
= help: `Gc` may run finalizers after the valid lifetime of this reference.

error: The drop method for `HasRef<'_>` cannot be safely finalized.
--> $DIR/gc_from.rs:15:34
|
LL | use_val(self.a); // should fail
| ------
| |
| a finalizer cannot safely dereference this `&u64`
| because it might not live long enough.
...
LL | let _: Gc<HasRef> = Gc::from(Box::new(HasRef::default()));
| ---------^^^^^^^^^^^^^^^^^^^^^^^^^^^- caused by trying to construct a `Gc<HasRef<'_>>` here.
|
= help: `Gc` may run finalizers after the valid lifetime of this reference.

error: The drop method for `HasRef<'_>` cannot be safely finalized.
--> $DIR/gc_from.rs:17:36
|
LL | use_val(self.a); // should fail
| ------
| |
| a finalizer cannot safely dereference this `&u64`
| because it might not live long enough.
...
LL | let _: Gc<[HasRef]> = Gc::from(vec![HasRef::default()]);
| ---------^^^^^^^^^^^^^^^^^^^^^^^- caused by trying to construct a `Gc<[HasRef<'_>]>` here.
|
= help: `Gc` may run finalizers after the valid lifetime of this reference.
= note: this error originates in the macro `vec` (in Nightly builds, run with -Z macro-backtrace for more info)

error: The drop method for `HasRef<'_>` cannot be safely finalized.
--> $DIR/gc_from.rs:19:36
|
LL | use_val(self.a); // should fail
| ------
| |
| a finalizer cannot safely dereference this `&u64`
| because it might not live long enough.
...
LL | let _: Gc<[HasRef]> = Gc::from(vec![HasRef::default()].into_boxed_slice());
| ---------^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- caused by trying to construct a `Gc<[HasRef<'_>]>` here.
|
= help: `Gc` may run finalizers after the valid lifetime of this reference.

error: aborting due to 4 previous errors

0 comments on commit bf6e042

Please sign in to comment.