Skip to content

Commit

Permalink
Auto merge of #115315 - RalfJung:field-capture-packed-alignment, r=ol…
Browse files Browse the repository at this point in the history 
…i-obk

closure field capturing: don't depend on alignment of packed fields

This fixes the closure field capture part of #115305: field capturing always stops at projections into packed structs, no matter the alignment of the field. This means changing a private field type from `u8` to `u64` can never change how closures capture fields, which is probably what we want.

Here's an example where, before this PR, changing the type of a private field in a repr(Rust) struct can change the output of a program:

```rust
#![allow(dead_code)]

mod m {
    // before patch
    #[derive(Default)]
    pub struct S1(u8);
    // after patch
    #[derive(Default)]
    pub struct S2(u64);
}

struct NoisyDrop;
impl Drop for NoisyDrop {
    fn drop(&mut self) {
        eprintln!("dropped!");
    }
}

#[repr(packed)]
struct MyType {
    field: m::S1, // output changes when this becomes S2
    other_field: NoisyDrop,
    third_field: Vec<()>,
}

fn test(r: MyType) {
    let c = || {
        let _val = std::ptr::addr_of!(r.field);
        let _val = r.third_field;
    };
    drop(c);
    eprintln!("before dropping");
}

fn main() {
    test(MyType {
        field: Default::default(),
        other_field: NoisyDrop,
        third_field: Vec::new(),
    });
}
```

Of course this is a breaking change for the same reason that doing field capturing in the first place was a breaking change. Packed fields are relatively rare and depending on drop order is relatively rare, so I don't expect this to have much impact, but it's hard to be sure and even a crater run will only tell us so much.

Also see the [nomination comment](#115315 (comment)).

Cc `@rust-lang/wg-rfc-2229` `@ehuss`
  • Loading branch information
@bors
bors committed Sep 16, 2023
2 parents 635c4a5 + a671127 commit 790309b
Show file tree
Hide file tree
Showing 3 changed files with 22 additions and 53 deletions.
46 changes: 10 additions & 36 deletions compiler/rustc_hir_typeck/src/upvar.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,6 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {

assert_eq!(self.tcx.hir().body_owner_def_id(body.id()), closure_def_id);
let mut delegate = InferBorrowKind {
fcx: self,
closure_def_id,
capture_information: Default::default(),
fake_reads: Default::default(),
Expand Down Expand Up @@ -1607,34 +1606,20 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
/// Truncate the capture so that the place being borrowed is in accordance with RFC 1240,
/// which states that it's unsafe to take a reference into a struct marked `repr(packed)`.
fn restrict_repr_packed_field_ref_capture<'tcx>(
tcx: TyCtxt<'tcx>,
param_env: ty::ParamEnv<'tcx>,
mut place: Place<'tcx>,
mut curr_borrow_kind: ty::UpvarCapture,
) -> (Place<'tcx>, ty::UpvarCapture) {
let pos = place.projections.iter().enumerate().position(|(i, p)| {
let ty = place.ty_before_projection(i);

// Return true for fields of packed structs, unless those fields have alignment 1.
// Return true for fields of packed structs.
match p.kind {
ProjectionKind::Field(..) => match ty.kind() {
ty::Adt(def, _) if def.repr().packed() => {
// We erase regions here because they cannot be hashed
match tcx.layout_of(param_env.and(tcx.erase_regions(p.ty))) {
Ok(layout) if layout.align.abi.bytes() == 1 => {
// if the alignment is 1, the type can't be further
// disaligned.
debug!(
"restrict_repr_packed_field_ref_capture: ({:?}) - align = 1",
place
);
false
}
_ => {
debug!("restrict_repr_packed_field_ref_capture: ({:?}) - true", place);
true
}
}
// We stop here regardless of field alignment. Field alignment can change as
// types change, including the types of private fields in other crates, and that
// shouldn't affect how we compute our captures.
true
}

_ => false,
Expand Down Expand Up @@ -1689,9 +1674,7 @@ fn drop_location_span(tcx: TyCtxt<'_>, hir_id: hir::HirId) -> Span {
tcx.sess.source_map().end_point(owner_span)
}

struct InferBorrowKind<'a, 'tcx> {
fcx: &'a FnCtxt<'a, 'tcx>,

struct InferBorrowKind<'tcx> {
// The def-id of the closure whose kind and upvar accesses are being inferred.
closure_def_id: LocalDefId,

Expand Down Expand Up @@ -1725,7 +1708,7 @@ struct InferBorrowKind<'a, 'tcx> {
fake_reads: Vec<(Place<'tcx>, FakeReadCause, hir::HirId)>,
}

impl<'a, 'tcx> euv::Delegate<'tcx> for InferBorrowKind<'a, 'tcx> {
impl<'tcx> euv::Delegate<'tcx> for InferBorrowKind<'tcx> {
fn fake_read(
&mut self,
place: &PlaceWithHirId<'tcx>,
Expand All @@ -1740,12 +1723,7 @@ impl<'a, 'tcx> euv::Delegate<'tcx> for InferBorrowKind<'a, 'tcx> {

let (place, _) = restrict_capture_precision(place.place.clone(), dummy_capture_kind);

let (place, _) = restrict_repr_packed_field_ref_capture(
self.fcx.tcx,
self.fcx.param_env,
place,
dummy_capture_kind,
);
let (place, _) = restrict_repr_packed_field_ref_capture(place, dummy_capture_kind);
self.fake_reads.push((place, cause, diag_expr_id));
}

Expand Down Expand Up @@ -1780,12 +1758,8 @@ impl<'a, 'tcx> euv::Delegate<'tcx> for InferBorrowKind<'a, 'tcx> {
// We only want repr packed restriction to be applied to reading references into a packed
// struct, and not when the data is being moved. Therefore we call this method here instead
// of in `restrict_capture_precision`.
let (place, mut capture_kind) = restrict_repr_packed_field_ref_capture(
self.fcx.tcx,
self.fcx.param_env,
place_with_id.place.clone(),
capture_kind,
);
let (place, mut capture_kind) =
restrict_repr_packed_field_ref_capture(place_with_id.place.clone(), capture_kind);

// Raw pointers don't inherit mutability
if place_with_id.place.deref_tys().any(Ty::is_unsafe_ptr) {
Expand Down
10 changes: 5 additions & 5 deletions tests/ui/closures/2229_closure_analysis/repr_packed.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
#![feature(rustc_attrs)]

// `u8` aligned at a byte and are unaffected by repr(packed).
// Therefore we can precisely (and safely) capture references to both the fields.
// Therefore we *could* precisely (and safely) capture references to both the fields,
// but we don't, since we don't want capturing to change when field types change alignment.
fn test_alignment_not_affected() {
#[repr(packed)]
struct Foo { x: u8, y: u8 }
Expand All @@ -17,11 +18,10 @@ fn test_alignment_not_affected() {
//~^ ERROR: First Pass analysis includes:
//~| ERROR: Min Capture analysis includes:
let z1: &u8 = &foo.x;
//~^ NOTE: Capturing foo[(0, 0)] -> ImmBorrow
//~| NOTE: Min Capture foo[(0, 0)] -> ImmBorrow
//~^ NOTE: Capturing foo[] -> ImmBorrow
let z2: &mut u8 = &mut foo.y;
//~^ NOTE: Capturing foo[(1, 0)] -> MutBorrow
//~| NOTE: Min Capture foo[(1, 0)] -> MutBorrow
//~^ NOTE: Capturing foo[] -> MutBorrow
//~| NOTE: Min Capture foo[] -> MutBorrow

*z2 = 42;

Expand Down
19 changes: 7 additions & 12 deletions tests/ui/closures/2229_closure_analysis/repr_packed.stderr
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error[E0658]: attributes on expressions are experimental
--> $DIR/repr_packed.rs:13:17
--> $DIR/repr_packed.rs:14:17
|
LL | let mut c = #[rustc_capture_analysis]
| ^^^^^^^^^^^^^^^^^^^^^^^^^
Expand All @@ -26,7 +26,7 @@ LL | let c = #[rustc_capture_analysis]
= help: add `#![feature(stmt_expr_attributes)]` to the crate attributes to enable

error: First Pass analysis includes:
--> $DIR/repr_packed.rs:16:5
--> $DIR/repr_packed.rs:17:5
|
LL | / || {
LL | |
Expand All @@ -37,19 +37,19 @@ LL | | println!("({}, {})", z1, z2);
LL | | };
| |_____^
|
note: Capturing foo[(0, 0)] -> ImmBorrow
--> $DIR/repr_packed.rs:19:24
note: Capturing foo[] -> ImmBorrow
--> $DIR/repr_packed.rs:20:24
|
LL | let z1: &u8 = &foo.x;
| ^^^^^
note: Capturing foo[(1, 0)] -> MutBorrow
note: Capturing foo[] -> MutBorrow
--> $DIR/repr_packed.rs:22:32
|
LL | let z2: &mut u8 = &mut foo.y;
| ^^^^^

error: Min Capture analysis includes:
--> $DIR/repr_packed.rs:16:5
--> $DIR/repr_packed.rs:17:5
|
LL | / || {
LL | |
Expand All @@ -60,12 +60,7 @@ LL | | println!("({}, {})", z1, z2);
LL | | };
| |_____^
|
note: Min Capture foo[(0, 0)] -> ImmBorrow
--> $DIR/repr_packed.rs:19:24
|
LL | let z1: &u8 = &foo.x;
| ^^^^^
note: Min Capture foo[(1, 0)] -> MutBorrow
note: Min Capture foo[] -> MutBorrow
--> $DIR/repr_packed.rs:22:32
|
LL | let z2: &mut u8 = &mut foo.y;
Expand Down

0 comments on commit 790309b

Please sign in to comment.