Skip to content

interpret: track place alignment together with the type, not the value #98846

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

Merged
merged 2 commits into from
Jul 5, 2022
Merged

interpret: track place alignment together with the type, not the value #98846

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8955686
interpret: track place alignment together with the type, not the value
RalfJung Jul 3, 2022
b1568e6
clarify comment
RalfJung Jul 4, 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
15 changes: 5 additions & 10 deletions compiler/rustc_const_eval/src/interpret/eval_context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -808,7 +808,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
self.stack_mut().pop().expect("tried to pop a stack frame, but there were none");

if !unwinding {
let op = self.access_local(&frame, mir::RETURN_PLACE, None)?;
let op = self.local_to_op(&frame, mir::RETURN_PLACE, None)?;
self.copy_op_transmute(&op, &frame.return_place)?;
trace!("{:?}", self.dump_place(*frame.return_place));
}
Expand Down Expand Up @@ -981,8 +981,7 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> std::fmt::Debug
LocalValue::Live(Operand::Indirect(mplace)) => {
write!(
fmt,
" by align({}){} ref {:?}:",
mplace.align.bytes(),
" by {} ref {:?}:",
match mplace.meta {
MemPlaceMeta::Meta(meta) => format!(" meta({:?})", meta),
MemPlaceMeta::Poison | MemPlaceMeta::None => String::new(),
Expand Down Expand Up @@ -1011,13 +1010,9 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> std::fmt::Debug
write!(fmt, ": {:?}", self.ecx.dump_allocs(allocs.into_iter().flatten().collect()))
}
Place::Ptr(mplace) => match mplace.ptr.provenance.and_then(Provenance::get_alloc_id) {
Some(alloc_id) => write!(
fmt,
"by align({}) ref {:?}: {:?}",
mplace.align.bytes(),
mplace.ptr,
self.ecx.dump_alloc(alloc_id)
),
Some(alloc_id) => {
write!(fmt, "by ref {:?}: {:?}", mplace.ptr, self.ecx.dump_alloc(alloc_id))
}
ptr => write!(fmt, " integral by ref: {:?}", ptr),
},
}
Expand Down
38 changes: 23 additions & 15 deletions compiler/rustc_const_eval/src/interpret/operand.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use rustc_middle::ty::layout::{LayoutOf, PrimitiveExt, TyAndLayout};
use rustc_middle::ty::print::{FmtPrinter, PrettyPrinter, Printer};
use rustc_middle::ty::{ConstInt, DelaySpanBugEmitted, Ty};
use rustc_middle::{mir, ty};
use rustc_target::abi::{self, Abi, HasDataLayout, Size, TagEncoding};
use rustc_target::abi::{self, Abi, Align, HasDataLayout, Size, TagEncoding};
use rustc_target::abi::{VariantIdx, Variants};

use super::{
Expand Down Expand Up @@ -177,10 +177,18 @@ pub enum Operand<Tag: Provenance = AllocId> {
pub struct OpTy<'tcx, Tag: Provenance = AllocId> {
op: Operand<Tag>, // Keep this private; it helps enforce invariants.
pub layout: TyAndLayout<'tcx>,
/// rustc does not have a proper way to represent the type of a field of a `repr(packed)` struct:
/// it needs to have a different alignment than the field type would usually have.
/// So we represent this here with a separate field that "overwrites" `layout.align`.
/// This means `layout.align` should never be used for an `OpTy`!
/// `None` means "alignment does not matter since this is a by-value operand"
/// (`Operand::Immediate`); this field is only relevant for `Operand::Indirect`.
/// Also CTFE ignores alignment anyway, so this is for Miri only.
pub align: Option<Align>,
Copy link
Contributor

Choose a reason for hiding this comment

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

So afaict this field is never used, and after lots of going through this PRs diff, I came to the conclusion it will only be used to check reads from Operand::Indirect, and only in miri.

Please document the miri part, the other part is already documented after all.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah currently this field cannot cause UB I think. But I want to relax the rules for *ptr and once we do, this can cause UB again.

}

#[cfg(all(target_arch = "x86_64", target_pointer_width = "64"))]
rustc_data_structures::static_assert_size!(OpTy<'_>, 80);
rustc_data_structures::static_assert_size!(OpTy<'_>, 88);

impl<'tcx, Tag: Provenance> std::ops::Deref for OpTy<'tcx, Tag> {
type Target = Operand<Tag>;
Expand All @@ -193,28 +201,28 @@ impl<'tcx, Tag: Provenance> std::ops::Deref for OpTy<'tcx, Tag> {
impl<'tcx, Tag: Provenance> From<MPlaceTy<'tcx, Tag>> for OpTy<'tcx, Tag> {
#[inline(always)]
fn from(mplace: MPlaceTy<'tcx, Tag>) -> Self {
OpTy { op: Operand::Indirect(*mplace), layout: mplace.layout }
OpTy { op: Operand::Indirect(*mplace), layout: mplace.layout, align: Some(mplace.align) }
}
}

impl<'tcx, Tag: Provenance> From<&'_ MPlaceTy<'tcx, Tag>> for OpTy<'tcx, Tag> {
#[inline(always)]
fn from(mplace: &MPlaceTy<'tcx, Tag>) -> Self {
OpTy { op: Operand::Indirect(**mplace), layout: mplace.layout }
OpTy { op: Operand::Indirect(**mplace), layout: mplace.layout, align: Some(mplace.align) }
}
}

impl<'tcx, Tag: Provenance> From<&'_ mut MPlaceTy<'tcx, Tag>> for OpTy<'tcx, Tag> {
#[inline(always)]
fn from(mplace: &mut MPlaceTy<'tcx, Tag>) -> Self {
OpTy { op: Operand::Indirect(**mplace), layout: mplace.layout }
OpTy { op: Operand::Indirect(**mplace), layout: mplace.layout, align: Some(mplace.align) }
}
}

impl<'tcx, Tag: Provenance> From<ImmTy<'tcx, Tag>> for OpTy<'tcx, Tag> {
#[inline(always)]
fn from(val: ImmTy<'tcx, Tag>) -> Self {
OpTy { op: Operand::Immediate(val.imm), layout: val.layout }
OpTy { op: Operand::Immediate(val.imm), layout: val.layout, align: None }
}
}

Expand Down Expand Up @@ -450,7 +458,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
),
};

Ok(OpTy { op: Operand::Immediate(field_val), layout: field_layout })
Ok(OpTy { op: Operand::Immediate(field_val), layout: field_layout, align: None })
}

pub fn operand_index(
Expand Down Expand Up @@ -522,7 +530,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
///
/// This is public because it is used by [priroda](https://github.com/oli-obk/priroda) to get an
/// OpTy from a local
pub fn access_local(
pub fn local_to_op(
&self,
frame: &super::Frame<'mir, 'tcx, M::PointerTag, M::FrameExtra>,
local: mir::Local,
Expand All @@ -535,7 +543,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
} else {
M::access_local(&self, frame, local)?
};
Ok(OpTy { op, layout })
Ok(OpTy { op, layout, align: Some(layout.align.abi) })
}

/// Every place can be read from, so we can turn them into an operand.
Expand All @@ -549,10 +557,10 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
let op = match **place {
Place::Ptr(mplace) => Operand::Indirect(mplace),
Place::Local { frame, local } => {
*self.access_local(&self.stack()[frame], local, None)?
*self.local_to_op(&self.stack()[frame], local, None)?
}
};
Ok(OpTy { op, layout: place.layout })
Ok(OpTy { op, layout: place.layout, align: Some(place.align) })
}

/// Evaluate a place with the goal of reading from it. This lets us sometimes
Expand All @@ -566,7 +574,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
// here is not the entire place.
let layout = if place.projection.is_empty() { layout } else { None };

let base_op = self.access_local(self.frame(), place.local, layout)?;
let base_op = self.local_to_op(self.frame(), place.local, layout)?;

let op = place
.projection
Expand Down Expand Up @@ -603,11 +611,11 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
Constant(ref constant) => {
let val =
self.subst_from_current_frame_and_normalize_erasing_regions(constant.literal)?;

// This can still fail:
// * During ConstProp, with `TooGeneric` or since the `required_consts` were not all
// checked yet.
// * During CTFE, since promoteds in `const`/`static` initializer bodies can fail.

self.mir_const_to_op(&val, layout)?
}
};
Expand Down Expand Up @@ -683,7 +691,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
// We rely on mutability being set correctly in that allocation to prevent writes
// where none should happen.
let ptr = self.global_base_pointer(Pointer::new(id, offset))?;
Operand::Indirect(MemPlace::from_ptr(ptr.into(), layout.align.abi))
Operand::Indirect(MemPlace::from_ptr(ptr.into()))
}
ConstValue::Scalar(x) => Operand::Immediate(tag_scalar(x)?.into()),
ConstValue::Slice { data, start, end } => {
Expand All @@ -700,7 +708,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
))
}
};
Ok(OpTy { op, layout })
Ok(OpTy { op, layout, align: Some(layout.align.abi) })
}

/// Read discriminant, return the runtime value as well as the variant index.
Expand Down
Loading