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

Deduplicate some code between miri and const prop #64419

Merged
merged 15 commits into from
Sep 28, 2019
Merged
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
6 changes: 6 additions & 0 deletions src/librustc/mir/interpret/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,10 @@ pub enum UnsupportedOpInfo<'tcx> {
/// Free-form case. Only for errors that are never caught!
Unsupported(String),

/// FIXME(#64506) Error used to work around accessing projections of
/// uninhabited types.
UninhabitedValue,
Copy link
Member

Choose a reason for hiding this comment

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

Why is "unsupported" the right category for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

To me, it seemed similar to some of the other "unsupported" cases like ReadForeignStatic, NoMirFor, DerefFunctionPointer, OutOfTls etc in that it represents allowed behavior in a valid program that simply isn't implemented (or can't be implemented) in miri. I dismissed InvalidProgramInfo because this doesn't indicate an invalid program, however reading the doc comment, it sounds like that may actually be a more appropriate classification. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Looks like @oli-obk is already working on removing this again so whatever.


// -- Everything below is not categorized yet --
FunctionAbiMismatch(Abi, Abi),
FunctionArgMismatch(Ty<'tcx>, Ty<'tcx>),
Expand Down Expand Up @@ -552,6 +556,8 @@ impl fmt::Debug for UnsupportedOpInfo<'tcx> {
not a power of two"),
Unsupported(ref msg) =>
write!(f, "{}", msg),
UninhabitedValue =>
write!(f, "tried to use an uninhabited value"),
}
}
}
Expand Down
10 changes: 5 additions & 5 deletions src/librustc_mir/const_eval.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use syntax::source_map::{Span, DUMMY_SP};

use crate::interpret::{self,
PlaceTy, MPlaceTy, OpTy, ImmTy, Immediate, Scalar, Pointer,
RawConst, ConstValue,
RawConst, ConstValue, Machine,
InterpResult, InterpErrorInfo, GlobalId, InterpCx, StackPopCleanup,
Allocation, AllocId, MemoryKind, Memory,
snapshot, RefTracking, intern_const_alloc_recursive,
Expand All @@ -41,7 +41,7 @@ const DETECTOR_SNAPSHOT_PERIOD: isize = 256;
/// that inform us about the generic bounds of the constant. E.g., using an associated constant
/// of a function's generic parameter will require knowledge about the bounds on the generic
/// parameter. These bounds are passed to `mk_eval_cx` via the `ParamEnv` argument.
pub(crate) fn mk_eval_cx<'mir, 'tcx>(
fn mk_eval_cx<'mir, 'tcx>(
tcx: TyCtxt<'tcx>,
span: Span,
param_env: ty::ParamEnv<'tcx>,
Expand Down Expand Up @@ -169,7 +169,7 @@ fn eval_body_using_ecx<'mir, 'tcx>(
}

#[derive(Clone, Debug)]
enum ConstEvalError {
pub enum ConstEvalError {
NeedsRfc(String),
}

Expand Down Expand Up @@ -521,8 +521,8 @@ pub fn const_variant_index<'tcx>(
/// Turn an interpreter error into something to report to the user.
/// As a side-effect, if RUSTC_CTFE_BACKTRACE is set, this prints the backtrace.
/// Should be called only if the error is actually going to to be reported!
pub fn error_to_const_error<'mir, 'tcx>(
ecx: &InterpCx<'mir, 'tcx, CompileTimeInterpreter<'mir, 'tcx>>,
pub fn error_to_const_error<'mir, 'tcx, M: Machine<'mir, 'tcx>>(
ecx: &InterpCx<'mir, 'tcx, M>,
mut error: InterpErrorInfo<'tcx>,
) -> ConstEvalErr<'tcx> {
error.print_backtrace();
Expand Down
17 changes: 17 additions & 0 deletions src/librustc_mir/interpret/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use rustc::ty::{self, Ty, TyCtxt};
use super::{
Allocation, AllocId, InterpResult, Scalar, AllocationExtra,
InterpCx, PlaceTy, OpTy, ImmTy, MemoryKind, Pointer, Memory,
Frame, Operand,
};

/// Whether this kind of memory is allowed to leak
Expand Down Expand Up @@ -184,6 +185,22 @@ pub trait Machine<'mir, 'tcx>: Sized {
dest: PlaceTy<'tcx, Self::PointerTag>,
) -> InterpResult<'tcx>;

/// Called to read the specified `local` from the `frame`.
fn access_local(
_ecx: &InterpCx<'mir, 'tcx, Self>,
frame: &Frame<'mir, 'tcx, Self::PointerTag, Self::FrameExtra>,
local: mir::Local,
) -> InterpResult<'tcx, Operand<Self::PointerTag>> {
frame.locals[local].access()
}

/// Called before a `StaticKind::Static` value is accessed.
Copy link
Member

Choose a reason for hiding this comment

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

Please specify what "access" means here. Is this called when the address of a static is taken? Or just on reads/writes?

Copy link
Member Author

Choose a reason for hiding this comment

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

I will double check and improve the comment in the other open PR I have but I believe this is just on reads/writes.

fn before_access_static(
_allocation: &Allocation,
) -> InterpResult<'tcx> {
Ok(())
}

/// Called to initialize the "extra" state of an allocation and make the pointers
/// it contains (in relocations) tagged. The way we construct allocations is
/// to always first construct it without extra and then add the extra.
Expand Down
2 changes: 2 additions & 0 deletions src/librustc_mir/interpret/memory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -462,6 +462,8 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
// Make sure we use the ID of the resolved memory, not the lazy one!
let id = raw_const.alloc_id;
let allocation = tcx.alloc_map.lock().unwrap_memory(id);

M::before_access_static(allocation)?;
Cow::Borrowed(allocation)
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/librustc_mir/interpret/operand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -458,7 +458,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
// Do not read from ZST, they might not be initialized
Operand::Immediate(Scalar::zst().into())
} else {
frame.locals[local].access()?
M::access_local(&self, frame, local)?
};
Ok(OpTy { op, layout })
}
Expand All @@ -481,7 +481,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {

// Evaluate a place with the goal of reading from it. This lets us sometimes
// avoid allocations.
pub(super) fn eval_place_to_op(
pub fn eval_place_to_op(
&self,
place: &mir::Place<'tcx>,
layout: Option<TyLayout<'tcx>>,
Expand Down
6 changes: 5 additions & 1 deletion src/librustc_mir/interpret/place.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use rustc::mir;
use rustc::mir::interpret::truncate;
use rustc::ty::{self, Ty};
use rustc::ty::layout::{
self, Size, Align, LayoutOf, TyLayout, HasDataLayout, VariantIdx, PrimitiveExt
self, Size, Abi, Align, LayoutOf, TyLayout, HasDataLayout, VariantIdx, PrimitiveExt
};
use rustc::ty::TypeFoldable;

Expand Down Expand Up @@ -385,6 +385,10 @@ where
stride * field
}
layout::FieldPlacement::Union(count) => {
// FIXME(#64506) `UninhabitedValue` can be removed when this issue is resolved
if base.layout.abi == Abi::Uninhabited {
wesleywiser marked this conversation as resolved.
Show resolved Hide resolved
throw_unsup!(UninhabitedValue);
}
assert!(field < count as u64,
"Tried to access field {} of union with {} fields", field, count);
// Offset is always 0
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_mir/interpret/step.rs
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
///
/// There is no separate `eval_rvalue` function. Instead, the code for handling each rvalue
/// type writes its results directly into the memory specified by the place.
fn eval_rvalue_into_place(
pub fn eval_rvalue_into_place(
Copy link
Member

@RalfJung RalfJung Oct 9, 2019

Choose a reason for hiding this comment

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

Oh, why this? We're trying to keep a clean interface for the interpreter, that'll never work if everyone just adds pub whenever they feel like it. :(

Copy link
Member

Choose a reason for hiding this comment

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

Looking at the const_prop changes, it seems like what happened is that const_prop moved up a layer or two of the interpreter abstractions. Shouldn't that mean that while some new operations need to be public now, some of the previously used one can be made private? If you could scan the functions that const_prop no longer needs and make them private, that would make me much less concerned about this change. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Shouldn't that mean that while some new operations need to be public now, some of the previously used one can be made private?

Yes, I think you're correct. I'll do that in the other PR as well.

&mut self,
rvalue: &mir::Rvalue<'tcx>,
place: &mir::Place<'tcx>,
Expand Down
Loading