From 3ac1df8b99341fa781aead2f3eeef955309a12f3 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 18 Oct 2020 13:53:54 +0200 Subject: [PATCH 1/7] consider assignments of union field of ManuallyDrop type safe --- compiler/rustc_middle/src/mir/query.rs | 6 +-- .../rustc_mir/src/transform/check_unsafety.rs | 53 ++++++++++--------- src/test/ui/union/union-unsafe.rs | 6 +-- src/test/ui/union/union-unsafe.stderr | 26 +-------- 4 files changed, 35 insertions(+), 56 deletions(-) diff --git a/compiler/rustc_middle/src/mir/query.rs b/compiler/rustc_middle/src/mir/query.rs index db0056e482be0..03faef4d9fa21 100644 --- a/compiler/rustc_middle/src/mir/query.rs +++ b/compiler/rustc_middle/src/mir/query.rs @@ -46,7 +46,7 @@ pub enum UnsafetyViolationDetails { UseOfMutableStatic, UseOfExternStatic, DerefOfRawPointer, - AssignToNonCopyUnionField, + AssignToDroppingUnionField, AccessToUnionField, MutationOfLayoutConstrainedField, BorrowOfLayoutConstrainedField, @@ -94,8 +94,8 @@ impl UnsafetyViolationDetails { "raw pointers may be NULL, dangling or unaligned; they can violate aliasing rules \ and cause data races: all of these are undefined behavior", ), - AssignToNonCopyUnionField => ( - "assignment to non-`Copy` union field", + AssignToDroppingUnionField => ( + "assignment to union field that needs dropping", "the previous content of the field will be dropped, which causes undefined \ behavior if the field was not properly initialized", ), diff --git a/compiler/rustc_mir/src/transform/check_unsafety.rs b/compiler/rustc_mir/src/transform/check_unsafety.rs index acec3e8f82f43..fdc033f9eb3f1 100644 --- a/compiler/rustc_mir/src/transform/check_unsafety.rs +++ b/compiler/rustc_mir/src/transform/check_unsafety.rs @@ -235,37 +235,40 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> { UnsafetyViolationKind::GeneralAndConstFn, UnsafetyViolationDetails::DerefOfRawPointer, ), - ty::Adt(adt, _) => { - if adt.is_union() { - if context == PlaceContext::MutatingUse(MutatingUseContext::Store) - || context == PlaceContext::MutatingUse(MutatingUseContext::Drop) - || context == PlaceContext::MutatingUse(MutatingUseContext::AsmOutput) - { - let elem_ty = match elem { - ProjectionElem::Field(_, ty) => ty, - _ => span_bug!( - self.source_info.span, - "non-field projection {:?} from union?", - place - ), - }; - if !elem_ty.is_copy_modulo_regions( + ty::Adt(adt, _) if adt.is_union() => { + if context == PlaceContext::MutatingUse(MutatingUseContext::Store) + || context == PlaceContext::MutatingUse(MutatingUseContext::Drop) + || context == PlaceContext::MutatingUse(MutatingUseContext::AsmOutput) + { + let elem_ty = match elem { + ProjectionElem::Field(_, ty) => ty, + _ => span_bug!( + self.source_info.span, + "non-field projection {:?} from union?", + place + ), + }; + let manually_drop = elem_ty + .ty_adt_def() + .map_or(false, |adt_def| adt_def.is_manually_drop()); + let nodrop = manually_drop + || elem_ty.is_copy_modulo_regions( self.tcx.at(self.source_info.span), self.param_env, - ) { - self.require_unsafe( - UnsafetyViolationKind::GeneralAndConstFn, - UnsafetyViolationDetails::AssignToNonCopyUnionField, - ) - } else { - // write to non-move union, safe - } - } else { + ); + if !nodrop { self.require_unsafe( UnsafetyViolationKind::GeneralAndConstFn, - UnsafetyViolationDetails::AccessToUnionField, + UnsafetyViolationDetails::AssignToDroppingUnionField, ) + } else { + // write to non-drop union field, safe } + } else { + self.require_unsafe( + UnsafetyViolationKind::GeneralAndConstFn, + UnsafetyViolationDetails::AccessToUnionField, + ) } } _ => {} diff --git a/src/test/ui/union/union-unsafe.rs b/src/test/ui/union/union-unsafe.rs index 10f0c467560f4..9810ed75d5916 100644 --- a/src/test/ui/union/union-unsafe.rs +++ b/src/test/ui/union/union-unsafe.rs @@ -18,7 +18,7 @@ union U4 { fn generic_noncopy() { let mut u3 = U3 { a: ManuallyDrop::new(T::default()) }; - u3.a = ManuallyDrop::new(T::default()); //~ ERROR assignment to non-`Copy` union field is unsafe + u3.a = ManuallyDrop::new(T::default()); // OK (assignment does not drop) *u3.a = T::default(); //~ ERROR access to union field is unsafe } @@ -41,7 +41,7 @@ fn main() { // let U1 { .. } = u1; // OK let mut u2 = U2 { a: ManuallyDrop::new(String::from("old")) }; // OK - u2.a = ManuallyDrop::new(String::from("new")); //~ ERROR assignment to non-`Copy` union + u2.a = ManuallyDrop::new(String::from("new")); // OK (assignment does not drop) *u2.a = String::from("new"); //~ ERROR access to union field is unsafe let mut u3 = U3 { a: ManuallyDrop::new(0) }; // OK @@ -49,6 +49,6 @@ fn main() { *u3.a = 1; //~ ERROR access to union field is unsafe let mut u3 = U3 { a: ManuallyDrop::new(String::from("old")) }; // OK - u3.a = ManuallyDrop::new(String::from("new")); //~ ERROR assignment to non-`Copy` union + u3.a = ManuallyDrop::new(String::from("new")); // OK (assignment does not drop) *u3.a = String::from("new"); //~ ERROR access to union field is unsafe } diff --git a/src/test/ui/union/union-unsafe.stderr b/src/test/ui/union/union-unsafe.stderr index b50d9e1750657..5b3a58814a936 100644 --- a/src/test/ui/union/union-unsafe.stderr +++ b/src/test/ui/union/union-unsafe.stderr @@ -1,11 +1,3 @@ -error[E0133]: assignment to non-`Copy` union field is unsafe and requires unsafe function or block - --> $DIR/union-unsafe.rs:21:5 - | -LL | u3.a = ManuallyDrop::new(T::default()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ assignment to non-`Copy` union field - | - = note: the previous content of the field will be dropped, which causes undefined behavior if the field was not properly initialized - error[E0133]: access to union field is unsafe and requires unsafe function or block --> $DIR/union-unsafe.rs:22:6 | @@ -46,14 +38,6 @@ LL | if let U1 { a: 12 } = u1 {} | = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior -error[E0133]: assignment to non-`Copy` union field is unsafe and requires unsafe function or block - --> $DIR/union-unsafe.rs:44:5 - | -LL | u2.a = ManuallyDrop::new(String::from("new")); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ assignment to non-`Copy` union field - | - = note: the previous content of the field will be dropped, which causes undefined behavior if the field was not properly initialized - error[E0133]: access to union field is unsafe and requires unsafe function or block --> $DIR/union-unsafe.rs:45:6 | @@ -70,14 +54,6 @@ LL | *u3.a = 1; | = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior -error[E0133]: assignment to non-`Copy` union field is unsafe and requires unsafe function or block - --> $DIR/union-unsafe.rs:52:5 - | -LL | u3.a = ManuallyDrop::new(String::from("new")); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ assignment to non-`Copy` union field - | - = note: the previous content of the field will be dropped, which causes undefined behavior if the field was not properly initialized - error[E0133]: access to union field is unsafe and requires unsafe function or block --> $DIR/union-unsafe.rs:53:6 | @@ -86,6 +62,6 @@ LL | *u3.a = String::from("new"); | = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior -error: aborting due to 11 previous errors +error: aborting due to 8 previous errors For more information about this error, try `rustc --explain E0133`. From 64856e29c1c780117348c196e05902476251bc92 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 24 Oct 2020 21:12:32 +0200 Subject: [PATCH 2/7] adjust union access unsafety check logic to take into account Deref and the actual type of the assignment --- .../rustc_mir/src/transform/check_unsafety.rs | 35 +++++++++------- src/test/ui/union/union-unsafe.rs | 21 ++++++++++ src/test/ui/union/union-unsafe.stderr | 42 +++++++++++++++---- 3 files changed, 74 insertions(+), 24 deletions(-) diff --git a/compiler/rustc_mir/src/transform/check_unsafety.rs b/compiler/rustc_mir/src/transform/check_unsafety.rs index fdc033f9eb3f1..e78f8d4c901d3 100644 --- a/compiler/rustc_mir/src/transform/check_unsafety.rs +++ b/compiler/rustc_mir/src/transform/check_unsafety.rs @@ -190,7 +190,7 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> { } } - for (i, elem) in place.projection.iter().enumerate() { + for (i, _elem) in place.projection.iter().enumerate() { let proj_base = &place.projection[..i]; if context.is_borrow() { if util::is_disaligned(self.tcx, self.body, self.param_env, *place) { @@ -236,23 +236,28 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> { UnsafetyViolationDetails::DerefOfRawPointer, ), ty::Adt(adt, _) if adt.is_union() => { - if context == PlaceContext::MutatingUse(MutatingUseContext::Store) + let assign_to_field = context + == PlaceContext::MutatingUse(MutatingUseContext::Store) || context == PlaceContext::MutatingUse(MutatingUseContext::Drop) - || context == PlaceContext::MutatingUse(MutatingUseContext::AsmOutput) - { - let elem_ty = match elem { - ProjectionElem::Field(_, ty) => ty, - _ => span_bug!( - self.source_info.span, - "non-field projection {:?} from union?", - place - ), - }; - let manually_drop = elem_ty + || context == PlaceContext::MutatingUse(MutatingUseContext::AsmOutput); + // If there is a `Deref` further along the projection chain, this is *not* an + // assignment to a union field. In that case the union field is just read to + // obtain the pointer/reference. + let assign_to_field = assign_to_field + && !place.projection[i..] + .iter() + .any(|elem| matches!(elem, ProjectionElem::Deref)); + // If this is just an assignment, determine if the assigned type needs dropping. + if assign_to_field { + // We have to check the actual type of the assignment, as that determines if the + // old value is being dropped. + let assigned_ty = place.ty(&self.body.local_decls, self.tcx).ty; + // To avoid semver hazard, we only consider `Copy` and `ManuallyDrop` non-dropping. + let manually_drop = assigned_ty .ty_adt_def() .map_or(false, |adt_def| adt_def.is_manually_drop()); let nodrop = manually_drop - || elem_ty.is_copy_modulo_regions( + || assigned_ty.is_copy_modulo_regions( self.tcx.at(self.source_info.span), self.param_env, ); @@ -260,7 +265,7 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> { self.require_unsafe( UnsafetyViolationKind::GeneralAndConstFn, UnsafetyViolationDetails::AssignToDroppingUnionField, - ) + ); } else { // write to non-drop union field, safe } diff --git a/src/test/ui/union/union-unsafe.rs b/src/test/ui/union/union-unsafe.rs index 9810ed75d5916..9c7ed1f7bfc3a 100644 --- a/src/test/ui/union/union-unsafe.rs +++ b/src/test/ui/union/union-unsafe.rs @@ -1,4 +1,6 @@ +#![feature(untagged_unions)] use std::mem::ManuallyDrop; +use std::cell::RefCell; union U1 { a: u8 @@ -16,6 +18,25 @@ union U4 { a: T } +union URef { + p: &'static mut i32, +} + +union URefCell { // field that does not drop but is not `Copy`, either + a: (RefCell, i32), +} + +fn deref_union_field(mut u: URef) { + // Not an assignment but an access to the union field! + *(u.p) = 13; //~ ERROR access to union field is unsafe +} + +fn assign_noncopy_union_field(mut u: URefCell) { + u.a = (RefCell::new(0), 1); //~ ERROR assignment to union field that needs dropping + u.a.0 = RefCell::new(0); //~ ERROR assignment to union field that needs dropping + u.a.1 = 1; // OK +} + fn generic_noncopy() { let mut u3 = U3 { a: ManuallyDrop::new(T::default()) }; u3.a = ManuallyDrop::new(T::default()); // OK (assignment does not drop) diff --git a/src/test/ui/union/union-unsafe.stderr b/src/test/ui/union/union-unsafe.stderr index 5b3a58814a936..f80d4394f8425 100644 --- a/src/test/ui/union/union-unsafe.stderr +++ b/src/test/ui/union/union-unsafe.stderr @@ -1,5 +1,29 @@ error[E0133]: access to union field is unsafe and requires unsafe function or block - --> $DIR/union-unsafe.rs:22:6 + --> $DIR/union-unsafe.rs:31:5 + | +LL | *(u.p) = 13; + | ^^^^^^^^^^^ access to union field + | + = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior + +error[E0133]: assignment to union field that needs dropping is unsafe and requires unsafe function or block + --> $DIR/union-unsafe.rs:35:5 + | +LL | u.a = (RefCell::new(0), 1); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ assignment to union field that needs dropping + | + = note: the previous content of the field will be dropped, which causes undefined behavior if the field was not properly initialized + +error[E0133]: assignment to union field that needs dropping is unsafe and requires unsafe function or block + --> $DIR/union-unsafe.rs:36:5 + | +LL | u.a.0 = RefCell::new(0); + | ^^^^^^^^^^^^^^^^^^^^^^^ assignment to union field that needs dropping + | + = note: the previous content of the field will be dropped, which causes undefined behavior if the field was not properly initialized + +error[E0133]: access to union field is unsafe and requires unsafe function or block + --> $DIR/union-unsafe.rs:43:6 | LL | *u3.a = T::default(); | ^^^^ access to union field @@ -7,7 +31,7 @@ LL | *u3.a = T::default(); = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior error[E0133]: access to union field is unsafe and requires unsafe function or block - --> $DIR/union-unsafe.rs:28:6 + --> $DIR/union-unsafe.rs:49:6 | LL | *u3.a = T::default(); | ^^^^ access to union field @@ -15,7 +39,7 @@ LL | *u3.a = T::default(); = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior error[E0133]: access to union field is unsafe and requires unsafe function or block - --> $DIR/union-unsafe.rs:36:13 + --> $DIR/union-unsafe.rs:57:13 | LL | let a = u1.a; | ^^^^ access to union field @@ -23,7 +47,7 @@ LL | let a = u1.a; = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior error[E0133]: access to union field is unsafe and requires unsafe function or block - --> $DIR/union-unsafe.rs:39:14 + --> $DIR/union-unsafe.rs:60:14 | LL | let U1 { a } = u1; | ^ access to union field @@ -31,7 +55,7 @@ LL | let U1 { a } = u1; = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior error[E0133]: access to union field is unsafe and requires unsafe function or block - --> $DIR/union-unsafe.rs:40:20 + --> $DIR/union-unsafe.rs:61:20 | LL | if let U1 { a: 12 } = u1 {} | ^^ access to union field @@ -39,7 +63,7 @@ LL | if let U1 { a: 12 } = u1 {} = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior error[E0133]: access to union field is unsafe and requires unsafe function or block - --> $DIR/union-unsafe.rs:45:6 + --> $DIR/union-unsafe.rs:66:6 | LL | *u2.a = String::from("new"); | ^^^^ access to union field @@ -47,7 +71,7 @@ LL | *u2.a = String::from("new"); = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior error[E0133]: access to union field is unsafe and requires unsafe function or block - --> $DIR/union-unsafe.rs:49:6 + --> $DIR/union-unsafe.rs:70:6 | LL | *u3.a = 1; | ^^^^ access to union field @@ -55,13 +79,13 @@ LL | *u3.a = 1; = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior error[E0133]: access to union field is unsafe and requires unsafe function or block - --> $DIR/union-unsafe.rs:53:6 + --> $DIR/union-unsafe.rs:74:6 | LL | *u3.a = String::from("new"); | ^^^^ access to union field | = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior -error: aborting due to 8 previous errors +error: aborting due to 11 previous errors For more information about this error, try `rustc --explain E0133`. From 63bdb3ac0948683fe7548680d3b78bb5d8b32076 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 26 Oct 2020 19:35:01 +0100 Subject: [PATCH 3/7] improve formatting --- compiler/rustc_mir/src/transform/check_unsafety.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/compiler/rustc_mir/src/transform/check_unsafety.rs b/compiler/rustc_mir/src/transform/check_unsafety.rs index e78f8d4c901d3..e90d5149c2f1e 100644 --- a/compiler/rustc_mir/src/transform/check_unsafety.rs +++ b/compiler/rustc_mir/src/transform/check_unsafety.rs @@ -236,10 +236,14 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> { UnsafetyViolationDetails::DerefOfRawPointer, ), ty::Adt(adt, _) if adt.is_union() => { - let assign_to_field = context - == PlaceContext::MutatingUse(MutatingUseContext::Store) - || context == PlaceContext::MutatingUse(MutatingUseContext::Drop) - || context == PlaceContext::MutatingUse(MutatingUseContext::AsmOutput); + let assign_to_field = matches!( + context, + PlaceContext::MutatingUse( + MutatingUseContext::Store + | MutatingUseContext::Drop + | MutatingUseContext::AsmOutput + ) + ); // If there is a `Deref` further along the projection chain, this is *not* an // assignment to a union field. In that case the union field is just read to // obtain the pointer/reference. From af309cc2d93ce1e8e57cedffbfc35f9df040376a Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 28 Oct 2020 08:57:57 +0100 Subject: [PATCH 4/7] needs -> might need --- compiler/rustc_middle/src/mir/query.rs | 2 +- src/test/ui/union/union-unsafe.rs | 4 ++-- src/test/ui/union/union-unsafe.stderr | 8 ++++---- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/compiler/rustc_middle/src/mir/query.rs b/compiler/rustc_middle/src/mir/query.rs index 03faef4d9fa21..89a93096f1c22 100644 --- a/compiler/rustc_middle/src/mir/query.rs +++ b/compiler/rustc_middle/src/mir/query.rs @@ -95,7 +95,7 @@ impl UnsafetyViolationDetails { and cause data races: all of these are undefined behavior", ), AssignToDroppingUnionField => ( - "assignment to union field that needs dropping", + "assignment to union field that might need dropping", "the previous content of the field will be dropped, which causes undefined \ behavior if the field was not properly initialized", ), diff --git a/src/test/ui/union/union-unsafe.rs b/src/test/ui/union/union-unsafe.rs index 9c7ed1f7bfc3a..6adf0ac59b93c 100644 --- a/src/test/ui/union/union-unsafe.rs +++ b/src/test/ui/union/union-unsafe.rs @@ -32,8 +32,8 @@ fn deref_union_field(mut u: URef) { } fn assign_noncopy_union_field(mut u: URefCell) { - u.a = (RefCell::new(0), 1); //~ ERROR assignment to union field that needs dropping - u.a.0 = RefCell::new(0); //~ ERROR assignment to union field that needs dropping + u.a = (RefCell::new(0), 1); //~ ERROR assignment to union field that might need dropping + u.a.0 = RefCell::new(0); //~ ERROR assignment to union field that might need dropping u.a.1 = 1; // OK } diff --git a/src/test/ui/union/union-unsafe.stderr b/src/test/ui/union/union-unsafe.stderr index f80d4394f8425..a25c09144f742 100644 --- a/src/test/ui/union/union-unsafe.stderr +++ b/src/test/ui/union/union-unsafe.stderr @@ -6,19 +6,19 @@ LL | *(u.p) = 13; | = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior -error[E0133]: assignment to union field that needs dropping is unsafe and requires unsafe function or block +error[E0133]: assignment to union field that might need dropping is unsafe and requires unsafe function or block --> $DIR/union-unsafe.rs:35:5 | LL | u.a = (RefCell::new(0), 1); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^ assignment to union field that needs dropping + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ assignment to union field that might need dropping | = note: the previous content of the field will be dropped, which causes undefined behavior if the field was not properly initialized -error[E0133]: assignment to union field that needs dropping is unsafe and requires unsafe function or block +error[E0133]: assignment to union field that might need dropping is unsafe and requires unsafe function or block --> $DIR/union-unsafe.rs:36:5 | LL | u.a.0 = RefCell::new(0); - | ^^^^^^^^^^^^^^^^^^^^^^^ assignment to union field that needs dropping + | ^^^^^^^^^^^^^^^^^^^^^^^ assignment to union field that might need dropping | = note: the previous content of the field will be dropped, which causes undefined behavior if the field was not properly initialized From df1c55a47434b2d79dea43673a3a97b9e955ce82 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 20 Nov 2020 11:50:15 +0100 Subject: [PATCH 5/7] add function to iterate through all sub-places, and add PlaceRef::ty --- compiler/rustc_middle/src/mir/mod.rs | 9 +++++++++ compiler/rustc_middle/src/mir/tcx.rs | 9 +++++++++ 2 files changed, 18 insertions(+) diff --git a/compiler/rustc_middle/src/mir/mod.rs b/compiler/rustc_middle/src/mir/mod.rs index 9289d4708de1e..4a7d4dab50794 100644 --- a/compiler/rustc_middle/src/mir/mod.rs +++ b/compiler/rustc_middle/src/mir/mod.rs @@ -1741,6 +1741,15 @@ impl<'tcx> Place<'tcx> { pub fn as_ref(&self) -> PlaceRef<'tcx> { PlaceRef { local: self.local, projection: &self.projection } } + + /// Iterate over the projections in evaluation order, i.e., the first element is the base with + /// its projection and then subsequently more projections are added. + pub fn iter_projections(self) -> impl Iterator, PlaceElem<'tcx>)> + DoubleEndedIterator { + self.projection.iter().enumerate().map(move |(i, proj)| { + let base = PlaceRef { local: self.local, projection: &self.projection[..i] }; + (base, proj) + }) + } } impl From for Place<'_> { diff --git a/compiler/rustc_middle/src/mir/tcx.rs b/compiler/rustc_middle/src/mir/tcx.rs index f0bfdae261c64..1b2c1076a6880 100644 --- a/compiler/rustc_middle/src/mir/tcx.rs +++ b/compiler/rustc_middle/src/mir/tcx.rs @@ -136,6 +136,15 @@ impl<'tcx> Place<'tcx> { } } +impl<'tcx> PlaceRef<'tcx> { + pub fn ty(&self, local_decls: &D, tcx: TyCtxt<'tcx>) -> PlaceTy<'tcx> + where + D: HasLocalDecls<'tcx>, + { + Place::ty_from(self.local, &self.projection, local_decls, tcx) + } +} + pub enum RvalueInitializationState { Shallow, Deep, From 571da2c62d0ac6360b78796e764ad5fe0753f553 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 21 Nov 2020 13:20:34 +0100 Subject: [PATCH 6/7] refactor unsafety checking of places --- compiler/rustc_middle/src/mir/mod.rs | 4 +- .../rustc_mir/src/transform/check_unsafety.rs | 159 +++++++++--------- 2 files changed, 87 insertions(+), 76 deletions(-) diff --git a/compiler/rustc_middle/src/mir/mod.rs b/compiler/rustc_middle/src/mir/mod.rs index 4a7d4dab50794..2bd30001a9153 100644 --- a/compiler/rustc_middle/src/mir/mod.rs +++ b/compiler/rustc_middle/src/mir/mod.rs @@ -1744,7 +1744,9 @@ impl<'tcx> Place<'tcx> { /// Iterate over the projections in evaluation order, i.e., the first element is the base with /// its projection and then subsequently more projections are added. - pub fn iter_projections(self) -> impl Iterator, PlaceElem<'tcx>)> + DoubleEndedIterator { + pub fn iter_projections( + self, + ) -> impl Iterator, PlaceElem<'tcx>)> + DoubleEndedIterator { self.projection.iter().enumerate().map(move |(i, proj)| { let base = PlaceRef { local: self.local, projection: &self.projection[..i] }; (base, proj) diff --git a/compiler/rustc_mir/src/transform/check_unsafety.rs b/compiler/rustc_mir/src/transform/check_unsafety.rs index e90d5149c2f1e..e64955c4986ce 100644 --- a/compiler/rustc_mir/src/transform/check_unsafety.rs +++ b/compiler/rustc_mir/src/transform/check_unsafety.rs @@ -181,6 +181,9 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> { self.check_mut_borrowing_layout_constrained_field(*place, context.is_mutating_use()); } + // Check for borrows to packed fields. + // `is_disaligned` already traverses the place to consider all projections after the last + // `Deref`, so this only needs to be called once at the top level. if context.is_borrow() { if util::is_disaligned(self.tcx, self.body, self.param_env, *place) { self.require_unsafe( @@ -190,53 +193,69 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> { } } - for (i, _elem) in place.projection.iter().enumerate() { - let proj_base = &place.projection[..i]; - if context.is_borrow() { - if util::is_disaligned(self.tcx, self.body, self.param_env, *place) { + // Some checks below need the extra metainfo of the local declaration. + let decl = &self.body.local_decls[place.local]; + + // Check the base local: it might be an unsafe-to-access static. We only check derefs of the + // temporary holding the static pointer to avoid duplicate errors + // . + if decl.internal && place.projection.first() == Some(&ProjectionElem::Deref) { + // If the projection root is an artifical local that we introduced when + // desugaring `static`, give a more specific error message + // (avoid the general "raw pointer" clause below, that would only be confusing). + if let Some(box LocalInfo::StaticRef { def_id, .. }) = decl.local_info { + if self.tcx.is_mutable_static(def_id) { self.require_unsafe( - UnsafetyViolationKind::BorrowPacked, - UnsafetyViolationDetails::BorrowOfPackedField, + UnsafetyViolationKind::General, + UnsafetyViolationDetails::UseOfMutableStatic, ); + return; + } else if self.tcx.is_foreign_item(def_id) { + self.require_unsafe( + UnsafetyViolationKind::General, + UnsafetyViolationDetails::UseOfExternStatic, + ); + return; } } - let source_info = self.source_info; - if let [] = proj_base { - let decl = &self.body.local_decls[place.local]; - if decl.internal { - // If the projection root is an artifical local that we introduced when - // desugaring `static`, give a more specific error message - // (avoid the general "raw pointer" clause below, that would only be confusing). - if let Some(box LocalInfo::StaticRef { def_id, .. }) = decl.local_info { - if self.tcx.is_mutable_static(def_id) { - self.require_unsafe( - UnsafetyViolationKind::General, - UnsafetyViolationDetails::UseOfMutableStatic, - ); - return; - } else if self.tcx.is_foreign_item(def_id) { - self.require_unsafe( - UnsafetyViolationKind::General, - UnsafetyViolationDetails::UseOfExternStatic, - ); - return; - } - } else { - // Internal locals are used in the `move_val_init` desugaring. - // We want to check unsafety against the source info of the - // desugaring, rather than the source info of the RHS. - self.source_info = self.body.local_decls[place.local].source_info; - } + } + + // Check for raw pointer `Deref`. + for (base, proj) in place.iter_projections() { + if proj == ProjectionElem::Deref { + let source_info = self.source_info; // Backup source_info so we can restore it later. + if base.projection.is_empty() && decl.internal { + // Internal locals are used in the `move_val_init` desugaring. + // We want to check unsafety against the source info of the + // desugaring, rather than the source info of the RHS. + self.source_info = self.body.local_decls[place.local].source_info; } + let base_ty = base.ty(self.body, self.tcx).ty; + if base_ty.is_unsafe_ptr() { + self.require_unsafe( + UnsafetyViolationKind::GeneralAndConstFn, + UnsafetyViolationDetails::DerefOfRawPointer, + ) + } + self.source_info = source_info; // Restore backed-up source_info. + } + } + + // Check for union fields. For this we traverse right-to-left, as the last `Deref` changes + // whether we *read* the union field or potentially *write* to it (if this place is being assigned to). + let mut saw_deref = false; + for (base, proj) in place.iter_projections().rev() { + if proj == ProjectionElem::Deref { + saw_deref = true; + continue; } - let base_ty = Place::ty_from(place.local, proj_base, self.body, self.tcx).ty; - match base_ty.kind() { - ty::RawPtr(..) => self.require_unsafe( - UnsafetyViolationKind::GeneralAndConstFn, - UnsafetyViolationDetails::DerefOfRawPointer, - ), - ty::Adt(adt, _) if adt.is_union() => { - let assign_to_field = matches!( + + let base_ty = base.ty(self.body, self.tcx).ty; + if base_ty.ty_adt_def().map_or(false, |adt| adt.is_union()) { + // If we did not hit a `Deref` yet and the overall place use is an assignment, the + // rules are different. + let assign_to_field = !saw_deref + && matches!( context, PlaceContext::MutatingUse( MutatingUseContext::Store @@ -244,45 +263,35 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> { | MutatingUseContext::AsmOutput ) ); - // If there is a `Deref` further along the projection chain, this is *not* an - // assignment to a union field. In that case the union field is just read to - // obtain the pointer/reference. - let assign_to_field = assign_to_field - && !place.projection[i..] - .iter() - .any(|elem| matches!(elem, ProjectionElem::Deref)); - // If this is just an assignment, determine if the assigned type needs dropping. - if assign_to_field { - // We have to check the actual type of the assignment, as that determines if the - // old value is being dropped. - let assigned_ty = place.ty(&self.body.local_decls, self.tcx).ty; - // To avoid semver hazard, we only consider `Copy` and `ManuallyDrop` non-dropping. - let manually_drop = assigned_ty - .ty_adt_def() - .map_or(false, |adt_def| adt_def.is_manually_drop()); - let nodrop = manually_drop - || assigned_ty.is_copy_modulo_regions( - self.tcx.at(self.source_info.span), - self.param_env, - ); - if !nodrop { - self.require_unsafe( - UnsafetyViolationKind::GeneralAndConstFn, - UnsafetyViolationDetails::AssignToDroppingUnionField, - ); - } else { - // write to non-drop union field, safe - } - } else { + // If this is just an assignment, determine if the assigned type needs dropping. + if assign_to_field { + // We have to check the actual type of the assignment, as that determines if the + // old value is being dropped. + let assigned_ty = place.ty(&self.body.local_decls, self.tcx).ty; + // To avoid semver hazard, we only consider `Copy` and `ManuallyDrop` non-dropping. + let manually_drop = assigned_ty + .ty_adt_def() + .map_or(false, |adt_def| adt_def.is_manually_drop()); + let nodrop = manually_drop + || assigned_ty.is_copy_modulo_regions( + self.tcx.at(self.source_info.span), + self.param_env, + ); + if !nodrop { self.require_unsafe( UnsafetyViolationKind::GeneralAndConstFn, - UnsafetyViolationDetails::AccessToUnionField, - ) + UnsafetyViolationDetails::AssignToDroppingUnionField, + ); + } else { + // write to non-drop union field, safe } + } else { + self.require_unsafe( + UnsafetyViolationKind::GeneralAndConstFn, + UnsafetyViolationDetails::AccessToUnionField, + ) } - _ => {} } - self.source_info = source_info; } } } From 0bb82c4a059934f78a67ff0ff9e4517171ad1dab Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 28 Nov 2020 18:02:12 +0100 Subject: [PATCH 7/7] expand iter_projections comment --- compiler/rustc_middle/src/mir/mod.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/compiler/rustc_middle/src/mir/mod.rs b/compiler/rustc_middle/src/mir/mod.rs index 2bd30001a9153..62e5a1e18f884 100644 --- a/compiler/rustc_middle/src/mir/mod.rs +++ b/compiler/rustc_middle/src/mir/mod.rs @@ -1744,6 +1744,10 @@ impl<'tcx> Place<'tcx> { /// Iterate over the projections in evaluation order, i.e., the first element is the base with /// its projection and then subsequently more projections are added. + /// As a concrete example, given the place a.b.c, this would yield: + /// - (a, .b) + /// - (a.b, .c) + /// Given a place without projections, the iterator is empty. pub fn iter_projections( self, ) -> impl Iterator, PlaceElem<'tcx>)> + DoubleEndedIterator {