Skip to content

Commit

Permalink
Auto merge of rust-lang#115524 - RalfJung:misalign, r=wesleywiser
Browse files Browse the repository at this point in the history 
const-eval: make misalignment a hard error

It's been a future-incompat error (showing up in cargo's reports) since rust-lang#104616, Rust 1.68, released in March.  That should be long enough.

The question for the lang team is simply -- should we move ahead with this, making const-eval alignment failures a hard error? (It turns out some of them accidentally already were hard errors since rust-lang#104616. But not all so this is still a breaking change. Crater found no regression.)
  • Loading branch information
@bors
bors committed Oct 14, 2023
2 parents fcab248 + bd33846 commit 75a5dd0
Show file tree
Hide file tree
Showing 12 changed files with 114 additions and 193 deletions.
14 changes: 3 additions & 11 deletions compiler/rustc_const_eval/src/const_eval/eval_queries.rs
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,3 @@
use crate::const_eval::CheckAlignment;
use crate::errors::ConstEvalError;

use either::{Left, Right};

use rustc_hir::def::DefKind;
Expand All @@ -15,7 +12,9 @@ use rustc_span::source_map::Span;
use rustc_target::abi::{self, Abi};

use super::{CanAccessStatics, CompileTimeEvalContext, CompileTimeInterpreter};
use crate::const_eval::CheckAlignment;
use crate::errors;
use crate::errors::ConstEvalError;
use crate::interpret::eval_nullary_intrinsic;
use crate::interpret::{
intern_const_alloc_recursive, CtfeValidationMode, GlobalId, Immediate, InternKind, InterpCx,
Expand Down Expand Up @@ -290,14 +289,7 @@ pub fn eval_to_allocation_raw_provider<'tcx>(
key.param_env,
// Statics (and promoteds inside statics) may access other statics, because unlike consts
// they do not have to behave "as if" they were evaluated at runtime.
CompileTimeInterpreter::new(
CanAccessStatics::from(is_static),
if tcx.sess.opts.unstable_opts.extra_const_ub_checks {
CheckAlignment::Error
} else {
CheckAlignment::FutureIncompat
},
),
CompileTimeInterpreter::new(CanAccessStatics::from(is_static), CheckAlignment::Error),
);

let res = ecx.load_mir(cid.instance.def, cid.promoted);
Expand Down
55 changes: 5 additions & 50 deletions compiler/rustc_const_eval/src/const_eval/machine.rs
View file
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
use rustc_hir::def::DefKind;
use rustc_hir::{LangItem, CRATE_HIR_ID};
use rustc_hir::LangItem;
use rustc_middle::mir;
use rustc_middle::mir::interpret::PointerArithmetic;
use rustc_middle::ty::layout::{FnAbiOf, TyAndLayout};
use rustc_middle::ty::{self, TyCtxt};
use rustc_session::lint::builtin::INVALID_ALIGNMENT;
use std::borrow::Borrow;
use std::hash::Hash;
use std::ops::ControlFlow;
Expand All @@ -21,11 +20,11 @@ use rustc_target::abi::{Align, Size};
use rustc_target::spec::abi::Abi as CallAbi;

use crate::errors::{LongRunning, LongRunningWarn};
use crate::fluent_generated as fluent;
use crate::interpret::{
self, compile_time_machine, AllocId, ConstAllocation, FnArg, FnVal, Frame, ImmTy, InterpCx,
InterpResult, OpTy, PlaceTy, Pointer, Scalar,
};
use crate::{errors, fluent_generated as fluent};

use super::error::*;

Expand Down Expand Up @@ -65,22 +64,11 @@ pub struct CompileTimeInterpreter<'mir, 'tcx> {

#[derive(Copy, Clone)]
pub enum CheckAlignment {
/// Ignore alignment when following relocations.
/// Ignore all alignment requirements.
/// This is mainly used in interning.
No,
/// Hard error when dereferencing a misaligned pointer.
Error,
/// Emit a future incompat lint when dereferencing a misaligned pointer.
FutureIncompat,
}

impl CheckAlignment {
pub fn should_check(&self) -> bool {
match self {
CheckAlignment::No => false,
CheckAlignment::Error | CheckAlignment::FutureIncompat => true,
}
}
}

#[derive(Copy, Clone, PartialEq)]
Expand Down Expand Up @@ -358,48 +346,15 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir,
const PANIC_ON_ALLOC_FAIL: bool = false; // will be raised as a proper error

#[inline(always)]
fn enforce_alignment(ecx: &InterpCx<'mir, 'tcx, Self>) -> CheckAlignment {
ecx.machine.check_alignment
fn enforce_alignment(ecx: &InterpCx<'mir, 'tcx, Self>) -> bool {
matches!(ecx.machine.check_alignment, CheckAlignment::Error)
}

#[inline(always)]
fn enforce_validity(ecx: &InterpCx<'mir, 'tcx, Self>, layout: TyAndLayout<'tcx>) -> bool {
ecx.tcx.sess.opts.unstable_opts.extra_const_ub_checks || layout.abi.is_uninhabited()
}

fn alignment_check_failed(
ecx: &InterpCx<'mir, 'tcx, Self>,
has: Align,
required: Align,
check: CheckAlignment,
) -> InterpResult<'tcx, ()> {
let err = err_ub!(AlignmentCheckFailed { has, required }).into();
match check {
CheckAlignment::Error => Err(err),
CheckAlignment::No => span_bug!(
ecx.cur_span(),
"`alignment_check_failed` called when no alignment check requested"
),
CheckAlignment::FutureIncompat => {
let (_, backtrace) = err.into_parts();
backtrace.print_backtrace();
let (span, frames) = super::get_span_and_frames(&ecx);

ecx.tcx.emit_spanned_lint(
INVALID_ALIGNMENT,
ecx.stack().iter().find_map(|frame| frame.lint_root()).unwrap_or(CRATE_HIR_ID),
span,
errors::AlignmentCheckFailed {
has: has.bytes(),
required: required.bytes(),
frames,
},
);
Ok(())
}
}
}

fn load_mir(
ecx: &InterpCx<'mir, 'tcx, Self>,
instance: ty::InstanceDef<'tcx>,
Expand Down
13 changes: 2 additions & 11 deletions compiler/rustc_const_eval/src/interpret/machine.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,9 @@ use rustc_middle::mir;
use rustc_middle::ty::layout::TyAndLayout;
use rustc_middle::ty::{self, TyCtxt};
use rustc_span::def_id::DefId;
use rustc_target::abi::{Align, Size};
use rustc_target::abi::Size;
use rustc_target::spec::abi::Abi as CallAbi;

use crate::const_eval::CheckAlignment;

use super::{
AllocBytes, AllocId, AllocRange, Allocation, ConstAllocation, FnArg, Frame, ImmTy, InterpCx,
InterpResult, MPlaceTy, MemoryKind, OpTy, PlaceTy, Pointer, Provenance,
Expand Down Expand Up @@ -135,21 +133,14 @@ pub trait Machine<'mir, 'tcx: 'mir>: Sized {
const POST_MONO_CHECKS: bool = true;

/// Whether memory accesses should be alignment-checked.
fn enforce_alignment(ecx: &InterpCx<'mir, 'tcx, Self>) -> CheckAlignment;
fn enforce_alignment(ecx: &InterpCx<'mir, 'tcx, Self>) -> bool;

/// Whether, when checking alignment, we should look at the actual address and thus support
/// custom alignment logic based on whatever the integer address happens to be.
///
/// If this returns true, Provenance::OFFSET_IS_ADDR must be true.
fn use_addr_for_alignment_check(ecx: &InterpCx<'mir, 'tcx, Self>) -> bool;

fn alignment_check_failed(
ecx: &InterpCx<'mir, 'tcx, Self>,
has: Align,
required: Align,
check: CheckAlignment,
) -> InterpResult<'tcx, ()>;

/// Whether to enforce the validity invariant for a specific layout.
fn enforce_validity(ecx: &InterpCx<'mir, 'tcx, Self>, layout: TyAndLayout<'tcx>) -> bool;

Expand Down
49 changes: 18 additions & 31 deletions compiler/rustc_const_eval/src/interpret/memory.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ use rustc_middle::mir::display_allocation;
use rustc_middle::ty::{self, Instance, ParamEnv, Ty, TyCtxt};
use rustc_target::abi::{Align, HasDataLayout, Size};

use crate::const_eval::CheckAlignment;
use crate::fluent_generated as fluent;

use super::{
Expand Down Expand Up @@ -373,8 +372,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
self.check_and_deref_ptr(
ptr,
size,
align,
M::enforce_alignment(self),
M::enforce_alignment(self).then_some(align),
CheckInAllocMsg::MemoryAccessTest,
|alloc_id, offset, prov| {
let (size, align) = self
Expand All @@ -395,17 +393,10 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
align: Align,
msg: CheckInAllocMsg,
) -> InterpResult<'tcx> {
self.check_and_deref_ptr(
ptr,
size,
align,
CheckAlignment::Error,
msg,
|alloc_id, _, _| {
let (size, align) = self.get_live_alloc_size_and_align(alloc_id, msg)?;
Ok((size, align, ()))
},
)?;
self.check_and_deref_ptr(ptr, size, Some(align), msg, |alloc_id, _, _| {
let (size, align) = self.get_live_alloc_size_and_align(alloc_id, msg)?;
Ok((size, align, ()))
})?;
Ok(())
}

Expand All @@ -419,8 +410,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
&self,
ptr: Pointer<Option<M::Provenance>>,
size: Size,
align: Align,
check: CheckAlignment,
align: Option<Align>,
msg: CheckInAllocMsg,
alloc_size: impl FnOnce(
AllocId,
Expand All @@ -436,8 +426,8 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
throw_ub!(DanglingIntPointer(addr, msg));
}
// Must be aligned.
if check.should_check() {
self.check_offset_align(addr, align, check)?;
if let Some(align) = align {
self.check_offset_align(addr, align)?;
}
None
}
Expand All @@ -460,16 +450,16 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
}
// Test align. Check this last; if both bounds and alignment are violated
// we want the error to be about the bounds.
if check.should_check() {
if let Some(align) = align {
if M::use_addr_for_alignment_check(self) {
// `use_addr_for_alignment_check` can only be true if `OFFSET_IS_ADDR` is true.
self.check_offset_align(ptr.addr().bytes(), align, check)?;
self.check_offset_align(ptr.addr().bytes(), align)?;
} else {
// Check allocation alignment and offset alignment.
if alloc_align.bytes() < align.bytes() {
M::alignment_check_failed(self, alloc_align, align, check)?;
throw_ub!(AlignmentCheckFailed { has: alloc_align, required: align });
}
self.check_offset_align(offset.bytes(), align, check)?;
self.check_offset_align(offset.bytes(), align)?;
}
}

Expand All @@ -480,18 +470,16 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
})
}

fn check_offset_align(
&self,
offset: u64,
align: Align,
check: CheckAlignment,
) -> InterpResult<'tcx> {
fn check_offset_align(&self, offset: u64, align: Align) -> InterpResult<'tcx> {
if offset % align.bytes() == 0 {
Ok(())
} else {
// The biggest power of two through which `offset` is divisible.
let offset_pow2 = 1 << offset.trailing_zeros();
M::alignment_check_failed(self, Align::from_bytes(offset_pow2).unwrap(), align, check)
throw_ub!(AlignmentCheckFailed {
has: Align::from_bytes(offset_pow2).unwrap(),
required: align
});
}
}
}
Expand Down Expand Up @@ -609,8 +597,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
let ptr_and_alloc = self.check_and_deref_ptr(
ptr,
size,
align,
M::enforce_alignment(self),
M::enforce_alignment(self).then_some(align),
CheckInAllocMsg::MemoryAccessTest,
|alloc_id, offset, prov| {
let alloc = self.get_alloc_raw(alloc_id)?;
Expand Down
3 changes: 1 addition & 2 deletions compiler/rustc_const_eval/src/interpret/place.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -500,8 +500,7 @@ where
.size_and_align_of_mplace(&mplace)?
.unwrap_or((mplace.layout.size, mplace.layout.align.abi));
// Due to packed places, only `mplace.align` matters.
let align =
if M::enforce_alignment(self).should_check() { mplace.align } else { Align::ONE };
let align = if M::enforce_alignment(self) { mplace.align } else { Align::ONE };
self.check_ptr_access_align(mplace.ptr(), size, align, CheckInAllocMsg::DerefTest)?;
Ok(())
}
Expand Down
5 changes: 5 additions & 0 deletions compiler/rustc_lint/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -506,6 +506,11 @@ fn register_builtins(store: &mut LintStore) {
"replaced with another group of lints, see RFC \
<https://rust-lang.github.io/rfcs/2145-type-privacy.html> for more information",
);
store.register_removed(
"invalid_alignment",
"converted into hard error, see PR #104616 \
<https://github.com/rust-lang/rust/pull/104616> for more information",
);
}

fn register_internals(store: &mut LintStore) {
Expand Down
40 changes: 0 additions & 40 deletions compiler/rustc_lint_defs/src/builtin.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -986,45 +986,6 @@ declare_lint! {
"detects trivial casts of numeric types which could be removed"
}

declare_lint! {
/// The `invalid_alignment` lint detects dereferences of misaligned pointers during
/// constant evaluation.
///
/// ### Example
///
/// ```rust,compile_fail
/// #![feature(const_mut_refs)]
/// const FOO: () = unsafe {
/// let x = &[0_u8; 4];
/// let y = x.as_ptr().cast::<u32>();
/// let mut z = 123;
/// y.copy_to_nonoverlapping(&mut z, 1); // the address of a `u8` array is unknown
/// // and thus we don't know if it is aligned enough for copying a `u32`.
/// };
/// ```
///
/// {{produces}}
///
/// ### Explanation
///
/// The compiler allowed dereferencing raw pointers irrespective of alignment
/// during const eval due to the const evaluator at the time not making it easy
/// or cheap to check. Now that it is both, this is not accepted anymore.
///
/// Since it was undefined behaviour to begin with, this breakage does not violate
/// Rust's stability guarantees. Using undefined behaviour can cause arbitrary
/// behaviour, including failure to build.
///
/// [future-incompatible]: ../index.md#future-incompatible-lints
pub INVALID_ALIGNMENT,
Deny,
"raw pointers must be aligned before dereferencing",
@future_incompatible = FutureIncompatibleInfo {
reason: FutureIncompatibilityReason::FutureReleaseErrorReportInDeps,
reference: "issue #68585 <https://github.com/rust-lang/rust/issues/104616>",
};
}

declare_lint! {
/// The `exported_private_dependencies` lint detects private dependencies
/// that are exposed in a public interface.
Expand Down Expand Up @@ -3430,7 +3391,6 @@ declare_lint_pass! {
INDIRECT_STRUCTURAL_MATCH,
INEFFECTIVE_UNSTABLE_TRAIT_IMPL,
INLINE_NO_SANITIZE,
INVALID_ALIGNMENT,
INVALID_DOC_ATTRIBUTES,
INVALID_MACRO_EXPORT_ARGUMENTS,
INVALID_TYPE_PARAM_DEFAULT,
Expand Down
Loading

0 comments on commit 75a5dd0

Please sign in to comment.