From e721f5e083d46853714e79c3c33eb492736cc189 Mon Sep 17 00:00:00 2001 From: QuietMisdreavus Date: Wed, 6 Mar 2019 10:16:41 -0600 Subject: [PATCH 01/13] don't process `external_traits` when collecting intra-doc links --- src/librustdoc/passes/collect_intra_doc_links.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index 67f291285c447..93e811f8b31d8 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -213,6 +213,7 @@ impl<'a, 'tcx, 'rcx> LinkCollector<'a, 'tcx, 'rcx> { _ => Err(()) } } else { + debug!("attempting to resolve item without parent module: {}", path_str); Err(()) } } @@ -434,6 +435,15 @@ impl<'a, 'tcx, 'rcx> DocFolder for LinkCollector<'a, 'tcx, 'rcx> { self.fold_item_recur(item) } } + + // FIXME: if we can resolve intra-doc links from other crates, we can use the stock + // `fold_crate`, but until then we should avoid scanning `krate.external_traits` since those + // will never resolve properly + fn fold_crate(&mut self, mut c: Crate) -> Crate { + c.module = c.module.take().and_then(|module| self.fold_item(module)); + + c + } } /// Resolves a string as a macro. From 49cde404124c8217808c71972f206443f49bffc7 Mon Sep 17 00:00:00 2001 From: QuietMisdreavus Date: Wed, 6 Mar 2019 10:57:38 -0600 Subject: [PATCH 02/13] add test for spurious intra-doc link warning --- .../rustdoc/auxiliary/intra-links-external-traits.rs | 6 ++++++ src/test/rustdoc/intra-links-external-traits.rs | 12 ++++++++++++ 2 files changed, 18 insertions(+) create mode 100644 src/test/rustdoc/auxiliary/intra-links-external-traits.rs create mode 100644 src/test/rustdoc/intra-links-external-traits.rs diff --git a/src/test/rustdoc/auxiliary/intra-links-external-traits.rs b/src/test/rustdoc/auxiliary/intra-links-external-traits.rs new file mode 100644 index 0000000000000..6142dcda986cf --- /dev/null +++ b/src/test/rustdoc/auxiliary/intra-links-external-traits.rs @@ -0,0 +1,6 @@ +pub trait ThisTrait { + fn asdf(&self); + + /// let's link to [`asdf`](ThisTrait::asdf) + fn qwop(&self); +} diff --git a/src/test/rustdoc/intra-links-external-traits.rs b/src/test/rustdoc/intra-links-external-traits.rs new file mode 100644 index 0000000000000..d6b4a8ad58ad7 --- /dev/null +++ b/src/test/rustdoc/intra-links-external-traits.rs @@ -0,0 +1,12 @@ +// aux-build:intra-links-external-traits.rs +// ignore-cross-compile + +#![crate_name = "outer"] +#![deny(intra_doc_link_resolution_failure)] + +// using a trait that has intra-doc links on it from another crate (whether re-exporting or just +// implementing it) used to give spurious resolution failure warnings + +extern crate intra_links_external_traits; + +pub use intra_links_external_traits::ThisTrait; From 968ea1ce32ac894a7d58702c409233638aa5592d Mon Sep 17 00:00:00 2001 From: Matthew Jasper Date: Thu, 4 Apr 2019 21:25:08 +0100 Subject: [PATCH 03/13] Mark variables captured by reference as mutable correctly --- src/librustc_mir/borrow_check/mod.rs | 96 ++++++++++++++++++++++------ src/test/ui/nll/extra-unused-mut.rs | 19 +++--- 2 files changed, 87 insertions(+), 28 deletions(-) diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index a8c151a22eebc..ab3239820e3a2 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -28,7 +28,7 @@ use std::collections::BTreeMap; use syntax_pos::Span; use crate::dataflow::indexes::{BorrowIndex, InitIndex, MoveOutIndex, MovePathIndex}; -use crate::dataflow::move_paths::{HasMoveData, LookupResult, MoveData, MoveError}; +use crate::dataflow::move_paths::{HasMoveData, InitLocation, LookupResult, MoveData, MoveError}; use crate::dataflow::Borrows; use crate::dataflow::DataflowResultsConsumer; use crate::dataflow::FlowAtLocation; @@ -1206,25 +1206,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { } = self.infcx.tcx.mir_borrowck(def_id); debug!("{:?} used_mut_upvars={:?}", def_id, used_mut_upvars); for field in used_mut_upvars { - // This relies on the current way that by-value - // captures of a closure are copied/moved directly - // when generating MIR. - match operands[field.index()] { - Operand::Move(Place::Base(PlaceBase::Local(local))) - | Operand::Copy(Place::Base(PlaceBase::Local(local))) => { - self.used_mut.insert(local); - } - Operand::Move(ref place @ Place::Projection(_)) - | Operand::Copy(ref place @ Place::Projection(_)) => { - if let Some(field) = place.is_upvar_field_projection( - self.mir, &self.infcx.tcx) { - self.used_mut_upvars.push(field); - } - } - Operand::Move(Place::Base(PlaceBase::Static(..))) - | Operand::Copy(Place::Base(PlaceBase::Static(..))) - | Operand::Constant(..) => {} - } + self.propagate_closure_used_mut_upvar(&operands[field.index()]); } } AggregateKind::Adt(..) @@ -1239,6 +1221,80 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { } } + fn propagate_closure_used_mut_upvar(&mut self, operand: &Operand<'tcx>) { + let propagate_closure_used_mut_place = |this: &mut Self, place: &Place<'tcx>| { + match *place { + Place::Projection { .. } => { + if let Some(field) = place.is_upvar_field_projection( + this.mir, &this.infcx.tcx) { + this.used_mut_upvars.push(field); + } + } + Place::Base(PlaceBase::Local(local)) => { + this.used_mut.insert(local); + } + Place::Base(PlaceBase::Static(_)) => {} + } + }; + + // This relies on the current way that by-value + // captures of a closure are copied/moved directly + // when generating MIR. + match *operand { + Operand::Move(Place::Base(PlaceBase::Local(local))) + | Operand::Copy(Place::Base(PlaceBase::Local(local))) + if self.mir.local_decls[local].is_user_variable.is_none() => + { + if self.mir.local_decls[local].ty.is_mutable_pointer() { + // The variable will be marked as mutable by the borrow. + return; + } + // This is an edge case where we have a `move` closure + // inside a non-move closure, and the inner closure + // contains a mutation: + // + // let mut i = 0; + // || { move || { i += 1; }; }; + // + // In this case our usual strategy of assuming that the + // variable will be captured by mutable reference is + // wrong, since `i` can be copied into the inner + // closure from a shared reference. + // + // As such we have to search for the local that this + // capture comes from and mark it as being used as mut. + + let temp_mpi = self.move_data.rev_lookup.find_local(local); + let init = if let [init_index] = *self.move_data.init_path_map[temp_mpi] { + &self.move_data.inits[init_index] + } else { + bug!("temporary should be initialized exactly once") + }; + + let loc = match init.location { + InitLocation::Statement(stmt) => stmt, + _ => bug!("temporary initialized in arguments"), + }; + + let bbd = &self.mir[loc.block]; + let stmt = &bbd.statements[loc.statement_index]; + debug!("temporary assigned in: stmt={:?}", stmt); + + if let StatementKind::Assign(_, box Rvalue::Ref(_, _, ref source)) = stmt.kind { + propagate_closure_used_mut_place(self, source); + } else { + bug!("closures should only capture user variables \ + or references to user variables"); + } + } + Operand::Move(ref place) + | Operand::Copy(ref place) => { + propagate_closure_used_mut_place(self, place); + } + Operand::Constant(..) => {} + } + } + fn consume_operand( &mut self, context: Context, diff --git a/src/test/ui/nll/extra-unused-mut.rs b/src/test/ui/nll/extra-unused-mut.rs index d5f0b0ddf18bf..6d0d6e16a6775 100644 --- a/src/test/ui/nll/extra-unused-mut.rs +++ b/src/test/ui/nll/extra-unused-mut.rs @@ -1,6 +1,6 @@ // extra unused mut lint tests for #51918 -// run-pass +// compile-pass #![feature(generators, nll)] #![deny(unused_mut)] @@ -53,11 +53,14 @@ fn if_guard(x: Result) { } } -fn main() { - ref_argument(0); - mutable_upvar(); - generator_mutable_upvar(); - ref_closure_argument(); - parse_dot_or_call_expr_with(Vec::new()); - if_guard(Ok(0)); +// #59620 +fn nested_closures() { + let mut i = 0; + [].iter().for_each(|_: &i32| { + [].iter().for_each(move |_: &i32| { + i += 1; + }); + }); } + +fn main() {} From 63080b3c25046b29cbbaef8d587c7da91a302fce Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Fri, 5 Apr 2019 22:42:40 +0300 Subject: [PATCH 04/13] remove lookup_char_pos_adj It is now exactly equivalent to lookup_char_pos. --- src/librustc/infer/error_reporting/mod.rs | 2 +- src/librustc/mir/interpret/error.rs | 4 ++-- src/libsyntax/diagnostics/metadata.rs | 4 ++-- src/libsyntax/source_map.rs | 16 +++------------- src/libsyntax_pos/lib.rs | 13 +------------ 5 files changed, 9 insertions(+), 30 deletions(-) diff --git a/src/librustc/infer/error_reporting/mod.rs b/src/librustc/infer/error_reporting/mod.rs index 19663161fe3fa..8cfcc466b4019 100644 --- a/src/librustc/infer/error_reporting/mod.rs +++ b/src/librustc/infer/error_reporting/mod.rs @@ -278,7 +278,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> { } fn explain_span(self, heading: &str, span: Span) -> (String, Option) { - let lo = self.sess.source_map().lookup_char_pos_adj(span.lo()); + let lo = self.sess.source_map().lookup_char_pos(span.lo()); ( format!("the {} at {}:{}", heading, lo.line, lo.col.to_usize() + 1), Some(span), diff --git a/src/librustc/mir/interpret/error.rs b/src/librustc/mir/interpret/error.rs index 280e75476b72a..b9c4d312adb7b 100644 --- a/src/librustc/mir/interpret/error.rs +++ b/src/librustc/mir/interpret/error.rs @@ -65,8 +65,8 @@ impl<'tcx> fmt::Display for FrameInfo<'tcx> { write!(f, "inside call to `{}`", self.instance)?; } if !self.call_site.is_dummy() { - let lo = tcx.sess.source_map().lookup_char_pos_adj(self.call_site.lo()); - write!(f, " at {}:{}:{}", lo.filename, lo.line, lo.col.to_usize() + 1)?; + let lo = tcx.sess.source_map().lookup_char_pos(self.call_site.lo()); + write!(f, " at {}:{}:{}", lo.file.name, lo.line, lo.col.to_usize() + 1)?; } Ok(()) }) diff --git a/src/libsyntax/diagnostics/metadata.rs b/src/libsyntax/diagnostics/metadata.rs index 704135fe1d589..53f37bb10bdc0 100644 --- a/src/libsyntax/diagnostics/metadata.rs +++ b/src/libsyntax/diagnostics/metadata.rs @@ -36,9 +36,9 @@ pub struct ErrorLocation { impl ErrorLocation { /// Creates an error location from a span. pub fn from_span(ecx: &ExtCtxt<'_>, sp: Span) -> ErrorLocation { - let loc = ecx.source_map().lookup_char_pos_adj(sp.lo()); + let loc = ecx.source_map().lookup_char_pos(sp.lo()); ErrorLocation { - filename: loc.filename, + filename: loc.file.name.clone(), line: loc.line } } diff --git a/src/libsyntax/source_map.rs b/src/libsyntax/source_map.rs index 62a6972122abd..08abbf5e8a4dc 100644 --- a/src/libsyntax/source_map.rs +++ b/src/libsyntax/source_map.rs @@ -388,16 +388,6 @@ impl SourceMap { } } - pub fn lookup_char_pos_adj(&self, pos: BytePos) -> LocWithOpt { - let loc = self.lookup_char_pos(pos); - LocWithOpt { - filename: loc.file.name.clone(), - line: loc.line, - col: loc.col, - file: Some(loc.file) - } - } - /// Returns `Some(span)`, a union of the lhs and rhs span. The lhs must precede the rhs. If /// there are gaps between lhs and rhs, the resulting union will cross these gaps. /// For this to work, the spans have to be: @@ -438,10 +428,10 @@ impl SourceMap { return "no-location".to_string(); } - let lo = self.lookup_char_pos_adj(sp.lo()); - let hi = self.lookup_char_pos_adj(sp.hi()); + let lo = self.lookup_char_pos(sp.lo()); + let hi = self.lookup_char_pos(sp.hi()); format!("{}:{}:{}: {}:{}", - lo.filename, + lo.file.name, lo.line, lo.col.to_usize() + 1, hi.line, diff --git a/src/libsyntax_pos/lib.rs b/src/libsyntax_pos/lib.rs index db1543ff13f7e..81cf804cf0b73 100644 --- a/src/libsyntax_pos/lib.rs +++ b/src/libsyntax_pos/lib.rs @@ -1295,7 +1295,7 @@ impl Sub for CharPos { } // _____________________________________________________________________________ -// Loc, LocWithOpt, SourceFileAndLine, SourceFileAndBytePos +// Loc, SourceFileAndLine, SourceFileAndBytePos // /// A source code location used for error reporting. @@ -1311,17 +1311,6 @@ pub struct Loc { pub col_display: usize, } -/// A source code location used as the result of `lookup_char_pos_adj`. -// Actually, *none* of the clients use the filename *or* file field; -// perhaps they should just be removed. -#[derive(Debug)] -pub struct LocWithOpt { - pub filename: FileName, - pub line: usize, - pub col: CharPos, - pub file: Option>, -} - // Used to be structural records. #[derive(Debug)] pub struct SourceFileAndLine { pub sf: Lrc, pub line: usize } From 2f948eaeae9189ba0d929fe6a308656bec065dd3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20K=C3=A5re=20Alsaker?= Date: Sat, 6 Apr 2019 18:07:53 +0200 Subject: [PATCH 05/13] Limit dylib symbols --- src/librustc_codegen_ssa/back/linker.rs | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/librustc_codegen_ssa/back/linker.rs b/src/librustc_codegen_ssa/back/linker.rs index c99fc17dd89a1..8884edbb24816 100644 --- a/src/librustc_codegen_ssa/back/linker.rs +++ b/src/librustc_codegen_ssa/back/linker.rs @@ -372,15 +372,11 @@ impl<'a> Linker for GccLinker<'a> { } fn export_symbols(&mut self, tmpdir: &Path, crate_type: CrateType) { - // If we're compiling a dylib, then we let symbol visibility in object - // files to take care of whether they're exported or not. - // - // If we're compiling a cdylib, however, we manually create a list of - // exported symbols to ensure we don't expose any more. The object files - // have far more public symbols than we actually want to export, so we - // hide them all here. - if crate_type == CrateType::Dylib || - crate_type == CrateType::ProcMacro { + // We manually create a list of exported symbols to ensure we don't expose any more. + // The object files have far more public symbols than we actually want to export, + // so we hide them all here. + + if crate_type == CrateType::ProcMacro { return } From cb51f872a841f658449f2dd3adebf0b243aa96e3 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 6 Apr 2019 23:58:59 +0200 Subject: [PATCH 06/13] miri engine: lazily allocate memory for locals on first write --- src/librustc_mir/interpret/eval_context.rs | 52 +++++++++++++++------- src/librustc_mir/interpret/place.rs | 51 ++++++++++++++------- src/librustc_mir/interpret/snapshot.rs | 9 ++-- src/librustc_mir/interpret/terminator.rs | 7 +-- 4 files changed, 79 insertions(+), 40 deletions(-) diff --git a/src/librustc_mir/interpret/eval_context.rs b/src/librustc_mir/interpret/eval_context.rs index 535fc58299bc4..c3726c63ea4a9 100644 --- a/src/librustc_mir/interpret/eval_context.rs +++ b/src/librustc_mir/interpret/eval_context.rs @@ -116,26 +116,41 @@ pub struct LocalState<'tcx, Tag=(), Id=AllocId> { /// State of a local variable #[derive(Copy, Clone, PartialEq, Eq, Hash)] pub enum LocalValue { + /// This local is not currently alive, and cannot be used at all. Dead, - // Mostly for convenience, we re-use the `Operand` type here. - // This is an optimization over just always having a pointer here; - // we can thus avoid doing an allocation when the local just stores - // immediate values *and* never has its address taken. + /// This local is alive but not yet initialized. It can be written to + /// but not read from or its address taken. Locals get initialized on + /// first write because for unsized locals, we do not know their size + /// before that. + Uninitialized, + /// A normal, live local. + /// Mostly for convenience, we re-use the `Operand` type here. + /// This is an optimization over just always having a pointer here; + /// we can thus avoid doing an allocation when the local just stores + /// immediate values *and* never has its address taken. Live(Operand), } -impl<'tcx, Tag> LocalState<'tcx, Tag> { +impl<'tcx, Tag: Copy> LocalState<'tcx, Tag> { pub fn access(&self) -> EvalResult<'tcx, &Operand> { match self.state { - LocalValue::Dead => err!(DeadLocal), + LocalValue::Dead | LocalValue::Uninitialized => err!(DeadLocal), LocalValue::Live(ref val) => Ok(val), } } - pub fn access_mut(&mut self) -> EvalResult<'tcx, &mut Operand> { + /// Overwrite the local. If the local can be overwritten in place, return a reference + /// to do so; otherwise return the `MemPlace` to consult instead. + pub fn access_mut( + &mut self, + ) -> EvalResult<'tcx, Result<&mut LocalValue, MemPlace>> { match self.state { LocalValue::Dead => err!(DeadLocal), - LocalValue::Live(ref mut val) => Ok(val), + LocalValue::Live(Operand::Indirect(mplace)) => Ok(Err(mplace)), + ref mut local @ LocalValue::Live(Operand::Immediate(_)) | + ref mut local @ LocalValue::Uninitialized => { + Ok(Ok(local)) + } } } } @@ -327,6 +342,7 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> InterpretCx<'a, 'mir, 'tc let local_ty = self.monomorphize_with_substs(local_ty, frame.instance.substs); self.layout_of(local_ty) })?; + // Layouts of locals are requested a lot, so we cache them. frame.locals[local].layout.set(Some(layout)); Ok(layout) } @@ -473,13 +489,9 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> InterpretCx<'a, 'mir, 'tc // don't allocate at all for trivial constants if mir.local_decls.len() > 1 { - // We put some marker immediate into the locals that we later want to initialize. - // This can be anything except for LocalValue::Dead -- because *that* is the - // value we use for things that we know are initially dead. + // Locals are initially uninitialized. let dummy = LocalState { - state: LocalValue::Live(Operand::Immediate(Immediate::Scalar( - ScalarMaybeUndef::Undef, - ))), + state: LocalValue::Uninitialized, layout: Cell::new(None), }; let mut locals = IndexVec::from_elem(dummy, &mir.local_decls); @@ -506,19 +518,25 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> InterpretCx<'a, 'mir, 'tc } }, } - // Finally, properly initialize all those that still have the dummy value + // FIXME: We initialize live ZST here. This should not be needed if MIR was + // consistently generated for ZST, but that seems to not be the case -- there + // is MIR (around promoteds in particular) that reads local ZSTs that never + // were written to. for (idx, local) in locals.iter_enumerated_mut() { match local.state { - LocalValue::Live(_) => { + LocalValue::Uninitialized => { // This needs to be properly initialized. let ty = self.monomorphize(mir.local_decls[idx].ty)?; let layout = self.layout_of(ty)?; - local.state = LocalValue::Live(self.uninit_operand(layout)?); + if layout.is_zst() { + local.state = LocalValue::Live(self.uninit_operand(layout)?); + } local.layout = Cell::new(Some(layout)); } LocalValue::Dead => { // Nothing to do } + LocalValue::Live(_) => bug!("Locals cannot be live yet"), } } // done diff --git a/src/librustc_mir/interpret/place.rs b/src/librustc_mir/interpret/place.rs index 4f7b59a5a9a95..f69ce4e0d3d17 100644 --- a/src/librustc_mir/interpret/place.rs +++ b/src/librustc_mir/interpret/place.rs @@ -15,7 +15,7 @@ use rustc::ty::TypeFoldable; use super::{ GlobalId, AllocId, Allocation, Scalar, EvalResult, Pointer, PointerArithmetic, InterpretCx, Machine, AllocMap, AllocationExtra, - RawConst, Immediate, ImmTy, ScalarMaybeUndef, Operand, OpTy, MemoryKind + RawConst, Immediate, ImmTy, ScalarMaybeUndef, Operand, OpTy, MemoryKind, LocalValue }; #[derive(Copy, Clone, Debug, Hash, PartialEq, Eq)] @@ -639,6 +639,7 @@ where None => return err!(InvalidNullPointerUsage), }, Base(PlaceBase::Local(local)) => PlaceTy { + // This works even for dead/uninitialized locals; we check further when writing place: Place::Local { frame: self.cur_frame(), local, @@ -714,16 +715,19 @@ where // but not factored as a separate function. let mplace = match dest.place { Place::Local { frame, local } => { - match *self.stack[frame].locals[local].access_mut()? { - Operand::Immediate(ref mut dest_val) => { - // Yay, we can just change the local directly. - *dest_val = src; + match self.stack[frame].locals[local].access_mut()? { + Ok(local) => { + // Local can be updated in-place. + *local = LocalValue::Live(Operand::Immediate(src)); return Ok(()); - }, - Operand::Indirect(mplace) => mplace, // already in memory + } + Err(mplace) => { + // The local is in memory, go on below. + mplace + } } }, - Place::Ptr(mplace) => mplace, // already in memory + Place::Ptr(mplace) => mplace, // already referring to memory }; let dest = MPlaceTy { mplace, layout: dest.layout }; @@ -904,27 +908,40 @@ where ) -> EvalResult<'tcx, MPlaceTy<'tcx, M::PointerTag>> { let mplace = match place.place { Place::Local { frame, local } => { - match *self.stack[frame].locals[local].access()? { - Operand::Indirect(mplace) => mplace, - Operand::Immediate(value) => { + match self.stack[frame].locals[local].access_mut()? { + Ok(local_val) => { // We need to make an allocation. // FIXME: Consider not doing anything for a ZST, and just returning // a fake pointer? Are we even called for ZST? + // We cannot hold on to the reference `local_val` while allocating, + // but we can hold on to the value in there. + let old_val = + if let LocalValue::Live(Operand::Immediate(value)) = *local_val { + Some(value) + } else { + None + }; + // We need the layout of the local. We can NOT use the layout we got, // that might e.g., be an inner field of a struct with `Scalar` layout, // that has different alignment than the outer field. let local_layout = self.layout_of_local(&self.stack[frame], local, None)?; let ptr = self.allocate(local_layout, MemoryKind::Stack); - // We don't have to validate as we can assume the local - // was already valid for its type. - self.write_immediate_to_mplace_no_validate(value, ptr)?; + if let Some(value) = old_val { + // Preserve old value. + // We don't have to validate as we can assume the local + // was already valid for its type. + self.write_immediate_to_mplace_no_validate(value, ptr)?; + } let mplace = ptr.mplace; - // Update the local - *self.stack[frame].locals[local].access_mut()? = - Operand::Indirect(mplace); + // Now we can call `access_mut` again, asserting it goes well, + // and actually overwrite things. + *self.stack[frame].locals[local].access_mut().unwrap().unwrap() = + LocalValue::Live(Operand::Indirect(mplace)); mplace } + Err(mplace) => mplace, // this already was an indirect local } } Place::Ptr(mplace) => mplace diff --git a/src/librustc_mir/interpret/snapshot.rs b/src/librustc_mir/interpret/snapshot.rs index 0bafe6d107a20..f440e2966063c 100644 --- a/src/librustc_mir/interpret/snapshot.rs +++ b/src/librustc_mir/interpret/snapshot.rs @@ -114,10 +114,11 @@ macro_rules! impl_snapshot_for { fn snapshot(&self, __ctx: &'a Ctx) -> Self::Item { match *self { $( - $enum_name::$variant $( ( $(ref $field),* ) )? => + $enum_name::$variant $( ( $(ref $field),* ) )? => { $enum_name::$variant $( - ( $( __impl_snapshot_field!($field, __ctx $(, $delegate)?) ),* ), + ( $( __impl_snapshot_field!($field, __ctx $(, $delegate)?) ),* ) )? + } )* } } @@ -250,11 +251,13 @@ impl_snapshot_for!(enum Operand { impl_stable_hash_for!(enum crate::interpret::LocalValue { Dead, + Uninitialized, Live(x), }); impl_snapshot_for!(enum LocalValue { - Live(v), Dead, + Uninitialized, + Live(v), }); impl<'a, Ctx> Snapshot<'a, Ctx> for Relocations diff --git a/src/librustc_mir/interpret/terminator.rs b/src/librustc_mir/interpret/terminator.rs index 2080a329bb06f..3bf0ff905ab7a 100644 --- a/src/librustc_mir/interpret/terminator.rs +++ b/src/librustc_mir/interpret/terminator.rs @@ -315,12 +315,13 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> InterpretCx<'a, 'mir, 'tcx, M> ); // Figure out how to pass which arguments. - // We have two iterators: Where the arguments come from, - // and where they go to. + // The Rust ABI is special: ZST get skipped. let rust_abi = match caller_abi { Abi::Rust | Abi::RustCall => true, _ => false }; + // We have two iterators: Where the arguments come from, + // and where they go to. // For where they come from: If the ABI is RustCall, we untuple the // last incoming argument. These two iterators do not have the same type, @@ -368,7 +369,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> InterpretCx<'a, 'mir, 'tcx, M> } // Now we should have no more caller args if caller_iter.next().is_some() { - trace!("Caller has too many args over"); + trace!("Caller has passed too many args"); return err!(FunctionArgCountMismatch); } // Don't forget to check the return type! From 525c68cf95d465a69372bf55cb75c20b2688f443 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 7 Apr 2019 12:27:48 +0200 Subject: [PATCH 07/13] make StorageLive lazy as well --- src/librustc_mir/interpret/eval_context.rs | 30 +++++++++++++++------- src/librustc_mir/interpret/operand.rs | 29 +-------------------- 2 files changed, 22 insertions(+), 37 deletions(-) diff --git a/src/librustc_mir/interpret/eval_context.rs b/src/librustc_mir/interpret/eval_context.rs index c3726c63ea4a9..05207c47d5d9d 100644 --- a/src/librustc_mir/interpret/eval_context.rs +++ b/src/librustc_mir/interpret/eval_context.rs @@ -131,6 +131,22 @@ pub enum LocalValue { Live(Operand), } +impl LocalValue { + /// The initial value of a local: ZST get "initialized" because they can be read from without + /// ever having been written to. + fn uninit_local( + layout: TyLayout<'_> + ) -> LocalValue { + // FIXME: Can we avoid this ZST special case? That would likely require MIR + // generation changes. + if layout.is_zst() { + LocalValue::Live(Operand::Immediate(Immediate::Scalar(Scalar::zst().into()))) + } else { + LocalValue::Uninitialized + } + } +} + impl<'tcx, Tag: Copy> LocalState<'tcx, Tag> { pub fn access(&self) -> EvalResult<'tcx, &Operand> { match self.state { @@ -518,19 +534,15 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> InterpretCx<'a, 'mir, 'tc } }, } - // FIXME: We initialize live ZST here. This should not be needed if MIR was - // consistently generated for ZST, but that seems to not be the case -- there - // is MIR (around promoteds in particular) that reads local ZSTs that never - // were written to. + // The remaining locals are uninitialized, fill them with `uninit_local`. + // (For ZST this is not a NOP.) for (idx, local) in locals.iter_enumerated_mut() { match local.state { LocalValue::Uninitialized => { // This needs to be properly initialized. let ty = self.monomorphize(mir.local_decls[idx].ty)?; let layout = self.layout_of(ty)?; - if layout.is_zst() { - local.state = LocalValue::Live(self.uninit_operand(layout)?); - } + local.state = LocalValue::uninit_local(layout); local.layout = Cell::new(Some(layout)); } LocalValue::Dead => { @@ -622,9 +634,9 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> InterpretCx<'a, 'mir, 'tc trace!("{:?} is now live", local); let layout = self.layout_of_local(self.frame(), local, None)?; - let init = LocalValue::Live(self.uninit_operand(layout)?); + let local_val = LocalValue::uninit_local(layout); // StorageLive *always* kills the value that's currently stored - Ok(mem::replace(&mut self.frame_mut().locals[local].state, init)) + Ok(mem::replace(&mut self.frame_mut().locals[local].state, local_val)) } /// Returns the old value of the local. diff --git a/src/librustc_mir/interpret/operand.rs b/src/librustc_mir/interpret/operand.rs index 7ea56e3647437..4ece062f380d6 100644 --- a/src/librustc_mir/interpret/operand.rs +++ b/src/librustc_mir/interpret/operand.rs @@ -14,7 +14,7 @@ use rustc::mir::interpret::{ }; use super::{ InterpretCx, Machine, - MemPlace, MPlaceTy, PlaceTy, Place, MemoryKind, + MemPlace, MPlaceTy, PlaceTy, Place, }; pub use rustc::mir::interpret::ScalarMaybeUndef; @@ -373,33 +373,6 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> InterpretCx<'a, 'mir, 'tcx, M> Ok(str) } - pub fn uninit_operand( - &mut self, - layout: TyLayout<'tcx> - ) -> EvalResult<'tcx, Operand> { - // This decides which types we will use the Immediate optimization for, and hence should - // match what `try_read_immediate` and `eval_place_to_op` support. - if layout.is_zst() { - return Ok(Operand::Immediate(Immediate::Scalar(Scalar::zst().into()))); - } - - Ok(match layout.abi { - layout::Abi::Scalar(..) => - Operand::Immediate(Immediate::Scalar(ScalarMaybeUndef::Undef)), - layout::Abi::ScalarPair(..) => - Operand::Immediate(Immediate::ScalarPair( - ScalarMaybeUndef::Undef, - ScalarMaybeUndef::Undef, - )), - _ => { - trace!("Forcing allocation for local of type {:?}", layout.ty); - Operand::Indirect( - *self.allocate(layout, MemoryKind::Stack) - ) - } - }) - } - /// Projection functions pub fn operand_field( &self, From ae1f8ab4aa6775ae589929a0921f794b1d31c161 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 7 Apr 2019 12:52:56 +0200 Subject: [PATCH 08/13] fix miri engine debug output for uninitialized locals --- src/librustc_mir/interpret/eval_context.rs | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/src/librustc_mir/interpret/eval_context.rs b/src/librustc_mir/interpret/eval_context.rs index 05207c47d5d9d..600d20be397c5 100644 --- a/src/librustc_mir/interpret/eval_context.rs +++ b/src/librustc_mir/interpret/eval_context.rs @@ -698,15 +698,10 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> InterpretCx<'a, 'mir, 'tc } write!(msg, ":").unwrap(); - match self.stack[frame].locals[local].access() { - Err(err) => { - if let InterpError::DeadLocal = err.kind { - write!(msg, " is dead").unwrap(); - } else { - panic!("Failed to access local: {:?}", err); - } - } - Ok(Operand::Indirect(mplace)) => { + match self.stack[frame].locals[local].state { + LocalValue::Dead => write!(msg, " is dead").unwrap(), + LocalValue::Uninitialized => write!(msg, " is uninitialized").unwrap(), + LocalValue::Live(Operand::Indirect(mplace)) => { let (ptr, align) = mplace.to_scalar_ptr_align(); match ptr { Scalar::Ptr(ptr) => { @@ -716,13 +711,13 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> InterpretCx<'a, 'mir, 'tc ptr => write!(msg, " by integral ref: {:?}", ptr).unwrap(), } } - Ok(Operand::Immediate(Immediate::Scalar(val))) => { + LocalValue::Live(Operand::Immediate(Immediate::Scalar(val))) => { write!(msg, " {:?}", val).unwrap(); if let ScalarMaybeUndef::Scalar(Scalar::Ptr(ptr)) = val { allocs.push(ptr.alloc_id); } } - Ok(Operand::Immediate(Immediate::ScalarPair(val1, val2))) => { + LocalValue::Live(Operand::Immediate(Immediate::ScalarPair(val1, val2))) => { write!(msg, " ({:?}, {:?})", val1, val2).unwrap(); if let ScalarMaybeUndef::Scalar(Scalar::Ptr(ptr)) = val1 { allocs.push(ptr.alloc_id); From a2d4e85d06bdefb5ab8396dcf6782dca58c06ec7 Mon Sep 17 00:00:00 2001 From: flip1995 Date: Sun, 7 Apr 2019 19:47:54 +0200 Subject: [PATCH 09/13] Uplift `get_def_path` from Clippy This uplifts `get_def_path` from Clippy. This is a follow up on the implementation of internal lints: #59316 The internal lint implementation also copied the implementation of the `AbsolutePathPrinter`. To get rid of this code duplication this also uplifts the `get_def_path` function from Clippy. This also renames `match_path` to `match_def_path`, as it was originally named in Clippy. --- src/librustc/lint/context.rs | 35 ++++++++++++++++++++++++++++++----- src/librustc/lint/internal.rs | 2 +- 2 files changed, 31 insertions(+), 6 deletions(-) diff --git a/src/librustc/lint/context.rs b/src/librustc/lint/context.rs index 15ea6403e38ba..2aca195aee058 100644 --- a/src/librustc/lint/context.rs +++ b/src/librustc/lint/context.rs @@ -755,8 +755,31 @@ impl<'a, 'tcx> LateContext<'a, 'tcx> { } /// Check if a `DefId`'s path matches the given absolute type path usage. + /// + /// # Examples + /// ```rust,ignore (no cx or def_id available) + /// if cx.match_def_path(def_id, &["core", "option", "Option"]) { + /// // The given `def_id` is that of an `Option` type + /// } + /// ``` // Uplifted from rust-lang/rust-clippy - pub fn match_path(&self, def_id: DefId, path: &[&str]) -> bool { + pub fn match_def_path(&self, def_id: DefId, path: &[&str]) -> bool { + let names = self.get_def_path(def_id); + + names.len() == path.len() && names.into_iter().zip(path.iter()).all(|(a, &b)| *a == *b) + } + + /// Gets the absolute path of `def_id` as a vector of `&str`. + /// + /// # Examples + /// ```rust,ignore (no cx or def_id available) + /// let def_path = cx.get_def_path(def_id); + /// if let &["core", "option", "Option"] = &def_path[..] { + /// // The given `def_id` is that of an `Option` type + /// } + /// ``` + // Uplifted from rust-lang/rust-clippy + pub fn get_def_path(&self, def_id: DefId) -> Vec<&'static str> { pub struct AbsolutePathPrinter<'a, 'tcx> { pub tcx: TyCtxt<'a, 'tcx, 'tcx>, } @@ -856,10 +879,12 @@ impl<'a, 'tcx> LateContext<'a, 'tcx> { } } - let names = AbsolutePathPrinter { tcx: self.tcx }.print_def_path(def_id, &[]).unwrap(); - - names.len() == path.len() - && names.into_iter().zip(path.iter()).all(|(a, &b)| *a == *b) + AbsolutePathPrinter { tcx: self.tcx } + .print_def_path(def_id, &[]) + .unwrap() + .iter() + .map(LocalInternedString::get) + .collect() } } diff --git a/src/librustc/lint/internal.rs b/src/librustc/lint/internal.rs index 0bafa93011ec4..91f1bee26de32 100644 --- a/src/librustc/lint/internal.rs +++ b/src/librustc/lint/internal.rs @@ -100,7 +100,7 @@ fn lint_ty_kind_usage(cx: &LateContext<'_, '_>, segment: &PathSegment) -> bool { if segment.ident.as_str() == "TyKind" { if let Some(def) = segment.def { if let Some(did) = def.opt_def_id() { - return cx.match_path(did, &["rustc", "ty", "sty", "TyKind"]); + return cx.match_def_path(did, &["rustc", "ty", "sty", "TyKind"]); } } } From 944ffbf5b550b4e6e2fa509d59ae1ae5d72d10ea Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 7 Apr 2019 19:54:29 +0200 Subject: [PATCH 10/13] initialize unsized locals when copying to the for the first time --- src/librustc_mir/interpret/eval_context.rs | 11 ++- src/librustc_mir/interpret/place.rs | 79 ++++++++++++++-------- 2 files changed, 57 insertions(+), 33 deletions(-) diff --git a/src/librustc_mir/interpret/eval_context.rs b/src/librustc_mir/interpret/eval_context.rs index 600d20be397c5..7c900bd596ac8 100644 --- a/src/librustc_mir/interpret/eval_context.rs +++ b/src/librustc_mir/interpret/eval_context.rs @@ -702,10 +702,15 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> InterpretCx<'a, 'mir, 'tc LocalValue::Dead => write!(msg, " is dead").unwrap(), LocalValue::Uninitialized => write!(msg, " is uninitialized").unwrap(), LocalValue::Live(Operand::Indirect(mplace)) => { - let (ptr, align) = mplace.to_scalar_ptr_align(); - match ptr { + match mplace.ptr { Scalar::Ptr(ptr) => { - write!(msg, " by align({}) ref:", align.bytes()).unwrap(); + write!(msg, " by align({}){} ref:", + mplace.align.bytes(), + match mplace.meta { + Some(meta) => format!(" meta({:?})", meta), + None => String::new() + } + ).unwrap(); allocs.push(ptr.alloc_id); } ptr => write!(msg, " by integral ref: {:?}", ptr).unwrap(), diff --git a/src/librustc_mir/interpret/place.rs b/src/librustc_mir/interpret/place.rs index f69ce4e0d3d17..93bef813ba691 100644 --- a/src/librustc_mir/interpret/place.rs +++ b/src/librustc_mir/interpret/place.rs @@ -826,8 +826,6 @@ where src: OpTy<'tcx, M::PointerTag>, dest: PlaceTy<'tcx, M::PointerTag>, ) -> EvalResult<'tcx> { - debug_assert!(!src.layout.is_unsized() && !dest.layout.is_unsized(), - "Cannot copy unsized data"); // We do NOT compare the types for equality, because well-typed code can // actually "transmute" `&mut T` to `&T` in an assignment without a cast. assert!(src.layout.details == dest.layout.details, @@ -836,6 +834,7 @@ where // Let us see if the layout is simple so we take a shortcut, avoid force_allocation. let src = match self.try_read_immediate(src)? { Ok(src_val) => { + assert!(!src.layout.is_unsized(), "cannot have unsized immediates"); // Yay, we got a value that we can write directly. // FIXME: Add a check to make sure that if `src` is indirect, // it does not overlap with `dest`. @@ -846,13 +845,19 @@ where // Slow path, this does not fit into an immediate. Just memcpy. trace!("copy_op: {:?} <- {:?}: {}", *dest, src, dest.layout.ty); - let dest = self.force_allocation(dest)?; - let (src_ptr, src_align) = src.to_scalar_ptr_align(); - let (dest_ptr, dest_align) = dest.to_scalar_ptr_align(); + // This interprets `src.meta` with the `dest` local's layout, if an unsized local + // is being initialized! + let (dest, size) = self.force_allocation_maybe_sized(dest, src.meta)?; + let size = size.unwrap_or_else(|| { + assert!(!dest.layout.is_unsized(), + "Cannot copy into already initialized unsized place"); + dest.layout.size + }); + assert_eq!(src.meta, dest.meta, "Can only copy between equally-sized instances"); self.memory.copy( - src_ptr, src_align, - dest_ptr, dest_align, - dest.layout.size, + src.ptr, src.align, + dest.ptr, dest.align, + size, /*nonoverlapping*/ true, )?; @@ -870,11 +875,13 @@ where // Fast path: Just use normal `copy_op` return self.copy_op(src, dest); } - // We still require the sizes to match - debug_assert!(!src.layout.is_unsized() && !dest.layout.is_unsized(), - "Cannot copy unsized data"); + // We still require the sizes to match. assert!(src.layout.size == dest.layout.size, "Size mismatch when transmuting!\nsrc: {:#?}\ndest: {:#?}", src, dest); + // Unsized copies rely on interpreting `src.meta` with `dest.layout`, we want + // to avoid that here. + assert!(!src.layout.is_unsized() && !dest.layout.is_unsized(), + "Cannot transmute unsized data"); // The hard case is `ScalarPair`. `src` is already read from memory in this case, // using `src.layout` to figure out which bytes to use for the 1st and 2nd field. @@ -902,11 +909,16 @@ where /// If the place currently refers to a local that doesn't yet have a matching allocation, /// create such an allocation. /// This is essentially `force_to_memplace`. - pub fn force_allocation( + /// + /// This supports unsized types and returnes the computed size to avoid some + /// redundant computation when copying; use `force_allocation` for a simpler, sized-only + /// version. + pub fn force_allocation_maybe_sized( &mut self, place: PlaceTy<'tcx, M::PointerTag>, - ) -> EvalResult<'tcx, MPlaceTy<'tcx, M::PointerTag>> { - let mplace = match place.place { + meta: Option>, + ) -> EvalResult<'tcx, (MPlaceTy<'tcx, M::PointerTag>, Option)> { + let (mplace, size) = match place.place { Place::Local { frame, local } => { match self.stack[frame].locals[local].access_mut()? { Ok(local_val) => { @@ -926,28 +938,41 @@ where // We need the layout of the local. We can NOT use the layout we got, // that might e.g., be an inner field of a struct with `Scalar` layout, // that has different alignment than the outer field. + // We also need to support unsized types, and hence cannot use `allocate`. let local_layout = self.layout_of_local(&self.stack[frame], local, None)?; - let ptr = self.allocate(local_layout, MemoryKind::Stack); + let (size, align) = self.size_and_align_of(meta, local_layout)? + .expect("Cannot allocate for non-dyn-sized type"); + let ptr = self.memory.allocate(size, align, MemoryKind::Stack); + let ptr = M::tag_new_allocation(self, ptr, MemoryKind::Stack); + let mplace = MemPlace { ptr: ptr.into(), align, meta }; if let Some(value) = old_val { // Preserve old value. // We don't have to validate as we can assume the local // was already valid for its type. - self.write_immediate_to_mplace_no_validate(value, ptr)?; + let mplace = MPlaceTy { mplace, layout: local_layout }; + self.write_immediate_to_mplace_no_validate(value, mplace)?; } - let mplace = ptr.mplace; // Now we can call `access_mut` again, asserting it goes well, // and actually overwrite things. *self.stack[frame].locals[local].access_mut().unwrap().unwrap() = LocalValue::Live(Operand::Indirect(mplace)); - mplace + (mplace, Some(size)) } - Err(mplace) => mplace, // this already was an indirect local + Err(mplace) => (mplace, None), // this already was an indirect local } } - Place::Ptr(mplace) => mplace + Place::Ptr(mplace) => (mplace, None) }; // Return with the original layout, so that the caller can go on - Ok(MPlaceTy { mplace, layout: place.layout }) + Ok((MPlaceTy { mplace, layout: place.layout }, size)) + } + + #[inline(always)] + pub fn force_allocation( + &mut self, + place: PlaceTy<'tcx, M::PointerTag>, + ) -> EvalResult<'tcx, MPlaceTy<'tcx, M::PointerTag>> { + Ok(self.force_allocation_maybe_sized(place, None)?.0) } pub fn allocate( @@ -955,15 +980,9 @@ where layout: TyLayout<'tcx>, kind: MemoryKind, ) -> MPlaceTy<'tcx, M::PointerTag> { - if layout.is_unsized() { - assert!(self.tcx.features().unsized_locals, "cannot alloc memory for unsized type"); - // FIXME: What should we do here? We should definitely also tag! - MPlaceTy::dangling(layout, self) - } else { - let ptr = self.memory.allocate(layout.size, layout.align.abi, kind); - let ptr = M::tag_new_allocation(self, ptr, kind); - MPlaceTy::from_aligned_ptr(ptr, layout) - } + let ptr = self.memory.allocate(layout.size, layout.align.abi, kind); + let ptr = M::tag_new_allocation(self, ptr, kind); + MPlaceTy::from_aligned_ptr(ptr, layout) } pub fn write_discriminant_index( From b00fd57075263f928a59b0d375f004427077c53a Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 7 Apr 2019 19:54:43 +0200 Subject: [PATCH 11/13] implement by-value object safety --- src/librustc_mir/interpret/terminator.rs | 38 +++++++++++++++++------- src/librustc_mir/interpret/traits.rs | 11 +++++-- 2 files changed, 36 insertions(+), 13 deletions(-) diff --git a/src/librustc_mir/interpret/terminator.rs b/src/librustc_mir/interpret/terminator.rs index 3bf0ff905ab7a..ba48a28fc8315 100644 --- a/src/librustc_mir/interpret/terminator.rs +++ b/src/librustc_mir/interpret/terminator.rs @@ -407,9 +407,24 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> InterpretCx<'a, 'mir, 'tcx, M> } // cannot use the shim here, because that will only result in infinite recursion ty::InstanceDef::Virtual(_, idx) => { + let mut args = args.to_vec(); let ptr_size = self.pointer_size(); - let ptr = self.deref_operand(args[0])?; - let vtable = ptr.vtable()?; + // We have to implement all "object safe receivers". Currently we + // support built-in pointers (&, &mut, Box) as well as unsized-self. We do + // not yet support custom self types. + // Also see librustc_codegen_llvm/abi.rs and librustc_codegen_llvm/mir/block.rs. + let receiver_place = match args[0].layout.ty.builtin_deref(true) { + Some(_) => { + // Built-in pointer. + self.deref_operand(args[0])? + } + None => { + // Unsized self. + args[0].to_mem_place() + } + }; + // Find and consult vtable + let vtable = receiver_place.vtable()?; self.memory.check_align(vtable.into(), self.tcx.data_layout.pointer_align.abi)?; let fn_ptr = self.memory.get(vtable.alloc_id)?.read_ptr_sized( self, @@ -417,15 +432,16 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> InterpretCx<'a, 'mir, 'tcx, M> )?.to_ptr()?; let instance = self.memory.get_fn(fn_ptr)?; - // We have to patch the self argument, in particular get the layout - // expected by the actual function. Cannot just use "field 0" due to - // Box. - let mut args = args.to_vec(); - let pointee = args[0].layout.ty.builtin_deref(true).unwrap().ty; - let fake_fat_ptr_ty = self.tcx.mk_mut_ptr(pointee); - args[0] = OpTy::from(ImmTy { // strip vtable - layout: self.layout_of(fake_fat_ptr_ty)?.field(self, 0)?, - imm: Immediate::Scalar(ptr.ptr.into()) + // `*mut receiver_place.layout.ty` is almost the layout that we + // want for args[0]: We have to project to field 0 because we want + // a thin pointer. + assert!(receiver_place.layout.is_unsized()); + let receiver_ptr_ty = self.tcx.mk_mut_ptr(receiver_place.layout.ty); + let this_receiver_ptr = self.layout_of(receiver_ptr_ty)?.field(self, 0)?; + // Adjust receiver argument. + args[0] = OpTy::from(ImmTy { + layout: this_receiver_ptr, + imm: Immediate::Scalar(receiver_place.ptr.into()) }); trace!("Patched self operand to {:#?}", args[0]); // recurse with concrete function diff --git a/src/librustc_mir/interpret/traits.rs b/src/librustc_mir/interpret/traits.rs index cce6c95a31240..94e7f883f575b 100644 --- a/src/librustc_mir/interpret/traits.rs +++ b/src/librustc_mir/interpret/traits.rs @@ -3,7 +3,7 @@ use rustc::ty::{self, Ty}; use rustc::ty::layout::{Size, Align, LayoutOf}; use rustc::mir::interpret::{Scalar, Pointer, EvalResult, PointerArithmetic}; -use super::{InterpretCx, Machine, MemoryKind}; +use super::{InterpretCx, InterpError, Machine, MemoryKind}; impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> InterpretCx<'a, 'mir, 'tcx, M> { /// Creates a dynamic vtable for the given type and vtable origin. This is used only for @@ -76,7 +76,14 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> InterpretCx<'a, 'mir, 'tcx, M> for (i, method) in methods.iter().enumerate() { if let Some((def_id, substs)) = *method { - let instance = self.resolve(def_id, substs)?; + // resolve for vtable: insert thims where needed + let substs = self.subst_and_normalize_erasing_regions(substs)?; + let instance = ty::Instance::resolve_for_vtable( + *self.tcx, + self.param_env, + def_id, + substs, + ).ok_or_else(|| InterpError::TooGeneric)?; let fn_ptr = self.memory.create_fn_alloc(instance).with_default_tag(); let method_ptr = vtable.offset(ptr_size * (3 + i as u64), self)?; self.memory From 5731945187785e87352cf112380fad685db89636 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 8 Apr 2019 10:29:22 +0200 Subject: [PATCH 12/13] Apply suggestions from code review typos Co-Authored-By: RalfJung --- src/librustc_mir/interpret/place.rs | 2 +- src/librustc_mir/interpret/traits.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/librustc_mir/interpret/place.rs b/src/librustc_mir/interpret/place.rs index 93bef813ba691..048d51acaf2a2 100644 --- a/src/librustc_mir/interpret/place.rs +++ b/src/librustc_mir/interpret/place.rs @@ -910,7 +910,7 @@ where /// create such an allocation. /// This is essentially `force_to_memplace`. /// - /// This supports unsized types and returnes the computed size to avoid some + /// This supports unsized types and returns the computed size to avoid some /// redundant computation when copying; use `force_allocation` for a simpler, sized-only /// version. pub fn force_allocation_maybe_sized( diff --git a/src/librustc_mir/interpret/traits.rs b/src/librustc_mir/interpret/traits.rs index 94e7f883f575b..d76d3a3301620 100644 --- a/src/librustc_mir/interpret/traits.rs +++ b/src/librustc_mir/interpret/traits.rs @@ -76,7 +76,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> InterpretCx<'a, 'mir, 'tcx, M> for (i, method) in methods.iter().enumerate() { if let Some((def_id, substs)) = *method { - // resolve for vtable: insert thims where needed + // resolve for vtable: insert shims where needed let substs = self.subst_and_normalize_erasing_regions(substs)?; let instance = ty::Instance::resolve_for_vtable( *self.tcx, From 4d79d391b0aa1175f493e3544d8f66e6600fbfc6 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 8 Apr 2019 13:28:51 +0200 Subject: [PATCH 13/13] avoid reading from ZST locals --- src/librustc_mir/interpret/eval_context.rs | 70 ++++++---------------- src/librustc_mir/interpret/operand.rs | 9 ++- src/librustc_mir/interpret/snapshot.rs | 6 +- 3 files changed, 29 insertions(+), 56 deletions(-) diff --git a/src/librustc_mir/interpret/eval_context.rs b/src/librustc_mir/interpret/eval_context.rs index 7c900bd596ac8..32f7ecd97b2ef 100644 --- a/src/librustc_mir/interpret/eval_context.rs +++ b/src/librustc_mir/interpret/eval_context.rs @@ -108,13 +108,13 @@ pub enum StackPopCleanup { /// State of a local variable including a memoized layout #[derive(Clone, PartialEq, Eq)] pub struct LocalState<'tcx, Tag=(), Id=AllocId> { - pub state: LocalValue, + pub value: LocalValue, /// Don't modify if `Some`, this is only used to prevent computing the layout twice pub layout: Cell>>, } -/// State of a local variable -#[derive(Copy, Clone, PartialEq, Eq, Hash)] +/// Current value of a local variable +#[derive(Copy, Clone, PartialEq, Eq, Hash, Debug)] pub enum LocalValue { /// This local is not currently alive, and cannot be used at all. Dead, @@ -131,27 +131,13 @@ pub enum LocalValue { Live(Operand), } -impl LocalValue { - /// The initial value of a local: ZST get "initialized" because they can be read from without - /// ever having been written to. - fn uninit_local( - layout: TyLayout<'_> - ) -> LocalValue { - // FIXME: Can we avoid this ZST special case? That would likely require MIR - // generation changes. - if layout.is_zst() { - LocalValue::Live(Operand::Immediate(Immediate::Scalar(Scalar::zst().into()))) - } else { - LocalValue::Uninitialized - } - } -} - -impl<'tcx, Tag: Copy> LocalState<'tcx, Tag> { - pub fn access(&self) -> EvalResult<'tcx, &Operand> { - match self.state { - LocalValue::Dead | LocalValue::Uninitialized => err!(DeadLocal), - LocalValue::Live(ref val) => Ok(val), +impl<'tcx, Tag: Copy + 'static> LocalState<'tcx, Tag> { + pub fn access(&self) -> EvalResult<'tcx, Operand> { + match self.value { + LocalValue::Dead => err!(DeadLocal), + LocalValue::Uninitialized => + bug!("The type checker should prevent reading from a never-written local"), + LocalValue::Live(val) => Ok(val), } } @@ -160,7 +146,7 @@ impl<'tcx, Tag: Copy> LocalState<'tcx, Tag> { pub fn access_mut( &mut self, ) -> EvalResult<'tcx, Result<&mut LocalValue, MemPlace>> { - match self.state { + match self.value { LocalValue::Dead => err!(DeadLocal), LocalValue::Live(Operand::Indirect(mplace)) => Ok(Err(mplace)), ref mut local @ LocalValue::Live(Operand::Immediate(_)) | @@ -507,13 +493,13 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> InterpretCx<'a, 'mir, 'tc if mir.local_decls.len() > 1 { // Locals are initially uninitialized. let dummy = LocalState { - state: LocalValue::Uninitialized, + value: LocalValue::Uninitialized, layout: Cell::new(None), }; let mut locals = IndexVec::from_elem(dummy, &mir.local_decls); // Return place is handled specially by the `eval_place` functions, and the // entry in `locals` should never be used. Make it dead, to be sure. - locals[mir::RETURN_PLACE].state = LocalValue::Dead; + locals[mir::RETURN_PLACE].value = LocalValue::Dead; // Now mark those locals as dead that we do not want to initialize match self.tcx.describe_def(instance.def_id()) { // statics and constants don't have `Storage*` statements, no need to look for them @@ -526,7 +512,7 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> InterpretCx<'a, 'mir, 'tc match stmt.kind { StorageLive(local) | StorageDead(local) => { - locals[local].state = LocalValue::Dead; + locals[local].value = LocalValue::Dead; } _ => {} } @@ -534,23 +520,6 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> InterpretCx<'a, 'mir, 'tc } }, } - // The remaining locals are uninitialized, fill them with `uninit_local`. - // (For ZST this is not a NOP.) - for (idx, local) in locals.iter_enumerated_mut() { - match local.state { - LocalValue::Uninitialized => { - // This needs to be properly initialized. - let ty = self.monomorphize(mir.local_decls[idx].ty)?; - let layout = self.layout_of(ty)?; - local.state = LocalValue::uninit_local(layout); - local.layout = Cell::new(Some(layout)); - } - LocalValue::Dead => { - // Nothing to do - } - LocalValue::Live(_) => bug!("Locals cannot be live yet"), - } - } // done self.frame_mut().locals = locals; } @@ -585,7 +554,7 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> InterpretCx<'a, 'mir, 'tc } // Deallocate all locals that are backed by an allocation. for local in frame.locals { - self.deallocate_local(local.state)?; + self.deallocate_local(local.value)?; } // Validate the return value. Do this after deallocating so that we catch dangling // references. @@ -633,10 +602,9 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> InterpretCx<'a, 'mir, 'tc assert!(local != mir::RETURN_PLACE, "Cannot make return place live"); trace!("{:?} is now live", local); - let layout = self.layout_of_local(self.frame(), local, None)?; - let local_val = LocalValue::uninit_local(layout); + let local_val = LocalValue::Uninitialized; // StorageLive *always* kills the value that's currently stored - Ok(mem::replace(&mut self.frame_mut().locals[local].state, local_val)) + Ok(mem::replace(&mut self.frame_mut().locals[local].value, local_val)) } /// Returns the old value of the local. @@ -645,7 +613,7 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> InterpretCx<'a, 'mir, 'tc assert!(local != mir::RETURN_PLACE, "Cannot make return place dead"); trace!("{:?} is now dead", local); - mem::replace(&mut self.frame_mut().locals[local].state, LocalValue::Dead) + mem::replace(&mut self.frame_mut().locals[local].value, LocalValue::Dead) } pub(super) fn deallocate_local( @@ -698,7 +666,7 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> InterpretCx<'a, 'mir, 'tc } write!(msg, ":").unwrap(); - match self.stack[frame].locals[local].state { + match self.stack[frame].locals[local].value { LocalValue::Dead => write!(msg, " is dead").unwrap(), LocalValue::Uninitialized => write!(msg, " is uninitialized").unwrap(), LocalValue::Live(Operand::Indirect(mplace)) => { diff --git a/src/librustc_mir/interpret/operand.rs b/src/librustc_mir/interpret/operand.rs index 4ece062f380d6..65cd7be8fd4b2 100644 --- a/src/librustc_mir/interpret/operand.rs +++ b/src/librustc_mir/interpret/operand.rs @@ -459,8 +459,13 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> InterpretCx<'a, 'mir, 'tcx, M> layout: Option>, ) -> EvalResult<'tcx, OpTy<'tcx, M::PointerTag>> { assert_ne!(local, mir::RETURN_PLACE); - let op = *frame.locals[local].access()?; let layout = self.layout_of_local(frame, local, layout)?; + let op = if layout.is_zst() { + // Do not read from ZST, they might not be initialized + Operand::Immediate(Immediate::Scalar(Scalar::zst().into())) + } else { + frame.locals[local].access()? + }; Ok(OpTy { op, layout }) } @@ -475,7 +480,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> InterpretCx<'a, 'mir, 'tcx, M> Operand::Indirect(mplace) } Place::Local { frame, local } => - *self.stack[frame].locals[local].access()? + *self.access_local(&self.stack[frame], local, None)? }; Ok(OpTy { op, layout: place.layout }) } diff --git a/src/librustc_mir/interpret/snapshot.rs b/src/librustc_mir/interpret/snapshot.rs index f440e2966063c..0bb8b1d9d02ca 100644 --- a/src/librustc_mir/interpret/snapshot.rs +++ b/src/librustc_mir/interpret/snapshot.rs @@ -363,13 +363,13 @@ impl<'a, 'tcx, Ctx> Snapshot<'a, Ctx> for &'a LocalState<'tcx> type Item = LocalValue<(), AllocIdSnapshot<'a>>; fn snapshot(&self, ctx: &'a Ctx) -> Self::Item { - let LocalState { state, layout: _ } = self; - state.snapshot(ctx) + let LocalState { value, layout: _ } = self; + value.snapshot(ctx) } } impl_stable_hash_for!(struct LocalState<'tcx> { - state, + value, layout -> _, });