Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

consider assignments of union field of ManuallyDrop type safe #78068

Merged
merged 7 commits into from
Dec 15, 2020
Merged

consider assignments of union field of ManuallyDrop type safe #78068

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3ac1df8
consider assignments of union field of ManuallyDrop type safe
RalfJung Oct 18, 2020
64856e2
adjust union access unsafety check logic to take into account Deref a…
RalfJung Oct 24, 2020
63bdb3a
improve formatting
RalfJung Oct 26, 2020
af309cc
needs -> might need
RalfJung Oct 28, 2020
df1c55a
add function to iterate through all sub-places, and add PlaceRef::ty
RalfJung Nov 20, 2020
571da2c
refactor unsafety checking of places
RalfJung Nov 21, 2020
0bb82c4
expand iter_projections comment
RalfJung Nov 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion compiler/rustc_middle/src/mir/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be useful to give an example based on Rust code. For example:

Given the place a.b.c, this would yield:

  • (a, b)
  • (a.b, c)

I am a bit surprised by this structure -- I guess I expected it to return a, a.b, and a.b.c, rather than a tuple, and to have people match on the "tail" projection (if any). But I guess this is ok too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expanded the comment.

I expected it to return a, a.b, and a.b.c, rather than a tuple, and to have people match on the "tail" projection (if any). But I guess this is ok too.

I first thought of something like this, but it doesn't really match what clients need, at least what this particular client needs. The point is to check the projections, so the iterator really should yield as often as there are projections. And given that it also seemed odd to not make the projection itself directly available.

In a follow-up PR I hope to port more clients to this API, I guess then we will see how generally useful it is.

pub fn iter_projections(self) -> impl Iterator<Item=(PlaceRef<'tcx>, PlaceElem<'tcx>)> + DoubleEndedIterator {
pub fn iter_projections(
self,
) -> impl Iterator<Item = (PlaceRef<'tcx>, PlaceElem<'tcx>)> + DoubleEndedIterator {
self.projection.iter().enumerate().map(move |(i, proj)| {
let base = PlaceRef { local: self.local, projection: &self.projection[..i] };
(base, proj)
Expand Down
159 changes: 84 additions & 75 deletions compiler/rustc_mir/src/transform/check_unsafety.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -190,99 +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
// <https://github.com/rust-lang/rust/pull/78068#issuecomment-731753506>.
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() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, cute, I missed that this was a double-ended iterator at first and wondered how you were going to manage this

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
| 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.
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;
}
}
}
Expand Down