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

Remove dereferencing of Box from codegen #95576

Merged
merged 9 commits into from
Jun 21, 2022
Merged

Remove dereferencing of Box from codegen #95576

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3e9d3d9
add From impls for BitSet and GrowableBitSet
beepster4096 May 14, 2022
cb41788
remove box derefs from codgen
beepster4096 May 14, 2022
954fbe3
fix inline_into_box_place test
beepster4096 May 14, 2022
e0b0fb0
fix typos & docs
beepster4096 May 14, 2022
6cb38fb
correct mirphase docs
beepster4096 May 19, 2022
1d1ff36
fix codegen assertion
beepster4096 May 19, 2022
dff1f9f
make sure miri ices when dereferencing a box
beepster4096 May 20, 2022
6003c25
condense duplicate code into seperate functions
beepster4096 May 20, 2022
28ff0df
fix box derefs in var debug info
beepster4096 May 20, 2022
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
14 changes: 6 additions & 8 deletions compiler/rustc_codegen_ssa/src/mir/operand.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,22 +118,20 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, V> {
}

pub fn deref<Cx: LayoutTypeMethods<'tcx>>(self, cx: &Cx) -> PlaceRef<'tcx, V> {
if self.layout.ty.is_box() {
Copy link
Member

@RalfJung RalfJung Apr 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Could you add a similar assertion to Miri? deref_operand would probably be a good spot.

Copy link
Member

Choose a reason for hiding this comment

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

Also, does this mean you can remove the special cases I listed at #95453 (comment) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first 3 special cases have been removed. The debuginfo one probably shouldn't be changed because it could make debugging more annoying for no benefit. I'm currently investigating to see if the last one can be changed safely without breaking things.

Copy link
Member

Choose a reason for hiding this comment

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

Also please remember to add a similar assertion to Miri. :)

Sorry for raising two points in the same thread. Sadly GitHub does not let one open multiple threads at the same line of code.

bug!("dereferencing {:?} in codegen", self.layout.ty);
}

let projected_ty = self
.layout
.ty
.builtin_deref(true)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe builtin_deref should take a mode enum like this:

enum BuiltinDerefMode {
    /// `Box<T>`, `&T`, and optionally `*T`.
    UserFacing {
        /// Whether to also allow `*T`.
        allow_unsafe: bool
    },

    /// `&T` and `*T`.
    LowLevel,
}

(Though LowLevel should probably be named something better)

.unwrap_or_else(|| bug!("deref of non-pointer {:?}", self))
.ty;

let (llptr, llextra) = match self.val {
OperandValue::Immediate(llptr) => (llptr, None),
OperandValue::Pair(llptr, llextra) => {
// if the box's allocator isn't a ZST, then "llextra" is actually the allocator
if self.layout.ty.is_box() && !self.layout.field(cx, 1).is_zst() {
(llptr, None)
} else {
(llptr, Some(llextra))
}
}
OperandValue::Pair(llptr, llextra) => (llptr, Some(llextra)),
OperandValue::Ref(..) => bug!("Deref of by-Ref operand {:?}", self),
};
let layout = cx.layout_of(projected_ty);
Expand Down
24 changes: 2 additions & 22 deletions compiler/rustc_codegen_ssa/src/mir/place.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,35 +446,15 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
mir::PlaceRef { projection: &place_ref.projection[..elem.0], ..place_ref },
);

// a box with a non-zst allocator should not be directly dereferenced
if cg_base.layout.ty.is_box() && !cg_base.layout.field(cx, 1).is_zst() {
// Extract `Box<T>` -> `Unique<T>` -> `NonNull<T>` -> `*const T`
let ptr =
cg_base.extract_field(bx, 0).extract_field(bx, 0).extract_field(bx, 0);

ptr.deref(bx.cx())
} else {
cg_base.deref(bx.cx())
}
cg_base.deref(bx.cx())
} else {
bug!("using operand local {:?} as place", place_ref);
}
}
};
for elem in place_ref.projection[base..].iter() {
cg_base = match *elem {
mir::ProjectionElem::Deref => {
// a box with a non-zst allocator should not be directly dereferenced
if cg_base.layout.ty.is_box() && !cg_base.layout.field(cx, 1).is_zst() {
// Project `Box<T>` -> `Unique<T>` -> `NonNull<T>` -> `*const T`
let ptr =
cg_base.project_field(bx, 0).project_field(bx, 0).project_field(bx, 0);

bx.load_operand(ptr).deref(bx.cx())
} else {
bx.load_operand(cg_base).deref(bx.cx())
}
}
mir::ProjectionElem::Deref => bx.load_operand(cg_base).deref(bx.cx()),
mir::ProjectionElem::Field(ref field, _) => {
cg_base.project_field(bx, field.index())
}
Expand Down
5 changes: 5 additions & 0 deletions compiler/rustc_const_eval/src/interpret/place.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,11 @@ where
) -> InterpResult<'tcx, MPlaceTy<'tcx, M::PointerTag>> {
let val = self.read_immediate(src)?;
trace!("deref to {} on {:?}", val.layout.ty, *val);

if val.layout.ty.is_box() {
bug!("dereferencing {:?}", val.layout.ty);
}

let mplace = self.ref_to_mplace(&val)?;
self.check_mplace_access(mplace, CheckInAllocMsg::DerefTest)?;
Ok(mplace)
Expand Down
114 changes: 64 additions & 50 deletions compiler/rustc_const_eval/src/transform/validate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,65 +240,79 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
context: PlaceContext,
location: Location,
) {
if let ProjectionElem::Index(index) = elem {
let index_ty = self.body.local_decls[index].ty;
if index_ty != self.tcx.types.usize {
self.fail(location, format!("bad index ({:?} != usize)", index_ty))
match elem {
ProjectionElem::Index(index) => {
let index_ty = self.body.local_decls[index].ty;
if index_ty != self.tcx.types.usize {
self.fail(location, format!("bad index ({:?} != usize)", index_ty))
}
}
}
if let ProjectionElem::Field(f, ty) = elem {
let parent = Place { local, projection: self.tcx.intern_place_elems(proj_base) };
let parent_ty = parent.ty(&self.body.local_decls, self.tcx);
let fail_out_of_bounds = |this: &Self, location| {
this.fail(location, format!("Out of bounds field {:?} for {:?}", f, parent_ty));
};
let check_equal = |this: &Self, location, f_ty| {
if !this.mir_assign_valid_types(ty, f_ty) {
this.fail(
ProjectionElem::Deref if self.mir_phase >= MirPhase::GeneratorsLowered => {
let base_ty = Place::ty_from(local, proj_base, &self.body.local_decls, self.tcx).ty;

if base_ty.is_box() {
self.fail(
location,
format!("{:?} dereferenced after ElaborateBoxDerefs", base_ty),
)
}
}
ProjectionElem::Field(f, ty) => {
let parent = Place { local, projection: self.tcx.intern_place_elems(proj_base) };
let parent_ty = parent.ty(&self.body.local_decls, self.tcx);
let fail_out_of_bounds = |this: &Self, location| {
this.fail(location, format!("Out of bounds field {:?} for {:?}", f, parent_ty));
};
let check_equal = |this: &Self, location, f_ty| {
if !this.mir_assign_valid_types(ty, f_ty) {
this.fail(
location,
format!(
"Field projection `{:?}.{:?}` specified type `{:?}`, but actual type is {:?}",
parent, f, ty, f_ty
)
)
}
};
match parent_ty.ty.kind() {
ty::Tuple(fields) => {
let Some(f_ty) = fields.get(f.as_usize()) else {
fail_out_of_bounds(self, location);
return;
};
check_equal(self, location, *f_ty);
}
ty::Adt(adt_def, substs) => {
let var = parent_ty.variant_index.unwrap_or(VariantIdx::from_u32(0));
let Some(field) = adt_def.variant(var).fields.get(f.as_usize()) else {
fail_out_of_bounds(self, location);
return;
};
check_equal(self, location, field.ty(self.tcx, substs));
}
ty::Closure(_, substs) => {
let substs = substs.as_closure();
let Some(f_ty) = substs.upvar_tys().nth(f.as_usize()) else {
fail_out_of_bounds(self, location);
return;
};
check_equal(self, location, f_ty);
}
ty::Generator(_, substs, _) => {
let substs = substs.as_generator();
let Some(f_ty) = substs.upvar_tys().nth(f.as_usize()) else {
fail_out_of_bounds(self, location);
return;
};
check_equal(self, location, f_ty);
}
_ => {
self.fail(location, format!("{:?} does not have fields", parent_ty.ty));
}
};

match parent_ty.ty.kind() {
ty::Tuple(fields) => {
let Some(f_ty) = fields.get(f.as_usize()) else {
fail_out_of_bounds(self, location);
return;
};
check_equal(self, location, *f_ty);
}
ty::Adt(adt_def, substs) => {
let var = parent_ty.variant_index.unwrap_or(VariantIdx::from_u32(0));
let Some(field) = adt_def.variant(var).fields.get(f.as_usize()) else {
fail_out_of_bounds(self, location);
return;
};
check_equal(self, location, field.ty(self.tcx, substs));
}
ty::Closure(_, substs) => {
let substs = substs.as_closure();
let Some(f_ty) = substs.upvar_tys().nth(f.as_usize()) else {
fail_out_of_bounds(self, location);
return;
};
check_equal(self, location, f_ty);
}
ty::Generator(_, substs, _) => {
let substs = substs.as_generator();
let Some(f_ty) = substs.upvar_tys().nth(f.as_usize()) else {
fail_out_of_bounds(self, location);
return;
};
check_equal(self, location, f_ty);
}
_ => {
self.fail(location, format!("{:?} does not have fields", parent_ty.ty));
}
}
}
_ => {}
}
self.super_projection_elem(local, proj_base, elem, context, location);
}
Expand Down
12 changes: 12 additions & 0 deletions compiler/rustc_index/src/bit_set.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,12 @@ impl<T: Idx> BitRelations<BitSet<T>> for BitSet<T> {
}
}

impl<T: Idx> From<GrowableBitSet<T>> for BitSet<T> {
fn from(bit_set: GrowableBitSet<T>) -> Self {
bit_set.bit_set
}
}

/// A fixed-size bitset type with a partially dense, partially sparse
/// representation. The bitset is broken into chunks, and chunks that are all
/// zeros or all ones are represented and handled very efficiently.
Expand Down Expand Up @@ -1469,6 +1475,12 @@ impl<T: Idx> GrowableBitSet<T> {
}
}

impl<T: Idx> From<BitSet<T>> for GrowableBitSet<T> {
fn from(bit_set: BitSet<T>) -> Self {
Self { bit_set }
}
}

/// A fixed-size 2D bit matrix type with a dense representation.
///
/// `R` and `C` are index types used to identify rows and columns respectively;
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_middle/src/mir/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,7 @@ pub enum MirPhase {
/// Beginning with this phase, the following variants are disallowed:
/// * [`TerminatorKind::Yield`](terminator::TerminatorKind::Yield)
/// * [`TerminatorKind::GeneratorDrop`](terminator::TerminatorKind::GeneratorDrop)
/// * [`ProjectionElem::Deref`] of `Box`
GeneratorsLowered = 6,
Optimized = 7,
}
Expand Down
13 changes: 12 additions & 1 deletion compiler/rustc_mir_dataflow/src/elaborate_drops.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -410,7 +410,18 @@ where
fn open_drop_for_box(&mut self, adt: ty::AdtDef<'tcx>, substs: SubstsRef<'tcx>) -> BasicBlock {
debug!("open_drop_for_box({:?}, {:?}, {:?})", self, adt, substs);

let interior = self.tcx().mk_place_deref(self.place);
// drop glue is sent straight to codegen
// box cannot be directly dereferenced
let unique_ty = adt.non_enum_variant().fields[0].ty(self.tcx(), substs);
let nonnull_ty =
unique_ty.ty_adt_def().unwrap().non_enum_variant().fields[0].ty(self.tcx(), substs);
let ptr_ty = self.tcx().mk_imm_ptr(substs[0].expect_ty());

let unique_place = self.tcx().mk_place_field(self.place, Field::new(0), unique_ty);
let nonnull_place = self.tcx().mk_place_field(unique_place, Field::new(0), nonnull_ty);
let ptr_place = self.tcx().mk_place_field(nonnull_place, Field::new(0), ptr_ty);
let interior = self.tcx().mk_place_deref(ptr_place);

let interior_path = self.elaborator.deref_subpath(self.path);

let succ = self.box_free_block(adt, substs, self.succ, self.unwind);
Expand Down
Loading