From cdc730457e9030feaf8c4376a668a9ef61c7f189 Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Fri, 6 Mar 2020 11:20:27 +0100 Subject: [PATCH 01/23] 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/23] 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/23] 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 24dc2cb133d5b1cdd0680b91e4295039d704a610 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Mi=C4=85sko?= Date: Wed, 11 Mar 2020 00:00:00 +0000 Subject: [PATCH 04/23] librustc_codegen_llvm: Replace deprecated API usage --- src/librustc_codegen_llvm/base.rs | 3 +-- src/librustc_codegen_llvm/consts.rs | 4 ++-- src/librustc_codegen_llvm/context.rs | 4 ++-- src/librustc_codegen_llvm/debuginfo/gdb.rs | 2 +- src/librustc_codegen_llvm/declare.rs | 2 +- src/librustc_codegen_llvm/intrinsic.rs | 6 +++--- src/librustc_codegen_llvm/llvm/ffi.rs | 14 +++++++++++--- src/librustc_codegen_llvm/llvm/mod.rs | 4 ++-- 8 files changed, 23 insertions(+), 16 deletions(-) diff --git a/src/librustc_codegen_llvm/base.rs b/src/librustc_codegen_llvm/base.rs index 04c084e459eab..115de2a851d6b 100644 --- a/src/librustc_codegen_llvm/base.rs +++ b/src/librustc_codegen_llvm/base.rs @@ -71,8 +71,7 @@ pub fn write_compressed_metadata<'tcx>( // flags, at least for ELF outputs, so that the // metadata doesn't get loaded into memory. let directive = format!(".section {}", section_name); - let directive = CString::new(directive).unwrap(); - llvm::LLVMSetModuleInlineAsm(metadata_llmod, directive.as_ptr()) + llvm::LLVMSetModuleInlineAsm2(metadata_llmod, directive.as_ptr().cast(), directive.len()) } } diff --git a/src/librustc_codegen_llvm/consts.rs b/src/librustc_codegen_llvm/consts.rs index 09a84aff16811..619dee2909281 100644 --- a/src/librustc_codegen_llvm/consts.rs +++ b/src/librustc_codegen_llvm/consts.rs @@ -1,7 +1,7 @@ use crate::base; use crate::common::CodegenCx; use crate::debuginfo; -use crate::llvm::{self, SetUnnamedAddr, True}; +use crate::llvm::{self, True}; use crate::type_::Type; use crate::type_of::LayoutLlvmExt; use crate::value::Value; @@ -183,7 +183,7 @@ impl CodegenCx<'ll, 'tcx> { }; llvm::LLVMSetInitializer(gv, cv); set_global_alignment(&self, gv, align); - SetUnnamedAddr(gv, true); + llvm::SetUnnamedAddress(gv, llvm::UnnamedAddr::Global); gv } } diff --git a/src/librustc_codegen_llvm/context.rs b/src/librustc_codegen_llvm/context.rs index 3466363ac7972..3f856c479c5a9 100644 --- a/src/librustc_codegen_llvm/context.rs +++ b/src/librustc_codegen_llvm/context.rs @@ -172,7 +172,7 @@ pub unsafe fn create_module( llvm::LLVMRustSetDataLayoutFromTargetMachine(llmod, tm); llvm::LLVMRustDisposeTargetMachine(tm); - let llvm_data_layout = llvm::LLVMGetDataLayout(llmod); + let llvm_data_layout = llvm::LLVMGetDataLayoutStr(llmod); let llvm_data_layout = str::from_utf8(CStr::from_ptr(llvm_data_layout).to_bytes()) .expect("got a non-UTF8 data-layout from LLVM"); @@ -504,7 +504,7 @@ impl CodegenCx<'b, 'tcx> { self.type_variadic_func(&[], ret) }; let f = self.declare_cfn(name, fn_ty); - llvm::SetUnnamedAddr(f, false); + llvm::SetUnnamedAddress(f, llvm::UnnamedAddr::No); self.intrinsics.borrow_mut().insert(name, f); f } diff --git a/src/librustc_codegen_llvm/debuginfo/gdb.rs b/src/librustc_codegen_llvm/debuginfo/gdb.rs index 753a4e18faf5e..d6fbc53fd3776 100644 --- a/src/librustc_codegen_llvm/debuginfo/gdb.rs +++ b/src/librustc_codegen_llvm/debuginfo/gdb.rs @@ -50,7 +50,7 @@ pub fn get_or_insert_gdb_debug_scripts_section_global(cx: &CodegenCx<'ll, '_>) - llvm::LLVMSetSection(section_var, section_name.as_ptr().cast()); llvm::LLVMSetInitializer(section_var, cx.const_bytes(section_contents)); llvm::LLVMSetGlobalConstant(section_var, llvm::True); - llvm::LLVMSetUnnamedAddr(section_var, llvm::True); + llvm::LLVMSetUnnamedAddress(section_var, llvm::UnnamedAddr::Global); llvm::LLVMRustSetLinkage(section_var, llvm::Linkage::LinkOnceODRLinkage); // This should make sure that the whole section is not larger than // the string it contains. Otherwise we get a warning from GDB. diff --git a/src/librustc_codegen_llvm/declare.rs b/src/librustc_codegen_llvm/declare.rs index fab6321186b2c..236f5bb1bfdfb 100644 --- a/src/librustc_codegen_llvm/declare.rs +++ b/src/librustc_codegen_llvm/declare.rs @@ -40,7 +40,7 @@ fn declare_raw_fn( llvm::SetFunctionCallConv(llfn, callconv); // Function addresses in Rust are never significant, allowing functions to // be merged. - llvm::SetUnnamedAddr(llfn, true); + llvm::SetUnnamedAddress(llfn, llvm::UnnamedAddr::Global); if cx.tcx.sess.opts.cg.no_redzone.unwrap_or(cx.tcx.sess.target.target.options.disable_redzone) { llvm::Attribute::NoRedZone.apply_llfn(Function, llfn); diff --git a/src/librustc_codegen_llvm/intrinsic.rs b/src/librustc_codegen_llvm/intrinsic.rs index 1ae9d2a684131..69af175bc54de 100644 --- a/src/librustc_codegen_llvm/intrinsic.rs +++ b/src/librustc_codegen_llvm/intrinsic.rs @@ -1664,7 +1664,7 @@ fn generic_simd_intrinsic( llvm_elem_vec_ty, ), ); - llvm::SetUnnamedAddr(f, false); + llvm::SetUnnamedAddress(f, llvm::UnnamedAddr::No); let v = bx.call(f, &[args[1].immediate(), alignment, mask, args[0].immediate()], None); return Ok(v); } @@ -1786,7 +1786,7 @@ fn generic_simd_intrinsic( &llvm_intrinsic, bx.type_func(&[llvm_elem_vec_ty, llvm_pointer_vec_ty, alignment_ty, mask_ty], ret_t), ); - llvm::SetUnnamedAddr(f, false); + llvm::SetUnnamedAddress(f, llvm::UnnamedAddr::No); let v = bx.call(f, &[args[0].immediate(), args[1].immediate(), alignment, mask], None); return Ok(v); } @@ -2085,7 +2085,7 @@ unsupported {} from `{}` with element `{}` of size `{}` to `{}`"#, let vec_ty = bx.cx.type_vector(elem_ty, in_len as u64); let f = bx.declare_cfn(&llvm_intrinsic, bx.type_func(&[vec_ty, vec_ty], vec_ty)); - llvm::SetUnnamedAddr(f, false); + llvm::SetUnnamedAddress(f, llvm::UnnamedAddr::No); let v = bx.call(f, &[lhs, rhs], None); return Ok(v); } diff --git a/src/librustc_codegen_llvm/llvm/ffi.rs b/src/librustc_codegen_llvm/llvm/ffi.rs index 388b6c7483958..d4fc546b8e657 100644 --- a/src/librustc_codegen_llvm/llvm/ffi.rs +++ b/src/librustc_codegen_llvm/llvm/ffi.rs @@ -73,6 +73,14 @@ pub enum Visibility { Protected = 2, } +/// LLVMUnnamedAddr +#[repr(C)] +pub enum UnnamedAddr { + No, + Local, + Global, +} + /// LLVMDLLStorageClass #[derive(Copy, Clone)] #[repr(C)] @@ -727,11 +735,11 @@ extern "C" { pub fn LLVMCloneModule(M: &Module) -> &Module; /// Data layout. See Module::getDataLayout. - pub fn LLVMGetDataLayout(M: &Module) -> *const c_char; + pub fn LLVMGetDataLayoutStr(M: &Module) -> *const c_char; pub fn LLVMSetDataLayout(M: &Module, Triple: *const c_char); /// See Module::setModuleInlineAsm. - pub fn LLVMSetModuleInlineAsm(M: &Module, Asm: *const c_char); + pub fn LLVMSetModuleInlineAsm2(M: &Module, Asm: *const c_char, AsmLen: size_t); pub fn LLVMRustAppendModuleInlineAsm(M: &Module, Asm: *const c_char, AsmLen: size_t); /// See llvm::LLVMTypeKind::getTypeID. @@ -1853,7 +1861,7 @@ extern "C" { UniqueIdLen: size_t, ) -> &'a DIDerivedType; - pub fn LLVMSetUnnamedAddr(GlobalVar: &Value, UnnamedAddr: Bool); + pub fn LLVMSetUnnamedAddress(Global: &Value, UnnamedAddr: UnnamedAddr); pub fn LLVMRustDIBuilderCreateTemplateTypeParameter( Builder: &DIBuilder<'a>, diff --git a/src/librustc_codegen_llvm/llvm/mod.rs b/src/librustc_codegen_llvm/llvm/mod.rs index 96014cbee5da1..a6a3e178c6f02 100644 --- a/src/librustc_codegen_llvm/llvm/mod.rs +++ b/src/librustc_codegen_llvm/llvm/mod.rs @@ -106,9 +106,9 @@ pub fn UnsetComdat(val: &'a Value) { } } -pub fn SetUnnamedAddr(global: &'a Value, unnamed: bool) { +pub fn SetUnnamedAddress(global: &'a Value, unnamed: UnnamedAddr) { unsafe { - LLVMSetUnnamedAddr(global, unnamed as Bool); + LLVMSetUnnamedAddress(global, unnamed); } } From bee151308d5c5090627772c04e17f783aa53451a Mon Sep 17 00:00:00 2001 From: David Wood Date: Wed, 11 Mar 2020 19:36:07 +0000 Subject: [PATCH 05/23] 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 0bf5cae489e828a6678cab5144e638ae909d7b93 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Fri, 6 Mar 2020 21:36:09 +0100 Subject: [PATCH 06/23] Remove __query_compute module. --- src/librustc/ty/query/plumbing.rs | 28 ++++++++-------------------- 1 file changed, 8 insertions(+), 20 deletions(-) diff --git a/src/librustc/ty/query/plumbing.rs b/src/librustc/ty/query/plumbing.rs index acf67f52dceaa..bfae0075c8d07 100644 --- a/src/librustc/ty/query/plumbing.rs +++ b/src/librustc/ty/query/plumbing.rs @@ -951,16 +951,6 @@ macro_rules! define_queries_inner { })* } - // This module and the functions in it exist only to provide a - // predictable symbol name prefix for query providers. This is helpful - // for analyzing queries in profilers. - pub(super) mod __query_compute { - $(#[inline(never)] - pub fn $name R, R>(f: F) -> R { - f() - })* - } - $(impl<$tcx> QueryConfig<$tcx> for queries::$name<$tcx> { type Key = $K; type Value = $V; @@ -997,16 +987,14 @@ macro_rules! define_queries_inner { #[inline] fn compute(tcx: TyCtxt<'tcx>, key: Self::Key) -> Self::Value { - __query_compute::$name(move || { - let provider = tcx.queries.providers.get(key.query_crate()) - // HACK(eddyb) it's possible crates may be loaded after - // the query engine is created, and because crate loading - // is not yet integrated with the query engine, such crates - // would be missing appropriate entries in `providers`. - .unwrap_or(&tcx.queries.fallback_extern_providers) - .$name; - provider(tcx, key) - }) + let provider = tcx.queries.providers.get(key.query_crate()) + // HACK(eddyb) it's possible crates may be loaded after + // the query engine is created, and because crate loading + // is not yet integrated with the query engine, such crates + // would be missing appropriate entries in `providers`. + .unwrap_or(&tcx.queries.fallback_extern_providers) + .$name; + provider(tcx, key) } fn hash_result( From fc82376bc437d4494832b171d924e2f116174578 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Sat, 7 Mar 2020 00:43:17 +0100 Subject: [PATCH 07/23] Make QueryAccessor::dep_kind an associated const. --- src/librustc/ty/query/config.rs | 3 +-- src/librustc/ty/query/plumbing.rs | 14 +++++--------- 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/src/librustc/ty/query/config.rs b/src/librustc/ty/query/config.rs index 178c2362def6e..5a05acc90e43f 100644 --- a/src/librustc/ty/query/config.rs +++ b/src/librustc/ty/query/config.rs @@ -28,6 +28,7 @@ pub trait QueryConfig<'tcx> { pub(crate) trait QueryAccessors<'tcx>: QueryConfig<'tcx> { const ANON: bool; const EVAL_ALWAYS: bool; + const DEP_KIND: DepKind; type Cache: QueryCache; @@ -38,8 +39,6 @@ pub(crate) trait QueryAccessors<'tcx>: QueryConfig<'tcx> { fn to_dep_node(tcx: TyCtxt<'tcx>, key: &Self::Key) -> DepNode; - fn dep_kind() -> DepKind; - // Don't use this method to compute query results, instead use the methods on TyCtxt fn compute(tcx: TyCtxt<'tcx>, key: Self::Key) -> Self::Value; diff --git a/src/librustc/ty/query/plumbing.rs b/src/librustc/ty/query/plumbing.rs index bfae0075c8d07..603c4fd9b72c3 100644 --- a/src/librustc/ty/query/plumbing.rs +++ b/src/librustc/ty/query/plumbing.rs @@ -154,7 +154,7 @@ impl<'tcx, Q: QueryDescription<'tcx>> JobOwner<'tcx, Q> { }; // Create the id of the job we're waiting for - let id = QueryJobId::new(job.id, lookup.shard, Q::dep_kind()); + let id = QueryJobId::new(job.id, lookup.shard, Q::DEP_KIND); (job.latch(id), _query_blocked_prof_timer) } @@ -169,7 +169,7 @@ impl<'tcx, Q: QueryDescription<'tcx>> JobOwner<'tcx, Q> { lock.jobs = id; let id = QueryShardJobId(NonZeroU32::new(id).unwrap()); - let global_id = QueryJobId::new(id, lookup.shard, Q::dep_kind()); + let global_id = QueryJobId::new(id, lookup.shard, Q::DEP_KIND); let job = tls::with_related_context(tcx, |icx| QueryJob::new(id, span, icx.query)); @@ -498,7 +498,7 @@ impl<'tcx> TyCtxt<'tcx> { let ((result, dep_node_index), diagnostics) = with_diagnostics(|diagnostics| { self.start_query(job.id, diagnostics, |tcx| { - tcx.dep_graph.with_anon_task(Q::dep_kind(), || Q::compute(tcx, key)) + tcx.dep_graph.with_anon_task(Q::DEP_KIND, || Q::compute(tcx, key)) }) }); @@ -873,7 +873,7 @@ macro_rules! define_queries_inner { job: job.id, shard: u16::try_from(shard_id).unwrap(), kind: - as QueryAccessors<'tcx>>::dep_kind(), + as QueryAccessors<'tcx>>::DEP_KIND, }; let info = QueryInfo { span: job.span, @@ -961,6 +961,7 @@ macro_rules! define_queries_inner { impl<$tcx> QueryAccessors<$tcx> for queries::$name<$tcx> { const ANON: bool = is_anon!([$($modifiers)*]); const EVAL_ALWAYS: bool = is_eval_always!([$($modifiers)*]); + const DEP_KIND: dep_graph::DepKind = dep_graph::DepKind::$node; type Cache = query_storage!([$($modifiers)*][$K, $V]); @@ -980,11 +981,6 @@ macro_rules! define_queries_inner { DepConstructor::$node(tcx, *key) } - #[inline(always)] - fn dep_kind() -> dep_graph::DepKind { - dep_graph::DepKind::$node - } - #[inline] fn compute(tcx: TyCtxt<'tcx>, key: Self::Key) -> Self::Value { let provider = tcx.queries.providers.get(key.query_crate()) From cf238fd057651371731fde47a2ebf251bf37cfb5 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Sat, 7 Mar 2020 01:01:01 +0100 Subject: [PATCH 08/23] Inline QueryAccessor::query. --- src/librustc/ty/query/config.rs | 4 +--- src/librustc/ty/query/plumbing.rs | 7 +------ 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/src/librustc/ty/query/config.rs b/src/librustc/ty/query/config.rs index 5a05acc90e43f..29b9e0c3b40ef 100644 --- a/src/librustc/ty/query/config.rs +++ b/src/librustc/ty/query/config.rs @@ -2,7 +2,7 @@ use crate::dep_graph::SerializedDepNodeIndex; use crate::dep_graph::{DepKind, DepNode}; use crate::ty::query::caches::QueryCache; use crate::ty::query::plumbing::CycleError; -use crate::ty::query::{Query, QueryState}; +use crate::ty::query::QueryState; use crate::ty::TyCtxt; use rustc_data_structures::profiling::ProfileCategory; use rustc_hir::def_id::DefId; @@ -32,8 +32,6 @@ pub(crate) trait QueryAccessors<'tcx>: QueryConfig<'tcx> { type Cache: QueryCache; - fn query(key: Self::Key) -> Query<'tcx>; - // Don't use this method to access query results, instead use the methods on TyCtxt fn query_state<'a>(tcx: TyCtxt<'tcx>) -> &'a QueryState<'tcx, Self>; diff --git a/src/librustc/ty/query/plumbing.rs b/src/librustc/ty/query/plumbing.rs index 603c4fd9b72c3..32138b3e1d5fe 100644 --- a/src/librustc/ty/query/plumbing.rs +++ b/src/librustc/ty/query/plumbing.rs @@ -877,7 +877,7 @@ macro_rules! define_queries_inner { }; let info = QueryInfo { span: job.span, - query: queries::$name::query(k.clone()) + query: Query::$name(k.clone()) }; Some((id, QueryJobInfo { info, job: job.clone() })) } else { @@ -965,11 +965,6 @@ macro_rules! define_queries_inner { type Cache = query_storage!([$($modifiers)*][$K, $V]); - #[inline(always)] - fn query(key: Self::Key) -> Query<'tcx> { - Query::$name(key) - } - #[inline(always)] fn query_state<'a>(tcx: TyCtxt<$tcx>) -> &'a QueryState<$tcx, Self> { &tcx.queries.$name From 1249032aabe3a0a80c0a852ef803702d7fb70d21 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Sat, 7 Mar 2020 17:10:31 +0100 Subject: [PATCH 09/23] Move impl of Queries with its definition. --- src/librustc/ty/query/plumbing.rs | 98 +++++++++++++++---------------- 1 file changed, 49 insertions(+), 49 deletions(-) diff --git a/src/librustc/ty/query/plumbing.rs b/src/librustc/ty/query/plumbing.rs index 32138b3e1d5fe..5532d7e08cc92 100644 --- a/src/librustc/ty/query/plumbing.rs +++ b/src/librustc/ty/query/plumbing.rs @@ -842,55 +842,6 @@ macro_rules! define_queries_inner { input: ($(([$($modifiers)*] [$($attr)*] [$name]))*) } - impl<$tcx> Queries<$tcx> { - pub fn new( - providers: IndexVec>, - fallback_extern_providers: Providers<$tcx>, - on_disk_cache: OnDiskCache<'tcx>, - ) -> Self { - Queries { - providers, - fallback_extern_providers: Box::new(fallback_extern_providers), - on_disk_cache, - $($name: Default::default()),* - } - } - - pub fn try_collect_active_jobs( - &self - ) -> Option>> { - let mut jobs = FxHashMap::default(); - - $( - // We use try_lock_shards here since we are called from the - // deadlock handler, and this shouldn't be locked. - let shards = self.$name.shards.try_lock_shards()?; - let shards = shards.iter().enumerate(); - jobs.extend(shards.flat_map(|(shard_id, shard)| { - shard.active.iter().filter_map(move |(k, v)| { - if let QueryResult::Started(ref job) = *v { - let id = QueryJobId { - job: job.id, - shard: u16::try_from(shard_id).unwrap(), - kind: - as QueryAccessors<'tcx>>::DEP_KIND, - }; - let info = QueryInfo { - span: job.span, - query: Query::$name(k.clone()) - }; - Some((id, QueryJobInfo { info, job: job.clone() })) - } else { - None - } - }) - })); - )* - - Some(jobs) - } - } - #[allow(nonstandard_style)] #[derive(Clone, Debug)] pub enum Query<$tcx> { @@ -1120,6 +1071,55 @@ macro_rules! define_queries_struct { $($(#[$attr])* $name: QueryState<$tcx, queries::$name<$tcx>>,)* } + + impl<$tcx> Queries<$tcx> { + pub fn new( + providers: IndexVec>, + fallback_extern_providers: Providers<$tcx>, + on_disk_cache: OnDiskCache<'tcx>, + ) -> Self { + Queries { + providers, + fallback_extern_providers: Box::new(fallback_extern_providers), + on_disk_cache, + $($name: Default::default()),* + } + } + + pub fn try_collect_active_jobs( + &self + ) -> Option>> { + let mut jobs = FxHashMap::default(); + + $( + // We use try_lock_shards here since we are called from the + // deadlock handler, and this shouldn't be locked. + let shards = self.$name.shards.try_lock_shards()?; + let shards = shards.iter().enumerate(); + jobs.extend(shards.flat_map(|(shard_id, shard)| { + shard.active.iter().filter_map(move |(k, v)| { + if let QueryResult::Started(ref job) = *v { + let id = QueryJobId { + job: job.id, + shard: u16::try_from(shard_id).unwrap(), + kind: + as QueryAccessors<'tcx>>::DEP_KIND, + }; + let info = QueryInfo { + span: job.span, + query: Query::$name(k.clone()) + }; + Some((id, QueryJobInfo { info, job: job.clone() })) + } else { + None + } + }) + })); + )* + + Some(jobs) + } + } }; } From b08943358ec8adc2d8e542659c7dcd2514311918 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Fri, 6 Mar 2020 18:36:11 +0100 Subject: [PATCH 10/23] Unpack type arguments for QueryStateShard. --- src/librustc/ty/query/plumbing.rs | 35 ++++++++++++++++++------------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/src/librustc/ty/query/plumbing.rs b/src/librustc/ty/query/plumbing.rs index 5532d7e08cc92..b688365b2bb9a 100644 --- a/src/librustc/ty/query/plumbing.rs +++ b/src/librustc/ty/query/plumbing.rs @@ -4,7 +4,7 @@ use crate::dep_graph::{DepNode, DepNodeIndex, SerializedDepNodeIndex}; use crate::ty::query::caches::QueryCache; -use crate::ty::query::config::{QueryAccessors, QueryDescription}; +use crate::ty::query::config::{QueryAccessors, QueryConfig, QueryDescription}; use crate::ty::query::job::{QueryInfo, QueryJob, QueryJobId, QueryShardJobId}; use crate::ty::query::Query; use crate::ty::tls; @@ -27,25 +27,32 @@ use std::ptr; #[cfg(debug_assertions)] use std::sync::atomic::{AtomicUsize, Ordering}; -pub(crate) struct QueryStateShard<'tcx, D: QueryAccessors<'tcx> + ?Sized> { - pub(super) cache: <>::Cache as QueryCache>::Sharded, - pub(super) active: FxHashMap>, +pub(crate) type QueryStateShard<'tcx, Q> = QueryStateShardImpl< + 'tcx, + >::Key, + <>::Cache as QueryCache< + >::Key, + >::Value, + >>::Sharded, +>; + +pub(crate) struct QueryStateShardImpl<'tcx, K, C> { + pub(super) cache: C, + pub(super) active: FxHashMap>, /// Used to generate unique ids for active jobs. pub(super) jobs: u32, } -impl<'tcx, Q: QueryAccessors<'tcx>> QueryStateShard<'tcx, Q> { - fn get_cache( - &mut self, - ) -> &mut <>::Cache as QueryCache>::Sharded { +impl<'tcx, K, C> QueryStateShardImpl<'tcx, K, C> { + fn get_cache(&mut self) -> &mut C { &mut self.cache } } -impl<'tcx, Q: QueryAccessors<'tcx>> Default for QueryStateShard<'tcx, Q> { - fn default() -> QueryStateShard<'tcx, Q> { - QueryStateShard { cache: Default::default(), active: Default::default(), jobs: 0 } +impl<'tcx, K, C: Default> Default for QueryStateShardImpl<'tcx, K, C> { + fn default() -> QueryStateShardImpl<'tcx, K, C> { + QueryStateShardImpl { cache: Default::default(), active: Default::default(), jobs: 0 } } } @@ -122,7 +129,7 @@ pub(super) struct JobOwner<'tcx, Q: QueryDescription<'tcx>> { id: QueryJobId, } -impl<'tcx, Q: QueryDescription<'tcx>> JobOwner<'tcx, Q> { +impl<'tcx, Q: QueryDescription<'tcx> + 'tcx> JobOwner<'tcx, Q> { /// Either gets a `JobOwner` corresponding the query, allowing us to /// start executing the query, or returns with the result of the query. /// This function assumes that `try_get_cached` is already called and returned `lookup`. @@ -470,7 +477,7 @@ impl<'tcx> TyCtxt<'tcx> { } #[inline(always)] - pub(super) fn try_execute_query>( + pub(super) fn try_execute_query + 'tcx>( self, span: Span, key: Q::Key, @@ -634,7 +641,7 @@ impl<'tcx> TyCtxt<'tcx> { } #[inline(always)] - fn force_query_with_job>( + fn force_query_with_job + 'tcx>( self, key: Q::Key, job: JobOwner<'tcx, Q>, From 486a082c58c60959328e0b394f9e58850b3c6341 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Fri, 6 Mar 2020 19:54:19 +0100 Subject: [PATCH 11/23] Unpack type arguments for QueryLookup. --- src/librustc/ty/query/plumbing.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/librustc/ty/query/plumbing.rs b/src/librustc/ty/query/plumbing.rs index b688365b2bb9a..67f0fdfcd5488 100644 --- a/src/librustc/ty/query/plumbing.rs +++ b/src/librustc/ty/query/plumbing.rs @@ -74,7 +74,7 @@ impl<'tcx, Q: QueryAccessors<'tcx>> QueryState<'tcx, Q> { let shard = self.shards.get_shard_index_by_hash(key_hash); let lock = self.shards.get_shard_by_index(shard).lock(); - QueryLookup { key_hash, shard, lock } + QueryLookupImpl { key_hash, shard, lock } } } @@ -115,10 +115,11 @@ impl<'tcx, M: QueryAccessors<'tcx>> Default for QueryState<'tcx, M> { } /// Values used when checking a query cache which can be reused on a cache-miss to execute the query. -pub(crate) struct QueryLookup<'tcx, Q: QueryAccessors<'tcx>> { +pub(crate) type QueryLookup<'tcx, Q> = QueryLookupImpl<'tcx, QueryStateShard<'tcx, Q>>; +pub(crate) struct QueryLookupImpl<'tcx, QSS> { pub(super) key_hash: u64, pub(super) shard: usize, - pub(super) lock: LockGuard<'tcx, QueryStateShard<'tcx, Q>>, + pub(super) lock: LockGuard<'tcx, QSS>, } /// A type representing the responsibility to execute the job in the `job` field. From a0f57e24e35a365d9d55f37611edfc6666b5d3c9 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Fri, 6 Mar 2020 20:05:05 +0100 Subject: [PATCH 12/23] Unpack type arguments for QueryState. --- src/librustc/ty/query/plumbing.rs | 38 +++++++++++++++++++------------ 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/src/librustc/ty/query/plumbing.rs b/src/librustc/ty/query/plumbing.rs index 67f0fdfcd5488..37fc339ee6395 100644 --- a/src/librustc/ty/query/plumbing.rs +++ b/src/librustc/ty/query/plumbing.rs @@ -56,15 +56,25 @@ impl<'tcx, K, C: Default> Default for QueryStateShardImpl<'tcx, K, C> { } } -pub(crate) struct QueryState<'tcx, D: QueryAccessors<'tcx> + ?Sized> { - pub(super) cache: D::Cache, - pub(super) shards: Sharded>, +pub(crate) type QueryState<'tcx, Q> = QueryStateImpl< + 'tcx, + >::Key, + >::Value, + >::Cache, +>; + +pub(crate) struct QueryStateImpl<'tcx, K, V, C: QueryCache> { + pub(super) cache: C, + pub(super) shards: Sharded>, #[cfg(debug_assertions)] pub(super) cache_hits: AtomicUsize, } -impl<'tcx, Q: QueryAccessors<'tcx>> QueryState<'tcx, Q> { - pub(super) fn get_lookup(&'tcx self, key: &K) -> QueryLookup<'tcx, Q> { +impl<'tcx, K, V, C: QueryCache> QueryStateImpl<'tcx, K, V, C> { + pub(super) fn get_lookup( + &'tcx self, + key: &K2, + ) -> QueryLookupImpl<'tcx, QueryStateShardImpl<'tcx, K, C::Sharded>> { // We compute the key's hash once and then use it for both the // shard lookup and the hashmap lookup. This relies on the fact // that both of them use `FxHasher`. @@ -88,12 +98,10 @@ pub(super) enum QueryResult<'tcx> { Poisoned, } -impl<'tcx, M: QueryAccessors<'tcx>> QueryState<'tcx, M> { +impl<'tcx, K, V, C: QueryCache> QueryStateImpl<'tcx, K, V, C> { pub fn iter_results( &self, - f: impl for<'a> FnOnce( - Box + 'a>, - ) -> R, + f: impl for<'a> FnOnce(Box + 'a>) -> R, ) -> R { self.cache.iter(&self.shards, |shard| &mut shard.cache, f) } @@ -103,10 +111,10 @@ impl<'tcx, M: QueryAccessors<'tcx>> QueryState<'tcx, M> { } } -impl<'tcx, M: QueryAccessors<'tcx>> Default for QueryState<'tcx, M> { - fn default() -> QueryState<'tcx, M> { - QueryState { - cache: M::Cache::default(), +impl<'tcx, K, V, C: QueryCache> Default for QueryStateImpl<'tcx, K, V, C> { + fn default() -> QueryStateImpl<'tcx, K, V, C> { + QueryStateImpl { + cache: C::default(), shards: Default::default(), #[cfg(debug_assertions)] cache_hits: AtomicUsize::new(0), @@ -441,7 +449,7 @@ impl<'tcx> TyCtxt<'tcx> { { let state = Q::query_state(self); - state.cache.lookup( + state.cache.lookup::<_, _, _, _, Q>( state, QueryStateShard::::get_cache, key, @@ -1035,7 +1043,7 @@ macro_rules! define_queries_inner { let mut string_cache = QueryKeyStringCache::new(); $({ - alloc_self_profile_query_strings_for_query_cache( + alloc_self_profile_query_strings_for_query_cache::>( self, stringify!($name), &self.queries.$name, From fa02dca428314676716c78b5aa1953eea1627bf0 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Fri, 6 Mar 2020 20:10:53 +0100 Subject: [PATCH 13/23] Remove Q parameter from QueryCache::lookup. --- src/librustc/ty/query/caches.rs | 25 +++++++++++++------------ src/librustc/ty/query/plumbing.rs | 2 +- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/src/librustc/ty/query/caches.rs b/src/librustc/ty/query/caches.rs index efc2804bd4d59..ae01dcb364bfc 100644 --- a/src/librustc/ty/query/caches.rs +++ b/src/librustc/ty/query/caches.rs @@ -1,6 +1,5 @@ use crate::dep_graph::DepNodeIndex; -use crate::ty::query::config::QueryAccessors; -use crate::ty::query::plumbing::{QueryLookup, QueryState, QueryStateShard}; +use crate::ty::query::plumbing::{QueryLookupImpl, QueryStateImpl, QueryStateShardImpl}; use crate::ty::TyCtxt; use rustc_data_structures::fx::FxHashMap; @@ -19,9 +18,9 @@ pub(crate) trait QueryCache: Default { /// It returns the shard index and a lock guard to the shard, /// which will be used if the query is not in the cache and we need /// to compute it. - fn lookup<'tcx, R, GetCache, OnHit, OnMiss, Q>( + fn lookup<'tcx, R, GetCache, OnHit, OnMiss>( &self, - state: &'tcx QueryState<'tcx, Q>, + state: &'tcx QueryStateImpl<'tcx, K, V, Self>, get_cache: GetCache, key: K, // `on_hit` can be called while holding a lock to the query state shard. @@ -29,10 +28,11 @@ pub(crate) trait QueryCache: Default { on_miss: OnMiss, ) -> R where - Q: QueryAccessors<'tcx>, - GetCache: for<'a> Fn(&'a mut QueryStateShard<'tcx, Q>) -> &'a mut Self::Sharded, + GetCache: for<'a> Fn( + &'a mut QueryStateShardImpl<'tcx, K, Self::Sharded>, + ) -> &'a mut Self::Sharded, OnHit: FnOnce(&V, DepNodeIndex) -> R, - OnMiss: FnOnce(K, QueryLookup<'tcx, Q>) -> R; + OnMiss: FnOnce(K, QueryLookupImpl<'tcx, QueryStateShardImpl<'tcx, K, Self::Sharded>>) -> R; fn complete( &self, @@ -64,19 +64,20 @@ impl QueryCache for DefaultCache { type Sharded = FxHashMap; #[inline(always)] - fn lookup<'tcx, R, GetCache, OnHit, OnMiss, Q>( + fn lookup<'tcx, R, GetCache, OnHit, OnMiss>( &self, - state: &'tcx QueryState<'tcx, Q>, + state: &'tcx QueryStateImpl<'tcx, K, V, Self>, get_cache: GetCache, key: K, on_hit: OnHit, on_miss: OnMiss, ) -> R where - Q: QueryAccessors<'tcx>, - GetCache: for<'a> Fn(&'a mut QueryStateShard<'tcx, Q>) -> &'a mut Self::Sharded, + GetCache: for<'a> Fn( + &'a mut QueryStateShardImpl<'tcx, K, Self::Sharded>, + ) -> &'a mut Self::Sharded, OnHit: FnOnce(&V, DepNodeIndex) -> R, - OnMiss: FnOnce(K, QueryLookup<'tcx, Q>) -> R, + OnMiss: FnOnce(K, QueryLookupImpl<'tcx, QueryStateShardImpl<'tcx, K, Self::Sharded>>) -> R, { let mut lookup = state.get_lookup(&key); let lock = &mut *lookup.lock; diff --git a/src/librustc/ty/query/plumbing.rs b/src/librustc/ty/query/plumbing.rs index 37fc339ee6395..e4d0e96d07c75 100644 --- a/src/librustc/ty/query/plumbing.rs +++ b/src/librustc/ty/query/plumbing.rs @@ -449,7 +449,7 @@ impl<'tcx> TyCtxt<'tcx> { { let state = Q::query_state(self); - state.cache.lookup::<_, _, _, _, Q>( + state.cache.lookup( state, QueryStateShard::::get_cache, key, From a18aa81bd8dd3012e43f195c52b8113a74479056 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Fri, 6 Mar 2020 20:17:03 +0100 Subject: [PATCH 14/23] Remove Q parameter from alloc_self_profile_query_strings_for_query_cache. --- src/librustc/ty/query/plumbing.rs | 2 +- src/librustc/ty/query/profiling_support.rs | 11 ++++++----- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/librustc/ty/query/plumbing.rs b/src/librustc/ty/query/plumbing.rs index e4d0e96d07c75..98d8de89d8e9e 100644 --- a/src/librustc/ty/query/plumbing.rs +++ b/src/librustc/ty/query/plumbing.rs @@ -1043,7 +1043,7 @@ macro_rules! define_queries_inner { let mut string_cache = QueryKeyStringCache::new(); $({ - alloc_self_profile_query_strings_for_query_cache::>( + alloc_self_profile_query_strings_for_query_cache( self, stringify!($name), &self.queries.$name, diff --git a/src/librustc/ty/query/profiling_support.rs b/src/librustc/ty/query/profiling_support.rs index 99ada34d59ebe..290e37f2cf825 100644 --- a/src/librustc/ty/query/profiling_support.rs +++ b/src/librustc/ty/query/profiling_support.rs @@ -1,7 +1,7 @@ use crate::hir::map::definitions::DefPathData; use crate::ty::context::TyCtxt; -use crate::ty::query::config::QueryAccessors; -use crate::ty::query::plumbing::QueryState; +use crate::ty::query::caches::QueryCache; +use crate::ty::query::plumbing::QueryStateImpl; use measureme::{StringComponent, StringId}; use rustc_data_structures::fx::FxHashMap; use rustc_data_structures::profiling::SelfProfiler; @@ -157,13 +157,14 @@ where /// Allocate the self-profiling query strings for a single query cache. This /// method is called from `alloc_self_profile_query_strings` which knows all /// the queries via macro magic. -pub(super) fn alloc_self_profile_query_strings_for_query_cache<'tcx, Q>( +pub(super) fn alloc_self_profile_query_strings_for_query_cache<'tcx, K, V, C>( tcx: TyCtxt<'tcx>, query_name: &'static str, - query_state: &QueryState<'tcx, Q>, + query_state: &QueryStateImpl<'tcx, K, V, C>, string_cache: &mut QueryKeyStringCache, ) where - Q: QueryAccessors<'tcx>, + K: Debug + Clone, + C: QueryCache, { tcx.prof.with_profiler(|profiler| { let event_id_builder = profiler.event_id_builder(); From d125bbb12b9b1839068706834d350acf5a91244c Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Fri, 6 Mar 2020 20:33:13 +0100 Subject: [PATCH 15/23] Remove Q parameter from query stats. --- src/librustc/ty/query/stats.rs | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/src/librustc/ty/query/stats.rs b/src/librustc/ty/query/stats.rs index d257320d4eaf6..c4c81cd5a13cf 100644 --- a/src/librustc/ty/query/stats.rs +++ b/src/librustc/ty/query/stats.rs @@ -1,5 +1,6 @@ -use crate::ty::query::config::QueryAccessors; -use crate::ty::query::plumbing::QueryState; +use crate::ty::query::caches::QueryCache; +use crate::ty::query::config::{QueryAccessors, QueryConfig}; +use crate::ty::query::plumbing::QueryStateImpl; use crate::ty::query::queries; use crate::ty::TyCtxt; use rustc_hir::def_id::{DefId, LOCAL_CRATE}; @@ -37,9 +38,9 @@ struct QueryStats { local_def_id_keys: Option, } -fn stats<'tcx, Q: QueryAccessors<'tcx>>( +fn stats<'tcx, K, V, C: QueryCache>( name: &'static str, - map: &QueryState<'tcx, Q>, + map: &QueryStateImpl<'tcx, K, V, C>, ) -> QueryStats { let mut stats = QueryStats { name, @@ -47,10 +48,10 @@ fn stats<'tcx, Q: QueryAccessors<'tcx>>( cache_hits: map.cache_hits.load(Ordering::Relaxed), #[cfg(not(debug_assertions))] cache_hits: 0, - key_size: mem::size_of::(), - key_type: type_name::(), - value_size: mem::size_of::(), - value_type: type_name::(), + key_size: mem::size_of::(), + key_type: type_name::(), + value_size: mem::size_of::(), + value_type: type_name::(), entry_count: map.iter_results(|results| results.count()), local_def_id_keys: None, }; @@ -125,7 +126,11 @@ macro_rules! print_stats { let mut queries = Vec::new(); $($( - queries.push(stats::>( + queries.push(stats::< + as QueryConfig<'_>>::Key, + as QueryConfig<'_>>::Value, + as QueryAccessors<'_>>::Cache, + >( stringify!($name), &tcx.queries.$name, )); From fa0794db239d588b5822233d32fa7aa82bd17069 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Sat, 7 Mar 2020 10:38:44 +0100 Subject: [PATCH 16/23] Remove Q parameter from JobOwner. --- src/librustc/ty/query/plumbing.rs | 61 ++++++++++++++++++++++--------- 1 file changed, 43 insertions(+), 18 deletions(-) diff --git a/src/librustc/ty/query/plumbing.rs b/src/librustc/ty/query/plumbing.rs index 98d8de89d8e9e..9228e569f5586 100644 --- a/src/librustc/ty/query/plumbing.rs +++ b/src/librustc/ty/query/plumbing.rs @@ -20,6 +20,7 @@ use rustc_errors::{struct_span_err, Diagnostic, DiagnosticBuilder, FatalError, H use rustc_span::source_map::DUMMY_SP; use rustc_span::Span; use std::collections::hash_map::Entry; +use std::fmt::Debug; use std::hash::{Hash, Hasher}; use std::mem; use std::num::NonZeroU32; @@ -132,13 +133,28 @@ pub(crate) struct QueryLookupImpl<'tcx, QSS> { /// A type representing the responsibility to execute the job in the `job` field. /// This will poison the relevant query if dropped. -pub(super) struct JobOwner<'tcx, Q: QueryDescription<'tcx>> { - tcx: TyCtxt<'tcx>, - key: Q::Key, +pub(super) type JobOwner<'tcx, Q> = JobOwnerImpl< + 'tcx, + >::Key, + >::Value, + >::Cache, +>; + +pub(super) struct JobOwnerImpl<'tcx, K, V, C: QueryCache> +where + K: Eq + Hash + Clone + Debug, + V: Clone, +{ + state: &'tcx QueryStateImpl<'tcx, K, V, C>, + key: K, id: QueryJobId, } -impl<'tcx, Q: QueryDescription<'tcx> + 'tcx> JobOwner<'tcx, Q> { +impl<'tcx, K, V, C: QueryCache> JobOwnerImpl<'tcx, K, V, C> +where + K: Eq + Hash + Clone + Debug, + V: Clone, +{ /// Either gets a `JobOwner` corresponding the query, allowing us to /// start executing the query, or returns with the result of the query. /// This function assumes that `try_get_cached` is already called and returned `lookup`. @@ -148,12 +164,17 @@ impl<'tcx, Q: QueryDescription<'tcx> + 'tcx> JobOwner<'tcx, Q> { /// This function is inlined because that results in a noticeable speed-up /// for some compile-time benchmarks. #[inline(always)] - pub(super) fn try_start( + pub(super) fn try_start( tcx: TyCtxt<'tcx>, span: Span, - key: &Q::Key, + key: &K, mut lookup: QueryLookup<'tcx, Q>, - ) -> TryGetJob<'tcx, Q> { + ) -> TryGetJob<'tcx, Q> + where + K: Eq + Hash + Clone + Debug, + V: Clone, + Q: QueryDescription<'tcx, Key = K, Value = V, Cache = C> + 'tcx, + { let lock = &mut *lookup.lock; let (latch, mut _query_blocked_prof_timer) = match lock.active.entry((*key).clone()) { @@ -191,7 +212,8 @@ impl<'tcx, Q: QueryDescription<'tcx> + 'tcx> JobOwner<'tcx, Q> { entry.insert(QueryResult::Started(job)); - let owner = JobOwner { tcx, id: global_id, key: (*key).clone() }; + let owner = + JobOwnerImpl { state: Q::query_state(tcx), id: global_id, key: (*key).clone() }; return TryGetJob::NotYetStarted(owner); } }; @@ -231,16 +253,15 @@ impl<'tcx, Q: QueryDescription<'tcx> + 'tcx> JobOwner<'tcx, Q> { /// Completes the query by updating the query cache with the `result`, /// signals the waiter and forgets the JobOwner, so it won't poison the query #[inline(always)] - pub(super) fn complete(self, result: &Q::Value, dep_node_index: DepNodeIndex) { + pub(super) fn complete(self, tcx: TyCtxt<'tcx>, result: &V, dep_node_index: DepNodeIndex) { // We can move out of `self` here because we `mem::forget` it below let key = unsafe { ptr::read(&self.key) }; - let tcx = self.tcx; + let state = self.state; // Forget ourself so our destructor won't poison the query mem::forget(self); let job = { - let state = Q::query_state(tcx); let result = result.clone(); let mut lock = state.shards.get_shard_by_value(&key).lock(); let job = match lock.active.remove(&key).unwrap() { @@ -265,12 +286,16 @@ where (result, diagnostics.into_inner()) } -impl<'tcx, Q: QueryDescription<'tcx>> Drop for JobOwner<'tcx, Q> { +impl<'tcx, K, V, C: QueryCache> Drop for JobOwnerImpl<'tcx, K, V, C> +where + K: Eq + Hash + Clone + Debug, + V: Clone, +{ #[inline(never)] #[cold] fn drop(&mut self) { // Poison the query so jobs waiting on it panic. - let state = Q::query_state(self.tcx); + let state = self.state; let shard = state.shards.get_shard_by_value(&self.key); let job = { let mut shard = shard.lock(); @@ -492,7 +517,7 @@ impl<'tcx> TyCtxt<'tcx> { key: Q::Key, lookup: QueryLookup<'tcx, Q>, ) -> Q::Value { - let job = match JobOwner::try_start(self, span, &key, lookup) { + let job = match JobOwnerImpl::try_start::(self, span, &key, lookup) { TryGetJob::NotYetStarted(job) => job, TryGetJob::Cycle(result) => return result, #[cfg(parallel_compiler)] @@ -528,7 +553,7 @@ impl<'tcx> TyCtxt<'tcx> { .store_diagnostics_for_anon_node(dep_node_index, diagnostics); } - job.complete(&result, dep_node_index); + job.complete(self, &result, dep_node_index); return result; } @@ -554,7 +579,7 @@ impl<'tcx> TyCtxt<'tcx> { }) }); if let Some((result, dep_node_index)) = loaded { - job.complete(&result, dep_node_index); + job.complete(self, &result, dep_node_index); return result; } } @@ -696,7 +721,7 @@ impl<'tcx> TyCtxt<'tcx> { } } - job.complete(&result, dep_node_index); + job.complete(self, &result, dep_node_index); (result, dep_node_index) } @@ -751,7 +776,7 @@ impl<'tcx> TyCtxt<'tcx> { // Cache hit, do nothing }, |key, lookup| { - let job = match JobOwner::try_start(self, span, &key, lookup) { + let job = match JobOwnerImpl::try_start::(self, span, &key, lookup) { TryGetJob::NotYetStarted(job) => job, TryGetJob::Cycle(_) => return, #[cfg(parallel_compiler)] From 5dc7c2ed1aab0c6b0bfb15b4fb5a4d079d3aaa36 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Sat, 7 Mar 2020 17:19:47 +0100 Subject: [PATCH 17/23] Remove Q parameter from try_get_cached. --- src/librustc/ty/query/plumbing.rs | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/src/librustc/ty/query/plumbing.rs b/src/librustc/ty/query/plumbing.rs index 9228e569f5586..1ff7e44bfbb9c 100644 --- a/src/librustc/ty/query/plumbing.rs +++ b/src/librustc/ty/query/plumbing.rs @@ -236,7 +236,8 @@ where return TryGetJob::Cycle(Q::handle_cycle_error(tcx, cycle)); } - let cached = tcx.try_get_cached::( + let cached = tcx.try_get_cached( + Q::query_state(tcx), (*key).clone(), |value, index| (value.clone(), index), |_, _| panic!("value must be in cache after waiting"), @@ -460,23 +461,22 @@ impl<'tcx> TyCtxt<'tcx> { /// which will be used if the query is not in the cache and we need /// to compute it. #[inline(always)] - fn try_get_cached( + fn try_get_cached( self, - key: Q::Key, + state: &'tcx QueryStateImpl<'tcx, K, V, C>, + key: K, // `on_hit` can be called while holding a lock to the query cache on_hit: OnHit, on_miss: OnMiss, ) -> R where - Q: QueryDescription<'tcx> + 'tcx, - OnHit: FnOnce(&Q::Value, DepNodeIndex) -> R, - OnMiss: FnOnce(Q::Key, QueryLookup<'tcx, Q>) -> R, + C: QueryCache, + OnHit: FnOnce(&V, DepNodeIndex) -> R, + OnMiss: FnOnce(K, QueryLookupImpl<'tcx, QueryStateShardImpl<'tcx, K, C::Sharded>>) -> R, { - let state = Q::query_state(self); - state.cache.lookup( state, - QueryStateShard::::get_cache, + QueryStateShardImpl::::get_cache, key, |value, index| { if unlikely!(self.prof.enabled()) { @@ -500,7 +500,8 @@ impl<'tcx> TyCtxt<'tcx> { ) -> Q::Value { debug!("ty::query::get_query<{}>(key={:?}, span={:?})", Q::NAME, key, span); - self.try_get_cached::( + self.try_get_cached( + Q::query_state(self), key, |value, index| { self.dep_graph.read_index(index); @@ -770,7 +771,8 @@ impl<'tcx> TyCtxt<'tcx> { // We may be concurrently trying both execute and force a query. // Ensure that only one of them runs the query. - self.try_get_cached::( + self.try_get_cached( + Q::query_state(self), key, |_, _| { // Cache hit, do nothing From 7d84f4fb169d55b0423e4e379828700ec0214400 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Sat, 7 Mar 2020 11:03:49 +0100 Subject: [PATCH 18/23] Offload try_collect_active_jobs. --- src/librustc/ty/query/mod.rs | 1 - src/librustc/ty/query/plumbing.rs | 62 ++++++++++++++++++------------- 2 files changed, 37 insertions(+), 26 deletions(-) diff --git a/src/librustc/ty/query/mod.rs b/src/librustc/ty/query/mod.rs index 3d17883fec3bd..54e80b4dc0b15 100644 --- a/src/librustc/ty/query/mod.rs +++ b/src/librustc/ty/query/mod.rs @@ -57,7 +57,6 @@ use rustc_span::symbol::Symbol; use rustc_span::{Span, DUMMY_SP}; use std::borrow::Cow; use std::collections::BTreeMap; -use std::convert::TryFrom; use std::ops::Deref; use std::sync::Arc; diff --git a/src/librustc/ty/query/plumbing.rs b/src/librustc/ty/query/plumbing.rs index 1ff7e44bfbb9c..406ca18b59105 100644 --- a/src/librustc/ty/query/plumbing.rs +++ b/src/librustc/ty/query/plumbing.rs @@ -2,10 +2,10 @@ //! generate the actual methods on tcx which find and execute the provider, //! manage the caches, and so forth. -use crate::dep_graph::{DepNode, DepNodeIndex, SerializedDepNodeIndex}; +use crate::dep_graph::{DepKind, DepNode, DepNodeIndex, SerializedDepNodeIndex}; use crate::ty::query::caches::QueryCache; use crate::ty::query::config::{QueryAccessors, QueryConfig, QueryDescription}; -use crate::ty::query::job::{QueryInfo, QueryJob, QueryJobId, QueryShardJobId}; +use crate::ty::query::job::{QueryInfo, QueryJob, QueryJobId, QueryJobInfo, QueryShardJobId}; use crate::ty::query::Query; use crate::ty::tls; use crate::ty::{self, TyCtxt}; @@ -20,6 +20,7 @@ use rustc_errors::{struct_span_err, Diagnostic, DiagnosticBuilder, FatalError, H use rustc_span::source_map::DUMMY_SP; use rustc_span::Span; use std::collections::hash_map::Entry; +use std::convert::TryFrom; use std::fmt::Debug; use std::hash::{Hash, Hasher}; use std::mem; @@ -110,6 +111,35 @@ impl<'tcx, K, V, C: QueryCache> QueryStateImpl<'tcx, K, V, C> { let shards = self.shards.lock_shards(); shards.iter().all(|shard| shard.active.is_empty()) } + + pub(super) fn try_collect_active_jobs( + &self, + kind: DepKind, + make_query: fn(K) -> Query<'tcx>, + jobs: &mut FxHashMap>, + ) -> Option<()> + where + K: Clone, + { + // We use try_lock_shards here since we are called from the + // deadlock handler, and this shouldn't be locked. + let shards = self.shards.try_lock_shards()?; + let shards = shards.iter().enumerate(); + jobs.extend(shards.flat_map(|(shard_id, shard)| { + shard.active.iter().filter_map(move |(k, v)| { + if let QueryResult::Started(ref job) = *v { + let id = + QueryJobId { job: job.id, shard: u16::try_from(shard_id).unwrap(), kind }; + let info = QueryInfo { span: job.span, query: make_query(k.clone()) }; + Some((id, QueryJobInfo { info, job: job.clone() })) + } else { + None + } + }) + })); + + Some(()) + } } impl<'tcx, K, V, C: QueryCache> Default for QueryStateImpl<'tcx, K, V, C> { @@ -1135,29 +1165,11 @@ macro_rules! define_queries_struct { let mut jobs = FxHashMap::default(); $( - // We use try_lock_shards here since we are called from the - // deadlock handler, and this shouldn't be locked. - let shards = self.$name.shards.try_lock_shards()?; - let shards = shards.iter().enumerate(); - jobs.extend(shards.flat_map(|(shard_id, shard)| { - shard.active.iter().filter_map(move |(k, v)| { - if let QueryResult::Started(ref job) = *v { - let id = QueryJobId { - job: job.id, - shard: u16::try_from(shard_id).unwrap(), - kind: - as QueryAccessors<'tcx>>::DEP_KIND, - }; - let info = QueryInfo { - span: job.span, - query: Query::$name(k.clone()) - }; - Some((id, QueryJobInfo { info, job: job.clone() })) - } else { - None - } - }) - })); + self.$name.try_collect_active_jobs( + as QueryAccessors<'tcx>>::DEP_KIND, + Query::$name, + &mut jobs, + )?; )* Some(jobs) From 7309b3cd8be928a0ec9e3219d9562ddeb54ff994 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Sat, 7 Mar 2020 17:56:59 +0100 Subject: [PATCH 19/23] Simplify type aliases. --- src/librustc/ty/query/caches.rs | 16 ++++++------ src/librustc/ty/query/plumbing.rs | 41 +++++++++++-------------------- 2 files changed, 21 insertions(+), 36 deletions(-) diff --git a/src/librustc/ty/query/caches.rs b/src/librustc/ty/query/caches.rs index ae01dcb364bfc..17639545d12a1 100644 --- a/src/librustc/ty/query/caches.rs +++ b/src/librustc/ty/query/caches.rs @@ -1,5 +1,5 @@ use crate::dep_graph::DepNodeIndex; -use crate::ty::query::plumbing::{QueryLookupImpl, QueryStateImpl, QueryStateShardImpl}; +use crate::ty::query::plumbing::{QueryLookup, QueryStateImpl, QueryStateShard}; use crate::ty::TyCtxt; use rustc_data_structures::fx::FxHashMap; @@ -28,11 +28,10 @@ pub(crate) trait QueryCache: Default { on_miss: OnMiss, ) -> R where - GetCache: for<'a> Fn( - &'a mut QueryStateShardImpl<'tcx, K, Self::Sharded>, - ) -> &'a mut Self::Sharded, + GetCache: + for<'a> Fn(&'a mut QueryStateShard<'tcx, K, Self::Sharded>) -> &'a mut Self::Sharded, OnHit: FnOnce(&V, DepNodeIndex) -> R, - OnMiss: FnOnce(K, QueryLookupImpl<'tcx, QueryStateShardImpl<'tcx, K, Self::Sharded>>) -> R; + OnMiss: FnOnce(K, QueryLookup<'tcx, K, Self::Sharded>) -> R; fn complete( &self, @@ -73,11 +72,10 @@ impl QueryCache for DefaultCache { on_miss: OnMiss, ) -> R where - GetCache: for<'a> Fn( - &'a mut QueryStateShardImpl<'tcx, K, Self::Sharded>, - ) -> &'a mut Self::Sharded, + GetCache: + for<'a> Fn(&'a mut QueryStateShard<'tcx, K, Self::Sharded>) -> &'a mut Self::Sharded, OnHit: FnOnce(&V, DepNodeIndex) -> R, - OnMiss: FnOnce(K, QueryLookupImpl<'tcx, QueryStateShardImpl<'tcx, K, Self::Sharded>>) -> R, + OnMiss: FnOnce(K, QueryLookup<'tcx, K, Self::Sharded>) -> R, { let mut lookup = state.get_lookup(&key); let lock = &mut *lookup.lock; diff --git a/src/librustc/ty/query/plumbing.rs b/src/librustc/ty/query/plumbing.rs index 406ca18b59105..467b1a7e4a177 100644 --- a/src/librustc/ty/query/plumbing.rs +++ b/src/librustc/ty/query/plumbing.rs @@ -29,16 +29,7 @@ use std::ptr; #[cfg(debug_assertions)] use std::sync::atomic::{AtomicUsize, Ordering}; -pub(crate) type QueryStateShard<'tcx, Q> = QueryStateShardImpl< - 'tcx, - >::Key, - <>::Cache as QueryCache< - >::Key, - >::Value, - >>::Sharded, ->; - -pub(crate) struct QueryStateShardImpl<'tcx, K, C> { +pub(crate) struct QueryStateShard<'tcx, K, C> { pub(super) cache: C, pub(super) active: FxHashMap>, @@ -46,15 +37,15 @@ pub(crate) struct QueryStateShardImpl<'tcx, K, C> { pub(super) jobs: u32, } -impl<'tcx, K, C> QueryStateShardImpl<'tcx, K, C> { +impl<'tcx, K, C> QueryStateShard<'tcx, K, C> { fn get_cache(&mut self) -> &mut C { &mut self.cache } } -impl<'tcx, K, C: Default> Default for QueryStateShardImpl<'tcx, K, C> { - fn default() -> QueryStateShardImpl<'tcx, K, C> { - QueryStateShardImpl { cache: Default::default(), active: Default::default(), jobs: 0 } +impl<'tcx, K, C: Default> Default for QueryStateShard<'tcx, K, C> { + fn default() -> QueryStateShard<'tcx, K, C> { + QueryStateShard { cache: Default::default(), active: Default::default(), jobs: 0 } } } @@ -67,16 +58,13 @@ pub(crate) type QueryState<'tcx, Q> = QueryStateImpl< pub(crate) struct QueryStateImpl<'tcx, K, V, C: QueryCache> { pub(super) cache: C, - pub(super) shards: Sharded>, + pub(super) shards: Sharded>, #[cfg(debug_assertions)] pub(super) cache_hits: AtomicUsize, } impl<'tcx, K, V, C: QueryCache> QueryStateImpl<'tcx, K, V, C> { - pub(super) fn get_lookup( - &'tcx self, - key: &K2, - ) -> QueryLookupImpl<'tcx, QueryStateShardImpl<'tcx, K, C::Sharded>> { + pub(super) fn get_lookup(&'tcx self, key: &K2) -> QueryLookup<'tcx, K, C::Sharded> { // We compute the key's hash once and then use it for both the // shard lookup and the hashmap lookup. This relies on the fact // that both of them use `FxHasher`. @@ -86,7 +74,7 @@ impl<'tcx, K, V, C: QueryCache> QueryStateImpl<'tcx, K, V, C> { let shard = self.shards.get_shard_index_by_hash(key_hash); let lock = self.shards.get_shard_by_index(shard).lock(); - QueryLookupImpl { key_hash, shard, lock } + QueryLookup { key_hash, shard, lock } } } @@ -154,11 +142,10 @@ impl<'tcx, K, V, C: QueryCache> Default for QueryStateImpl<'tcx, K, V, C> } /// Values used when checking a query cache which can be reused on a cache-miss to execute the query. -pub(crate) type QueryLookup<'tcx, Q> = QueryLookupImpl<'tcx, QueryStateShard<'tcx, Q>>; -pub(crate) struct QueryLookupImpl<'tcx, QSS> { +pub(crate) struct QueryLookup<'tcx, K, C> { pub(super) key_hash: u64, pub(super) shard: usize, - pub(super) lock: LockGuard<'tcx, QSS>, + pub(super) lock: LockGuard<'tcx, QueryStateShard<'tcx, K, C>>, } /// A type representing the responsibility to execute the job in the `job` field. @@ -198,7 +185,7 @@ where tcx: TyCtxt<'tcx>, span: Span, key: &K, - mut lookup: QueryLookup<'tcx, Q>, + mut lookup: QueryLookup<'tcx, K, C::Sharded>, ) -> TryGetJob<'tcx, Q> where K: Eq + Hash + Clone + Debug, @@ -502,11 +489,11 @@ impl<'tcx> TyCtxt<'tcx> { where C: QueryCache, OnHit: FnOnce(&V, DepNodeIndex) -> R, - OnMiss: FnOnce(K, QueryLookupImpl<'tcx, QueryStateShardImpl<'tcx, K, C::Sharded>>) -> R, + OnMiss: FnOnce(K, QueryLookup<'tcx, K, C::Sharded>) -> R, { state.cache.lookup( state, - QueryStateShardImpl::::get_cache, + QueryStateShard::::get_cache, key, |value, index| { if unlikely!(self.prof.enabled()) { @@ -546,7 +533,7 @@ impl<'tcx> TyCtxt<'tcx> { self, span: Span, key: Q::Key, - lookup: QueryLookup<'tcx, Q>, + lookup: QueryLookup<'tcx, Q::Key, >::Sharded>, ) -> Q::Value { let job = match JobOwnerImpl::try_start::(self, span, &key, lookup) { TryGetJob::NotYetStarted(job) => job, From 3abd4753b7c9fa9a3855d4b41b557f81b3e06aab Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Sat, 7 Mar 2020 18:36:24 +0100 Subject: [PATCH 20/23] Make QueryCache parameters associated types. --- src/librustc/ty/query/caches.rs | 45 +++++--- src/librustc/ty/query/config.rs | 7 +- src/librustc/ty/query/plumbing.rs | 118 +++++++++++---------- src/librustc/ty/query/profiling_support.rs | 8 +- src/librustc/ty/query/stats.rs | 17 ++- 5 files changed, 101 insertions(+), 94 deletions(-) diff --git a/src/librustc/ty/query/caches.rs b/src/librustc/ty/query/caches.rs index 17639545d12a1..71523ea39ca3e 100644 --- a/src/librustc/ty/query/caches.rs +++ b/src/librustc/ty/query/caches.rs @@ -6,12 +6,15 @@ use rustc_data_structures::fx::FxHashMap; use rustc_data_structures::sharded::Sharded; use std::default::Default; use std::hash::Hash; +use std::marker::PhantomData; pub(crate) trait CacheSelector { - type Cache: QueryCache; + type Cache: QueryCache; } -pub(crate) trait QueryCache: Default { +pub(crate) trait QueryCache: Default { + type Key; + type Value; type Sharded: Default; /// Checks if the query is already computed and in the cache. @@ -20,25 +23,26 @@ pub(crate) trait QueryCache: Default { /// to compute it. fn lookup<'tcx, R, GetCache, OnHit, OnMiss>( &self, - state: &'tcx QueryStateImpl<'tcx, K, V, Self>, + state: &'tcx QueryStateImpl<'tcx, Self>, get_cache: GetCache, - key: K, + key: Self::Key, // `on_hit` can be called while holding a lock to the query state shard. on_hit: OnHit, on_miss: OnMiss, ) -> R where - GetCache: - for<'a> Fn(&'a mut QueryStateShard<'tcx, K, Self::Sharded>) -> &'a mut Self::Sharded, - OnHit: FnOnce(&V, DepNodeIndex) -> R, - OnMiss: FnOnce(K, QueryLookup<'tcx, K, Self::Sharded>) -> R; + GetCache: for<'a> Fn( + &'a mut QueryStateShard<'tcx, Self::Key, Self::Sharded>, + ) -> &'a mut Self::Sharded, + OnHit: FnOnce(&Self::Value, DepNodeIndex) -> R, + OnMiss: FnOnce(Self::Key, QueryLookup<'tcx, Self::Key, Self::Sharded>) -> R; fn complete( &self, tcx: TyCtxt<'tcx>, lock_sharded_storage: &mut Self::Sharded, - key: K, - value: V, + key: Self::Key, + value: Self::Value, index: DepNodeIndex, ); @@ -46,26 +50,35 @@ pub(crate) trait QueryCache: Default { &self, shards: &Sharded, get_shard: impl Fn(&mut L) -> &mut Self::Sharded, - f: impl for<'a> FnOnce(Box + 'a>) -> R, + f: impl for<'a> FnOnce( + Box + 'a>, + ) -> R, ) -> R; } pub struct DefaultCacheSelector; impl CacheSelector for DefaultCacheSelector { - type Cache = DefaultCache; + type Cache = DefaultCache; } -#[derive(Default)] -pub struct DefaultCache; +pub struct DefaultCache(PhantomData<(K, V)>); + +impl Default for DefaultCache { + fn default() -> Self { + DefaultCache(PhantomData) + } +} -impl QueryCache for DefaultCache { +impl QueryCache for DefaultCache { + type Key = K; + type Value = V; type Sharded = FxHashMap; #[inline(always)] fn lookup<'tcx, R, GetCache, OnHit, OnMiss>( &self, - state: &'tcx QueryStateImpl<'tcx, K, V, Self>, + state: &'tcx QueryStateImpl<'tcx, Self>, get_cache: GetCache, key: K, on_hit: OnHit, diff --git a/src/librustc/ty/query/config.rs b/src/librustc/ty/query/config.rs index 29b9e0c3b40ef..5b0653bd599e1 100644 --- a/src/librustc/ty/query/config.rs +++ b/src/librustc/ty/query/config.rs @@ -30,7 +30,7 @@ pub(crate) trait QueryAccessors<'tcx>: QueryConfig<'tcx> { const EVAL_ALWAYS: bool; const DEP_KIND: DepKind; - type Cache: QueryCache; + type Cache: QueryCache; // Don't use this method to access query results, instead use the methods on TyCtxt fn query_state<'a>(tcx: TyCtxt<'tcx>) -> &'a QueryState<'tcx, Self>; @@ -59,10 +59,7 @@ pub(crate) trait QueryDescription<'tcx>: QueryAccessors<'tcx> { } } -impl<'tcx, M: QueryAccessors<'tcx, Key = DefId>> QueryDescription<'tcx> for M -where - >::Cache: QueryCache>::Value>, -{ +impl<'tcx, M: QueryAccessors<'tcx, Key = DefId>> QueryDescription<'tcx> for M { default fn describe(tcx: TyCtxt<'_>, def_id: DefId) -> Cow<'static, str> { if !tcx.sess.verbose() { format!("processing `{}`", tcx.def_path_str(def_id)).into() diff --git a/src/librustc/ty/query/plumbing.rs b/src/librustc/ty/query/plumbing.rs index 467b1a7e4a177..c2740733977e8 100644 --- a/src/librustc/ty/query/plumbing.rs +++ b/src/librustc/ty/query/plumbing.rs @@ -4,7 +4,7 @@ use crate::dep_graph::{DepKind, DepNode, DepNodeIndex, SerializedDepNodeIndex}; use crate::ty::query::caches::QueryCache; -use crate::ty::query::config::{QueryAccessors, QueryConfig, QueryDescription}; +use crate::ty::query::config::{QueryAccessors, QueryDescription}; use crate::ty::query::job::{QueryInfo, QueryJob, QueryJobId, QueryJobInfo, QueryShardJobId}; use crate::ty::query::Query; use crate::ty::tls; @@ -49,22 +49,20 @@ impl<'tcx, K, C: Default> Default for QueryStateShard<'tcx, K, C> { } } -pub(crate) type QueryState<'tcx, Q> = QueryStateImpl< - 'tcx, - >::Key, - >::Value, - >::Cache, ->; +pub(crate) type QueryState<'tcx, Q> = QueryStateImpl<'tcx, >::Cache>; -pub(crate) struct QueryStateImpl<'tcx, K, V, C: QueryCache> { +pub(crate) struct QueryStateImpl<'tcx, C: QueryCache> { pub(super) cache: C, - pub(super) shards: Sharded>, + pub(super) shards: Sharded>, #[cfg(debug_assertions)] pub(super) cache_hits: AtomicUsize, } -impl<'tcx, K, V, C: QueryCache> QueryStateImpl<'tcx, K, V, C> { - pub(super) fn get_lookup(&'tcx self, key: &K2) -> QueryLookup<'tcx, K, C::Sharded> { +impl<'tcx, C: QueryCache> QueryStateImpl<'tcx, C> { + pub(super) fn get_lookup( + &'tcx self, + key: &K2, + ) -> QueryLookup<'tcx, C::Key, C::Sharded> { // We compute the key's hash once and then use it for both the // shard lookup and the hashmap lookup. This relies on the fact // that both of them use `FxHasher`. @@ -88,10 +86,12 @@ pub(super) enum QueryResult<'tcx> { Poisoned, } -impl<'tcx, K, V, C: QueryCache> QueryStateImpl<'tcx, K, V, C> { +impl<'tcx, C: QueryCache> QueryStateImpl<'tcx, C> { pub fn iter_results( &self, - f: impl for<'a> FnOnce(Box + 'a>) -> R, + f: impl for<'a> FnOnce( + Box + 'a>, + ) -> R, ) -> R { self.cache.iter(&self.shards, |shard| &mut shard.cache, f) } @@ -103,11 +103,11 @@ impl<'tcx, K, V, C: QueryCache> QueryStateImpl<'tcx, K, V, C> { pub(super) fn try_collect_active_jobs( &self, kind: DepKind, - make_query: fn(K) -> Query<'tcx>, + make_query: fn(C::Key) -> Query<'tcx>, jobs: &mut FxHashMap>, ) -> Option<()> where - K: Clone, + C::Key: Clone, { // We use try_lock_shards here since we are called from the // deadlock handler, and this shouldn't be locked. @@ -130,8 +130,8 @@ impl<'tcx, K, V, C: QueryCache> QueryStateImpl<'tcx, K, V, C> { } } -impl<'tcx, K, V, C: QueryCache> Default for QueryStateImpl<'tcx, K, V, C> { - fn default() -> QueryStateImpl<'tcx, K, V, C> { +impl<'tcx, C: QueryCache> Default for QueryStateImpl<'tcx, C> { + fn default() -> QueryStateImpl<'tcx, C> { QueryStateImpl { cache: C::default(), shards: Default::default(), @@ -150,27 +150,22 @@ pub(crate) struct QueryLookup<'tcx, K, C> { /// A type representing the responsibility to execute the job in the `job` field. /// This will poison the relevant query if dropped. -pub(super) type JobOwner<'tcx, Q> = JobOwnerImpl< - 'tcx, - >::Key, - >::Value, - >::Cache, ->; - -pub(super) struct JobOwnerImpl<'tcx, K, V, C: QueryCache> +pub(super) struct JobOwner<'tcx, C> where - K: Eq + Hash + Clone + Debug, - V: Clone, + C: QueryCache, + C::Key: Eq + Hash + Clone + Debug, + C::Value: Clone, { - state: &'tcx QueryStateImpl<'tcx, K, V, C>, - key: K, + state: &'tcx QueryStateImpl<'tcx, C>, + key: C::Key, id: QueryJobId, } -impl<'tcx, K, V, C: QueryCache> JobOwnerImpl<'tcx, K, V, C> +impl<'tcx, C: QueryCache> JobOwner<'tcx, C> where - K: Eq + Hash + Clone + Debug, - V: Clone, + C: QueryCache, + C::Key: Eq + Hash + Clone + Debug, + C::Value: Clone, { /// Either gets a `JobOwner` corresponding the query, allowing us to /// start executing the query, or returns with the result of the query. @@ -184,13 +179,11 @@ where pub(super) fn try_start( tcx: TyCtxt<'tcx>, span: Span, - key: &K, - mut lookup: QueryLookup<'tcx, K, C::Sharded>, - ) -> TryGetJob<'tcx, Q> + key: &C::Key, + mut lookup: QueryLookup<'tcx, C::Key, C::Sharded>, + ) -> TryGetJob<'tcx, C> where - K: Eq + Hash + Clone + Debug, - V: Clone, - Q: QueryDescription<'tcx, Key = K, Value = V, Cache = C> + 'tcx, + Q: QueryDescription<'tcx, Key = C::Key, Value = C::Value, Cache = C>, { let lock = &mut *lookup.lock; @@ -230,7 +223,7 @@ where entry.insert(QueryResult::Started(job)); let owner = - JobOwnerImpl { state: Q::query_state(tcx), id: global_id, key: (*key).clone() }; + JobOwner { state: Q::query_state(tcx), id: global_id, key: (*key).clone() }; return TryGetJob::NotYetStarted(owner); } }; @@ -271,7 +264,12 @@ where /// Completes the query by updating the query cache with the `result`, /// signals the waiter and forgets the JobOwner, so it won't poison the query #[inline(always)] - pub(super) fn complete(self, tcx: TyCtxt<'tcx>, result: &V, dep_node_index: DepNodeIndex) { + pub(super) fn complete( + self, + tcx: TyCtxt<'tcx>, + result: &C::Value, + dep_node_index: DepNodeIndex, + ) { // We can move out of `self` here because we `mem::forget` it below let key = unsafe { ptr::read(&self.key) }; let state = self.state; @@ -304,10 +302,10 @@ where (result, diagnostics.into_inner()) } -impl<'tcx, K, V, C: QueryCache> Drop for JobOwnerImpl<'tcx, K, V, C> +impl<'tcx, C: QueryCache> Drop for JobOwner<'tcx, C> where - K: Eq + Hash + Clone + Debug, - V: Clone, + C::Key: Eq + Hash + Clone + Debug, + C::Value: Clone, { #[inline(never)] #[cold] @@ -338,18 +336,22 @@ pub struct CycleError<'tcx> { } /// The result of `try_start`. -pub(super) enum TryGetJob<'tcx, D: QueryDescription<'tcx>> { +pub(super) enum TryGetJob<'tcx, C: QueryCache> +where + C::Key: Eq + Hash + Clone + Debug, + C::Value: Clone, +{ /// The query is not yet started. Contains a guard to the cache eventually used to start it. - NotYetStarted(JobOwner<'tcx, D>), + NotYetStarted(JobOwner<'tcx, C>), /// The query was already completed. /// Returns the result of the query and its dep-node index /// if it succeeded or a cycle error if it failed. #[cfg(parallel_compiler)] - JobCompleted((D::Value, DepNodeIndex)), + JobCompleted((C::Value, DepNodeIndex)), /// Trying to execute the query resulted in a cycle. - Cycle(D::Value), + Cycle(C::Value), } impl<'tcx> TyCtxt<'tcx> { @@ -478,22 +480,22 @@ impl<'tcx> TyCtxt<'tcx> { /// which will be used if the query is not in the cache and we need /// to compute it. #[inline(always)] - fn try_get_cached( + fn try_get_cached( self, - state: &'tcx QueryStateImpl<'tcx, K, V, C>, - key: K, + state: &'tcx QueryStateImpl<'tcx, C>, + key: C::Key, // `on_hit` can be called while holding a lock to the query cache on_hit: OnHit, on_miss: OnMiss, ) -> R where - C: QueryCache, - OnHit: FnOnce(&V, DepNodeIndex) -> R, - OnMiss: FnOnce(K, QueryLookup<'tcx, K, C::Sharded>) -> R, + C: QueryCache, + OnHit: FnOnce(&C::Value, DepNodeIndex) -> R, + OnMiss: FnOnce(C::Key, QueryLookup<'tcx, C::Key, C::Sharded>) -> R, { state.cache.lookup( state, - QueryStateShard::::get_cache, + QueryStateShard::::get_cache, key, |value, index| { if unlikely!(self.prof.enabled()) { @@ -533,9 +535,9 @@ impl<'tcx> TyCtxt<'tcx> { self, span: Span, key: Q::Key, - lookup: QueryLookup<'tcx, Q::Key, >::Sharded>, + lookup: QueryLookup<'tcx, Q::Key, ::Sharded>, ) -> Q::Value { - let job = match JobOwnerImpl::try_start::(self, span, &key, lookup) { + let job = match JobOwner::try_start::(self, span, &key, lookup) { TryGetJob::NotYetStarted(job) => job, TryGetJob::Cycle(result) => return result, #[cfg(parallel_compiler)] @@ -696,7 +698,7 @@ impl<'tcx> TyCtxt<'tcx> { fn force_query_with_job + 'tcx>( self, key: Q::Key, - job: JobOwner<'tcx, Q>, + job: JobOwner<'tcx, Q::Cache>, dep_node: DepNode, ) -> (Q::Value, DepNodeIndex) { // If the following assertion triggers, it can have two reasons: @@ -795,7 +797,7 @@ impl<'tcx> TyCtxt<'tcx> { // Cache hit, do nothing }, |key, lookup| { - let job = match JobOwnerImpl::try_start::(self, span, &key, lookup) { + let job = match JobOwner::try_start::(self, span, &key, lookup) { TryGetJob::NotYetStarted(job) => job, TryGetJob::Cycle(_) => return, #[cfg(parallel_compiler)] diff --git a/src/librustc/ty/query/profiling_support.rs b/src/librustc/ty/query/profiling_support.rs index 290e37f2cf825..256bd86a3de38 100644 --- a/src/librustc/ty/query/profiling_support.rs +++ b/src/librustc/ty/query/profiling_support.rs @@ -157,14 +157,14 @@ where /// Allocate the self-profiling query strings for a single query cache. This /// method is called from `alloc_self_profile_query_strings` which knows all /// the queries via macro magic. -pub(super) fn alloc_self_profile_query_strings_for_query_cache<'tcx, K, V, C>( +pub(super) fn alloc_self_profile_query_strings_for_query_cache<'tcx, C>( tcx: TyCtxt<'tcx>, query_name: &'static str, - query_state: &QueryStateImpl<'tcx, K, V, C>, + query_state: &QueryStateImpl<'tcx, C>, string_cache: &mut QueryKeyStringCache, ) where - K: Debug + Clone, - C: QueryCache, + C: QueryCache, + C::Key: Debug + Clone, { tcx.prof.with_profiler(|profiler| { let event_id_builder = profiler.event_id_builder(); diff --git a/src/librustc/ty/query/stats.rs b/src/librustc/ty/query/stats.rs index c4c81cd5a13cf..20894a2a5d1e1 100644 --- a/src/librustc/ty/query/stats.rs +++ b/src/librustc/ty/query/stats.rs @@ -1,5 +1,5 @@ use crate::ty::query::caches::QueryCache; -use crate::ty::query::config::{QueryAccessors, QueryConfig}; +use crate::ty::query::config::QueryAccessors; use crate::ty::query::plumbing::QueryStateImpl; use crate::ty::query::queries; use crate::ty::TyCtxt; @@ -38,20 +38,17 @@ struct QueryStats { local_def_id_keys: Option, } -fn stats<'tcx, K, V, C: QueryCache>( - name: &'static str, - map: &QueryStateImpl<'tcx, K, V, C>, -) -> QueryStats { +fn stats<'tcx, C: QueryCache>(name: &'static str, map: &QueryStateImpl<'tcx, C>) -> QueryStats { let mut stats = QueryStats { name, #[cfg(debug_assertions)] cache_hits: map.cache_hits.load(Ordering::Relaxed), #[cfg(not(debug_assertions))] cache_hits: 0, - key_size: mem::size_of::(), - key_type: type_name::(), - value_size: mem::size_of::(), - value_type: type_name::(), + key_size: mem::size_of::(), + key_type: type_name::(), + value_size: mem::size_of::(), + value_type: type_name::(), entry_count: map.iter_results(|results| results.count()), local_def_id_keys: None, }; @@ -127,8 +124,6 @@ macro_rules! print_stats { $($( queries.push(stats::< - as QueryConfig<'_>>::Key, - as QueryConfig<'_>>::Value, as QueryAccessors<'_>>::Cache, >( stringify!($name), From 5557407fbb322dc1265ba3c05cd5474a20c2e1d3 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Sat, 7 Mar 2020 18:45:44 +0100 Subject: [PATCH 21/23] Remove QueryState type alias. --- src/librustc/ty/query/caches.rs | 6 ++--- src/librustc/ty/query/config.rs | 2 +- src/librustc/ty/query/plumbing.rs | 27 +++++++++++----------- src/librustc/ty/query/profiling_support.rs | 4 ++-- src/librustc/ty/query/stats.rs | 4 ++-- 5 files changed, 22 insertions(+), 21 deletions(-) diff --git a/src/librustc/ty/query/caches.rs b/src/librustc/ty/query/caches.rs index 71523ea39ca3e..a11b3bcba3ed3 100644 --- a/src/librustc/ty/query/caches.rs +++ b/src/librustc/ty/query/caches.rs @@ -1,5 +1,5 @@ use crate::dep_graph::DepNodeIndex; -use crate::ty::query::plumbing::{QueryLookup, QueryStateImpl, QueryStateShard}; +use crate::ty::query::plumbing::{QueryLookup, QueryState, QueryStateShard}; use crate::ty::TyCtxt; use rustc_data_structures::fx::FxHashMap; @@ -23,7 +23,7 @@ pub(crate) trait QueryCache: Default { /// to compute it. fn lookup<'tcx, R, GetCache, OnHit, OnMiss>( &self, - state: &'tcx QueryStateImpl<'tcx, Self>, + state: &'tcx QueryState<'tcx, Self>, get_cache: GetCache, key: Self::Key, // `on_hit` can be called while holding a lock to the query state shard. @@ -78,7 +78,7 @@ impl QueryCache for DefaultCache { #[inline(always)] fn lookup<'tcx, R, GetCache, OnHit, OnMiss>( &self, - state: &'tcx QueryStateImpl<'tcx, Self>, + state: &'tcx QueryState<'tcx, Self>, get_cache: GetCache, key: K, on_hit: OnHit, diff --git a/src/librustc/ty/query/config.rs b/src/librustc/ty/query/config.rs index 5b0653bd599e1..72a0fdf156726 100644 --- a/src/librustc/ty/query/config.rs +++ b/src/librustc/ty/query/config.rs @@ -33,7 +33,7 @@ pub(crate) trait QueryAccessors<'tcx>: QueryConfig<'tcx> { type Cache: QueryCache; // Don't use this method to access query results, instead use the methods on TyCtxt - fn query_state<'a>(tcx: TyCtxt<'tcx>) -> &'a QueryState<'tcx, Self>; + fn query_state<'a>(tcx: TyCtxt<'tcx>) -> &'a QueryState<'tcx, Self::Cache>; fn to_dep_node(tcx: TyCtxt<'tcx>, key: &Self::Key) -> DepNode; diff --git a/src/librustc/ty/query/plumbing.rs b/src/librustc/ty/query/plumbing.rs index c2740733977e8..442b3edcafe03 100644 --- a/src/librustc/ty/query/plumbing.rs +++ b/src/librustc/ty/query/plumbing.rs @@ -4,7 +4,7 @@ use crate::dep_graph::{DepKind, DepNode, DepNodeIndex, SerializedDepNodeIndex}; use crate::ty::query::caches::QueryCache; -use crate::ty::query::config::{QueryAccessors, QueryDescription}; +use crate::ty::query::config::QueryDescription; use crate::ty::query::job::{QueryInfo, QueryJob, QueryJobId, QueryJobInfo, QueryShardJobId}; use crate::ty::query::Query; use crate::ty::tls; @@ -49,16 +49,14 @@ impl<'tcx, K, C: Default> Default for QueryStateShard<'tcx, K, C> { } } -pub(crate) type QueryState<'tcx, Q> = QueryStateImpl<'tcx, >::Cache>; - -pub(crate) struct QueryStateImpl<'tcx, C: QueryCache> { +pub(crate) struct QueryState<'tcx, C: QueryCache> { pub(super) cache: C, pub(super) shards: Sharded>, #[cfg(debug_assertions)] pub(super) cache_hits: AtomicUsize, } -impl<'tcx, C: QueryCache> QueryStateImpl<'tcx, C> { +impl<'tcx, C: QueryCache> QueryState<'tcx, C> { pub(super) fn get_lookup( &'tcx self, key: &K2, @@ -86,7 +84,7 @@ pub(super) enum QueryResult<'tcx> { Poisoned, } -impl<'tcx, C: QueryCache> QueryStateImpl<'tcx, C> { +impl<'tcx, C: QueryCache> QueryState<'tcx, C> { pub fn iter_results( &self, f: impl for<'a> FnOnce( @@ -130,9 +128,9 @@ impl<'tcx, C: QueryCache> QueryStateImpl<'tcx, C> { } } -impl<'tcx, C: QueryCache> Default for QueryStateImpl<'tcx, C> { - fn default() -> QueryStateImpl<'tcx, C> { - QueryStateImpl { +impl<'tcx, C: QueryCache> Default for QueryState<'tcx, C> { + fn default() -> QueryState<'tcx, C> { + QueryState { cache: C::default(), shards: Default::default(), #[cfg(debug_assertions)] @@ -156,7 +154,7 @@ where C::Key: Eq + Hash + Clone + Debug, C::Value: Clone, { - state: &'tcx QueryStateImpl<'tcx, C>, + state: &'tcx QueryState<'tcx, C>, key: C::Key, id: QueryJobId, } @@ -482,7 +480,7 @@ impl<'tcx> TyCtxt<'tcx> { #[inline(always)] fn try_get_cached( self, - state: &'tcx QueryStateImpl<'tcx, C>, + state: &'tcx QueryState<'tcx, C>, key: C::Key, // `on_hit` can be called while holding a lock to the query cache on_hit: OnHit, @@ -979,7 +977,7 @@ macro_rules! define_queries_inner { type Cache = query_storage!([$($modifiers)*][$K, $V]); #[inline(always)] - fn query_state<'a>(tcx: TyCtxt<$tcx>) -> &'a QueryState<$tcx, Self> { + fn query_state<'a>(tcx: TyCtxt<$tcx>) -> &'a QueryState<$tcx, Self::Cache> { &tcx.queries.$name } @@ -1131,7 +1129,10 @@ macro_rules! define_queries_struct { providers: IndexVec>, fallback_extern_providers: Box>, - $($(#[$attr])* $name: QueryState<$tcx, queries::$name<$tcx>>,)* + $($(#[$attr])* $name: QueryState< + $tcx, + as QueryAccessors<'tcx>>::Cache, + >,)* } impl<$tcx> Queries<$tcx> { diff --git a/src/librustc/ty/query/profiling_support.rs b/src/librustc/ty/query/profiling_support.rs index 256bd86a3de38..58ace917786cf 100644 --- a/src/librustc/ty/query/profiling_support.rs +++ b/src/librustc/ty/query/profiling_support.rs @@ -1,7 +1,7 @@ use crate::hir::map::definitions::DefPathData; use crate::ty::context::TyCtxt; use crate::ty::query::caches::QueryCache; -use crate::ty::query::plumbing::QueryStateImpl; +use crate::ty::query::plumbing::QueryState; use measureme::{StringComponent, StringId}; use rustc_data_structures::fx::FxHashMap; use rustc_data_structures::profiling::SelfProfiler; @@ -160,7 +160,7 @@ where pub(super) fn alloc_self_profile_query_strings_for_query_cache<'tcx, C>( tcx: TyCtxt<'tcx>, query_name: &'static str, - query_state: &QueryStateImpl<'tcx, C>, + query_state: &QueryState<'tcx, C>, string_cache: &mut QueryKeyStringCache, ) where C: QueryCache, diff --git a/src/librustc/ty/query/stats.rs b/src/librustc/ty/query/stats.rs index 20894a2a5d1e1..527bb46c90888 100644 --- a/src/librustc/ty/query/stats.rs +++ b/src/librustc/ty/query/stats.rs @@ -1,6 +1,6 @@ use crate::ty::query::caches::QueryCache; use crate::ty::query::config::QueryAccessors; -use crate::ty::query::plumbing::QueryStateImpl; +use crate::ty::query::plumbing::QueryState; use crate::ty::query::queries; use crate::ty::TyCtxt; use rustc_hir::def_id::{DefId, LOCAL_CRATE}; @@ -38,7 +38,7 @@ struct QueryStats { local_def_id_keys: Option, } -fn stats<'tcx, C: QueryCache>(name: &'static str, map: &QueryStateImpl<'tcx, C>) -> QueryStats { +fn stats<'tcx, C: QueryCache>(name: &'static str, map: &QueryState<'tcx, C>) -> QueryStats { let mut stats = QueryStats { name, #[cfg(debug_assertions)] From 8aa13289b5fbfcbdf5afb19d15a41a15d8f8aa22 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Sun, 15 Mar 2020 09:44:23 +0100 Subject: [PATCH 22/23] Make stuff private. --- src/librustc/ty/query/mod.rs | 2 +- src/librustc/ty/query/plumbing.rs | 41 ++++++++++++++----------------- 2 files changed, 19 insertions(+), 24 deletions(-) diff --git a/src/librustc/ty/query/mod.rs b/src/librustc/ty/query/mod.rs index 54e80b4dc0b15..1279ae2059baa 100644 --- a/src/librustc/ty/query/mod.rs +++ b/src/librustc/ty/query/mod.rs @@ -62,7 +62,7 @@ use std::sync::Arc; #[macro_use] mod plumbing; -pub use self::plumbing::CycleError; +pub(crate) use self::plumbing::CycleError; use self::plumbing::*; mod stats; diff --git a/src/librustc/ty/query/plumbing.rs b/src/librustc/ty/query/plumbing.rs index 442b3edcafe03..0bfcae5fa2e66 100644 --- a/src/librustc/ty/query/plumbing.rs +++ b/src/librustc/ty/query/plumbing.rs @@ -30,11 +30,11 @@ use std::ptr; use std::sync::atomic::{AtomicUsize, Ordering}; pub(crate) struct QueryStateShard<'tcx, K, C> { - pub(super) cache: C, - pub(super) active: FxHashMap>, + cache: C, + active: FxHashMap>, /// Used to generate unique ids for active jobs. - pub(super) jobs: u32, + jobs: u32, } impl<'tcx, K, C> QueryStateShard<'tcx, K, C> { @@ -50,8 +50,8 @@ impl<'tcx, K, C: Default> Default for QueryStateShard<'tcx, K, C> { } pub(crate) struct QueryState<'tcx, C: QueryCache> { - pub(super) cache: C, - pub(super) shards: Sharded>, + cache: C, + shards: Sharded>, #[cfg(debug_assertions)] pub(super) cache_hits: AtomicUsize, } @@ -75,7 +75,7 @@ impl<'tcx, C: QueryCache> QueryState<'tcx, C> { } /// Indicates the state of a query for a given key in a query map. -pub(super) enum QueryResult<'tcx> { +enum QueryResult<'tcx> { /// An already executing query. The query job can be used to await for its completion. Started(QueryJob<'tcx>), @@ -85,7 +85,7 @@ pub(super) enum QueryResult<'tcx> { } impl<'tcx, C: QueryCache> QueryState<'tcx, C> { - pub fn iter_results( + pub(super) fn iter_results( &self, f: impl for<'a> FnOnce( Box + 'a>, @@ -93,7 +93,7 @@ impl<'tcx, C: QueryCache> QueryState<'tcx, C> { ) -> R { self.cache.iter(&self.shards, |shard| &mut shard.cache, f) } - pub fn all_inactive(&self) -> bool { + pub(super) fn all_inactive(&self) -> bool { let shards = self.shards.lock_shards(); shards.iter().all(|shard| shard.active.is_empty()) } @@ -142,13 +142,13 @@ impl<'tcx, C: QueryCache> Default for QueryState<'tcx, C> { /// Values used when checking a query cache which can be reused on a cache-miss to execute the query. pub(crate) struct QueryLookup<'tcx, K, C> { pub(super) key_hash: u64, - pub(super) shard: usize, + shard: usize, pub(super) lock: LockGuard<'tcx, QueryStateShard<'tcx, K, C>>, } /// A type representing the responsibility to execute the job in the `job` field. /// This will poison the relevant query if dropped. -pub(super) struct JobOwner<'tcx, C> +struct JobOwner<'tcx, C> where C: QueryCache, C::Key: Eq + Hash + Clone + Debug, @@ -174,7 +174,7 @@ where /// This function is inlined because that results in a noticeable speed-up /// for some compile-time benchmarks. #[inline(always)] - pub(super) fn try_start( + fn try_start( tcx: TyCtxt<'tcx>, span: Span, key: &C::Key, @@ -262,12 +262,7 @@ where /// Completes the query by updating the query cache with the `result`, /// signals the waiter and forgets the JobOwner, so it won't poison the query #[inline(always)] - pub(super) fn complete( - self, - tcx: TyCtxt<'tcx>, - result: &C::Value, - dep_node_index: DepNodeIndex, - ) { + fn complete(self, tcx: TyCtxt<'tcx>, result: &C::Value, dep_node_index: DepNodeIndex) { // We can move out of `self` here because we `mem::forget` it below let key = unsafe { ptr::read(&self.key) }; let state = self.state; @@ -327,14 +322,14 @@ where } #[derive(Clone)] -pub struct CycleError<'tcx> { +pub(crate) struct CycleError<'tcx> { /// The query and related span that uses the cycle. pub(super) usage: Option<(Span, Query<'tcx>)>, pub(super) cycle: Vec>, } /// The result of `try_start`. -pub(super) enum TryGetJob<'tcx, C: QueryCache> +enum TryGetJob<'tcx, C: QueryCache> where C::Key: Eq + Hash + Clone + Debug, C::Value: Clone, @@ -357,7 +352,7 @@ impl<'tcx> TyCtxt<'tcx> { /// new query job while it executes. It returns the diagnostics /// captured during execution and the actual result. #[inline(always)] - pub(super) fn start_query( + fn start_query( self, token: QueryJobId, diagnostics: Option<&Lock>>, @@ -529,7 +524,7 @@ impl<'tcx> TyCtxt<'tcx> { } #[inline(always)] - pub(super) fn try_execute_query + 'tcx>( + fn try_execute_query + 'tcx>( self, span: Span, key: Q::Key, @@ -1136,7 +1131,7 @@ macro_rules! define_queries_struct { } impl<$tcx> Queries<$tcx> { - pub fn new( + pub(crate) fn new( providers: IndexVec>, fallback_extern_providers: Providers<$tcx>, on_disk_cache: OnDiskCache<'tcx>, @@ -1149,7 +1144,7 @@ macro_rules! define_queries_struct { } } - pub fn try_collect_active_jobs( + pub(crate) fn try_collect_active_jobs( &self ) -> Option>> { let mut jobs = FxHashMap::default(); From 2c22da096878cab7fa1c1cabc45b3197b48f2915 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Thu, 12 Mar 2020 11:39:30 -0700 Subject: [PATCH 23/23] Fix abort-on-eprintln during process shutdown This commit fixes an issue where if `eprintln!` is used in a TLS destructor it can accidentally cause the process to abort. TLS destructors are executed after `main` returns on the main thread, and at this point we've also deinitialized global `Lazy` values like those which store the `Stderr` and `Stdout` internals. This means that despite handling TLS not being accessible in `eprintln!`, we will fail due to not being able to call `stderr()`. This means that we'll double-panic quickly because panicking also attempt to write to stderr. The fix here is to reimplement the global stderr handle to avoid the need for destruction. This avoids the need for `Lazy` as well as the hidden panic inside of the `stderr` function. Overall this should improve the robustness of printing errors and/or panics in weird situations, since the `stderr` accessor should be infallible in more situations. --- src/libstd/io/stdio.rs | 49 +++++++----- src/libstd/sys/cloudabi/mutex.rs | 8 +- src/libstd/sys/hermit/mutex.rs | 6 +- src/libstd/sys/sgx/mutex.rs | 2 +- src/libstd/sys/unix/mutex.rs | 4 +- src/libstd/sys/vxworks/mutex.rs | 4 +- src/libstd/sys/wasm/mutex.rs | 4 +- src/libstd/sys/wasm/mutex_atomics.rs | 4 +- src/libstd/sys/windows/mutex.rs | 6 +- src/libstd/sys_common/remutex.rs | 114 ++++++++++++--------------- src/test/ui/eprint-on-tls-drop.rs | 44 +++++++++++ 11 files changed, 145 insertions(+), 100 deletions(-) create mode 100644 src/test/ui/eprint-on-tls-drop.rs diff --git a/src/libstd/io/stdio.rs b/src/libstd/io/stdio.rs index d410faca30d9e..0ac928817184d 100644 --- a/src/libstd/io/stdio.rs +++ b/src/libstd/io/stdio.rs @@ -6,7 +6,7 @@ use crate::cell::RefCell; use crate::fmt; use crate::io::lazy::Lazy; use crate::io::{self, BufReader, Initializer, IoSlice, IoSliceMut, LineWriter}; -use crate::sync::{Arc, Mutex, MutexGuard}; +use crate::sync::{Arc, Mutex, MutexGuard, Once}; use crate::sys::stdio; use crate::sys_common::remutex::{ReentrantMutex, ReentrantMutexGuard}; use crate::thread::LocalKey; @@ -493,7 +493,11 @@ pub fn stdout() -> Stdout { Ok(stdout) => Maybe::Real(stdout), _ => Maybe::Fake, }; - Arc::new(ReentrantMutex::new(RefCell::new(LineWriter::new(stdout)))) + unsafe { + let ret = Arc::new(ReentrantMutex::new(RefCell::new(LineWriter::new(stdout)))); + ret.init(); + return ret; + } } } @@ -520,7 +524,7 @@ impl Stdout { /// ``` #[stable(feature = "rust1", since = "1.0.0")] pub fn lock(&self) -> StdoutLock<'_> { - StdoutLock { inner: self.inner.lock().unwrap_or_else(|e| e.into_inner()) } + StdoutLock { inner: self.inner.lock() } } } @@ -581,7 +585,7 @@ impl fmt::Debug for StdoutLock<'_> { /// an error. #[stable(feature = "rust1", since = "1.0.0")] pub struct Stderr { - inner: Arc>>>, + inner: &'static ReentrantMutex>>, } /// A locked reference to the `Stderr` handle. @@ -639,19 +643,28 @@ pub struct StderrLock<'a> { /// ``` #[stable(feature = "rust1", since = "1.0.0")] pub fn stderr() -> Stderr { - static INSTANCE: Lazy>>> = Lazy::new(); - return Stderr { - inner: unsafe { INSTANCE.get(stderr_init).expect("cannot access stderr during shutdown") }, - }; - - fn stderr_init() -> Arc>>> { - // This must not reentrantly access `INSTANCE` - let stderr = match stderr_raw() { - Ok(stderr) => Maybe::Real(stderr), - _ => Maybe::Fake, - }; - Arc::new(ReentrantMutex::new(RefCell::new(stderr))) - } + // Note that unlike `stdout()` we don't use `Lazy` here which registers a + // destructor. Stderr is not buffered nor does the `stderr_raw` type consume + // any owned resources, so there's no need to run any destructors at some + // point in the future. + // + // This has the added benefit of allowing `stderr` to be usable during + // process shutdown as well! + static INSTANCE: ReentrantMutex>> = + unsafe { ReentrantMutex::new(RefCell::new(Maybe::Fake)) }; + + // When accessing stderr we need one-time initialization of the reentrant + // mutex, followed by one-time detection of whether we actually have a + // stderr handle or not. Afterwards we can just always use the now-filled-in + // `INSTANCE` value. + static INIT: Once = Once::new(); + INIT.call_once(|| unsafe { + INSTANCE.init(); + if let Ok(stderr) = stderr_raw() { + *INSTANCE.lock().borrow_mut() = Maybe::Real(stderr); + } + }); + return Stderr { inner: &INSTANCE }; } impl Stderr { @@ -677,7 +690,7 @@ impl Stderr { /// ``` #[stable(feature = "rust1", since = "1.0.0")] pub fn lock(&self) -> StderrLock<'_> { - StderrLock { inner: self.inner.lock().unwrap_or_else(|e| e.into_inner()) } + StderrLock { inner: self.inner.lock() } } } diff --git a/src/libstd/sys/cloudabi/mutex.rs b/src/libstd/sys/cloudabi/mutex.rs index 4aa25e2505271..580ab0e8ad863 100644 --- a/src/libstd/sys/cloudabi/mutex.rs +++ b/src/libstd/sys/cloudabi/mutex.rs @@ -53,16 +53,16 @@ pub struct ReentrantMutex { } impl ReentrantMutex { - pub unsafe fn uninitialized() -> ReentrantMutex { + pub const unsafe fn uninitialized() -> ReentrantMutex { ReentrantMutex { lock: UnsafeCell::new(MaybeUninit::uninit()), recursion: UnsafeCell::new(MaybeUninit::uninit()), } } - pub unsafe fn init(&mut self) { - self.lock = UnsafeCell::new(MaybeUninit::new(AtomicU32::new(abi::LOCK_UNLOCKED.0))); - self.recursion = UnsafeCell::new(MaybeUninit::new(0)); + pub unsafe fn init(&self) { + *self.lock.get() = MaybeUninit::new(AtomicU32::new(abi::LOCK_UNLOCKED.0)); + *self.recursion.get() = MaybeUninit::new(0); } pub unsafe fn try_lock(&self) -> bool { diff --git a/src/libstd/sys/hermit/mutex.rs b/src/libstd/sys/hermit/mutex.rs index b5c75f738d228..3d4813209cbc4 100644 --- a/src/libstd/sys/hermit/mutex.rs +++ b/src/libstd/sys/hermit/mutex.rs @@ -46,13 +46,13 @@ pub struct ReentrantMutex { } impl ReentrantMutex { - pub unsafe fn uninitialized() -> ReentrantMutex { + pub const unsafe fn uninitialized() -> ReentrantMutex { ReentrantMutex { inner: ptr::null() } } #[inline] - pub unsafe fn init(&mut self) { - let _ = abi::recmutex_init(&mut self.inner as *mut *const c_void); + pub unsafe fn init(&self) { + let _ = abi::recmutex_init(&self.inner as *const *const c_void as *mut _); } #[inline] diff --git a/src/libstd/sys/sgx/mutex.rs b/src/libstd/sys/sgx/mutex.rs index eebbea1b285ba..4911c2f538769 100644 --- a/src/libstd/sys/sgx/mutex.rs +++ b/src/libstd/sys/sgx/mutex.rs @@ -75,7 +75,7 @@ impl ReentrantMutex { } #[inline] - pub unsafe fn init(&mut self) {} + pub unsafe fn init(&self) {} #[inline] pub unsafe fn lock(&self) { diff --git a/src/libstd/sys/unix/mutex.rs b/src/libstd/sys/unix/mutex.rs index b38375a2e03c5..103d87e3d2f91 100644 --- a/src/libstd/sys/unix/mutex.rs +++ b/src/libstd/sys/unix/mutex.rs @@ -92,11 +92,11 @@ unsafe impl Send for ReentrantMutex {} unsafe impl Sync for ReentrantMutex {} impl ReentrantMutex { - pub unsafe fn uninitialized() -> ReentrantMutex { + pub const unsafe fn uninitialized() -> ReentrantMutex { ReentrantMutex { inner: UnsafeCell::new(libc::PTHREAD_MUTEX_INITIALIZER) } } - pub unsafe fn init(&mut self) { + pub unsafe fn init(&self) { let mut attr = MaybeUninit::::uninit(); let result = libc::pthread_mutexattr_init(attr.as_mut_ptr()); debug_assert_eq!(result, 0); diff --git a/src/libstd/sys/vxworks/mutex.rs b/src/libstd/sys/vxworks/mutex.rs index b38375a2e03c5..103d87e3d2f91 100644 --- a/src/libstd/sys/vxworks/mutex.rs +++ b/src/libstd/sys/vxworks/mutex.rs @@ -92,11 +92,11 @@ unsafe impl Send for ReentrantMutex {} unsafe impl Sync for ReentrantMutex {} impl ReentrantMutex { - pub unsafe fn uninitialized() -> ReentrantMutex { + pub const unsafe fn uninitialized() -> ReentrantMutex { ReentrantMutex { inner: UnsafeCell::new(libc::PTHREAD_MUTEX_INITIALIZER) } } - pub unsafe fn init(&mut self) { + pub unsafe fn init(&self) { let mut attr = MaybeUninit::::uninit(); let result = libc::pthread_mutexattr_init(attr.as_mut_ptr()); debug_assert_eq!(result, 0); diff --git a/src/libstd/sys/wasm/mutex.rs b/src/libstd/sys/wasm/mutex.rs index 07238d087308f..7aaf1b3a343b6 100644 --- a/src/libstd/sys/wasm/mutex.rs +++ b/src/libstd/sys/wasm/mutex.rs @@ -47,11 +47,11 @@ impl Mutex { pub struct ReentrantMutex {} impl ReentrantMutex { - pub unsafe fn uninitialized() -> ReentrantMutex { + pub const unsafe fn uninitialized() -> ReentrantMutex { ReentrantMutex {} } - pub unsafe fn init(&mut self) {} + pub unsafe fn init(&self) {} pub unsafe fn lock(&self) {} diff --git a/src/libstd/sys/wasm/mutex_atomics.rs b/src/libstd/sys/wasm/mutex_atomics.rs index 90c628a19c22e..268a53bb5641c 100644 --- a/src/libstd/sys/wasm/mutex_atomics.rs +++ b/src/libstd/sys/wasm/mutex_atomics.rs @@ -80,11 +80,11 @@ unsafe impl Sync for ReentrantMutex {} // released when this recursion counter reaches 0. impl ReentrantMutex { - pub unsafe fn uninitialized() -> ReentrantMutex { + pub const unsafe fn uninitialized() -> ReentrantMutex { ReentrantMutex { owner: AtomicU32::new(0), recursions: UnsafeCell::new(0) } } - pub unsafe fn init(&mut self) { + pub unsafe fn init(&self) { // nothing to do... } diff --git a/src/libstd/sys/windows/mutex.rs b/src/libstd/sys/windows/mutex.rs index 281eb294c65d8..63dfc640908e9 100644 --- a/src/libstd/sys/windows/mutex.rs +++ b/src/libstd/sys/windows/mutex.rs @@ -109,7 +109,7 @@ impl Mutex { 0 => {} n => return n as *mut _, } - let mut re = box ReentrantMutex::uninitialized(); + let re = box ReentrantMutex::uninitialized(); re.init(); let re = Box::into_raw(re); match self.lock.compare_and_swap(0, re as usize, Ordering::SeqCst) { @@ -157,11 +157,11 @@ unsafe impl Send for ReentrantMutex {} unsafe impl Sync for ReentrantMutex {} impl ReentrantMutex { - pub fn uninitialized() -> ReentrantMutex { + pub const fn uninitialized() -> ReentrantMutex { ReentrantMutex { inner: UnsafeCell::new(MaybeUninit::uninit()) } } - pub unsafe fn init(&mut self) { + pub unsafe fn init(&self) { c::InitializeCriticalSection((&mut *self.inner.get()).as_mut_ptr()); } diff --git a/src/libstd/sys_common/remutex.rs b/src/libstd/sys_common/remutex.rs index a1ad44a3666ed..4f19bbc467f33 100644 --- a/src/libstd/sys_common/remutex.rs +++ b/src/libstd/sys_common/remutex.rs @@ -3,7 +3,6 @@ use crate::marker; use crate::ops::Deref; use crate::panic::{RefUnwindSafe, UnwindSafe}; use crate::sys::mutex as sys; -use crate::sys_common::poison::{self, LockResult, TryLockError, TryLockResult}; /// A re-entrant mutual exclusion /// @@ -11,8 +10,7 @@ use crate::sys_common::poison::{self, LockResult, TryLockError, TryLockResult}; /// available. The thread which has already locked the mutex can lock it /// multiple times without blocking, preventing a common source of deadlocks. pub struct ReentrantMutex { - inner: Box, - poison: poison::Flag, + inner: sys::ReentrantMutex, data: T, } @@ -39,23 +37,30 @@ pub struct ReentrantMutexGuard<'a, T: 'a> { // funny underscores due to how Deref currently works (it disregards field // privacy). __lock: &'a ReentrantMutex, - __poison: poison::Guard, } impl !marker::Send for ReentrantMutexGuard<'_, T> {} impl ReentrantMutex { /// Creates a new reentrant mutex in an unlocked state. - pub fn new(t: T) -> ReentrantMutex { - unsafe { - let mut mutex = ReentrantMutex { - inner: box sys::ReentrantMutex::uninitialized(), - poison: poison::Flag::new(), - data: t, - }; - mutex.inner.init(); - mutex - } + /// + /// # Unsafety + /// + /// This function is unsafe because it is required that `init` is called + /// once this mutex is in its final resting place, and only then are the + /// lock/unlock methods safe. + pub const unsafe fn new(t: T) -> ReentrantMutex { + ReentrantMutex { inner: sys::ReentrantMutex::uninitialized(), data: t } + } + + /// Initializes this mutex so it's ready for use. + /// + /// # Unsafety + /// + /// Unsafe to call more than once, and must be called after this will no + /// longer move in memory. + pub unsafe fn init(&self) { + self.inner.init(); } /// Acquires a mutex, blocking the current thread until it is able to do so. @@ -70,7 +75,7 @@ impl ReentrantMutex { /// If another user of this mutex panicked while holding the mutex, then /// this call will return failure if the mutex would otherwise be /// acquired. - pub fn lock(&self) -> LockResult> { + pub fn lock(&self) -> ReentrantMutexGuard<'_, T> { unsafe { self.inner.lock() } ReentrantMutexGuard::new(&self) } @@ -87,12 +92,8 @@ impl ReentrantMutex { /// If another user of this mutex panicked while holding the mutex, then /// this call will return failure if the mutex would otherwise be /// acquired. - pub fn try_lock(&self) -> TryLockResult> { - if unsafe { self.inner.try_lock() } { - Ok(ReentrantMutexGuard::new(&self)?) - } else { - Err(TryLockError::WouldBlock) - } + pub fn try_lock(&self) -> Option> { + if unsafe { self.inner.try_lock() } { Some(ReentrantMutexGuard::new(&self)) } else { None } } } @@ -108,11 +109,8 @@ impl Drop for ReentrantMutex { impl fmt::Debug for ReentrantMutex { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self.try_lock() { - Ok(guard) => f.debug_struct("ReentrantMutex").field("data", &*guard).finish(), - Err(TryLockError::Poisoned(err)) => { - f.debug_struct("ReentrantMutex").field("data", &**err.get_ref()).finish() - } - Err(TryLockError::WouldBlock) => { + Some(guard) => f.debug_struct("ReentrantMutex").field("data", &*guard).finish(), + None => { struct LockedPlaceholder; impl fmt::Debug for LockedPlaceholder { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { @@ -127,11 +125,8 @@ impl fmt::Debug for ReentrantMutex { } impl<'mutex, T> ReentrantMutexGuard<'mutex, T> { - fn new(lock: &'mutex ReentrantMutex) -> LockResult> { - poison::map_result(lock.poison.borrow(), |guard| ReentrantMutexGuard { - __lock: lock, - __poison: guard, - }) + fn new(lock: &'mutex ReentrantMutex) -> ReentrantMutexGuard<'mutex, T> { + ReentrantMutexGuard { __lock: lock } } } @@ -147,7 +142,6 @@ impl Drop for ReentrantMutexGuard<'_, T> { #[inline] fn drop(&mut self) { unsafe { - self.__lock.poison.done(&self.__poison); self.__lock.inner.unlock(); } } @@ -162,13 +156,17 @@ mod tests { #[test] fn smoke() { - let m = ReentrantMutex::new(()); + let m = unsafe { + let m = ReentrantMutex::new(()); + m.init(); + m + }; { - let a = m.lock().unwrap(); + let a = m.lock(); { - let b = m.lock().unwrap(); + let b = m.lock(); { - let c = m.lock().unwrap(); + let c = m.lock(); assert_eq!(*c, ()); } assert_eq!(*b, ()); @@ -179,15 +177,19 @@ mod tests { #[test] fn is_mutex() { - let m = Arc::new(ReentrantMutex::new(RefCell::new(0))); + let m = unsafe { + let m = Arc::new(ReentrantMutex::new(RefCell::new(0))); + m.init(); + m + }; let m2 = m.clone(); - let lock = m.lock().unwrap(); + let lock = m.lock(); let child = thread::spawn(move || { - let lock = m2.lock().unwrap(); + let lock = m2.lock(); assert_eq!(*lock.borrow(), 4950); }); for i in 0..100 { - let lock = m.lock().unwrap(); + let lock = m.lock(); *lock.borrow_mut() += i; } drop(lock); @@ -196,17 +198,21 @@ mod tests { #[test] fn trylock_works() { - let m = Arc::new(ReentrantMutex::new(())); + let m = unsafe { + let m = Arc::new(ReentrantMutex::new(())); + m.init(); + m + }; let m2 = m.clone(); - let _lock = m.try_lock().unwrap(); - let _lock2 = m.try_lock().unwrap(); + let _lock = m.try_lock(); + let _lock2 = m.try_lock(); thread::spawn(move || { let lock = m2.try_lock(); - assert!(lock.is_err()); + assert!(lock.is_none()); }) .join() .unwrap(); - let _lock3 = m.try_lock().unwrap(); + let _lock3 = m.try_lock(); } pub struct Answer<'a>(pub ReentrantMutexGuard<'a, RefCell>); @@ -215,22 +221,4 @@ mod tests { *self.0.borrow_mut() = 42; } } - - #[test] - fn poison_works() { - let m = Arc::new(ReentrantMutex::new(RefCell::new(0))); - let mc = m.clone(); - let result = thread::spawn(move || { - let lock = mc.lock().unwrap(); - *lock.borrow_mut() = 1; - let lock2 = mc.lock().unwrap(); - *lock.borrow_mut() = 2; - let _answer = Answer(lock2); - panic!("What the answer to my lifetimes dilemma is?"); - }) - .join(); - assert!(result.is_err()); - let r = m.lock().err().unwrap().into_inner(); - assert_eq!(*r.borrow(), 42); - } } diff --git a/src/test/ui/eprint-on-tls-drop.rs b/src/test/ui/eprint-on-tls-drop.rs new file mode 100644 index 0000000000000..77b04625bf91d --- /dev/null +++ b/src/test/ui/eprint-on-tls-drop.rs @@ -0,0 +1,44 @@ +// run-pass +// ignore-emscripten no processes + +use std::cell::RefCell; +use std::env; +use std::process::Command; + +fn main() { + let name = "YOU_ARE_THE_TEST"; + if env::var(name).is_ok() { + TLS.with(|f| f.borrow().ensure()); + } else { + let me = env::current_exe().unwrap(); + let output = Command::new(&me).env(name, "1").output().unwrap(); + println!("{:?}", output); + assert!(output.status.success()); + let stderr = String::from_utf8(output.stderr).unwrap(); + assert!(stderr.contains("hello new\n")); + assert!(stderr.contains("hello drop\n")); + } +} + +struct Stuff { + _x: usize, +} + +impl Stuff { + fn new() -> Self { + eprintln!("hello new"); + Self { _x: 0 } + } + + fn ensure(&self) {} +} + +impl Drop for Stuff { + fn drop(&mut self) { + eprintln!("hello drop"); + } +} + +thread_local! { + static TLS: RefCell = RefCell::new(Stuff::new()); +}