diff --git a/compiler/rustc_middle/src/mir/mod.rs b/compiler/rustc_middle/src/mir/mod.rs index b54930e8d5955..ad48c3510484b 100644 --- a/compiler/rustc_middle/src/mir/mod.rs +++ b/compiler/rustc_middle/src/mir/mod.rs @@ -1756,6 +1756,21 @@ 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. + /// 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 { + 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/query.rs b/compiler/rustc_middle/src/mir/query.rs index db0056e482be0..89a93096f1c22 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 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/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, diff --git a/compiler/rustc_mir/src/transform/check_unsafety.rs b/compiler/rustc_mir/src/transform/check_unsafety.rs index acec3e8f82f43..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,87 +193,105 @@ 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. } - 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() { - 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( - 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 { - self.require_unsafe( - UnsafetyViolationKind::GeneralAndConstFn, - UnsafetyViolationDetails::AccessToUnionField, - ) - } + } + + // 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 = 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 + | MutatingUseContext::Drop + | MutatingUseContext::AsmOutput + ) + ); + // 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 { + self.require_unsafe( + UnsafetyViolationKind::GeneralAndConstFn, + UnsafetyViolationDetails::AccessToUnionField, + ) } - _ => {} } - self.source_info = source_info; } } } diff --git a/src/test/ui/union/union-unsafe.rs b/src/test/ui/union/union-unsafe.rs index 10f0c467560f4..6adf0ac59b93c 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,9 +18,28 @@ 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 might need dropping + u.a.0 = RefCell::new(0); //~ ERROR assignment to union field that might need dropping + u.a.1 = 1; // OK +} + 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 +62,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 +70,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..a25c09144f742 100644 --- a/src/test/ui/union/union-unsafe.stderr +++ b/src/test/ui/union/union-unsafe.stderr @@ -1,13 +1,29 @@ -error[E0133]: assignment to non-`Copy` union field is unsafe and requires unsafe function or block - --> $DIR/union-unsafe.rs:21:5 +error[E0133]: access to union field is unsafe and requires unsafe function or block + --> $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 might need dropping is unsafe and requires unsafe function or block + --> $DIR/union-unsafe.rs:35:5 | -LL | u3.a = ManuallyDrop::new(T::default()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ assignment to non-`Copy` union field +LL | u.a = (RefCell::new(0), 1); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ 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 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 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]: access to union field is unsafe and requires unsafe function or block - --> $DIR/union-unsafe.rs:22:6 + --> $DIR/union-unsafe.rs:43:6 | LL | *u3.a = T::default(); | ^^^^ access to union field @@ -15,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 @@ -23,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 @@ -31,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 @@ -39,23 +55,15 @@ 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 | = 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 + --> $DIR/union-unsafe.rs:66:6 | LL | *u2.a = String::from("new"); | ^^^^ access to union field @@ -63,23 +71,15 @@ 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 | = 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 + --> $DIR/union-unsafe.rs:74:6 | LL | *u3.a = String::from("new"); | ^^^^ access to union field