diff options
| author | bors <bors@rust-lang.org> | 2022-12-15 17:04:25 +0000 |
|---|---|---|
| committer | bors <bors@rust-lang.org> | 2022-12-15 17:04:25 +0000 |
| commit | 4954a7ef5c5bb45f1c0d021491702ad05d470821 (patch) | |
| tree | 69aff75808ced42e756b27f6a9812ac022ebcb3c /compiler/rustc_const_eval/src | |
| parent | 984eab57f708e62c09b3d708033fe620130b5f39 (diff) | |
| parent | 2d89027fac84d7ba95cf2d66b49136d3f0d08ba2 (diff) | |
| download | rust-4954a7ef5c5bb45f1c0d021491702ad05d470821.tar.gz rust-4954a7ef5c5bb45f1c0d021491702ad05d470821.zip | |
Auto merge of #104616 - RalfJung:ctfe-alignment, r=oli-obk,RalfJung
always check alignment during CTFE We originally disabled alignment checks because they got in the way -- there are some things we do with the interpreter during CTFE which does not correspond to actually running user-written code, but is purely administrative, and we didn't want alignment checks there, so we just disabled them entirely. But with `-Zextra-const-ub-checks` we anyway had to figure out how to disable those alignment checks while doing checks in regular code. So now it is easy to enable CTFE alignment checking by default. Let's see what the perf consequences of that are. r? `@oli-obk`
Diffstat (limited to 'compiler/rustc_const_eval/src')
8 files changed, 191 insertions, 124 deletions
diff --git a/compiler/rustc_const_eval/src/const_eval/error.rs b/compiler/rustc_const_eval/src/const_eval/error.rs index c60d6e4fed9..13472cc2bfa 100644 --- a/compiler/rustc_const_eval/src/const_eval/error.rs +++ b/compiler/rustc_const_eval/src/const_eval/error.rs @@ -86,6 +86,59 @@ impl<'tcx> ConstEvalErr<'tcx> { self.report_decorated(tcx, message, |_| {}) } + #[instrument(level = "trace", skip(self, decorate))] + pub(super) fn decorate(&self, err: &mut Diagnostic, decorate: impl FnOnce(&mut Diagnostic)) { + trace!("reporting const eval failure at {:?}", self.span); + // Add some more context for select error types. + match self.error { + InterpError::Unsupported( + UnsupportedOpInfo::ReadPointerAsBytes + | UnsupportedOpInfo::PartialPointerOverwrite(_) + | UnsupportedOpInfo::PartialPointerCopy(_), + ) => { + err.help("this code performed an operation that depends on the underlying bytes representing a pointer"); + err.help("the absolute address of a pointer is not known at compile-time, so such operations are not supported"); + } + _ => {} + } + // Add spans for the stacktrace. Don't print a single-line backtrace though. + if self.stacktrace.len() > 1 { + // Helper closure to print duplicated lines. + let mut flush_last_line = |last_frame, times| { + if let Some((line, span)) = last_frame { + err.span_note(span, &line); + // Don't print [... additional calls ...] if the number of lines is small + if times < 3 { + for _ in 0..times { + err.span_note(span, &line); + } + } else { + err.span_note( + span, + format!("[... {} additional calls {} ...]", times, &line), + ); + } + } + }; + + let mut last_frame = None; + let mut times = 0; + for frame_info in &self.stacktrace { + let frame = (frame_info.to_string(), frame_info.span); + if last_frame.as_ref() == Some(&frame) { + times += 1; + } else { + flush_last_line(last_frame, times); + last_frame = Some(frame); + times = 0; + } + } + flush_last_line(last_frame, times); + } + // Let the caller attach any additional information it wants. + decorate(err); + } + /// Create a diagnostic for this const eval error. /// /// Sets the message passed in via `message` and adds span labels with detailed error @@ -101,88 +154,30 @@ impl<'tcx> ConstEvalErr<'tcx> { message: &str, decorate: impl FnOnce(&mut Diagnostic), ) -> ErrorHandled { - let finish = |err: &mut Diagnostic, span_msg: Option<String>| { - trace!("reporting const eval failure at {:?}", self.span); - if let Some(span_msg) = span_msg { - err.span_label(self.span, span_msg); - } - // Add some more context for select error types. - match self.error { - InterpError::Unsupported( - UnsupportedOpInfo::ReadPointerAsBytes - | UnsupportedOpInfo::PartialPointerOverwrite(_) - | UnsupportedOpInfo::PartialPointerCopy(_), - ) => { - err.help("this code performed an operation that depends on the underlying bytes representing a pointer"); - err.help("the absolute address of a pointer is not known at compile-time, so such operations are not supported"); - } - _ => {} - } - // Add spans for the stacktrace. Don't print a single-line backtrace though. - if self.stacktrace.len() > 1 { - // Helper closure to print duplicated lines. - let mut flush_last_line = |last_frame, times| { - if let Some((line, span)) = last_frame { - err.span_note(span, &line); - // Don't print [... additional calls ...] if the number of lines is small - if times < 3 { - for _ in 0..times { - err.span_note(span, &line); - } - } else { - err.span_note( - span, - format!("[... {} additional calls {} ...]", times, &line), - ); - } - } - }; - - let mut last_frame = None; - let mut times = 0; - for frame_info in &self.stacktrace { - let frame = (frame_info.to_string(), frame_info.span); - if last_frame.as_ref() == Some(&frame) { - times += 1; - } else { - flush_last_line(last_frame, times); - last_frame = Some(frame); - times = 0; - } - } - flush_last_line(last_frame, times); - } - // Let the caller attach any additional information it wants. - decorate(err); - }; - debug!("self.error: {:?}", self.error); // Special handling for certain errors match &self.error { // Don't emit a new diagnostic for these errors err_inval!(Layout(LayoutError::Unknown(_))) | err_inval!(TooGeneric) => { - return ErrorHandled::TooGeneric; - } - err_inval!(AlreadyReported(error_reported)) => { - return ErrorHandled::Reported(*error_reported); + ErrorHandled::TooGeneric } + err_inval!(AlreadyReported(error_reported)) => ErrorHandled::Reported(*error_reported), err_inval!(Layout(LayoutError::SizeOverflow(_))) => { // We must *always* hard error on these, even if the caller wants just a lint. // The `message` makes little sense here, this is a more serious error than the // caller thinks anyway. // See <https://github.com/rust-lang/rust/pull/63152>. let mut err = struct_error(tcx, &self.error.to_string()); - finish(&mut err, None); - return ErrorHandled::Reported(err.emit()); + self.decorate(&mut err, decorate); + ErrorHandled::Reported(err.emit()) } - _ => {} - }; - - let err_msg = self.error.to_string(); - - // Report as hard error. - let mut err = struct_error(tcx, message); - finish(&mut err, Some(err_msg)); - ErrorHandled::Reported(err.emit()) + _ => { + // Report as hard error. + let mut err = struct_error(tcx, message); + err.span_label(self.span, self.error.to_string()); + self.decorate(&mut err, decorate); + ErrorHandled::Reported(err.emit()) + } + } } } diff --git a/compiler/rustc_const_eval/src/const_eval/eval_queries.rs b/compiler/rustc_const_eval/src/const_eval/eval_queries.rs index 319f2b2c25e..18e01567ca3 100644 --- a/compiler/rustc_const_eval/src/const_eval/eval_queries.rs +++ b/compiler/rustc_const_eval/src/const_eval/eval_queries.rs @@ -1,3 +1,4 @@ +use crate::const_eval::CheckAlignment; use std::borrow::Cow; use either::{Left, Right}; @@ -76,7 +77,7 @@ fn eval_body_using_ecx<'mir, 'tcx>( None => InternKind::Constant, } }; - ecx.machine.check_alignment = false; // interning doesn't need to respect alignment + ecx.machine.check_alignment = CheckAlignment::No; // interning doesn't need to respect alignment intern_const_alloc_recursive(ecx, intern_kind, &ret)?; // we leave alignment checks off, since this `ecx` will not be used for further evaluation anyway @@ -102,11 +103,7 @@ pub(super) fn mk_eval_cx<'mir, 'tcx>( tcx, root_span, param_env, - CompileTimeInterpreter::new( - tcx.const_eval_limit(), - can_access_statics, - /*check_alignment:*/ false, - ), + CompileTimeInterpreter::new(tcx.const_eval_limit(), can_access_statics, CheckAlignment::No), ) } @@ -311,7 +308,11 @@ pub fn eval_to_allocation_raw_provider<'tcx>( CompileTimeInterpreter::new( tcx.const_eval_limit(), /*can_access_statics:*/ is_static, - /*check_alignment:*/ tcx.sess.opts.unstable_opts.extra_const_ub_checks, + if tcx.sess.opts.unstable_opts.extra_const_ub_checks { + CheckAlignment::Error + } else { + CheckAlignment::FutureIncompat + }, ), ); diff --git a/compiler/rustc_const_eval/src/const_eval/machine.rs b/compiler/rustc_const_eval/src/const_eval/machine.rs index 3dfded2d930..e006a62feea 100644 --- a/compiler/rustc_const_eval/src/const_eval/machine.rs +++ b/compiler/rustc_const_eval/src/const_eval/machine.rs @@ -1,9 +1,10 @@ use rustc_hir::def::DefKind; -use rustc_hir::LangItem; +use rustc_hir::{LangItem, CRATE_HIR_ID}; use rustc_middle::mir; use rustc_middle::mir::interpret::PointerArithmetic; use rustc_middle::ty::layout::FnAbiOf; use rustc_middle::ty::{self, Ty, TyCtxt}; +use rustc_session::lint::builtin::INVALID_ALIGNMENT; use std::borrow::Borrow; use std::hash::Hash; use std::ops::ControlFlow; @@ -47,14 +48,34 @@ pub struct CompileTimeInterpreter<'mir, 'tcx> { pub(super) can_access_statics: bool, /// Whether to check alignment during evaluation. - pub(super) check_alignment: bool, + pub(super) check_alignment: CheckAlignment, +} + +#[derive(Copy, Clone)] +pub enum CheckAlignment { + /// Ignore alignment when following relocations. + /// 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, + } + } } impl<'mir, 'tcx> CompileTimeInterpreter<'mir, 'tcx> { pub(crate) fn new( const_eval_limit: Limit, can_access_statics: bool, - check_alignment: bool, + check_alignment: CheckAlignment, ) -> Self { CompileTimeInterpreter { steps_remaining: const_eval_limit.0, @@ -309,7 +330,7 @@ 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>) -> bool { + fn enforce_alignment(ecx: &InterpCx<'mir, 'tcx, Self>) -> CheckAlignment { ecx.machine.check_alignment } @@ -318,6 +339,36 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir, ecx.tcx.sess.opts.unstable_opts.extra_const_ub_checks } + 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 err = ConstEvalErr::new(ecx, err, None); + ecx.tcx.struct_span_lint_hir( + INVALID_ALIGNMENT, + ecx.stack().iter().find_map(|frame| frame.lint_root()).unwrap_or(CRATE_HIR_ID), + err.span, + err.error.to_string(), + |db| { + err.decorate(db, |_| {}); + db + }, + ); + Ok(()) + } + } + } + fn load_mir( ecx: &InterpCx<'mir, 'tcx, Self>, instance: ty::InstanceDef<'tcx>, diff --git a/compiler/rustc_const_eval/src/interpret/eval_context.rs b/compiler/rustc_const_eval/src/interpret/eval_context.rs index 0b2809f1d2c..f551b5c2911 100644 --- a/compiler/rustc_const_eval/src/interpret/eval_context.rs +++ b/compiler/rustc_const_eval/src/interpret/eval_context.rs @@ -248,6 +248,15 @@ impl<'mir, 'tcx, Prov: Provenance, Extra> Frame<'mir, 'tcx, Prov, Extra> { Right(span) => span, } } + + pub fn lint_root(&self) -> Option<hir::HirId> { + self.current_source_info().and_then(|source_info| { + match &self.body.source_scopes[source_info.scope].local_data { + mir::ClearCrossCrate::Set(data) => Some(data.lint_root), + mir::ClearCrossCrate::Clear => None, + } + }) + } } impl<'tcx> fmt::Display for FrameInfo<'tcx> { @@ -954,12 +963,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { // This deliberately does *not* honor `requires_caller_location` since it is used for much // more than just panics. for frame in stack.iter().rev() { - let lint_root = frame.current_source_info().and_then(|source_info| { - match &frame.body.source_scopes[source_info.scope].local_data { - mir::ClearCrossCrate::Set(data) => Some(data.lint_root), - mir::ClearCrossCrate::Clear => None, - } - }); + let lint_root = frame.lint_root(); let span = frame.current_span(); frames.push(FrameInfo { span, instance: frame.instance, lint_root }); diff --git a/compiler/rustc_const_eval/src/interpret/machine.rs b/compiler/rustc_const_eval/src/interpret/machine.rs index 0604d5ee6fa..1d4ef20d065 100644 --- a/compiler/rustc_const_eval/src/interpret/machine.rs +++ b/compiler/rustc_const_eval/src/interpret/machine.rs @@ -10,9 +10,11 @@ use rustc_ast::{InlineAsmOptions, InlineAsmTemplatePiece}; use rustc_middle::mir; use rustc_middle::ty::{self, Ty, TyCtxt}; use rustc_span::def_id::DefId; -use rustc_target::abi::Size; +use rustc_target::abi::{Align, Size}; use rustc_target::spec::abi::Abi as CallAbi; +use crate::const_eval::CheckAlignment; + use super::{ AllocId, AllocRange, Allocation, ConstAllocation, Frame, ImmTy, InterpCx, InterpResult, MemoryKind, OpTy, Operand, PlaceTy, Pointer, Provenance, Scalar, StackPopUnwind, @@ -122,7 +124,7 @@ pub trait Machine<'mir, 'tcx>: Sized { const PANIC_ON_ALLOC_FAIL: bool; /// Whether memory accesses should be alignment-checked. - fn enforce_alignment(ecx: &InterpCx<'mir, 'tcx, Self>) -> bool; + fn enforce_alignment(ecx: &InterpCx<'mir, 'tcx, Self>) -> CheckAlignment; /// 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. @@ -130,6 +132,13 @@ pub trait Machine<'mir, 'tcx>: Sized { /// 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 fn enforce_validity(ecx: &InterpCx<'mir, 'tcx, Self>) -> bool; diff --git a/compiler/rustc_const_eval/src/interpret/memory.rs b/compiler/rustc_const_eval/src/interpret/memory.rs index 528c1cb06c0..5b1ac6b2f65 100644 --- a/compiler/rustc_const_eval/src/interpret/memory.rs +++ b/compiler/rustc_const_eval/src/interpret/memory.rs @@ -18,6 +18,8 @@ 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 super::{ alloc_range, AllocId, AllocMap, AllocRange, Allocation, CheckInAllocMsg, GlobalAlloc, InterpCx, InterpResult, Machine, MayLeak, Pointer, PointerArithmetic, Provenance, Scalar, @@ -349,11 +351,11 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { size: Size, align: Align, ) -> InterpResult<'tcx, Option<(AllocId, Size, M::ProvenanceExtra)>> { - let align = M::enforce_alignment(&self).then_some(align); self.check_and_deref_ptr( ptr, size, align, + M::enforce_alignment(self), CheckInAllocMsg::MemoryAccessTest, |alloc_id, offset, prov| { let (size, align) = self.get_live_alloc_size_and_align(alloc_id)?; @@ -373,10 +375,17 @@ 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, Some(align), msg, |alloc_id, _, _| { - let (size, align) = self.get_live_alloc_size_and_align(alloc_id)?; - Ok((size, align, ())) - })?; + 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)?; + Ok((size, align, ())) + }, + )?; Ok(()) } @@ -388,7 +397,8 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { &self, ptr: Pointer<Option<M::Provenance>>, size: Size, - align: Option<Align>, + align: Align, + check: CheckAlignment, msg: CheckInAllocMsg, alloc_size: impl FnOnce( AllocId, @@ -396,19 +406,6 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { M::ProvenanceExtra, ) -> InterpResult<'tcx, (Size, Align, T)>, ) -> InterpResult<'tcx, Option<T>> { - fn check_offset_align<'tcx>(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(); - throw_ub!(AlignmentCheckFailed { - has: Align::from_bytes(offset_pow2).unwrap(), - required: align, - }) - } - } - Ok(match self.ptr_try_get_alloc_id(ptr) { Err(addr) => { // We couldn't get a proper allocation. This is only okay if the access size is 0, @@ -417,8 +414,8 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { throw_ub!(DanglingIntPointer(addr, msg)); } // Must be aligned. - if let Some(align) = align { - check_offset_align(addr, align)?; + if check.should_check() { + self.check_offset_align(addr, align, check)?; } None } @@ -441,16 +438,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 let Some(align) = align { + if check.should_check() { if M::use_addr_for_alignment_check(self) { // `use_addr_for_alignment_check` can only be true if `OFFSET_IS_ADDR` is true. - check_offset_align(ptr.addr().bytes(), align)?; + self.check_offset_align(ptr.addr().bytes(), align, check)?; } else { // Check allocation alignment and offset alignment. if alloc_align.bytes() < align.bytes() { - throw_ub!(AlignmentCheckFailed { has: alloc_align, required: align }); + M::alignment_check_failed(self, alloc_align, align, check)?; } - check_offset_align(offset.bytes(), align)?; + self.check_offset_align(offset.bytes(), align, check)?; } } @@ -460,6 +457,21 @@ 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> { + 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) + } + } } /// Allocation accessors @@ -560,11 +572,11 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { size: Size, align: Align, ) -> InterpResult<'tcx, Option<AllocRef<'a, 'tcx, M::Provenance, M::AllocExtra>>> { - let align = M::enforce_alignment(self).then_some(align); let ptr_and_alloc = self.check_and_deref_ptr( ptr, size, align, + M::enforce_alignment(self), CheckInAllocMsg::MemoryAccessTest, |alloc_id, offset, prov| { let alloc = self.get_alloc_raw(alloc_id)?; diff --git a/compiler/rustc_const_eval/src/interpret/place.rs b/compiler/rustc_const_eval/src/interpret/place.rs index c47cfe8bb69..905eb71bb18 100644 --- a/compiler/rustc_const_eval/src/interpret/place.rs +++ b/compiler/rustc_const_eval/src/interpret/place.rs @@ -364,13 +364,8 @@ where .size_and_align_of_mplace(&mplace)? .unwrap_or((mplace.layout.size, mplace.layout.align.abi)); assert!(mplace.align <= align, "dynamic alignment less strict than static one?"); - let align = M::enforce_alignment(self).then_some(align); - self.check_ptr_access_align( - mplace.ptr, - size, - align.unwrap_or(Align::ONE), - CheckInAllocMsg::DerefTest, - )?; + let align = if M::enforce_alignment(self).should_check() { align } else { Align::ONE }; + self.check_ptr_access_align(mplace.ptr, size, align, CheckInAllocMsg::DerefTest)?; Ok(()) } diff --git a/compiler/rustc_const_eval/src/util/might_permit_raw_init.rs b/compiler/rustc_const_eval/src/util/might_permit_raw_init.rs index 6ca71223391..4ce107ea68d 100644 --- a/compiler/rustc_const_eval/src/util/might_permit_raw_init.rs +++ b/compiler/rustc_const_eval/src/util/might_permit_raw_init.rs @@ -3,7 +3,7 @@ use rustc_middle::ty::{ParamEnv, TyCtxt}; use rustc_session::Limit; use rustc_target::abi::{Abi, FieldsShape, InitKind, Scalar, Variants}; -use crate::const_eval::CompileTimeInterpreter; +use crate::const_eval::{CheckAlignment, CompileTimeInterpreter}; use crate::interpret::{InterpCx, MemoryKind, OpTy}; /// Determines if this type permits "raw" initialization by just transmuting some memory into an @@ -41,7 +41,7 @@ fn might_permit_raw_init_strict<'tcx>( let machine = CompileTimeInterpreter::new( Limit::new(0), /*can_access_statics:*/ false, - /*check_alignment:*/ true, + CheckAlignment::Error, ); let mut cx = InterpCx::new(tcx, rustc_span::DUMMY_SP, ParamEnv::reveal_all(), machine); |
