Skip to content

give Pointer::into_parts a more scary name and offer a safer alternative #143140

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
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
2 changes: 1 addition & 1 deletion compiler/rustc_codegen_cranelift/src/constant.rs
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ pub(crate) fn codegen_const_value<'tcx>(
}
}
Scalar::Ptr(ptr, _size) => {
let (prov, offset) = ptr.into_parts(); // we know the `offset` is relative
let (prov, offset) = ptr.prov_and_relative_offset();
let alloc_id = prov.alloc_id();
let base_addr = match fx.tcx.global_alloc(alloc_id) {
GlobalAlloc::Memory(alloc) => {
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_codegen_gcc/src/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,7 @@ impl<'gcc, 'tcx> ConstCodegenMethods for CodegenCx<'gcc, 'tcx> {
}
}
Scalar::Ptr(ptr, _size) => {
let (prov, offset) = ptr.into_parts(); // we know the `offset` is relative
let (prov, offset) = ptr.prov_and_relative_offset();
let alloc_id = prov.alloc_id();
let base_addr = match self.tcx.global_alloc(alloc_id) {
GlobalAlloc::Memory(alloc) => {
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_codegen_llvm/src/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,7 @@ impl<'ll, 'tcx> ConstCodegenMethods for CodegenCx<'ll, 'tcx> {
}
}
Scalar::Ptr(ptr, _size) => {
let (prov, offset) = ptr.into_parts();
let (prov, offset) = ptr.prov_and_relative_offset();
let global_alloc = self.tcx.global_alloc(prov.alloc_id());
let base_addr = match global_alloc {
GlobalAlloc::Memory(alloc) => {
Expand Down
13 changes: 7 additions & 6 deletions compiler/rustc_const_eval/src/const_eval/eval_queries.rs
Original file line number Diff line number Diff line change
Expand Up @@ -209,9 +209,9 @@ pub(super) fn op_to_const<'tcx>(

match immediate {
Left(ref mplace) => {
// We know `offset` is relative to the allocation, so we can use `into_parts`.
let (prov, offset) = mplace.ptr().into_parts();
let alloc_id = prov.expect("cannot have `fake` place for non-ZST type").alloc_id();
let (prov, offset) =
mplace.ptr().into_pointer_or_addr().unwrap().prov_and_relative_offset();
let alloc_id = prov.alloc_id();
ConstValue::Indirect { alloc_id, offset }
}
// see comment on `let force_as_immediate` above
Expand All @@ -232,9 +232,10 @@ pub(super) fn op_to_const<'tcx>(
imm.layout.ty,
);
let msg = "`op_to_const` on an immediate scalar pair must only be used on slice references to the beginning of an actual allocation";
// We know `offset` is relative to the allocation, so we can use `into_parts`.
let (prov, offset) = a.to_pointer(ecx).expect(msg).into_parts();
let alloc_id = prov.expect(msg).alloc_id();
let ptr = a.to_pointer(ecx).expect(msg);
let (prov, offset) =
ptr.into_pointer_or_addr().expect(msg).prov_and_relative_offset();
let alloc_id = prov.alloc_id();
let data = ecx.tcx.global_alloc(alloc_id).unwrap_memory();
assert!(offset == abi::Size::ZERO, "{}", msg);
let meta = b.to_target_usize(ecx).expect(msg);
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_const_eval/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -574,7 +574,7 @@ impl<'a> ReportErrorExt for UndefinedBehaviorInfo<'a> {
if addr != 0 {
diag.arg(
"pointer",
Pointer::<Option<CtfeProvenance>>::from_addr_invalid(addr).to_string(),
Pointer::<Option<CtfeProvenance>>::without_provenance(addr).to_string(),
);
}

Expand Down
5 changes: 2 additions & 3 deletions compiler/rustc_const_eval/src/interpret/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -747,7 +747,7 @@ pub macro compile_time_machine(<$tcx: lifetime>) {
// Allow these casts, but make the pointer not dereferenceable.
// (I.e., they behave like transmutation.)
// This is correct because no pointers can ever be exposed in compile-time evaluation.
interp_ok(Pointer::from_addr_invalid(addr))
interp_ok(Pointer::without_provenance(addr))
}

#[inline(always)]
Expand All @@ -756,8 +756,7 @@ pub macro compile_time_machine(<$tcx: lifetime>) {
ptr: Pointer<CtfeProvenance>,
_size: i64,
) -> Option<(AllocId, Size, Self::ProvenanceExtra)> {
// We know `offset` is relative to the allocation, so we can use `into_parts`.
let (prov, offset) = ptr.into_parts();
let (prov, offset) = ptr.prov_and_relative_offset();
Some((prov.alloc_id(), offset, prov.immutable()))
}

Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_const_eval/src/interpret/memory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1596,7 +1596,8 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
Some((alloc_id, offset, extra)) => Ok((alloc_id, offset, extra)),
None => {
assert!(M::Provenance::OFFSET_IS_ADDR);
let (_, addr) = ptr.into_parts();
// Offset is absolute, as we just asserted.
let (_, addr) = ptr.into_raw_parts();
Err(addr.bytes())
}
},
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_const_eval/src/interpret/place.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ impl<'tcx, Prov: Provenance> MPlaceTy<'tcx, Prov> {
pub fn fake_alloc_zst(layout: TyAndLayout<'tcx>) -> Self {
assert!(layout.is_zst());
let align = layout.align.abi;
let ptr = Pointer::from_addr_invalid(align.bytes()); // no provenance, absolute address
let ptr = Pointer::without_provenance(align.bytes()); // no provenance, absolute address
MPlaceTy { mplace: MemPlace { ptr, meta: MemPlaceMeta::None, misaligned: None }, layout }
}

Expand Down
10 changes: 6 additions & 4 deletions compiler/rustc_const_eval/src/interpret/validity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -518,7 +518,7 @@ impl<'rt, 'tcx, M: Machine<'tcx>> ValidityVisitor<'rt, 'tcx, M> {
Ub(DanglingIntPointer { addr: i, .. }) => DanglingPtrNoProvenance {
ptr_kind,
// FIXME this says "null pointer" when null but we need translate
pointer: format!("{}", Pointer::<Option<AllocId>>::from_addr_invalid(i))
pointer: format!("{}", Pointer::<Option<AllocId>>::without_provenance(i))
},
Ub(PointerOutOfBounds { .. }) => DanglingPtrOutOfBounds {
ptr_kind
Expand Down Expand Up @@ -868,7 +868,9 @@ impl<'rt, 'tcx, M: Machine<'tcx>> ValidityVisitor<'rt, 'tcx, M> {
fn add_data_range(&mut self, ptr: Pointer<Option<M::Provenance>>, size: Size) {
if let Some(data_bytes) = self.data_bytes.as_mut() {
// We only have to store the offset, the rest is the same for all pointers here.
let (_prov, offset) = ptr.into_parts();
// The logic is agnostic to wether the offset is relative or absolute as long as
// it is consistent.
let (_prov, offset) = ptr.into_raw_parts();
// Add this.
data_bytes.add_range(offset, size);
};
Expand All @@ -894,7 +896,7 @@ impl<'rt, 'tcx, M: Machine<'tcx>> ValidityVisitor<'rt, 'tcx, M> {
.as_mplace_or_imm()
.expect_left("place must be in memory")
.ptr();
let (_prov, offset) = ptr.into_parts();
let (_prov, offset) = ptr.into_raw_parts();
offset
}

Expand All @@ -903,7 +905,7 @@ impl<'rt, 'tcx, M: Machine<'tcx>> ValidityVisitor<'rt, 'tcx, M> {
// Our value must be in memory, otherwise we would not have set up `data_bytes`.
let mplace = self.ecx.force_allocation(place)?;
// Determine starting offset and size.
let (_prov, start_offset) = mplace.ptr().into_parts();
let (_prov, start_offset) = mplace.ptr().into_raw_parts();
let (size, _align) = self
.ecx
.size_and_align_of_val(&mplace)?
Expand Down
5 changes: 3 additions & 2 deletions compiler/rustc_middle/src/mir/consts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -168,8 +168,9 @@ impl<'tcx> ConstValue<'tcx> {
return Some(&[]);
}
// Non-empty slice, must have memory. We know this is a relative pointer.
let (inner_prov, offset) = ptr.into_parts();
let data = tcx.global_alloc(inner_prov?.alloc_id()).unwrap_memory();
let (inner_prov, offset) =
ptr.into_pointer_or_addr().ok()?.prov_and_relative_offset();
let data = tcx.global_alloc(inner_prov.alloc_id()).unwrap_memory();
(data, offset.bytes(), offset.bytes() + len)
}
};
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_middle/src/mir/interpret/allocation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -526,7 +526,7 @@ impl Allocation {
let ptr_bytes = &mut bytes[idx..idx + ptr_size];
let bits = read_target_uint(endian, ptr_bytes).unwrap();
let (ptr_prov, ptr_offset) =
adjust_ptr(Pointer::new(alloc_id, Size::from_bytes(bits)))?.into_parts();
adjust_ptr(Pointer::new(alloc_id, Size::from_bytes(bits)))?.into_raw_parts();
write_target_uint(endian, ptr_bytes, ptr_offset.bytes().into()).unwrap();
new_provenance.push((offset, ptr_prov));
}
Expand Down Expand Up @@ -769,7 +769,7 @@ impl<Prov: Provenance, Extra, Bytes: AllocBytes> Allocation<Prov, Extra, Bytes>
// as-is into memory. This also double-checks that `val.size()` matches `range.size`.
let (bytes, provenance) = match val.to_bits_or_ptr_internal(range.size)? {
Right(ptr) => {
let (provenance, offset) = ptr.into_parts();
let (provenance, offset) = ptr.into_raw_parts();
(u128::from(offset.bytes()), Some(provenance))
}
Left(data) => (data, None),
Expand Down
25 changes: 16 additions & 9 deletions compiler/rustc_middle/src/mir/interpret/pointer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ impl From<CtfeProvenance> for Pointer {
impl<Prov> From<Pointer<Prov>> for Pointer<Option<Prov>> {
#[inline(always)]
fn from(ptr: Pointer<Prov>) -> Self {
let (prov, offset) = ptr.into_parts();
let (prov, offset) = ptr.into_raw_parts();
Pointer::new(Some(prov), offset)
}
}
Expand All @@ -314,19 +314,17 @@ impl<Prov> Pointer<Option<Prov>> {
assert!(Prov::OFFSET_IS_ADDR);
self.offset
}
}

impl<Prov> Pointer<Option<Prov>> {
/// Creates a pointer to the given address, with invalid provenance (i.e., cannot be used for
/// any memory access).
#[inline(always)]
pub fn from_addr_invalid(addr: u64) -> Self {
pub fn without_provenance(addr: u64) -> Self {
Pointer { provenance: None, offset: Size::from_bytes(addr) }
}

#[inline(always)]
pub fn null() -> Self {
Pointer::from_addr_invalid(0)
Pointer::without_provenance(0)
}
}

Expand All @@ -336,11 +334,11 @@ impl<Prov> Pointer<Prov> {
Pointer { provenance, offset }
}

/// Obtain the constituents of this pointer. Not that the meaning of the offset depends on the type `Prov`!
/// This function must only be used in the implementation of `Machine::ptr_get_alloc`,
/// and when a `Pointer` is taken apart to be stored efficiently in an `Allocation`.
/// Obtain the constituents of this pointer. Note that the meaning of the offset depends on the
/// type `Prov`! This is a low-level function that should only be used when absolutely
/// necessary. Prefer `prov_and_relative_offset` if possible.
#[inline(always)]
pub fn into_parts(self) -> (Prov, Size) {
pub fn into_raw_parts(self) -> (Prov, Size) {
(self.provenance, self.offset)
}

Expand All @@ -361,3 +359,12 @@ impl<Prov> Pointer<Prov> {
self.wrapping_offset(Size::from_bytes(i as u64), cx)
}
}

impl Pointer<CtfeProvenance> {
/// Return the provenance and relative offset stored in this pointer. Safer alternative to
/// `into_raw_parts` since the type ensures that the offset is indeed relative.
#[inline(always)]
pub fn prov_and_relative_offset(self) -> (CtfeProvenance, Size) {
(self.provenance, self.offset)
}
}
6 changes: 3 additions & 3 deletions compiler/rustc_middle/src/mir/interpret/value.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ impl<Prov> Scalar<Prov> {
/// Create a Scalar from a pointer with an `Option<_>` provenance (where `None` represents a
/// plain integer / "invalid" pointer).
pub fn from_maybe_pointer(ptr: Pointer<Option<Prov>>, cx: &impl HasDataLayout) -> Self {
match ptr.into_parts() {
match ptr.into_raw_parts() {
(Some(prov), offset) => Scalar::from_pointer(Pointer::new(prov, offset), cx),
(None, offset) => {
Scalar::Int(ScalarInt::try_from_uint(offset.bytes(), cx.pointer_size()).unwrap())
Expand Down Expand Up @@ -276,7 +276,7 @@ impl<'tcx, Prov: Provenance> Scalar<Prov> {
Right(ptr) => interp_ok(ptr.into()),
Left(bits) => {
let addr = u64::try_from(bits).unwrap();
interp_ok(Pointer::from_addr_invalid(addr))
interp_ok(Pointer::without_provenance(addr))
}
}
}
Expand All @@ -299,7 +299,7 @@ impl<'tcx, Prov: Provenance> Scalar<Prov> {
Ok(ScalarInt::try_from_uint(ptr.offset.bytes(), Size::from_bytes(sz)).unwrap())
} else {
// We know `offset` is relative, since `OFFSET_IS_ADDR == false`.
let (prov, offset) = ptr.into_parts();
let (prov, offset) = ptr.into_raw_parts();
// Because `OFFSET_IS_ADDR == false`, this unwrap can never fail.
Err(Scalar::Ptr(Pointer::new(prov.get_alloc_id().unwrap(), offset), sz))
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_middle/src/ty/print/pretty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1755,7 +1755,7 @@ pub trait PrettyPrinter<'tcx>: Printer<'tcx> + fmt::Write {
) -> Result<(), PrintError> {
define_scoped_cx!(self);

let (prov, offset) = ptr.into_parts();
let (prov, offset) = ptr.prov_and_relative_offset();
match ty.kind() {
// Byte strings (&[u8; N])
ty::Ref(_, inner, _) => {
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir_transform/src/gvn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1636,7 +1636,7 @@ fn op_to_prop_const<'tcx>(
}

let pointer = mplace.ptr().into_pointer_or_addr().ok()?;
let (prov, offset) = pointer.into_parts();
let (prov, offset) = pointer.prov_and_relative_offset();
let alloc_id = prov.alloc_id();
intern_const_alloc_for_constprop(ecx, alloc_id).discard_err()?;

Expand Down
4 changes: 2 additions & 2 deletions src/tools/miri/src/alloc_addresses/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
) -> InterpResult<'tcx, interpret::Pointer<Provenance>> {
let this = self.eval_context_ref();

let (prov, offset) = ptr.into_parts(); // offset is relative (AllocId provenance)
let (prov, offset) = ptr.prov_and_relative_offset();
let alloc_id = prov.alloc_id();

// Get a pointer to the beginning of this allocation.
Expand Down Expand Up @@ -447,7 +447,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
) -> Option<(AllocId, Size)> {
let this = self.eval_context_ref();

let (tag, addr) = ptr.into_parts(); // addr is absolute (Tag provenance)
let (tag, addr) = ptr.into_raw_parts(); // addr is absolute (Miri provenance)

let alloc_id = if let Provenance::Concrete { alloc_id, .. } = tag {
alloc_id
Expand Down
2 changes: 1 addition & 1 deletion src/tools/miri/src/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ impl interpret::Provenance for Provenance {
}

fn fmt(ptr: &interpret::Pointer<Self>, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let (prov, addr) = ptr.into_parts(); // address is absolute
let (prov, addr) = ptr.into_raw_parts(); // offset is absolute address
write!(f, "{:#x}", addr.bytes())?;
if f.alternate() {
write!(f, "{prov:#?}")?;
Expand Down
6 changes: 2 additions & 4 deletions src/tools/miri/src/provenance_gc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,15 +68,13 @@ impl VisitProvenance for Provenance {

impl VisitProvenance for StrictPointer {
fn visit_provenance(&self, visit: &mut VisitWith<'_>) {
let (prov, _offset) = self.into_parts();
prov.visit_provenance(visit);
self.provenance.visit_provenance(visit);
}
}

impl VisitProvenance for Pointer {
fn visit_provenance(&self, visit: &mut VisitWith<'_>) {
let (prov, _offset) = self.into_parts();
prov.visit_provenance(visit);
self.provenance.visit_provenance(visit);
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/tools/miri/src/shims/foreign_items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -411,7 +411,7 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
AlignFromBytesError::TooLarge(_) => Align::MAX,
}
});
let (_, addr) = ptr.into_parts(); // we know the offset is absolute
let addr = ptr.addr();
// Cannot panic since `align` is a power of 2 and hence non-zero.
if addr.bytes().strict_rem(align.bytes()) != 0 {
throw_unsup_format!(
Expand Down
2 changes: 1 addition & 1 deletion src/tools/miri/src/shims/unix/mem.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
&& matches!(&*this.tcx.sess.target.os, "macos" | "solaris" | "illumos")
&& (flags & map_fixed) != 0
{
return interp_ok(Scalar::from_maybe_pointer(Pointer::from_addr_invalid(addr), this));
return interp_ok(Scalar::from_maybe_pointer(Pointer::without_provenance(addr), this));
}

let prot_read = this.eval_libc_i32("PROT_READ");
Expand Down
Loading