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

Lower index bounds checking to PtrMetadata, this time with the right fake borrow semantics 😸 #135748

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
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
Prev Previous commit
Represent the raw pointer for a array length check as a new kind of f…
…ake borrow
  • Loading branch information
compiler-errors committed Jan 20, 2025
commit a8e871e3c18fe312db81a475ca0a85ab8697016d
11 changes: 7 additions & 4 deletions compiler/rustc_borrowck/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1284,15 +1284,18 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, '_, 'tcx> {
);
}

&Rvalue::RawPtr(mutability, place) => {
let access_kind = match mutability {
Mutability::Mut => (
&Rvalue::RawPtr(kind, place) => {
let access_kind = match kind {
RawPtrKind::Mut => (
Deep,
Write(WriteKind::MutableBorrow(BorrowKind::Mut {
kind: MutBorrowKind::Default,
})),
),
Mutability::Not => (Deep, Read(ReadKind::Borrow(BorrowKind::Shared))),
RawPtrKind::Not => (Deep, Read(ReadKind::Borrow(BorrowKind::Shared))),
RawPtrKind::FakeForPtrMetadata => {
(Shallow(Some(ArtificialField::ArrayLength)), Read(ReadKind::Copy))
}
};

self.access_place(
Expand Down
21 changes: 10 additions & 11 deletions compiler/rustc_borrowck/src/polonius/legacy/loan_invalidations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,7 @@ use std::ops::ControlFlow;
use rustc_data_structures::graph::dominators::Dominators;
use rustc_middle::bug;
use rustc_middle::mir::visit::Visitor;
use rustc_middle::mir::{
self, BasicBlock, Body, BorrowKind, FakeBorrowKind, InlineAsmOperand, Location, Mutability,
NonDivergingIntrinsic, Operand, Place, Rvalue, Statement, StatementKind, Terminator,
TerminatorKind,
};
use rustc_middle::mir::*;
use rustc_middle::ty::TyCtxt;
use tracing::debug;

Expand Down Expand Up @@ -60,7 +56,7 @@ impl<'a, 'tcx> Visitor<'tcx> for LoanInvalidationsGenerator<'a, 'tcx> {
StatementKind::Intrinsic(box NonDivergingIntrinsic::Assume(op)) => {
self.consume_operand(location, op);
}
StatementKind::Intrinsic(box NonDivergingIntrinsic::CopyNonOverlapping(mir::CopyNonOverlapping {
StatementKind::Intrinsic(box NonDivergingIntrinsic::CopyNonOverlapping(CopyNonOverlapping {
src,
dst,
count,
Expand Down Expand Up @@ -273,15 +269,18 @@ impl<'a, 'tcx> LoanInvalidationsGenerator<'a, 'tcx> {
self.access_place(location, place, access_kind, LocalMutationIsAllowed::No);
}

&Rvalue::RawPtr(mutability, place) => {
let access_kind = match mutability {
Mutability::Mut => (
&Rvalue::RawPtr(kind, place) => {
let access_kind = match kind {
RawPtrKind::Mut => (
Deep,
Write(WriteKind::MutableBorrow(BorrowKind::Mut {
kind: mir::MutBorrowKind::Default,
kind: MutBorrowKind::Default,
})),
),
Mutability::Not => (Deep, Read(ReadKind::Borrow(BorrowKind::Shared))),
RawPtrKind::Not => (Deep, Read(ReadKind::Borrow(BorrowKind::Shared))),
RawPtrKind::FakeForPtrMetadata => {
(Shallow(Some(ArtificialField::ArrayLength)), Read(ReadKind::Copy))
}
};

self.access_place(location, place, access_kind, LocalMutationIsAllowed::No);
Expand Down
7 changes: 4 additions & 3 deletions compiler/rustc_codegen_ssa/src/mir/rvalue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -601,9 +601,10 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
mir::Rvalue::CopyForDeref(place) => {
self.codegen_operand(bx, &mir::Operand::Copy(place))
}
mir::Rvalue::RawPtr(mutability, place) => {
let mk_ptr =
move |tcx: TyCtxt<'tcx>, ty: Ty<'tcx>| Ty::new_ptr(tcx, ty, mutability);
mir::Rvalue::RawPtr(kind, place) => {
let mk_ptr = move |tcx: TyCtxt<'tcx>, ty: Ty<'tcx>| {
Ty::new_ptr(tcx, ty, kind.to_mutbl_lossy())
};
self.codegen_place_to_pointer(bx, place, mk_ptr)
}

Expand Down
8 changes: 6 additions & 2 deletions compiler/rustc_const_eval/src/check_consts/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -518,7 +518,7 @@ impl<'tcx> Visitor<'tcx> for Checker<'_, 'tcx> {
}

Rvalue::Ref(_, BorrowKind::Mut { .. }, place)
| Rvalue::RawPtr(Mutability::Mut, place) => {
| Rvalue::RawPtr(RawPtrKind::Mut, place) => {
// Inside mutable statics, we allow arbitrary mutable references.
// We've allowed `static mut FOO = &mut [elements];` for a long time (the exact
// reasons why are lost to history), and there is no reason to restrict that to
Expand All @@ -536,7 +536,7 @@ impl<'tcx> Visitor<'tcx> for Checker<'_, 'tcx> {
}

Rvalue::Ref(_, BorrowKind::Shared | BorrowKind::Fake(_), place)
| Rvalue::RawPtr(Mutability::Not, place) => {
| Rvalue::RawPtr(RawPtrKind::Not, place) => {
let borrowed_place_has_mut_interior = qualifs::in_place::<HasMutInterior, _>(
self.ccx,
&mut |local| self.qualifs.has_mut_interior(self.ccx, local, location),
Expand All @@ -548,6 +548,10 @@ impl<'tcx> Visitor<'tcx> for Checker<'_, 'tcx> {
}
}

Rvalue::RawPtr(RawPtrKind::FakeForPtrMetadata, _) => {
// Do nothing.
Copy link
Member

Choose a reason for hiding this comment

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

Uh, this is dangerous. What is the justification for allowing all these raw borrows in const? It seems to be this relies on a non-local invariant about where and how FakeForPtrMetadata can occur. I am not a fan if such invariants; I'd prefer the const checker to be correct at least for all MIR that passes the MIR validator.

}

Rvalue::Cast(
CastKind::PointerCoercion(
PointerCoercion::MutToConstPointer
Expand Down
15 changes: 3 additions & 12 deletions compiler/rustc_const_eval/src/interpret/step.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ use rustc_middle::ty::layout::FnAbiOf;
use rustc_middle::ty::{self, Instance, Ty};
use rustc_middle::{bug, mir, span_bug};
use rustc_span::source_map::Spanned;
use rustc_span::{DesugaringKind, Span};
use rustc_target::callconv::FnAbi;
use tracing::{info, instrument, trace};

Expand Down Expand Up @@ -81,9 +80,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
use rustc_middle::mir::StatementKind::*;

match &stmt.kind {
Assign(box (place, rvalue)) => {
self.eval_rvalue_into_place(rvalue, *place, stmt.source_info.span)?
}
Assign(box (place, rvalue)) => self.eval_rvalue_into_place(rvalue, *place)?,

SetDiscriminant { place, variant_index } => {
let dest = self.eval_place(**place)?;
Expand Down Expand Up @@ -162,7 +159,6 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
&mut self,
rvalue: &mir::Rvalue<'tcx>,
place: mir::Place<'tcx>,
span: Span,
) -> InterpResult<'tcx> {
let dest = self.eval_place(place)?;
// FIXME: ensure some kind of non-aliasing between LHS and RHS?
Expand Down Expand Up @@ -241,7 +237,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
self.write_immediate(*val, &dest)?;
}

RawPtr(_, place) => {
RawPtr(kind, place) => {
// Figure out whether this is an addr_of of an already raw place.
let place_base_raw = if place.is_indirect_first_projection() {
let ty = self.frame().body.local_decls[place.local].ty;
Expand All @@ -254,13 +250,8 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
let src = self.eval_place(place)?;
let place = self.force_allocation(&src)?;
let mut val = ImmTy::from_immediate(place.to_ref(self), dest.layout);
if !place_base_raw
&& span.desugaring_kind() != Some(DesugaringKind::IndexBoundsCheckReborrow)
{
if !place_base_raw && !kind.is_fake() {
// If this was not already raw, it needs retagging.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// If this was not already raw, it needs retagging.
// If this was not already raw, it needs retagging -- except for "fake"
// raw borrows whose defining property is that they do not get retagged.

// As a special hack, we exclude the desugared `PtrMetadata(&raw const *_n)`
// from indexing. (Really we should not do any retag on `&raw` but that does not
// currently work with Stacked Borrows.)
val = M::retag_ptr_value(self, mir::RetagKind::Raw, &val)?;
}
self.write_immediate(*val, &dest)?;
Expand Down
47 changes: 46 additions & 1 deletion compiler/rustc_middle/src/mir/syntax.rs
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,51 @@ pub enum BorrowKind {
Mut { kind: MutBorrowKind },
}

#[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord, TyEncodable, TyDecodable)]
#[derive(Hash, HashStable)]
pub enum RawPtrKind {
Mut,
Not,
FakeForPtrMetadata,
Copy link
Member

Choose a reason for hiding this comment

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

Please add a doc comment explaining this variant.

}

impl From<Mutability> for RawPtrKind {
fn from(other: Mutability) -> Self {
match other {
Mutability::Mut => RawPtrKind::Mut,
Mutability::Not => RawPtrKind::Not,
}
}
}

impl RawPtrKind {
pub fn is_fake(self) -> bool {
match self {
RawPtrKind::Mut | RawPtrKind::Not => false,
RawPtrKind::FakeForPtrMetadata => true,
}
}

pub fn to_mutbl_lossy(self) -> Mutability {
match self {
RawPtrKind::Mut => Mutability::Mut,
RawPtrKind::Not => Mutability::Not,

// We have no type corresponding to a shallow borrow, so use
// `&` as an approximation.
Comment on lines +213 to +214
Copy link
Member

Choose a reason for hiding this comment

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

This comment looks like it still needs updating.

RawPtrKind::FakeForPtrMetadata => Mutability::Not,
}
}

pub fn ptr_str(self) -> &'static str {
match self {
RawPtrKind::Mut => "mut",
RawPtrKind::Not => "const",
RawPtrKind::FakeForPtrMetadata => "const (fake)",
}
}
}

#[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord, TyEncodable, TyDecodable)]
#[derive(Hash, HashStable)]
pub enum MutBorrowKind {
Expand Down Expand Up @@ -1349,7 +1394,7 @@ pub enum Rvalue<'tcx> {
///
/// Like with references, the semantics of this operation are heavily dependent on the aliasing
/// model.
RawPtr(Mutability, Place<'tcx>),
RawPtr(RawPtrKind, Place<'tcx>),

/// Yields the length of the place, as a `usize`.
///
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_middle/src/mir/tcx.rs
Original file line number Diff line number Diff line change
Expand Up @@ -206,9 +206,9 @@ impl<'tcx> Rvalue<'tcx> {
let place_ty = place.ty(local_decls, tcx).ty;
Ty::new_ref(tcx, reg, place_ty, bk.to_mutbl_lossy())
}
Rvalue::RawPtr(mutability, ref place) => {
Rvalue::RawPtr(kind, ref place) => {
let place_ty = place.ty(local_decls, tcx).ty;
Ty::new_ptr(tcx, place_ty, mutability)
Ty::new_ptr(tcx, place_ty, kind.to_mutbl_lossy())
}
Rvalue::Len(..) => tcx.types.usize,
Rvalue::Cast(.., ty) => 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 @@ -15,6 +15,7 @@ TrivialTypeTraversalImpls! {
SourceScopeLocalData,
UserTypeAnnotationIndex,
BorrowKind,
RawPtrKind,
CastKind,
BasicBlock,
SwitchTargets,
Expand Down
7 changes: 5 additions & 2 deletions compiler/rustc_middle/src/mir/visit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -685,12 +685,15 @@ macro_rules! make_mir_visitor {

Rvalue::RawPtr(m, path) => {
let ctx = match m {
Mutability::Mut => PlaceContext::MutatingUse(
RawPtrKind::Mut => PlaceContext::MutatingUse(
MutatingUseContext::RawBorrow
),
Mutability::Not => PlaceContext::NonMutatingUse(
RawPtrKind::Not => PlaceContext::NonMutatingUse(
NonMutatingUseContext::RawBorrow
),
RawPtrKind::FakeForPtrMetadata => PlaceContext::NonMutatingUse(
NonMutatingUseContext::Inspect
),
};
self.visit_place(path, ctx, location);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ impl<'a, 'tcx> ParseCtxt<'a, 'tcx> {
Rvalue::Ref(self.tcx.lifetimes.re_erased, *borrow_kind, self.parse_place(*arg)?)
),
ExprKind::RawBorrow { mutability, arg } => Ok(
Rvalue::RawPtr(*mutability, self.parse_place(*arg)?)
Rvalue::RawPtr((*mutability).into(), self.parse_place(*arg)?)
),
ExprKind::Binary { op, lhs, rhs } => Ok(
Rvalue::BinaryOp(*op, Box::new((self.parse_operand(*lhs)?, self.parse_operand(*rhs)?)))
Expand Down
21 changes: 5 additions & 16 deletions compiler/rustc_mir_build/src/builder/expr/as_place.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use rustc_middle::mir::*;
use rustc_middle::thir::*;
use rustc_middle::ty::{self, AdtDef, CanonicalUserTypeAnnotation, Ty, Variance};
use rustc_middle::{bug, span_bug};
use rustc_span::{DesugaringKind, Span};
use rustc_span::Span;
use tracing::{debug, instrument, trace};

use crate::builder::ForGuard::{OutsideGuard, RefWithinGuard};
Expand Down Expand Up @@ -643,8 +643,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
source_info: SourceInfo,
) -> Operand<'tcx> {
let place_ty = place.ty(&self.local_decls, self.tcx).ty;
let usize_ty = self.tcx.types.usize;

match place_ty.kind() {
ty::Array(_elem_ty, len_const) => {
// We know how long an array is, so just use that as a constant
Expand All @@ -653,7 +651,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
// though. FIXME: Do we really *need* to count this as a use?
// Could partial array tracking work off something else instead?
self.cfg.push_fake_read(block, source_info, FakeReadCause::ForIndex, place);
let const_ = Const::from_ty_const(*len_const, usize_ty, self.tcx);
let const_ = Const::from_ty_const(*len_const, self.tcx.types.usize, self.tcx);
Operand::Constant(Box::new(ConstOperand { span, user_ty: None, const_ }))
}
ty::Slice(_elem_ty) => {
Expand All @@ -668,27 +666,18 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
// the MIR we're building here needs to pass NLL later.
Operand::Copy(Place::from(place.local))
} else {
let len_span = self.tcx.with_stable_hashing_context(|hcx| {
let span = source_info.span;
span.mark_with_reason(
None,
DesugaringKind::IndexBoundsCheckReborrow,
span.edition(),
hcx,
)
});
let ptr_ty = Ty::new_imm_ptr(self.tcx, place_ty);
let slice_ptr = self.temp(ptr_ty, span);
self.cfg.push_assign(
block,
SourceInfo { span: len_span, ..source_info },
source_info,
slice_ptr,
Rvalue::RawPtr(Mutability::Not, place),
Rvalue::RawPtr(RawPtrKind::FakeForPtrMetadata, place),
);
Operand::Move(slice_ptr)
};

let len = self.temp(usize_ty, span);
let len = self.temp(self.tcx.types.usize, span);
self.cfg.push_assign(
block,
source_info,
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir_build/src/builder/expr/into.rs
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
hir::Mutability::Not => this.as_read_only_place(block, arg),
hir::Mutability::Mut => this.as_place(block, arg),
};
let address_of = Rvalue::RawPtr(mutability, unpack!(block = place));
let address_of = Rvalue::RawPtr(mutability.into(), unpack!(block = place));
this.cfg.push_assign(block, source_info, destination, address_of);
block.unit()
}
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_mir_dataflow/src/elaborate_drops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -700,7 +700,7 @@ where
statements: vec![
self.assign(
ptr,
Rvalue::RawPtr(Mutability::Mut, tcx.mk_place_index(self.place, cur)),
Rvalue::RawPtr(RawPtrKind::Mut, tcx.mk_place_index(self.place, cur)),
),
self.assign(
cur.into(),
Expand Down Expand Up @@ -816,7 +816,7 @@ where

let mut delegate_block = BasicBlockData {
statements: vec![
self.assign(Place::from(array_ptr), Rvalue::RawPtr(Mutability::Mut, self.place)),
self.assign(Place::from(array_ptr), Rvalue::RawPtr(RawPtrKind::Mut, self.place)),
self.assign(
Place::from(slice_ptr),
Rvalue::Cast(
Expand Down
6 changes: 4 additions & 2 deletions compiler/rustc_mir_transform/src/gvn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ enum AggregateTy<'tcx> {
#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
enum AddressKind {
Ref(BorrowKind),
Address(Mutability),
Address(RawPtrKind),
}

#[derive(Debug, PartialEq, Eq, Hash)]
Expand Down Expand Up @@ -500,7 +500,9 @@ impl<'body, 'tcx> VnState<'body, 'tcx> {
mplace.layout.ty,
bk.to_mutbl_lossy(),
),
AddressKind::Address(mutbl) => Ty::new_ptr(self.tcx, mplace.layout.ty, mutbl),
AddressKind::Address(mutbl) => {
Ty::new_ptr(self.tcx, mplace.layout.ty, mutbl.to_mutbl_lossy())
}
};
let layout = self.ecx.layout_of(ty).ok()?;
ImmTy::from_immediate(pointer, layout).into()
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_mir_transform/src/large_enums.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ impl<'tcx> crate::MirPass<'tcx> for EnumSizeOpt {
source_info,
kind: StatementKind::Assign(Box::new((
dst,
Rvalue::RawPtr(Mutability::Mut, *lhs),
Rvalue::RawPtr(RawPtrKind::Mut, *lhs),
))),
};

Expand All @@ -146,7 +146,7 @@ impl<'tcx> crate::MirPass<'tcx> for EnumSizeOpt {
source_info,
kind: StatementKind::Assign(Box::new((
src,
Rvalue::RawPtr(Mutability::Not, *rhs),
Rvalue::RawPtr(RawPtrKind::Not, *rhs),
))),
};

Expand Down
Loading
Loading