From cdc730457e9030feaf8c4376a668a9ef61c7f189 Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Fri, 6 Mar 2020 11:20:27 +0100 Subject: [PATCH 01/14] Compute the correct layout for variants of uninhabited enums and readd a long lost assertion This reverts part of commit 9712fa405944cb8d5416556ac4b1f26365a10658. --- src/librustc/ty/layout.rs | 14 +++++++++++--- src/librustc_mir/interpret/operand.rs | 2 +- src/librustc_mir/interpret/place.rs | 8 -------- src/librustc_target/abi/mod.rs | 6 +++++- 4 files changed, 17 insertions(+), 13 deletions(-) diff --git a/src/librustc/ty/layout.rs b/src/librustc/ty/layout.rs index dedb3035cedb3..f616d81603775 100644 --- a/src/librustc/ty/layout.rs +++ b/src/librustc/ty/layout.rs @@ -782,8 +782,8 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> { present_first @ Some(_) => present_first, // Uninhabited because it has no variants, or only absent ones. None if def.is_enum() => return tcx.layout_raw(param_env.and(tcx.types.never)), - // if it's a struct, still compute a layout so that we can still compute the - // field offsets + // If it's a struct, still compute a layout so that we can still compute the + // field offsets. None => Some(VariantIdx::new(0)), }; @@ -1990,7 +1990,15 @@ where { fn for_variant(this: TyLayout<'tcx>, cx: &C, variant_index: VariantIdx) -> TyLayout<'tcx> { let details = match this.variants { - Variants::Single { index } if index == variant_index => this.details, + Variants::Single { index } + // If all variants but one are uninhabited, the variant layout is the enum layout. + if index == variant_index && + // Don't confuse variants of uninhabited enums with the enum itself. + // For more details see https://github.com/rust-lang/rust/issues/69763. + this.fields != FieldPlacement::Union(0) => + { + this.details + } Variants::Single { index } => { // Deny calling for_variant more than once for non-Single enums. diff --git a/src/librustc_mir/interpret/operand.rs b/src/librustc_mir/interpret/operand.rs index 22b1a7b7137d9..5d035bbeb27c7 100644 --- a/src/librustc_mir/interpret/operand.rs +++ b/src/librustc_mir/interpret/operand.rs @@ -356,7 +356,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { ) -> InterpResult<'tcx, OpTy<'tcx, M::PointerTag>> { let base = match op.try_as_mplace(self) { Ok(mplace) => { - // The easy case + // We can reuse the mplace field computation logic for indirect operands let field = self.mplace_field(mplace, field)?; return Ok(field.into()); } diff --git a/src/librustc_mir/interpret/place.rs b/src/librustc_mir/interpret/place.rs index a4815b9696ebb..856c654980ab7 100644 --- a/src/librustc_mir/interpret/place.rs +++ b/src/librustc_mir/interpret/place.rs @@ -410,14 +410,6 @@ where stride * field } layout::FieldPlacement::Union(count) => { - // This is a narrow bug-fix for rust-lang/rust#69191: if we are - // trying to access absent field of uninhabited variant, then - // signal UB (but don't ICE the compiler). - // FIXME temporary hack to work around incoherence between - // layout computation and MIR building - if field >= count as u64 && base.layout.abi == layout::Abi::Uninhabited { - throw_ub!(Unreachable); - } assert!( field < count as u64, "Tried to access field {} of union {:#?} with {} fields", diff --git a/src/librustc_target/abi/mod.rs b/src/librustc_target/abi/mod.rs index 2f8bbd66c322b..681326b0f150a 100644 --- a/src/librustc_target/abi/mod.rs +++ b/src/librustc_target/abi/mod.rs @@ -660,7 +660,11 @@ impl FieldPlacement { pub fn offset(&self, i: usize) -> Size { match *self { - FieldPlacement::Union(_) => Size::ZERO, + FieldPlacement::Union(count) => { + assert!(i < count, + "Tried to access field {} of union with {} fields", i, count); + Size::ZERO + }, FieldPlacement::Array { stride, count } => { let i = i as u64; assert!(i < count); From ec88ffa38cee51fa7290aa6c99d928ffe346ca6c Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Wed, 11 Mar 2020 13:57:54 +0100 Subject: [PATCH 02/14] Comment nits Co-Authored-By: Ralf Jung --- src/librustc_mir/interpret/operand.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_mir/interpret/operand.rs b/src/librustc_mir/interpret/operand.rs index 5d035bbeb27c7..07c0f76e03a7e 100644 --- a/src/librustc_mir/interpret/operand.rs +++ b/src/librustc_mir/interpret/operand.rs @@ -356,7 +356,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { ) -> InterpResult<'tcx, OpTy<'tcx, M::PointerTag>> { let base = match op.try_as_mplace(self) { Ok(mplace) => { - // We can reuse the mplace field computation logic for indirect operands + // We can reuse the mplace field computation logic for indirect operands. let field = self.mplace_field(mplace, field)?; return Ok(field.into()); } From 74608c7f206171cb72c020a03800b2d9035a35fa Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Wed, 11 Mar 2020 14:31:07 +0100 Subject: [PATCH 03/14] Rustfmt and adjust capitalization --- src/librustc_target/abi/mod.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/librustc_target/abi/mod.rs b/src/librustc_target/abi/mod.rs index 681326b0f150a..afa30e7e632a7 100644 --- a/src/librustc_target/abi/mod.rs +++ b/src/librustc_target/abi/mod.rs @@ -661,10 +661,9 @@ impl FieldPlacement { pub fn offset(&self, i: usize) -> Size { match *self { FieldPlacement::Union(count) => { - assert!(i < count, - "Tried to access field {} of union with {} fields", i, count); + assert!(i < count, "tried to access field {} of union with {} fields", i, count); Size::ZERO - }, + } FieldPlacement::Array { stride, count } => { let i = i as u64; assert!(i < count); From bee151308d5c5090627772c04e17f783aa53451a Mon Sep 17 00:00:00 2001 From: David Wood Date: Wed, 11 Mar 2020 19:36:07 +0000 Subject: [PATCH 04/14] codegen/mir: support polymorphic `InstanceDef`s This commit modifies the use of `subst_and_normalize_erasing_regions` on parts of the MIR bodies returned from `instance_mir`, so that `InstanceDef::CloneShim` and `InstanceDef::DropGlue` (where there is a type) do not perform substitutions. This avoids double substitutions and enables polymorphic `InstanceDef`s. Signed-off-by: David Wood --- src/librustc/ty/instance.rs | 26 ++++++ src/librustc_codegen_ssa/mir/mod.rs | 17 ++-- src/librustc_mir/interpret/eval_context.rs | 27 ++++-- src/librustc_mir/interpret/operand.rs | 3 +- src/librustc_mir/interpret/place.rs | 8 +- src/librustc_mir/interpret/step.rs | 2 +- src/librustc_mir/monomorphize/collector.rs | 102 ++++++++------------- 7 files changed, 102 insertions(+), 83 deletions(-) diff --git a/src/librustc/ty/instance.rs b/src/librustc/ty/instance.rs index 445df76cd32be..78fcc494f6c73 100644 --- a/src/librustc/ty/instance.rs +++ b/src/librustc/ty/instance.rs @@ -369,6 +369,32 @@ impl<'tcx> Instance<'tcx> { Instance { def, substs } } + /// FIXME(#69925) Depending on the kind of `InstanceDef`, the MIR body associated with an + /// instance is expressed in terms of the generic parameters of `self.def_id()`, and in other + /// cases the MIR body is expressed in terms of the types found in the substitution array. + /// In the former case, we want to substitute those generic types and replace them with the + /// values from the substs when monomorphizing the function body. But in the latter case, we + /// don't want to do that substitution, since it has already been done effectively. + /// + /// This function returns `Some(substs)` in the former case and None otherwise -- i.e., if + /// this function returns `None`, then the MIR body does not require substitution during + /// monomorphization. + pub fn substs_for_mir_body(&self) -> Option> { + match self.def { + InstanceDef::CloneShim(..) + | InstanceDef::DropGlue(_, Some(_)) => None, + InstanceDef::ClosureOnceShim { .. } + | InstanceDef::DropGlue(..) + // FIXME(#69925): `FnPtrShim` should be in the other branch. + | InstanceDef::FnPtrShim(..) + | InstanceDef::Item(_) + | InstanceDef::Intrinsic(..) + | InstanceDef::ReifyShim(..) + | InstanceDef::Virtual(..) + | InstanceDef::VtableShim(..) => Some(self.substs), + } + } + pub fn is_vtable_shim(&self) -> bool { if let InstanceDef::VtableShim(..) = self.def { true } else { false } } diff --git a/src/librustc_codegen_ssa/mir/mod.rs b/src/librustc_codegen_ssa/mir/mod.rs index 64ead19b35869..000db0155ada3 100644 --- a/src/librustc_codegen_ssa/mir/mod.rs +++ b/src/librustc_codegen_ssa/mir/mod.rs @@ -86,13 +86,18 @@ pub struct FunctionCx<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> { impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { pub fn monomorphize(&self, value: &T) -> T where - T: TypeFoldable<'tcx>, + T: Copy + TypeFoldable<'tcx>, { - self.cx.tcx().subst_and_normalize_erasing_regions( - self.instance.substs, - ty::ParamEnv::reveal_all(), - value, - ) + debug!("monomorphize: self.instance={:?}", self.instance); + if let Some(substs) = self.instance.substs_for_mir_body() { + self.cx.tcx().subst_and_normalize_erasing_regions( + substs, + ty::ParamEnv::reveal_all(), + &value, + ) + } else { + self.cx.tcx().normalize_erasing_regions(ty::ParamEnv::reveal_all(), *value) + } } } diff --git a/src/librustc_mir/interpret/eval_context.rs b/src/librustc_mir/interpret/eval_context.rs index 9b28b7a20c044..85ac225bd2eda 100644 --- a/src/librustc_mir/interpret/eval_context.rs +++ b/src/librustc_mir/interpret/eval_context.rs @@ -335,15 +335,25 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { /// Call this on things you got out of the MIR (so it is as generic as the current /// stack frame), to bring it into the proper environment for this interpreter. + pub(super) fn subst_from_current_frame_and_normalize_erasing_regions>( + &self, + value: T, + ) -> T { + self.subst_from_frame_and_normalize_erasing_regions(self.frame(), value) + } + + /// Call this on things you got out of the MIR (so it is as generic as the provided + /// stack frame), to bring it into the proper environment for this interpreter. pub(super) fn subst_from_frame_and_normalize_erasing_regions>( &self, + frame: &Frame<'mir, 'tcx, M::PointerTag, M::FrameExtra>, value: T, ) -> T { - self.tcx.subst_and_normalize_erasing_regions( - self.frame().instance.substs, - self.param_env, - &value, - ) + if let Some(substs) = frame.instance.substs_for_mir_body() { + self.tcx.subst_and_normalize_erasing_regions(substs, self.param_env, &value) + } else { + self.tcx.normalize_erasing_regions(self.param_env, value) + } } /// The `substs` are assumed to already be in our interpreter "universe" (param_env). @@ -371,11 +381,8 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { None => { let layout = crate::interpret::operand::from_known_layout(layout, || { let local_ty = frame.body.local_decls[local].ty; - let local_ty = self.tcx.subst_and_normalize_erasing_regions( - frame.instance.substs, - self.param_env, - &local_ty, - ); + let local_ty = + self.subst_from_frame_and_normalize_erasing_regions(frame, local_ty); self.layout_of(local_ty) })?; if let Some(state) = frame.locals.get(local) { diff --git a/src/librustc_mir/interpret/operand.rs b/src/librustc_mir/interpret/operand.rs index 22b1a7b7137d9..79cd082433061 100644 --- a/src/librustc_mir/interpret/operand.rs +++ b/src/librustc_mir/interpret/operand.rs @@ -491,7 +491,8 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { Copy(ref place) | Move(ref place) => self.eval_place_to_op(place, layout)?, Constant(ref constant) => { - let val = self.subst_from_frame_and_normalize_erasing_regions(constant.literal); + let val = + self.subst_from_current_frame_and_normalize_erasing_regions(constant.literal); self.eval_const_to_op(val, layout)? } }; diff --git a/src/librustc_mir/interpret/place.rs b/src/librustc_mir/interpret/place.rs index a4815b9696ebb..c0ec2e5844842 100644 --- a/src/librustc_mir/interpret/place.rs +++ b/src/librustc_mir/interpret/place.rs @@ -648,9 +648,11 @@ where // bail out. None => Place::null(&*self), }, - layout: self.layout_of(self.subst_from_frame_and_normalize_erasing_regions( - self.frame().body.return_ty(), - ))?, + layout: self.layout_of( + self.subst_from_current_frame_and_normalize_erasing_regions( + self.frame().body.return_ty(), + ), + )?, } } local => PlaceTy { diff --git a/src/librustc_mir/interpret/step.rs b/src/librustc_mir/interpret/step.rs index f298a6677d6dc..cb11df18378d9 100644 --- a/src/librustc_mir/interpret/step.rs +++ b/src/librustc_mir/interpret/step.rs @@ -248,7 +248,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { } NullaryOp(mir::NullOp::SizeOf, ty) => { - let ty = self.subst_from_frame_and_normalize_erasing_regions(ty); + let ty = self.subst_from_current_frame_and_normalize_erasing_regions(ty); let layout = self.layout_of(ty)?; assert!( !layout.is_unsized(), diff --git a/src/librustc_mir/monomorphize/collector.rs b/src/librustc_mir/monomorphize/collector.rs index 862a7ef1e73c0..fbb7d8c6ee3ab 100644 --- a/src/librustc_mir/monomorphize/collector.rs +++ b/src/librustc_mir/monomorphize/collector.rs @@ -186,7 +186,7 @@ use rustc::mir::{self, Local, Location}; use rustc::session::config::EntryFnType; use rustc::ty::adjustment::{CustomCoerceUnsized, PointerCast}; use rustc::ty::print::obsolete::DefPathBasedNames; -use rustc::ty::subst::{InternalSubsts, SubstsRef}; +use rustc::ty::subst::InternalSubsts; use rustc::ty::{self, GenericParamDefKind, Instance, Ty, TyCtxt, TypeFoldable}; use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use rustc_data_structures::sync::{par_iter, MTLock, MTRef, ParallelIterator}; @@ -493,7 +493,21 @@ struct MirNeighborCollector<'a, 'tcx> { tcx: TyCtxt<'tcx>, body: &'a mir::Body<'tcx>, output: &'a mut Vec>, - param_substs: SubstsRef<'tcx>, + instance: Instance<'tcx>, +} + +impl<'a, 'tcx> MirNeighborCollector<'a, 'tcx> { + pub fn monomorphize(&self, value: T) -> T + where + T: TypeFoldable<'tcx>, + { + debug!("monomorphize: self.instance={:?}", self.instance); + if let Some(substs) = self.instance.substs_for_mir_body() { + self.tcx.subst_and_normalize_erasing_regions(substs, ty::ParamEnv::reveal_all(), &value) + } else { + self.tcx.normalize_erasing_regions(ty::ParamEnv::reveal_all(), value) + } + } } impl<'a, 'tcx> MirVisitor<'tcx> for MirNeighborCollector<'a, 'tcx> { @@ -509,17 +523,9 @@ impl<'a, 'tcx> MirVisitor<'tcx> for MirNeighborCollector<'a, 'tcx> { ref operand, target_ty, ) => { - let target_ty = self.tcx.subst_and_normalize_erasing_regions( - self.param_substs, - ty::ParamEnv::reveal_all(), - &target_ty, - ); + let target_ty = self.monomorphize(target_ty); let source_ty = operand.ty(self.body, self.tcx); - let source_ty = self.tcx.subst_and_normalize_erasing_regions( - self.param_substs, - ty::ParamEnv::reveal_all(), - &source_ty, - ); + let source_ty = self.monomorphize(source_ty); let (source_ty, target_ty) = find_vtable_types_for_unsizing(self.tcx, source_ty, target_ty); // This could also be a different Unsize instruction, like @@ -540,11 +546,7 @@ impl<'a, 'tcx> MirVisitor<'tcx> for MirNeighborCollector<'a, 'tcx> { _, ) => { let fn_ty = operand.ty(self.body, self.tcx); - let fn_ty = self.tcx.subst_and_normalize_erasing_regions( - self.param_substs, - ty::ParamEnv::reveal_all(), - &fn_ty, - ); + let fn_ty = self.monomorphize(fn_ty); visit_fn_use(self.tcx, fn_ty, false, &mut self.output); } mir::Rvalue::Cast( @@ -553,11 +555,7 @@ impl<'a, 'tcx> MirVisitor<'tcx> for MirNeighborCollector<'a, 'tcx> { _, ) => { let source_ty = operand.ty(self.body, self.tcx); - let source_ty = self.tcx.subst_and_normalize_erasing_regions( - self.param_substs, - ty::ParamEnv::reveal_all(), - &source_ty, - ); + let source_ty = self.monomorphize(source_ty); match source_ty.kind { ty::Closure(def_id, substs) => { let instance = Instance::resolve_closure( @@ -593,7 +591,23 @@ impl<'a, 'tcx> MirVisitor<'tcx> for MirNeighborCollector<'a, 'tcx> { fn visit_const(&mut self, constant: &&'tcx ty::Const<'tcx>, location: Location) { debug!("visiting const {:?} @ {:?}", *constant, location); - collect_const(self.tcx, *constant, self.param_substs, self.output); + let substituted_constant = self.monomorphize(*constant); + let param_env = ty::ParamEnv::reveal_all(); + + match substituted_constant.val { + ty::ConstKind::Value(val) => collect_const_value(self.tcx, val, self.output), + ty::ConstKind::Unevaluated(def_id, substs, promoted) => { + match self.tcx.const_eval_resolve(param_env, def_id, substs, promoted, None) { + Ok(val) => collect_const_value(self.tcx, val, self.output), + Err(ErrorHandled::Reported) => {} + Err(ErrorHandled::TooGeneric) => span_bug!( + self.tcx.def_span(def_id), + "collection encountered polymorphic constant", + ), + } + } + _ => {} + } self.super_const(constant); } @@ -605,21 +619,13 @@ impl<'a, 'tcx> MirVisitor<'tcx> for MirNeighborCollector<'a, 'tcx> { match *kind { mir::TerminatorKind::Call { ref func, .. } => { let callee_ty = func.ty(self.body, tcx); - let callee_ty = tcx.subst_and_normalize_erasing_regions( - self.param_substs, - ty::ParamEnv::reveal_all(), - &callee_ty, - ); + let callee_ty = self.monomorphize(callee_ty); visit_fn_use(self.tcx, callee_ty, true, &mut self.output); } mir::TerminatorKind::Drop { ref location, .. } | mir::TerminatorKind::DropAndReplace { ref location, .. } => { let ty = location.ty(self.body, self.tcx).ty; - let ty = tcx.subst_and_normalize_erasing_regions( - self.param_substs, - ty::ParamEnv::reveal_all(), - &ty, - ); + let ty = self.monomorphize(ty); visit_drop_use(self.tcx, ty, true, self.output); } mir::TerminatorKind::Goto { .. } @@ -1156,8 +1162,7 @@ fn collect_neighbours<'tcx>( debug!("collect_neighbours: {:?}", instance.def_id()); let body = tcx.instance_mir(instance.def); - MirNeighborCollector { tcx, body: &body, output, param_substs: instance.substs } - .visit_body(body); + MirNeighborCollector { tcx, body: &body, output, instance }.visit_body(body); } fn def_id_to_string(tcx: TyCtxt<'_>, def_id: DefId) -> String { @@ -1167,33 +1172,6 @@ fn def_id_to_string(tcx: TyCtxt<'_>, def_id: DefId) -> String { output } -fn collect_const<'tcx>( - tcx: TyCtxt<'tcx>, - constant: &'tcx ty::Const<'tcx>, - param_substs: SubstsRef<'tcx>, - output: &mut Vec>, -) { - debug!("visiting const {:?}", constant); - - let param_env = ty::ParamEnv::reveal_all(); - let substituted_constant = - tcx.subst_and_normalize_erasing_regions(param_substs, param_env, &constant); - - match substituted_constant.val { - ty::ConstKind::Value(val) => collect_const_value(tcx, val, output), - ty::ConstKind::Unevaluated(def_id, substs, promoted) => { - match tcx.const_eval_resolve(param_env, def_id, substs, promoted, None) { - Ok(val) => collect_const_value(tcx, val, output), - Err(ErrorHandled::Reported) => {} - Err(ErrorHandled::TooGeneric) => { - span_bug!(tcx.def_span(def_id), "collection encountered polymorphic constant",) - } - } - } - _ => {} - } -} - fn collect_const_value<'tcx>( tcx: TyCtxt<'tcx>, value: ConstValue<'tcx>, From fda913baae46ddaff88754bde06d02ccb824d921 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Thu, 19 Mar 2020 00:13:06 -0400 Subject: [PATCH 05/14] Add regression test for TAIT lifetime inference (issue #55099) Fixes #55099 The minimized reproducer in issue #55099 now compiles successfully. This commit adds a regression test for it. --- .../issue-55099-lifetime-inference.rs | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 src/test/ui/type-alias-impl-trait/issue-55099-lifetime-inference.rs diff --git a/src/test/ui/type-alias-impl-trait/issue-55099-lifetime-inference.rs b/src/test/ui/type-alias-impl-trait/issue-55099-lifetime-inference.rs new file mode 100644 index 0000000000000..8e8508cdd6f30 --- /dev/null +++ b/src/test/ui/type-alias-impl-trait/issue-55099-lifetime-inference.rs @@ -0,0 +1,28 @@ +// check-pass +// Regression test for issue #55099 +// Tests that we don't incorrectly consider a lifetime to part +// of the concrete type + +#![feature(type_alias_impl_trait)] + +trait Future { +} + +struct AndThen(F); + +impl Future for AndThen { +} + +struct Foo<'a> { + x: &'a mut (), +} + +type F = impl Future; + +impl<'a> Foo<'a> { + fn reply(&mut self) -> F { + AndThen(|| ()) + } +} + +fn main() {} From 410cd7a3e0db8e83800173347ef9d08103abdd74 Mon Sep 17 00:00:00 2001 From: Stefan Lankes Date: Thu, 19 Mar 2020 07:43:16 +0100 Subject: [PATCH 06/14] remove unused imports patch is required to avoid compiler errors by building src/libpanic_unwind/hermit.rs --- src/libpanic_unwind/hermit.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/libpanic_unwind/hermit.rs b/src/libpanic_unwind/hermit.rs index 6bded4dd499bd..69b9edb77c564 100644 --- a/src/libpanic_unwind/hermit.rs +++ b/src/libpanic_unwind/hermit.rs @@ -4,7 +4,6 @@ use alloc::boxed::Box; use core::any::Any; -use core::ptr; pub unsafe fn cleanup(_ptr: *mut u8) -> Box { extern "C" { From 2c38ecf72df871e9c564c1125e61d86fbcfb5679 Mon Sep 17 00:00:00 2001 From: lzutao Date: Thu, 19 Mar 2020 17:35:28 +0700 Subject: [PATCH 07/14] doc: Add quote to .init_array --- src/libstd/env.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libstd/env.rs b/src/libstd/env.rs index af35a5d9b7c47..6aad082a97f9a 100644 --- a/src/libstd/env.rs +++ b/src/libstd/env.rs @@ -723,8 +723,8 @@ pub struct ArgsOs { /// (such as `*` and `?`). On Windows this is not done, and such arguments are /// passed as-is. /// -/// On glibc Linux, arguments are retrieved by placing a function in .init_array. -/// glibc passes argc, argv, and envp to functions in .init_array, as a non-standard extension. +/// On glibc Linux systems, arguments are retrieved by placing a function in ".init_array". +/// Glibc passes argc, argv, and envp to functions in ".init_array", as a non-standard extension. /// This allows `std::env::args` to work even in a `cdylib` or `staticlib`, as it does on macOS /// and Windows. /// @@ -758,8 +758,8 @@ pub fn args() -> Args { /// set to arbitrary text, and it may not even exist, so this property should /// not be relied upon for security purposes. /// -/// On glibc Linux, arguments are retrieved by placing a function in .init_array. -/// glibc passes argc, argv, and envp to functions in .init_array, as a non-standard extension. +/// On glibc Linux systems, arguments are retrieved by placing a function in ".init_array". +/// Glibc passes argc, argv, and envp to functions in ".init_array", as a non-standard extension. /// This allows `std::env::args` to work even in a `cdylib` or `staticlib`, as it does on macOS /// and Windows. /// From be06f678e11b2773002e65ad63ecf235c0162dd0 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Wed, 18 Mar 2020 14:13:48 +0100 Subject: [PATCH 08/14] Clean up E0437 explanation --- src/librustc_error_codes/error_codes/E0437.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/librustc_error_codes/error_codes/E0437.md b/src/librustc_error_codes/error_codes/E0437.md index 834cf33dbc7f0..0f924ba692064 100644 --- a/src/librustc_error_codes/error_codes/E0437.md +++ b/src/librustc_error_codes/error_codes/E0437.md @@ -1,7 +1,5 @@ -Trait implementations can only implement associated types that are members of -the trait in question. This error indicates that you attempted to implement -an associated type whose name does not match the name of any associated type -in the trait. +An associated type whose name does not match any of the associated types +in the trait was used when implementing the trait. Erroneous code example: @@ -13,6 +11,9 @@ impl Foo for i32 { } ``` +Trait implementations can only implement associated types that are members of +the trait in question. + The solution to this problem is to remove the extraneous associated type: ``` From 6f16118f281bbd3d9a152af905cd3a8702f35ded Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Thu, 19 Mar 2020 14:11:27 +0100 Subject: [PATCH 09/14] Clean up e0438 explanation --- src/librustc_error_codes/error_codes/E0438.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/librustc_error_codes/error_codes/E0438.md b/src/librustc_error_codes/error_codes/E0438.md index cb141a5d24aed..13723bc30090e 100644 --- a/src/librustc_error_codes/error_codes/E0438.md +++ b/src/librustc_error_codes/error_codes/E0438.md @@ -1,7 +1,5 @@ -Trait implementations can only implement associated constants that are -members of the trait in question. This error indicates that you -attempted to implement an associated constant whose name does not -match the name of any associated constant in the trait. +An associated constant whose name does not match any of the associated constants +in the trait was used when implementing the trait. Erroneous code example: @@ -13,6 +11,9 @@ impl Foo for i32 { } ``` +Trait implementations can only implement associated constants that are +members of the trait in question. + The solution to this problem is to remove the extraneous associated constant: ``` From 8e0398c060ba50bde4fe47a3685c0857285001bd Mon Sep 17 00:00:00 2001 From: Hrvoje Niksic Date: Sun, 1 Mar 2020 21:31:08 +0100 Subject: [PATCH 10/14] Clarify the relationship between `forget()` and `ManuallyDrop`. As discussed on reddit, this commit addresses two issues with the documentation of `mem::forget()`: * The documentation of `mem::forget()` can confuse the reader because of the discrepancy between usage examples that show correct usage and the accompanying text which speaks of the possibility of double-free. The text that says "if the panic occurs before `mem::forget` was called" refers to a variant of the second example that was never shown, modified to use `mem::forget` instead of `ManuallyDrop`. Ideally the documentation should show both variants, so it's clear what it's talking about. Also, the double free could be fixed just by placing `mem::forget(v)` before the construction of `s`. Since the lifetimes of `s` and `v` wouldn't overlap, there would be no point where panic could cause a double free. This could be mentioned, and contrasted against the more robust fix of using `ManuallyDrop`. * This sentence seems unjustified: "For some types, operations such as passing ownership (to a funcion like `mem::forget`) requires them to actually be fully owned right now [...]". Unlike C++, Rust has no move constructors, its moves are (possibly elided) bitwise copies. Even if you pass an invalid object to `mem::forget`, no harm should come to pass because `mem::forget` consumes the object and exists solely to prevent drop, so there no one left to observe the invalid state state. --- src/libcore/mem/mod.rs | 38 +++++++++++++++++++++++++++----------- 1 file changed, 27 insertions(+), 11 deletions(-) diff --git a/src/libcore/mem/mod.rs b/src/libcore/mem/mod.rs index 1cf2b40e93068..19e3b2a8bb922 100644 --- a/src/libcore/mem/mod.rs +++ b/src/libcore/mem/mod.rs @@ -69,8 +69,26 @@ pub use crate::intrinsics::transmute; /// ``` /// /// The practical use cases for `forget` are rather specialized and mainly come -/// up in unsafe or FFI code. However, [`ManuallyDrop`] is usually preferred -/// for such cases, e.g.: +/// up in unsafe or FFI code. For example: +/// +/// ``` +/// use std::mem; +/// +/// let mut v = vec![65, 122]; +/// // Build a `String` using the contents of `v` +/// let s = unsafe { String::from_raw_parts(v.as_mut_ptr(), 2, v.capacity()) }; +/// // immediately leak `v` because its memory is now managed by `s` +/// mem::forget(v); +/// assert_eq!(s, "Az"); +/// // `s` is implicitly dropped and its memory deallocated. +/// ``` +/// +/// The above is correct, but brittle. If code gets added between the construction of +/// `String` and the invocation of `mem::forget()`, a panic within it will cause a double +/// free because the same memory is handled by both `v` and `s`. This can be fixed by +/// storing the result of `v.as_mut_ptr()` in a local variable and calling `mem::forget()` +/// before `String::from_raw_parts`. This kind of issue can be more robustly prevented by +/// using [`ManuallyDrop`], which is usually preferred for such cases: /// /// ``` /// use std::mem::ManuallyDrop; @@ -88,16 +106,14 @@ pub use crate::intrinsics::transmute; /// // `s` is implicitly dropped and its memory deallocated. /// ``` /// -/// Using `ManuallyDrop` here has two advantages: +/// `ManuallyDrop` robustly prevents double-free because we disable `v`'s destructor +/// before doing anything else. `mem::forget()` doesn't allow this because it consumes its +/// argument, forcing us to call it only after extracting anything we need from `v`. /// -/// * We do not "touch" `v` after disassembling it. For some types, operations -/// such as passing ownership (to a function like `mem::forget`) requires them to actually -/// be fully owned right now; that is a promise we do not want to make here as we are -/// in the process of transferring ownership to the new `String` we are building. -/// * In case of an unexpected panic, `ManuallyDrop` is not dropped, but if the panic -/// occurs before `mem::forget` was called we might end up dropping invalid data, -/// or double-dropping. In other words, `ManuallyDrop` errs on the side of leaking -/// instead of erring on the side of dropping. +/// Note that the above code cannot panic between construction of `ManuallyDrop` and +/// building the string. But even if it could (after a modification), a panic there would +/// result in a leak and not a double free. In other words, `ManuallyDrop` errs on the +/// side of leaking instead of erring on the side of dropping. /// /// [drop]: fn.drop.html /// [uninit]: fn.uninitialized.html From 2a08b0e3006aeb997e42d5df135eea63b071fa75 Mon Sep 17 00:00:00 2001 From: Hrvoje Niksic Date: Wed, 4 Mar 2020 22:12:53 +0100 Subject: [PATCH 11/14] Restore (and reword) the warning against passing invalid values to mem::forget. As pointed out by Ralf Jung, dangling references and boxes are undefined behavior as per https://doc.rust-lang.org/reference/behavior-considered-undefined.html and the Miri checker. --- src/libcore/mem/mod.rs | 52 ++++++++++++++++++++++++++++-------------- 1 file changed, 35 insertions(+), 17 deletions(-) diff --git a/src/libcore/mem/mod.rs b/src/libcore/mem/mod.rs index 19e3b2a8bb922..7297f66970de5 100644 --- a/src/libcore/mem/mod.rs +++ b/src/libcore/mem/mod.rs @@ -58,7 +58,9 @@ pub use crate::intrinsics::transmute; /// /// # Examples /// -/// Leak an I/O object, never closing the file: +/// The canonical safe use of `mem::forget` is to circumvent a value's destructor +/// implemented by the `Drop` trait. For example, this will leak a `File`, i.e. reclaim +/// the space taken by the variable but never close the underlying system resource: /// /// ```no_run /// use std::mem; @@ -68,8 +70,14 @@ pub use crate::intrinsics::transmute; /// mem::forget(file); /// ``` /// -/// The practical use cases for `forget` are rather specialized and mainly come -/// up in unsafe or FFI code. For example: +/// This is useful when the ownership of the underlying was previously +/// transferred to code outside of Rust, for example by transmitting the raw +/// file descriptor to C code. +/// +/// # Relationship with `ManuallyDrop` +/// +/// Using `mem::forget` to transmit memory ownership is error-prone and is best +/// replaced with `ManuallyDrop`. Consider, for example, this code: /// /// ``` /// use std::mem; @@ -77,18 +85,25 @@ pub use crate::intrinsics::transmute; /// let mut v = vec![65, 122]; /// // Build a `String` using the contents of `v` /// let s = unsafe { String::from_raw_parts(v.as_mut_ptr(), 2, v.capacity()) }; -/// // immediately leak `v` because its memory is now managed by `s` -/// mem::forget(v); +/// // leak `v` because its memory is now managed by `s` +/// mem::forget(v); // ERROR - v is invalid and must not be passed to a function /// assert_eq!(s, "Az"); /// // `s` is implicitly dropped and its memory deallocated. /// ``` /// -/// The above is correct, but brittle. If code gets added between the construction of -/// `String` and the invocation of `mem::forget()`, a panic within it will cause a double -/// free because the same memory is handled by both `v` and `s`. This can be fixed by -/// storing the result of `v.as_mut_ptr()` in a local variable and calling `mem::forget()` -/// before `String::from_raw_parts`. This kind of issue can be more robustly prevented by -/// using [`ManuallyDrop`], which is usually preferred for such cases: +/// There are two issues with the above example: +/// +/// * If more code were added between the construction of `String` and the invocation of +/// `mem::forget()`, a panic within it would cause a double free because the same memory +/// is handled by both `v` and `s`. +/// * After calling `v.as_mut_ptr()` and transmitting the ownership of the data to `s`, +/// the `v` value is invalid. Although moving a value to `mem::forget` (which won't +/// inspect it) seems safe, some types have strict requirements on their values that +/// make them invalid when dangling or no longer owned. Using invalid values in any +/// way, including passing them to or returning them from functions, constitutes +/// undefined behavior and may break the assumptions made by the compiler. +/// +/// Switching to `ManuallyDrop` avoids both issues: /// /// ``` /// use std::mem::ManuallyDrop; @@ -108,12 +123,15 @@ pub use crate::intrinsics::transmute; /// /// `ManuallyDrop` robustly prevents double-free because we disable `v`'s destructor /// before doing anything else. `mem::forget()` doesn't allow this because it consumes its -/// argument, forcing us to call it only after extracting anything we need from `v`. -/// -/// Note that the above code cannot panic between construction of `ManuallyDrop` and -/// building the string. But even if it could (after a modification), a panic there would -/// result in a leak and not a double free. In other words, `ManuallyDrop` errs on the -/// side of leaking instead of erring on the side of dropping. +/// argument, forcing us to call it only after extracting anything we need from `v`. Even +/// if a panic were introduced between construction of `ManuallyDrop` and building the +/// string (which cannot happen in the code as shown), it would result in a leak and not a +/// double free. In other words, `ManuallyDrop` errs on the side of leaking instead of +/// erring on the side of dropping. +/// +/// Also, `ManuallyDrop` prevents us from having to "touch" `v` after transferring the +/// ownership to `s` - the final step of interacting with `v` to dispoe of it without +/// running its destructor is entirely avoided. /// /// [drop]: fn.drop.html /// [uninit]: fn.uninitialized.html From 755434121cda7d21d301592cf6fbddb7042e59a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hrvoje=20Nik=C5=A1i=C4=87?= Date: Wed, 18 Mar 2020 11:30:39 +0100 Subject: [PATCH 12/14] Minor re-wordings and typo fixes. Co-Authored-By: Ralf Jung --- src/libcore/mem/mod.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/libcore/mem/mod.rs b/src/libcore/mem/mod.rs index 7297f66970de5..253847612adad 100644 --- a/src/libcore/mem/mod.rs +++ b/src/libcore/mem/mod.rs @@ -70,14 +70,14 @@ pub use crate::intrinsics::transmute; /// mem::forget(file); /// ``` /// -/// This is useful when the ownership of the underlying was previously +/// This is useful when the ownership of the underlying resource was previously /// transferred to code outside of Rust, for example by transmitting the raw /// file descriptor to C code. /// /// # Relationship with `ManuallyDrop` /// -/// Using `mem::forget` to transmit memory ownership is error-prone and is best -/// replaced with `ManuallyDrop`. Consider, for example, this code: +/// While `mem::forget` can also be used to transfer *memory* ownership, doing so is error-prone. +/// [`ManuallyDrop`] should be used instead. Consider, for example, this code: /// /// ``` /// use std::mem; @@ -97,9 +97,9 @@ pub use crate::intrinsics::transmute; /// `mem::forget()`, a panic within it would cause a double free because the same memory /// is handled by both `v` and `s`. /// * After calling `v.as_mut_ptr()` and transmitting the ownership of the data to `s`, -/// the `v` value is invalid. Although moving a value to `mem::forget` (which won't -/// inspect it) seems safe, some types have strict requirements on their values that -/// make them invalid when dangling or no longer owned. Using invalid values in any +/// the `v` value is invalid. Even when a value is just moved to `mem::forget` (which won't +/// inspect it), some types have strict requirements on their values that +/// make them invalid when dangling or no longer owned. Using invalid values in any /// way, including passing them to or returning them from functions, constitutes /// undefined behavior and may break the assumptions made by the compiler. /// @@ -123,11 +123,11 @@ pub use crate::intrinsics::transmute; /// /// `ManuallyDrop` robustly prevents double-free because we disable `v`'s destructor /// before doing anything else. `mem::forget()` doesn't allow this because it consumes its -/// argument, forcing us to call it only after extracting anything we need from `v`. Even +/// argument, forcing us to call it only after extracting anything we need from `v`. Even /// if a panic were introduced between construction of `ManuallyDrop` and building the /// string (which cannot happen in the code as shown), it would result in a leak and not a /// double free. In other words, `ManuallyDrop` errs on the side of leaking instead of -/// erring on the side of dropping. +/// erring on the side of (double-)dropping. /// /// Also, `ManuallyDrop` prevents us from having to "touch" `v` after transferring the /// ownership to `s` - the final step of interacting with `v` to dispoe of it without From 2bebe8d87111b544b8f5600fe93cc96391d5c91e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hrvoje=20Nik=C5=A1i=C4=87?= Date: Thu, 19 Mar 2020 13:56:48 +0100 Subject: [PATCH 13/14] Don't hard-code the vector length in the examples. Co-Authored-By: lzutao --- src/libcore/mem/mod.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/libcore/mem/mod.rs b/src/libcore/mem/mod.rs index 253847612adad..dac9ee6a5d91e 100644 --- a/src/libcore/mem/mod.rs +++ b/src/libcore/mem/mod.rs @@ -84,7 +84,7 @@ pub use crate::intrinsics::transmute; /// /// let mut v = vec![65, 122]; /// // Build a `String` using the contents of `v` -/// let s = unsafe { String::from_raw_parts(v.as_mut_ptr(), 2, v.capacity()) }; +/// let s = unsafe { String::from_raw_parts(v.as_mut_ptr(), v.len(), v.capacity()) }; /// // leak `v` because its memory is now managed by `s` /// mem::forget(v); // ERROR - v is invalid and must not be passed to a function /// assert_eq!(s, "Az"); @@ -113,10 +113,9 @@ pub use crate::intrinsics::transmute; /// // does not get dropped! /// let mut v = ManuallyDrop::new(v); /// // Now disassemble `v`. These operations cannot panic, so there cannot be a leak. -/// let ptr = v.as_mut_ptr(); -/// let cap = v.capacity(); +/// let (ptr, len, cap) = (v.as_mut_ptr(), v.len(), v.capacity()); /// // Finally, build a `String`. -/// let s = unsafe { String::from_raw_parts(ptr, 2, cap) }; +/// let s = unsafe { String::from_raw_parts(ptr, len, cap) }; /// assert_eq!(s, "Az"); /// // `s` is implicitly dropped and its memory deallocated. /// ``` From 89ef59af78a9d73dea2093b247ec74e1d3c6af87 Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Thu, 19 Mar 2020 15:38:31 +0100 Subject: [PATCH 14/14] triagebot.toml: accept typo due to pnkfelix --- triagebot.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/triagebot.toml b/triagebot.toml index ec32771583334..2476f414e0e03 100644 --- a/triagebot.toml +++ b/triagebot.toml @@ -23,7 +23,7 @@ Thanks! <3 label = "ICEBreaker-LLVM" [ping.icebreakers-cleanup-crew] -alias = ["cleanup", "cleanups", "shrink", "reduce", "bisect"] +alias = ["cleanup", "cleanups", "cleanup-crew", "shrink", "reduce", "bisect"] message = """\ Hey Cleanup Crew ICE-breakers! This bug has been identified as a good "Cleanup ICE-breaking candidate". In case it's useful, here are some