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.

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

Pull Derefer before ElaborateDrops #98145

Merged
merged 2 commits into from
Jul 13, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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: 4 additions & 0 deletions compiler/rustc_borrowck/src/invalidation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,10 @@ impl<'cx, 'tcx> InvalidationGenerator<'cx, 'tcx> {
| Rvalue::ShallowInitBox(ref operand, _ /*ty*/) => {
self.consume_operand(location, operand)
}
Rvalue::CopyForDeref(ref place) => {
let op = &Operand::Copy(*place);
self.consume_operand(location, op);
}

Rvalue::Len(place) | Rvalue::Discriminant(place) => {
let af = match *rvalue {
Expand Down
17 changes: 17 additions & 0 deletions compiler/rustc_borrowck/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1236,6 +1236,23 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
| Rvalue::ShallowInitBox(ref operand, _ /*ty*/) => {
self.consume_operand(location, (operand, span), flow_state)
}
Rvalue::CopyForDeref(place) => {
self.access_place(
location,
(place, span),
(Deep, Read(ReadKind::Copy)),
LocalMutationIsAllowed::No,
flow_state,
);

// Finally, check if path was already moved.
self.check_if_path_or_subpath_is_moved(
location,
InitializationRequiringAction::Use,
(place.as_ref(), span),
flow_state,
);
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
}

Rvalue::Len(place) | Rvalue::Discriminant(place) => {
let af = match *rvalue {
Expand Down
5 changes: 5 additions & 0 deletions compiler/rustc_borrowck/src/type_check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2269,6 +2269,10 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
Rvalue::Use(operand) | Rvalue::UnaryOp(_, operand) => {
self.check_operand(operand, location);
}
Rvalue::CopyForDeref(place) => {
let op = &Operand::Copy(*place);
self.check_operand(op, location);
}

Rvalue::BinaryOp(_, box (left, right))
| Rvalue::CheckedBinaryOp(_, box (left, right)) => {
Expand Down Expand Up @@ -2299,6 +2303,7 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
| Rvalue::BinaryOp(..)
| Rvalue::CheckedBinaryOp(..)
| Rvalue::NullaryOp(..)
| Rvalue::CopyForDeref(..)
| Rvalue::UnaryOp(..)
| Rvalue::Discriminant(..) => None,

Expand Down
5 changes: 5 additions & 0 deletions compiler/rustc_codegen_cranelift/src/base.rs
Original file line number Diff line number Diff line change
Expand Up @@ -503,6 +503,11 @@ fn codegen_stmt<'tcx>(
let val = codegen_operand(fx, operand);
lval.write_cvalue(fx, val);
}
Rvalue::CopyForDeref(place) => {
let cplace = codegen_place(fx, place);
let val = cplace.to_cvalue(fx);
lval.write_cvalue(fx, val)
}
Rvalue::Ref(_, _, place) | Rvalue::AddressOf(_, place) => {
let place = codegen_place(fx, place);
let ref_ = place.place_ref(fx, lval.layout());
Expand Down
6 changes: 6 additions & 0 deletions compiler/rustc_codegen_ssa/src/mir/rvalue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use crate::traits::*;
use crate::MemFlags;

use rustc_middle::mir;
use rustc_middle::mir::Operand;
use rustc_middle::ty::cast::{CastTy, IntTy};
use rustc_middle::ty::layout::{HasTyCtxt, LayoutOf};
use rustc_middle::ty::{self, adjustment::PointerCast, Instance, Ty, TyCtxt};
Expand Down Expand Up @@ -344,6 +345,10 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
self.codegen_place_to_pointer(bx, place, mk_ref)
}

mir::Rvalue::CopyForDeref(place) => {
let operand = self.codegen_operand(&mut bx, &Operand::Copy(place));
(bx, operand)
}
mir::Rvalue::AddressOf(mutability, place) => {
let mk_ptr = move |tcx: TyCtxt<'tcx>, ty: Ty<'tcx>| {
tcx.mk_ptr(ty::TypeAndMut { ty, mutbl: mutability })
Expand Down Expand Up @@ -698,6 +703,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
pub fn rvalue_creates_operand(&self, rvalue: &mir::Rvalue<'tcx>, span: Span) -> bool {
match *rvalue {
mir::Rvalue::Ref(..) |
mir::Rvalue::CopyForDeref(..) |
mir::Rvalue::AddressOf(..) |
mir::Rvalue::Len(..) |
mir::Rvalue::Cast(..) | // (*)
Expand Down
5 changes: 5 additions & 0 deletions compiler/rustc_const_eval/src/interpret/step.rs
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,11 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
self.copy_op(&op, &dest, /*allow_transmute*/ false)?;
}

CopyForDeref(ref place) => {
let op = self.eval_place_to_op(*place, Some(dest.layout))?;
self.copy_op(&op, &dest, /* allow_transmute*/ false)?;
}

BinaryOp(bin_op, box (ref left, ref right)) => {
let layout = binop_left_homogeneous(bin_op).then_some(dest.layout);
let left = self.read_immediate(&self.eval_operand(left, layout)?)?;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -446,6 +446,7 @@ impl<'tcx> Visitor<'tcx> for Checker<'_, 'tcx> {
Rvalue::ThreadLocalRef(_) => self.check_op(ops::ThreadLocalAccess),

Rvalue::Use(_)
| Rvalue::CopyForDeref(..)
| Rvalue::Repeat(..)
| Rvalue::Discriminant(..)
| Rvalue::Len(_)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,8 @@ where
in_place::<Q, _>(cx, in_local, place.as_ref())
}

Rvalue::CopyForDeref(place) => in_place::<Q, _>(cx, in_local, place.as_ref()),

Rvalue::Use(operand)
| Rvalue::Repeat(operand, _)
| Rvalue::UnaryOp(_, operand)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,7 @@ where
mir::Rvalue::Cast(..)
| mir::Rvalue::ShallowInitBox(..)
| mir::Rvalue::Use(..)
| mir::Rvalue::CopyForDeref(..)
| mir::Rvalue::ThreadLocalRef(..)
| mir::Rvalue::Repeat(..)
| mir::Rvalue::Len(..)
Expand Down
4 changes: 4 additions & 0 deletions compiler/rustc_const_eval/src/transform/promote_consts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -494,6 +494,10 @@ impl<'tcx> Validator<'_, 'tcx> {
Rvalue::Use(operand) | Rvalue::Repeat(operand, _) => {
self.validate_operand(operand)?;
}
Rvalue::CopyForDeref(place) => {
let op = &Operand::Copy(*place);
self.validate_operand(op)?
}

Rvalue::Discriminant(place) | Rvalue::Len(place) => {
self.validate_place(place.as_ref())?
Expand Down
11 changes: 10 additions & 1 deletion compiler/rustc_const_eval/src/transform/validate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,7 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
};
}
match rvalue {
Rvalue::Use(_) => {}
Rvalue::Use(_) | Rvalue::CopyForDeref(_) => {}
Rvalue::Aggregate(agg_kind, _) => {
let disallowed = match **agg_kind {
AggregateKind::Array(..) => false,
Expand Down Expand Up @@ -592,6 +592,15 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
),
);
}
if let Rvalue::CopyForDeref(place) = rvalue {
if !place.ty(&self.body.local_decls, self.tcx).ty.builtin_deref(true).is_some()
{
self.fail(
location,
"`CopyForDeref` should only be used for dereferenceable types",
)
}
}
// FIXME(JakobDegen): Check this for all rvalues, not just this one.
if let Rvalue::Use(Operand::Copy(src) | Operand::Move(src)) = rvalue {
// The sides of an assignment must not alias. Currently this just checks whether
Expand Down
14 changes: 13 additions & 1 deletion compiler/rustc_middle/src/mir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ mod syntax;
pub use syntax::*;
mod switch_sources;
pub mod tcx;
mod terminator;
pub mod terminator;
pub use terminator::*;

pub mod traversal;
Expand Down Expand Up @@ -925,6 +925,15 @@ impl<'tcx> LocalDecl<'tcx> {
}
}

/// Returns `true` if this is a DerefTemp
pub fn is_deref_temp(&self) -> bool {
match self.local_info {
Some(box LocalInfo::DerefTemp) => return true,
_ => (),
}
return false;
}

/// Returns `true` is the local is from a compiler desugaring, e.g.,
/// `__next` from a `for` loop.
#[inline]
Expand Down Expand Up @@ -1795,6 +1804,7 @@ impl<'tcx> Rvalue<'tcx> {
Rvalue::Cast(CastKind::PointerExposeAddress, _, _) => false,

Rvalue::Use(_)
| Rvalue::CopyForDeref(_)
| Rvalue::Repeat(_, _)
| Rvalue::Ref(_, _, _)
| Rvalue::ThreadLocalRef(_)
Expand Down Expand Up @@ -1889,6 +1899,8 @@ impl<'tcx> Debug for Rvalue<'tcx> {
write!(fmt, "&{}{}{:?}", region, kind_str, place)
}

CopyForDeref(ref place) => write!(fmt, "deref_copy {:#?}", place),

AddressOf(mutability, ref place) => {
let kind_str = match mutability {
Mutability::Mut => "mut",
Expand Down
16 changes: 13 additions & 3 deletions compiler/rustc_middle/src/mir/syntax.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ pub enum MirPhase {
/// of the `mir_promoted` query), these promoted elements are available in the `promoted_mir`
/// query.
ConstsPromoted = 2,
/// After this projections may only contain deref projections as the first element.
Derefered = 3,
/// Beginning with this phase, the following variants are disallowed:
/// * [`TerminatorKind::DropAndReplace`]
/// * [`TerminatorKind::FalseUnwind`]
Expand All @@ -66,9 +68,7 @@ pub enum MirPhase {
/// Furthermore, `Drop` now uses explicit drop flags visible in the MIR and reaching a `Drop`
/// terminator means that the auto-generated drop glue will be invoked. Also, `Copy` operands
/// are allowed for non-`Copy` types.
DropsLowered = 3,
/// After this projections may only contain deref projections as the first element.
Derefered = 4,
DropsLowered = 4,
/// Beginning with this phase, the following variant is disallowed:
/// * [`Rvalue::Aggregate`] for any `AggregateKind` except `Array`
///
Expand Down Expand Up @@ -1051,6 +1051,16 @@ pub enum Rvalue<'tcx> {
/// initialized but its content as uninitialized. Like other pointer casts, this in general
/// affects alias analysis.
ShallowInitBox(Operand<'tcx>, Ty<'tcx>),

/// A CopyForDeref is equivalent to a read from a place at the
/// codegen level, but is treated specially by drop elaboration. When such a read happens, it
/// is guaranteed (via nature of the mir_opt `Derefer` in rustc_mir_transform/src/deref_separator)
/// that the only use of the returned value is a deref operation, immediately
/// followed by one or more projections. Drop elaboration treats this rvalue as if the
Copy link
Member

Choose a reason for hiding this comment

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

Why are the projections relevant? What is the problem if the only use of this value is in *v without a projection?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without projection, we wouldn't even run deref_separator on it so it wouldn't be used.

Copy link
Member

Choose a reason for hiding this comment

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

Sure but there's nothing wrong with that situation, right? If it somehow happened, all passes could handle it fine?
I just want to figure out if there is a correctness argument here that relies on "at least one projection", which would seem odd.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I never considered the if part so I made some local changes that makes sure it will work no matter what , I will push it alongside CI fix (Once I solve it 😭 ).

/// read never happened and just projects further. This allows simplifying various MIR
/// optimizations and codegen backends that previously had to handle deref operations anywhere
/// in a place.
CopyForDeref(Place<'tcx>),
}

#[cfg(all(target_arch = "x86_64", target_pointer_width = "64"))]
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_middle/src/mir/tcx.rs
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,7 @@ impl<'tcx> Rvalue<'tcx> {
}
},
Rvalue::ShallowInitBox(_, ty) => tcx.mk_box(ty),
Rvalue::CopyForDeref(ref place) => place.ty(local_decls, tcx).ty,
}
}

Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_middle/src/mir/type_foldable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ impl<'tcx> TypeFoldable<'tcx> for Rvalue<'tcx> {
Ref(region, bk, place) => {
Ref(region.try_fold_with(folder)?, bk, place.try_fold_with(folder)?)
}
CopyForDeref(place) => CopyForDeref(place.try_fold_with(folder)?),
AddressOf(mutability, place) => AddressOf(mutability, place.try_fold_with(folder)?),
Len(place) => Len(place.try_fold_with(folder)?),
Cast(kind, op, ty) => Cast(kind, op.try_fold_with(folder)?, ty.try_fold_with(folder)?),
Expand Down
4 changes: 4 additions & 0 deletions compiler/rustc_middle/src/mir/type_visitable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,10 @@ impl<'tcx> TypeVisitable<'tcx> for Rvalue<'tcx> {
use crate::mir::Rvalue::*;
match *self {
Use(ref op) => op.visit_with(visitor),
CopyForDeref(ref place) => {
let op = &Operand::Copy(*place);
op.visit_with(visitor)
}
Repeat(ref op, _) => op.visit_with(visitor),
ThreadLocalRef(did) => did.visit_with(visitor),
Ref(region, _, ref place) => {
Expand Down
7 changes: 7 additions & 0 deletions compiler/rustc_middle/src/mir/visit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -711,6 +711,13 @@ macro_rules! make_mir_visitor {
};
self.visit_place(path, ctx, location);
}
Rvalue::CopyForDeref(place) => {
self.visit_place(
place,
PlaceContext::NonMutatingUse(NonMutatingUseContext::Inspect),
location
);
}

Rvalue::AddressOf(m, path) => {
let ctx = match m {
Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_mir_dataflow/src/impls/borrowed_locals.rs
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,8 @@ where
| mir::Rvalue::NullaryOp(..)
| mir::Rvalue::UnaryOp(..)
| mir::Rvalue::Discriminant(..)
| mir::Rvalue::Aggregate(..) => {}
| mir::Rvalue::Aggregate(..)
| mir::Rvalue::CopyForDeref(..) => {}
}
}

Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_mir_dataflow/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ pub mod impls;
pub mod move_paths;
pub mod rustc_peek;
pub mod storage;
pub mod un_derefer;

pub(crate) mod indexes {
pub(crate) use super::move_paths::MovePathIndex;
Expand Down
30 changes: 29 additions & 1 deletion compiler/rustc_mir_dataflow/src/move_paths/builder.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use crate::un_derefer::UnDerefer;
use rustc_index::vec::IndexVec;
use rustc_middle::mir::tcx::RvalueInitializationState;
use rustc_middle::mir::*;
Expand All @@ -19,6 +20,7 @@ struct MoveDataBuilder<'a, 'tcx> {
param_env: ty::ParamEnv<'tcx>,
data: MoveData<'tcx>,
errors: Vec<(Place<'tcx>, MoveError<'tcx>)>,
un_derefer: UnDerefer<'tcx>,
}

impl<'a, 'tcx> MoveDataBuilder<'a, 'tcx> {
Expand All @@ -32,6 +34,7 @@ impl<'a, 'tcx> MoveDataBuilder<'a, 'tcx> {
tcx,
param_env,
errors: Vec::new(),
un_derefer: UnDerefer { tcx: tcx, derefer_sidetable: Default::default() },
data: MoveData {
moves: IndexVec::new(),
loc_map: LocationMap::new(body),
Expand Down Expand Up @@ -94,6 +97,11 @@ impl<'b, 'a, 'tcx> Gatherer<'b, 'a, 'tcx> {
///
/// Maybe we should have separate "borrowck" and "moveck" modes.
fn move_path_for(&mut self, place: Place<'tcx>) -> Result<MovePathIndex, MoveError<'tcx>> {
if let Some(new_place) = self.builder.un_derefer.derefer(place.as_ref(), self.builder.body)
{
return self.move_path_for(new_place);
}

debug!("lookup({:?})", place);
let mut base = self.builder.data.rev_lookup.locals[place.local];

Expand Down Expand Up @@ -276,6 +284,12 @@ struct Gatherer<'b, 'a, 'tcx> {
impl<'b, 'a, 'tcx> Gatherer<'b, 'a, 'tcx> {
fn gather_statement(&mut self, stmt: &Statement<'tcx>) {
match &stmt.kind {
StatementKind::Assign(box (place, Rvalue::CopyForDeref(reffed))) => {
assert!(place.projection.is_empty());
if self.builder.body.local_decls[place.local].is_deref_temp() {
self.builder.un_derefer.derefer_sidetable.insert(place.local, *reffed);
}
}
StatementKind::Assign(box (place, rval)) => {
self.create_move_path(*place);
if let RvalueInitializationState::Shallow = rval.initialization_state() {
Expand All @@ -294,7 +308,10 @@ impl<'b, 'a, 'tcx> Gatherer<'b, 'a, 'tcx> {
}
StatementKind::StorageLive(_) => {}
StatementKind::StorageDead(local) => {
self.gather_move(Place::from(*local));
// DerefTemp locals (results of CopyForDeref) don't actually move anything.
if !self.builder.un_derefer.derefer_sidetable.contains_key(&local) {
self.gather_move(Place::from(*local));
}
}
StatementKind::SetDiscriminant { .. } | StatementKind::Deinit(..) => {
span_bug!(
Expand Down Expand Up @@ -328,6 +345,7 @@ impl<'b, 'a, 'tcx> Gatherer<'b, 'a, 'tcx> {
self.gather_operand(operand);
}
}
Rvalue::CopyForDeref(..) => unreachable!(),
Rvalue::Ref(..)
| Rvalue::AddressOf(..)
| Rvalue::Discriminant(..)
Expand Down Expand Up @@ -439,6 +457,11 @@ impl<'b, 'a, 'tcx> Gatherer<'b, 'a, 'tcx> {

fn gather_move(&mut self, place: Place<'tcx>) {
debug!("gather_move({:?}, {:?})", self.loc, place);
if let Some(new_place) = self.builder.un_derefer.derefer(place.as_ref(), self.builder.body)
{
self.gather_move(new_place);
return;
}

if let [ref base @ .., ProjectionElem::Subslice { from, to, from_end: false }] =
**place.projection
Expand Down Expand Up @@ -494,6 +517,11 @@ impl<'b, 'a, 'tcx> Gatherer<'b, 'a, 'tcx> {
fn gather_init(&mut self, place: PlaceRef<'tcx>, kind: InitKind) {
debug!("gather_init({:?}, {:?})", self.loc, place);

if let Some(new_place) = self.builder.un_derefer.derefer(place, self.builder.body) {
self.gather_init(new_place.as_ref(), kind);
return;
}

let mut place = place;

// Check if we are assigning into a field of a union, if so, lookup the place
Expand Down
Loading