From d428f0a920b2a555ee941fefe04fdc7f45dba90b Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Wed, 5 Jun 2024 15:33:01 +0000 Subject: [PATCH] Also store the DefPathHash as part of a DefId creation side effect, so we can reliably recreate the same `DefId` over and over again --- compiler/rustc_hir/src/definitions.rs | 8 ++- compiler/rustc_middle/src/ty/context.rs | 51 ++++++++++++------- compiler/rustc_query_impl/src/plumbing.rs | 7 +-- .../rustc_query_system/src/dep_graph/graph.rs | 4 +- compiler/rustc_query_system/src/query/mod.rs | 2 + 5 files changed, 48 insertions(+), 24 deletions(-) diff --git a/compiler/rustc_hir/src/definitions.rs b/compiler/rustc_hir/src/definitions.rs index 35833e258d558..4ffec66c6de20 100644 --- a/compiler/rustc_hir/src/definitions.rs +++ b/compiler/rustc_hir/src/definitions.rs @@ -344,7 +344,11 @@ impl Definitions { } /// Adds a definition with a parent definition. - pub fn create_def(&mut self, parent: LocalDefId, data: DefPathData) -> LocalDefId { + pub fn create_def( + &mut self, + parent: LocalDefId, + data: DefPathData, + ) -> (LocalDefId, DefPathHash) { // We can't use `Debug` implementation for `LocalDefId` here, since it tries to acquire a // reference to `Definitions` and we're already holding a mutable reference. debug!( @@ -373,7 +377,7 @@ impl Definitions { debug!("create_def: after disambiguation, key = {:?}", key); // Create the definition. - LocalDefId { local_def_index: self.table.allocate(key, def_path_hash) } + (LocalDefId { local_def_index: self.table.allocate(key, def_path_hash) }, def_path_hash) } #[inline(always)] diff --git a/compiler/rustc_middle/src/ty/context.rs b/compiler/rustc_middle/src/ty/context.rs index be680812ae84a..2dbba58bc152b 100644 --- a/compiler/rustc_middle/src/ty/context.rs +++ b/compiler/rustc_middle/src/ty/context.rs @@ -4,6 +4,7 @@ pub mod tls; +use rustc_query_system::query::DefIdInfo; pub use rustc_type_ir::lift::Lift; use crate::arena::Arena; @@ -59,7 +60,6 @@ use rustc_index::IndexVec; use rustc_macros::{HashStable, TyDecodable, TyEncodable}; use rustc_query_system::dep_graph::{DepNodeIndex, TaskDepsRef}; use rustc_query_system::ich::StableHashingContext; -use rustc_query_system::query::DefIdInfo; use rustc_serialize::opaque::{FileEncodeResult, FileEncoder}; use rustc_session::config::CrateType; use rustc_session::cstore::{CrateStoreDyn, Untracked}; @@ -73,7 +73,7 @@ use rustc_target::spec::abi; use rustc_type_ir::TyKind::*; use rustc_type_ir::WithCachedTypeInfo; use rustc_type_ir::{CollectAndApply, Interner, TypeFlags}; -use tracing::{debug, instrument}; +use tracing::{debug, instrument, trace}; use std::assert_matches::assert_matches; use std::borrow::Borrow; @@ -1330,6 +1330,7 @@ impl<'tcx> TyCtxtAt<'tcx> { impl<'tcx> TyCtxt<'tcx> { /// `tcx`-dependent operations performed for every created definition. + #[instrument(level = "trace", skip(self))] pub fn create_def( self, parent: LocalDefId, @@ -1337,7 +1338,7 @@ impl<'tcx> TyCtxt<'tcx> { def_kind: DefKind, ) -> TyCtxtFeed<'tcx, LocalDefId> { let data = def_kind.def_path_data(name); - // The following call has the side effect of modifying the tables inside `definitions`. + // The following create_def calls have the side effect of modifying the tables inside `definitions`. // These very tables are relied on by the incr. comp. engine to decode DepNodes and to // decode the on-disk cache. // @@ -1350,31 +1351,47 @@ impl<'tcx> TyCtxt<'tcx> { // This is fine because: // - those queries are `eval_always` so we won't miss their result changing; // - this write will have happened before these queries are called. - let def_id = self.untracked.definitions.write().create_def(parent, data); - - // This function modifies `self.definitions` using a side-effect. - // We need to ensure that these side effects are re-run by the incr. comp. engine. - tls::with_context(|icx| { + let def_id = tls::with_context(|icx| { match icx.task_deps { // Always gets rerun anyway, so nothing to replay - TaskDepsRef::EvalAlways => {} + TaskDepsRef::EvalAlways => { + let def_id = self.untracked.definitions.write().create_def(parent, data).0; + trace!(?def_id, "eval always"); + def_id + } // Top-level queries like the resolver get rerun every time anyway - TaskDepsRef::Ignore => {} + TaskDepsRef::Ignore => { + let def_id = self.untracked.definitions.write().create_def(parent, data).0; + trace!(?def_id, "ignore"); + def_id + } TaskDepsRef::Forbid => bug!( "cannot create definition {parent:?}, {name:?}, {def_kind:?} without being able to register task dependencies" ), TaskDepsRef::Allow(_) => { - icx.side_effects - .as_ref() - .unwrap() - .lock() - .definitions - .push(DefIdInfo { parent, data }); + let (def_id, hash) = + self.untracked.definitions.write().create_def(parent, data); + trace!(?def_id, "record side effects"); + + icx.side_effects.as_ref().unwrap().lock().definitions.push(DefIdInfo { + parent, + data, + hash, + }); + def_id } TaskDepsRef::Replay { prev_side_effects, created_def_ids } => { + trace!(?created_def_ids, "replay side effects"); + trace!("num_defs : {}", prev_side_effects.definitions.len()); let index = created_def_ids.fetch_add(1, std::sync::atomic::Ordering::Relaxed); let prev_info = &prev_side_effects.definitions[index]; - assert_eq!(*prev_info, DefIdInfo { parent, data }); + let def_id = self.untracked.definitions.read().local_def_path_hash_to_def_id( + prev_info.hash, + &"should have already recreated def id in try_mark_green", + ); + assert_eq!(prev_info.data, data); + assert_eq!(prev_info.parent, parent); + def_id } } }); diff --git a/compiler/rustc_query_impl/src/plumbing.rs b/compiler/rustc_query_impl/src/plumbing.rs index a8fde21764971..f28f9564aad2b 100644 --- a/compiler/rustc_query_impl/src/plumbing.rs +++ b/compiler/rustc_query_impl/src/plumbing.rs @@ -171,7 +171,7 @@ impl QueryContext for QueryCtxt<'_> { }); } - #[tracing::instrument(level = "trace", skip(self))] + #[tracing::instrument(level = "trace", skip(self, side_effects))] fn apply_side_effects(self, side_effects: QuerySideEffects) { let dcx = self.dep_context().sess().dcx(); let QuerySideEffects { diagnostics, definitions } = side_effects; @@ -180,8 +180,9 @@ impl QueryContext for QueryCtxt<'_> { dcx.emit_diagnostic(diagnostic); } - for DefIdInfo { parent, data } in definitions { - self.tcx.untracked().definitions.write().create_def(parent, data); + for DefIdInfo { parent, data, hash } in definitions { + let (_def_id, h) = self.tcx.untracked().definitions.write().create_def(parent, data); + debug_assert_eq!(h, hash); } } } diff --git a/compiler/rustc_query_system/src/dep_graph/graph.rs b/compiler/rustc_query_system/src/dep_graph/graph.rs index bbfdc5e77cfae..84fbb78034e60 100644 --- a/compiler/rustc_query_system/src/dep_graph/graph.rs +++ b/compiler/rustc_query_system/src/dep_graph/graph.rs @@ -908,7 +908,7 @@ impl DepGraphData { Some(dep_node_index) } - /// Atomically emits some loaded diagnostics. + /// Atomically emits some loaded side effects. /// This may be called concurrently on multiple threads for the same dep node. #[cold] #[inline(never)] @@ -924,7 +924,7 @@ impl DepGraphData { // We were the first to insert the node in the set so this thread // must process side effects - // Promote the previous diagnostics to the current session. + // Promote the previous side effects to the current session. qcx.store_side_effects(dep_node_index, side_effects.clone()); qcx.apply_side_effects(side_effects); diff --git a/compiler/rustc_query_system/src/query/mod.rs b/compiler/rustc_query_system/src/query/mod.rs index 68c67155713dc..6a78dee85a064 100644 --- a/compiler/rustc_query_system/src/query/mod.rs +++ b/compiler/rustc_query_system/src/query/mod.rs @@ -20,6 +20,7 @@ use rustc_data_structures::stable_hasher::Hash64; use rustc_data_structures::sync::Lock; use rustc_errors::DiagInner; use rustc_hir::def::DefKind; +use rustc_hir::def_id::DefPathHash; use rustc_macros::{Decodable, Encodable}; use rustc_span::def_id::{DefId, LocalDefId}; use rustc_span::Span; @@ -97,6 +98,7 @@ pub struct QuerySideEffects { pub struct DefIdInfo { pub parent: LocalDefId, pub data: rustc_hir::definitions::DefPathData, + pub hash: DefPathHash, } impl QuerySideEffects {