From 5768c165af0673d691b4abb42dbe1b0dccd5be0e Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Wed, 6 Sep 2023 10:46:54 +0000 Subject: [PATCH 1/9] Make the panic info more useful --- src/tools/compiletest/src/runtest.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/tools/compiletest/src/runtest.rs b/src/tools/compiletest/src/runtest.rs index 9bd0002a3d9a6..3464e1893d037 100644 --- a/src/tools/compiletest/src/runtest.rs +++ b/src/tools/compiletest/src/runtest.rs @@ -2469,6 +2469,7 @@ impl<'test> TestCx<'test> { } } + #[track_caller] fn fatal(&self, err: &str) -> ! { self.error(err); error!("fatal error, panic: {:?}", err); From 28ee46334b3c84d7aa8a76cdf8e502998e6d5d1f Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Wed, 6 Sep 2023 10:51:03 +0000 Subject: [PATCH 2/9] Add regression test --- tests/incremental/rpitit-feeding.rs | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 tests/incremental/rpitit-feeding.rs diff --git a/tests/incremental/rpitit-feeding.rs b/tests/incremental/rpitit-feeding.rs new file mode 100644 index 0000000000000..0786879808f57 --- /dev/null +++ b/tests/incremental/rpitit-feeding.rs @@ -0,0 +1,27 @@ +//@ revisions: cpass cpass2 cpass3 + +// This test checks that creating a new `DefId` from within a query `A` +// recreates that `DefId` before reexecuting queries that depend on query `A`. +// Otherwise we'd end up referring to a `DefId` that doesn't exist. +// At present this is handled by always marking all queries as red if they create +// a new `DefId` and thus subsequently rerunning the query. + +trait Foo { + fn foo() -> impl Sized; +} + +#[cfg(any(cpass, cpass3))] +impl Foo for String { + fn foo() -> i32 { + 22 + } +} + +#[cfg(cpass2)] +impl Foo for String { + fn foo() -> u32 { + 22 + } +} + +fn main() {} From 490d015e90ddce596af0fc3840824430c0e1d91e Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Thu, 7 Sep 2023 08:41:11 +0000 Subject: [PATCH 3/9] Generalize query side effect handling to make future additions easier --- compiler/rustc_interface/src/callbacks.rs | 6 ++++-- compiler/rustc_middle/src/ty/context/tls.rs | 7 +++---- compiler/rustc_query_impl/src/plumbing.rs | 16 ++++++++++++---- .../rustc_query_system/src/dep_graph/graph.rs | 6 +----- compiler/rustc_query_system/src/query/mod.rs | 7 +++++-- .../rustc_query_system/src/query/plumbing.rs | 7 +++---- 6 files changed, 28 insertions(+), 21 deletions(-) diff --git a/compiler/rustc_interface/src/callbacks.rs b/compiler/rustc_interface/src/callbacks.rs index a27f73789cdef..34ef6a95c5b7b 100644 --- a/compiler/rustc_interface/src/callbacks.rs +++ b/compiler/rustc_interface/src/callbacks.rs @@ -32,8 +32,10 @@ fn track_span_parent(def_id: rustc_span::def_id::LocalDefId) { fn track_diagnostic(diagnostic: DiagInner, f: &mut dyn FnMut(DiagInner) -> R) -> R { tls::with_context_opt(|icx| { if let Some(icx) = icx { - if let Some(diagnostics) = icx.diagnostics { - diagnostics.lock().extend(Some(diagnostic.clone())); + if let Some(side_effects) = icx.side_effects { + let mut side_effects = side_effects.lock(); + side_effects.diagnostics.push(diagnostic.clone()); + std::mem::drop(side_effects); } // Diagnostics are tracked, we can ignore the dependency. diff --git a/compiler/rustc_middle/src/ty/context/tls.rs b/compiler/rustc_middle/src/ty/context/tls.rs index 5e256dc8d26e2..93cbb7d52a389 100644 --- a/compiler/rustc_middle/src/ty/context/tls.rs +++ b/compiler/rustc_middle/src/ty/context/tls.rs @@ -3,12 +3,11 @@ use super::{GlobalCtxt, TyCtxt}; use crate::dep_graph::TaskDepsRef; use crate::query::plumbing::QueryJobId; use rustc_data_structures::sync::{self, Lock}; -use rustc_errors::DiagInner; +use rustc_query_system::query::QuerySideEffects; #[cfg(not(parallel_compiler))] use std::cell::Cell; use std::mem; use std::ptr; -use thin_vec::ThinVec; /// This is the implicit state of rustc. It contains the current /// `TyCtxt` and query. It is updated when creating a local interner or @@ -26,7 +25,7 @@ pub struct ImplicitCtxt<'a, 'tcx> { /// Where to store diagnostics for the current query job, if any. /// This is updated by `JobOwner::start` in `ty::query::plumbing` when executing a query. - pub diagnostics: Option<&'a Lock>>, + pub side_effects: Option<&'a Lock>, /// Used to prevent queries from calling too deeply. pub query_depth: usize, @@ -42,7 +41,7 @@ impl<'a, 'tcx> ImplicitCtxt<'a, 'tcx> { ImplicitCtxt { tcx, query: None, - diagnostics: None, + side_effects: None, query_depth: 0, task_deps: TaskDepsRef::Ignore, } diff --git a/compiler/rustc_query_impl/src/plumbing.rs b/compiler/rustc_query_impl/src/plumbing.rs index 86531bd95900a..e88154baeb79c 100644 --- a/compiler/rustc_query_impl/src/plumbing.rs +++ b/compiler/rustc_query_impl/src/plumbing.rs @@ -6,7 +6,6 @@ use crate::QueryConfigRestored; use rustc_data_structures::stable_hasher::{Hash64, HashStable, StableHasher}; use rustc_data_structures::sync::Lock; use rustc_data_structures::unord::UnordMap; -use rustc_errors::DiagInner; use rustc_index::Idx; use rustc_middle::bug; use rustc_middle::dep_graph::dep_kinds; @@ -31,7 +30,6 @@ use rustc_serialize::Encodable; use rustc_session::Limit; use rustc_span::def_id::LOCAL_CRATE; use std::num::NonZero; -use thin_vec::ThinVec; #[derive(Copy, Clone)] pub struct QueryCtxt<'tcx> { @@ -125,7 +123,7 @@ impl QueryContext for QueryCtxt<'_> { self, token: QueryJobId, depth_limit: bool, - diagnostics: Option<&Lock>>, + side_effects: Option<&Lock>, compute: impl FnOnce() -> R, ) -> R { // The `TyCtxt` stored in TLS has the same global interner lifetime @@ -140,7 +138,7 @@ impl QueryContext for QueryCtxt<'_> { let new_icx = ImplicitCtxt { tcx: self.tcx, query: Some(token), - diagnostics, + side_effects, query_depth: current_icx.query_depth + depth_limit as usize, task_deps: current_icx.task_deps, }; @@ -172,6 +170,16 @@ impl QueryContext for QueryCtxt<'_> { crate_name: self.crate_name(LOCAL_CRATE), }); } + + #[tracing::instrument(level = "trace", skip(self))] + fn apply_side_effects(self, side_effects: QuerySideEffects) { + let dcx = self.dep_context().sess().dcx(); + let QuerySideEffects { diagnostics } = side_effects; + + for diagnostic in diagnostics { + dcx.emit_diagnostic(diagnostic); + } + } } pub(super) fn try_mark_green<'tcx>(tcx: TyCtxt<'tcx>, dep_node: &dep_graph::DepNode) -> bool { diff --git a/compiler/rustc_query_system/src/dep_graph/graph.rs b/compiler/rustc_query_system/src/dep_graph/graph.rs index 76227a78c3d68..32c32e5a0f774 100644 --- a/compiler/rustc_query_system/src/dep_graph/graph.rs +++ b/compiler/rustc_query_system/src/dep_graph/graph.rs @@ -912,11 +912,7 @@ impl DepGraphData { // Promote the previous diagnostics to the current session. qcx.store_side_effects(dep_node_index, side_effects.clone()); - let dcx = qcx.dep_context().sess().dcx(); - - for diagnostic in side_effects.diagnostics { - dcx.emit_diagnostic(diagnostic); - } + 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 ab4f48fcd3206..51d7790e23fb9 100644 --- a/compiler/rustc_query_system/src/query/mod.rs +++ b/compiler/rustc_query_system/src/query/mod.rs @@ -87,7 +87,7 @@ pub struct QuerySideEffects { /// Stores any diagnostics emitted during query execution. /// These diagnostics will be re-emitted if we mark /// the query as green. - pub(super) diagnostics: ThinVec, + pub diagnostics: ThinVec, } impl QuerySideEffects { @@ -119,6 +119,9 @@ pub trait QueryContext: HasDepContext { /// Register diagnostics for the given node, for use in next session. fn store_side_effects(self, dep_node_index: DepNodeIndex, side_effects: QuerySideEffects); + /// Actually execute the side effects + fn apply_side_effects(self, side_effects: QuerySideEffects); + /// Register diagnostics for the given node, for use in next session. fn store_side_effects_for_anon_node( self, @@ -133,7 +136,7 @@ pub trait QueryContext: HasDepContext { self, token: QueryJobId, depth_limit: bool, - diagnostics: Option<&Lock>>, + side_effects: Option<&Lock>, compute: impl FnOnce() -> R, ) -> R; diff --git a/compiler/rustc_query_system/src/query/plumbing.rs b/compiler/rustc_query_system/src/query/plumbing.rs index d37d5bce9cc36..42ded010d0ad2 100644 --- a/compiler/rustc_query_system/src/query/plumbing.rs +++ b/compiler/rustc_query_system/src/query/plumbing.rs @@ -26,7 +26,6 @@ use std::collections::hash_map::Entry; use std::fmt::Debug; use std::hash::Hash; use std::mem; -use thin_vec::ThinVec; use tracing::instrument; use super::QueryConfig; @@ -512,10 +511,10 @@ where } let prof_timer = qcx.dep_context().profiler().query_provider(); - let diagnostics = Lock::new(ThinVec::new()); + let side_effects = Lock::new(QuerySideEffects::default()); let (result, dep_node_index) = - qcx.start_query(job_id, query.depth_limit(), Some(&diagnostics), || { + qcx.start_query(job_id, query.depth_limit(), Some(&side_effects), || { if query.anon() { return dep_graph_data.with_anon_task(*qcx.dep_context(), query.dep_kind(), || { query.compute(qcx, key) @@ -537,7 +536,7 @@ where prof_timer.finish_with_query_invocation_id(dep_node_index.into()); - let side_effects = QuerySideEffects { diagnostics: diagnostics.into_inner() }; + let side_effects = side_effects.into_inner(); if std::intrinsics::unlikely(side_effects.maybe_any()) { if query.anon() { From 6b5241b201d39835802c89a041d9538e6848127a Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Thu, 7 Sep 2023 08:44:45 +0000 Subject: [PATCH 4/9] No need to manually drop a lock --- compiler/rustc_interface/src/callbacks.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/compiler/rustc_interface/src/callbacks.rs b/compiler/rustc_interface/src/callbacks.rs index 34ef6a95c5b7b..5c6f7f61b3a0c 100644 --- a/compiler/rustc_interface/src/callbacks.rs +++ b/compiler/rustc_interface/src/callbacks.rs @@ -33,9 +33,8 @@ fn track_diagnostic(diagnostic: DiagInner, f: &mut dyn FnMut(DiagInner) -> R) tls::with_context_opt(|icx| { if let Some(icx) = icx { if let Some(side_effects) = icx.side_effects { - let mut side_effects = side_effects.lock(); - side_effects.diagnostics.push(diagnostic.clone()); - std::mem::drop(side_effects); + let diagnostic = diagnostic.clone(); + side_effects.lock().diagnostics.push(diagnostic); } // Diagnostics are tracked, we can ignore the dependency. From 12aa5af5c0eb38e5777353150f2980358df0289c Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Wed, 27 Sep 2023 09:24:11 +0000 Subject: [PATCH 5/9] Remove `PartialEq` from `DepNodeColor` --- compiler/rustc_query_system/src/dep_graph/graph.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/compiler/rustc_query_system/src/dep_graph/graph.rs b/compiler/rustc_query_system/src/dep_graph/graph.rs index 32c32e5a0f774..0eba6a5858ba8 100644 --- a/compiler/rustc_query_system/src/dep_graph/graph.rs +++ b/compiler/rustc_query_system/src/dep_graph/graph.rs @@ -64,7 +64,6 @@ pub struct MarkFrame<'a> { parent: Option<&'a MarkFrame<'a>>, } -#[derive(PartialEq)] enum DepNodeColor { Red, Green(DepNodeIndex), @@ -921,7 +920,7 @@ impl DepGraph { /// Returns true if the given node has been marked as red during the /// current compilation session. Used in various assertions pub fn is_red(&self, dep_node: &DepNode) -> bool { - self.node_color(dep_node) == Some(DepNodeColor::Red) + matches!(self.node_color(dep_node), Some(DepNodeColor::Red)) } /// Returns true if the given node has been marked as green during the From a45e7e262476981f21df2bc5748ab00d5ddfa7ef Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Fri, 16 Feb 2024 16:52:51 +0000 Subject: [PATCH 6/9] Recreate `DefId`s when a cached query gets replayed --- compiler/rustc_middle/src/query/mod.rs | 1 + compiler/rustc_middle/src/ty/context.rs | 35 ++++++++++++--- compiler/rustc_query_impl/src/plumbing.rs | 10 +++-- .../rustc_query_system/src/dep_graph/graph.rs | 44 +++++++++++++++---- compiler/rustc_query_system/src/query/mod.rs | 18 ++++++-- .../rustc_query_system/src/query/plumbing.rs | 17 +++++-- 6 files changed, 100 insertions(+), 25 deletions(-) diff --git a/compiler/rustc_middle/src/query/mod.rs b/compiler/rustc_middle/src/query/mod.rs index 3c4aae73bc49c..a2323a3145778 100644 --- a/compiler/rustc_middle/src/query/mod.rs +++ b/compiler/rustc_middle/src/query/mod.rs @@ -1711,6 +1711,7 @@ rustc_queries! { desc { |tcx| "computing visibility of `{}`", tcx.def_path_str(def_id) } separate_provide_extern feedable + cache_on_disk_if { def_id.is_local() } } query inhabited_predicate_adt(key: DefId) -> ty::inhabitedness::InhabitedPredicate<'tcx> { diff --git a/compiler/rustc_middle/src/ty/context.rs b/compiler/rustc_middle/src/ty/context.rs index 47f66c6440627..70356ae5a842d 100644 --- a/compiler/rustc_middle/src/ty/context.rs +++ b/compiler/rustc_middle/src/ty/context.rs @@ -57,8 +57,9 @@ use rustc_hir::lang_items::LangItem; use rustc_hir::{HirId, Node, TraitCandidate}; use rustc_index::IndexVec; use rustc_macros::{HashStable, TyDecodable, TyEncodable}; -use rustc_query_system::dep_graph::DepNodeIndex; +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}; @@ -1344,10 +1345,9 @@ impl<'tcx> TyCtxt<'tcx> { // decode the on-disk cache. // // Any LocalDefId which is used within queries, either as key or result, either: - // - has been created before the construction of the TyCtxt; + // - has been created before the construction of the TyCtxt, + // - has been created when marking a query as green (recreating definitions it created in the actual run), // - has been created by this call to `create_def`. - // As a consequence, this LocalDefId is always re-created before it is needed by the incr. - // comp. engine itself. // // This call also writes to the value of `source_span` and `expn_that_defined` queries. // This is fine because: @@ -1357,9 +1357,30 @@ impl<'tcx> TyCtxt<'tcx> { // 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. - // Depending on the forever-red node will tell the graph that the calling query - // needs to be re-evaluated. - self.dep_graph.read_index(DepNodeIndex::FOREVER_RED_NODE); + tls::with_context(|icx| { + match icx.task_deps { + // Always gets rerun anyway, so nothing to replay + TaskDepsRef::EvalAlways => {} + // Top-level queries like the resolver get rerun every time anyway + TaskDepsRef::Ignore => {} + 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 }); + } + TaskDepsRef::Replay { prev_side_effects, created_def_ids } => { + 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 feed = TyCtxtFeed { tcx: self, key: def_id }; feed.def_kind(def_kind); diff --git a/compiler/rustc_query_impl/src/plumbing.rs b/compiler/rustc_query_impl/src/plumbing.rs index e88154baeb79c..a8fde21764971 100644 --- a/compiler/rustc_query_impl/src/plumbing.rs +++ b/compiler/rustc_query_impl/src/plumbing.rs @@ -21,8 +21,8 @@ use rustc_middle::ty::{self, TyCtxt, TyEncoder}; use rustc_query_system::dep_graph::{DepNodeParams, HasDepContext}; use rustc_query_system::ich::StableHashingContext; use rustc_query_system::query::{ - force_query, QueryCache, QueryConfig, QueryContext, QueryJobId, QueryMap, QuerySideEffects, - QueryStackFrame, + force_query, DefIdInfo, QueryCache, QueryConfig, QueryContext, QueryJobId, QueryMap, + QuerySideEffects, QueryStackFrame, }; use rustc_query_system::{LayoutOfDepth, QueryOverflow}; use rustc_serialize::Decodable; @@ -174,11 +174,15 @@ impl QueryContext for QueryCtxt<'_> { #[tracing::instrument(level = "trace", skip(self))] fn apply_side_effects(self, side_effects: QuerySideEffects) { let dcx = self.dep_context().sess().dcx(); - let QuerySideEffects { diagnostics } = side_effects; + let QuerySideEffects { diagnostics, definitions } = side_effects; for diagnostic in diagnostics { dcx.emit_diagnostic(diagnostic); } + + for DefIdInfo { parent, data } in definitions { + self.tcx.untracked().definitions.write().create_def(parent, data); + } } } diff --git a/compiler/rustc_query_system/src/dep_graph/graph.rs b/compiler/rustc_query_system/src/dep_graph/graph.rs index 0eba6a5858ba8..bbfdc5e77cfae 100644 --- a/compiler/rustc_query_system/src/dep_graph/graph.rs +++ b/compiler/rustc_query_system/src/dep_graph/graph.rs @@ -1,9 +1,15 @@ +use super::query::DepGraphQuery; +use super::serialized::{GraphEncoder, SerializedDepGraph, SerializedDepNodeIndex}; +use super::{DepContext, DepKind, DepNode, Deps, HasDepContext, WorkProductId}; +use crate::dep_graph::edges::EdgesVec; +use crate::ich::StableHashingContext; +use crate::query::{QueryContext, QuerySideEffects}; use rustc_data_structures::fingerprint::Fingerprint; use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use rustc_data_structures::profiling::{QueryInvocationId, SelfProfilerRef}; use rustc_data_structures::sharded::{self, Sharded}; use rustc_data_structures::stable_hasher::{HashStable, StableHasher}; -use rustc_data_structures::sync::{AtomicU32, AtomicU64, Lock, Lrc}; +use rustc_data_structures::sync::{AtomicU32, AtomicU64, AtomicUsize, Lock, Lrc}; use rustc_data_structures::unord::UnordMap; use rustc_index::IndexVec; use rustc_macros::{Decodable, Encodable}; @@ -17,13 +23,6 @@ use std::sync::atomic::Ordering; use std::sync::Arc; use tracing::{debug, instrument}; -use super::query::DepGraphQuery; -use super::serialized::{GraphEncoder, SerializedDepGraph, SerializedDepNodeIndex}; -use super::{DepContext, DepKind, DepNode, Deps, HasDepContext, WorkProductId}; -use crate::dep_graph::edges::EdgesVec; -use crate::ich::StableHashingContext; -use crate::query::{QueryContext, QuerySideEffects}; - #[cfg(debug_assertions)] use {super::debug::EdgeFilter, std::env}; @@ -224,6 +223,15 @@ impl DepGraph { D::with_deps(TaskDepsRef::Ignore, op) } + pub(crate) fn with_replay( + &self, + prev_side_effects: &QuerySideEffects, + created_def_ids: &AtomicUsize, + op: impl FnOnce() -> R, + ) -> R { + D::with_deps(TaskDepsRef::Replay { prev_side_effects, created_def_ids }, op) + } + /// Used to wrap the deserialization of a query result from disk, /// This method enforces that no new `DepNodes` are created during /// query result deserialization. @@ -278,6 +286,7 @@ impl DepGraph { } #[inline(always)] + /// A helper for `codegen_cranelift`. pub fn with_task, A: Debug, R>( &self, key: DepNode, @@ -464,6 +473,12 @@ impl DepGraph { return; } TaskDepsRef::Ignore => return, + // We don't need to record dependencies when rerunning a query + // because we have no disk cache entry to load. The dependencies + // are preserved. + // FIXME: assert that the dependencies don't change instead of + // recording them. + TaskDepsRef::Replay { .. } => return, TaskDepsRef::Forbid => { // Reading is forbidden in this context. ICE with a useful error message. panic_on_forbidden_read(data, dep_node_index) @@ -570,6 +585,7 @@ impl DepGraph { edges.push(DepNodeIndex::FOREVER_RED_NODE); } TaskDepsRef::Ignore => {} + TaskDepsRef::Replay { .. } => {} TaskDepsRef::Forbid => { panic!("Cannot summarize when dependencies are not recorded.") } @@ -1284,6 +1300,18 @@ pub enum TaskDepsRef<'a> { /// to ensure that the decoding process doesn't itself /// require the execution of any queries. Forbid, + /// Side effects from the previous run made available to + /// queries when they are reexecuted because their result was not + /// available in the cache. Whenever the query creates a new `DefId`, + /// it is checked against the entries in `QuerySideEffects::definitions` + /// to ensure that the new `DefId`s are the same as the ones that were + /// created the last time the query was executed. + Replay { + prev_side_effects: &'a QuerySideEffects, + /// Every new `DefId` is pushed here so we can check + /// that they match the cached ones. + created_def_ids: &'a AtomicUsize, + }, } #[derive(Debug)] diff --git a/compiler/rustc_query_system/src/query/mod.rs b/compiler/rustc_query_system/src/query/mod.rs index 51d7790e23fb9..68c67155713dc 100644 --- a/compiler/rustc_query_system/src/query/mod.rs +++ b/compiler/rustc_query_system/src/query/mod.rs @@ -21,7 +21,7 @@ use rustc_data_structures::sync::Lock; use rustc_errors::DiagInner; use rustc_hir::def::DefKind; use rustc_macros::{Decodable, Encodable}; -use rustc_span::def_id::DefId; +use rustc_span::def_id::{DefId, LocalDefId}; use rustc_span::Span; use thin_vec::ThinVec; @@ -88,20 +88,30 @@ pub struct QuerySideEffects { /// These diagnostics will be re-emitted if we mark /// the query as green. pub diagnostics: ThinVec, + /// Stores any `DefId`s that were created during query execution. + /// These `DefId`s will be re-created when we mark the query as green. + pub definitions: ThinVec, +} + +#[derive(Debug, Clone, Encodable, Decodable, PartialEq)] +pub struct DefIdInfo { + pub parent: LocalDefId, + pub data: rustc_hir::definitions::DefPathData, } impl QuerySideEffects { /// Returns true if there might be side effects. #[inline] pub fn maybe_any(&self) -> bool { - let QuerySideEffects { diagnostics } = self; + let QuerySideEffects { diagnostics, definitions } = self; // Use `has_capacity` so that the destructor for `self.diagnostics` can be skipped // if `maybe_any` is known to be false. - diagnostics.has_capacity() + diagnostics.has_capacity() || definitions.has_capacity() } pub fn append(&mut self, other: QuerySideEffects) { - let QuerySideEffects { diagnostics } = self; + let QuerySideEffects { diagnostics, definitions } = self; diagnostics.extend(other.diagnostics); + definitions.extend(other.definitions); } } diff --git a/compiler/rustc_query_system/src/query/plumbing.rs b/compiler/rustc_query_system/src/query/plumbing.rs index 42ded010d0ad2..827aef08e17eb 100644 --- a/compiler/rustc_query_system/src/query/plumbing.rs +++ b/compiler/rustc_query_system/src/query/plumbing.rs @@ -16,7 +16,7 @@ use rustc_data_structures::fingerprint::Fingerprint; use rustc_data_structures::fx::FxHashMap; use rustc_data_structures::sharded::Sharded; use rustc_data_structures::stack::ensure_sufficient_stack; -use rustc_data_structures::sync::Lock; +use rustc_data_structures::sync::{AtomicUsize, Lock}; #[cfg(parallel_compiler)] use rustc_data_structures::{outline, sync}; use rustc_errors::{Diag, FatalError, StashKey}; @@ -501,7 +501,7 @@ where let dep_node = dep_node_opt.get_or_insert_with(|| query.construct_dep_node(*qcx.dep_context(), &key)); - // The diagnostics for this query will be promoted to the current session during + // The side_effects for this query will be promoted to the current session during // `try_mark_green()`, so we can ignore them here. if let Some(ret) = qcx.start_query(job_id, false, None, || { try_load_from_disk_and_cache_in_memory(query, dep_graph_data, qcx, &key, dep_node) @@ -619,8 +619,19 @@ where // recompute. let prof_timer = qcx.dep_context().profiler().query_provider(); + let prev_side_effects = qcx.load_side_effects(prev_dep_node_index); + let created_def_ids = AtomicUsize::new(0); // The dep-graph for this computation is already in-place. - let result = qcx.dep_context().dep_graph().with_ignore(|| query.compute(qcx, *key)); + let result = + qcx.dep_context() + .dep_graph() + .with_replay(&prev_side_effects, &created_def_ids, || query.compute(qcx, *key)); + + // We want to verify that the `DefId`s created by the call to `query.compute` are consistent with + // those from the previous compilation. We already checked at `DefId` creation time, that the + // created `DefId`s have the same parent and `DefPathData` as the cached ones. + // We check here that we have not missed any. + assert_eq!(created_def_ids.into_inner(), prev_side_effects.definitions.len()); prof_timer.finish_with_query_invocation_id(dep_node_index.into()); From 6f5ad45b71d0ca42bb464421b18412d617110362 Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Mon, 27 May 2024 12:54:43 +0000 Subject: [PATCH 7/9] Turn some assertions into proper proc macro errors --- compiler/rustc_macros/src/query.rs | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/compiler/rustc_macros/src/query.rs b/compiler/rustc_macros/src/query.rs index 25675e06e38d6..e3610a6f5e1d3 100644 --- a/compiler/rustc_macros/src/query.rs +++ b/compiler/rustc_macros/src/query.rs @@ -314,6 +314,17 @@ pub fn rustc_queries(input: TokenStream) -> TokenStream { let mut query_description_stream = quote! {}; let mut query_cached_stream = quote! {}; let mut feedable_queries = quote! {}; + let mut errors = quote! {}; + + macro_rules! assert { + ( $cond:expr, $span:expr, $( $tt:tt )+ ) => { + if !$cond { + errors.extend( + Error::new($span, format!($($tt)*)).into_compile_error(), + ); + } + } + } for query in queries.0 { let Query { name, arg, modifiers, .. } = &query; @@ -369,10 +380,15 @@ pub fn rustc_queries(input: TokenStream) -> TokenStream { [#attribute_stream] fn #name(#arg) #result, }); - if modifiers.feedable.is_some() { - assert!(modifiers.anon.is_none(), "Query {name} cannot be both `feedable` and `anon`."); + if let Some(feedable) = &modifiers.feedable { + assert!( + modifiers.anon.is_none(), + feedable.span(), + "Query {name} cannot be both `feedable` and `anon`." + ); assert!( modifiers.eval_always.is_none(), + feedable.span(), "Query {name} cannot be both `feedable` and `eval_always`." ); feedable_queries.extend(quote! { @@ -407,5 +423,6 @@ pub fn rustc_queries(input: TokenStream) -> TokenStream { use super::*; #query_cached_stream } + #errors }) } From e14c6653684fc1c6134eb03c13bfba3860ee8361 Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Wed, 5 Jun 2024 10:55:41 +0000 Subject: [PATCH 8/9] Reproduce issue --- compiler/rustc_hir_analysis/src/lib.rs | 9 ++++++ compiler/rustc_middle/src/query/mod.rs | 1 - tests/incremental/static_cycle/feed_nested.rs | 28 +++++++++++++++++++ 3 files changed, 37 insertions(+), 1 deletion(-) create mode 100644 tests/incremental/static_cycle/feed_nested.rs diff --git a/compiler/rustc_hir_analysis/src/lib.rs b/compiler/rustc_hir_analysis/src/lib.rs index 65b02a2ec5653..cf4ba866b8a5a 100644 --- a/compiler/rustc_hir_analysis/src/lib.rs +++ b/compiler/rustc_hir_analysis/src/lib.rs @@ -201,6 +201,15 @@ pub fn check_crate(tcx: TyCtxt<'_>) { } }); + // Make sure we actually have a value for static items, as they aren't cached in incremental. + // While we could just wait for codegen to invoke this, the definitions freeze below will cause + // that to ICE, because evaluating statics can create more items. + tcx.hir().par_body_owners(|item_def_id| { + if let DefKind::Static { .. } = tcx.def_kind(item_def_id) { + let _ = tcx.eval_static_initializer(item_def_id); + } + }); + // Freeze definitions as we don't add new ones at this point. This improves performance by // allowing lock-free access to them. tcx.untracked().definitions.freeze(); diff --git a/compiler/rustc_middle/src/query/mod.rs b/compiler/rustc_middle/src/query/mod.rs index a2323a3145778..1b1b04558e534 100644 --- a/compiler/rustc_middle/src/query/mod.rs +++ b/compiler/rustc_middle/src/query/mod.rs @@ -1065,7 +1065,6 @@ rustc_queries! { "evaluating initializer of static `{}`", tcx.def_path_str(key) } - cache_on_disk_if { key.is_local() } separate_provide_extern feedable } diff --git a/tests/incremental/static_cycle/feed_nested.rs b/tests/incremental/static_cycle/feed_nested.rs new file mode 100644 index 0000000000000..4a2a10de49a1b --- /dev/null +++ b/tests/incremental/static_cycle/feed_nested.rs @@ -0,0 +1,28 @@ +//@ revisions:rpass1 rpass2 + +//! Test that the following order of instructions will not create duplicate +//! `DefId`s for the nested static items. +// ensure(eval_static_initializer(FOO)) +// -> try_mark_green(eval_static_initializer(FOO)) +// -> green +// -> replay side effects +// -> create some definitions. +// +// get(eval_static_initializer(FOO)) +// -> graph in place +// -> replay +// -> eval_static_initializer.compute +// -> how do we skip re-creating the same definitions ? + +#![feature(const_mut_refs)] +#![cfg_attr(rpass2, warn(dead_code))] + +pub static mut FOO: &mut i32 = &mut 42; + +pub static mut BAR: &mut i32 = unsafe { FOO }; + +fn main() { + unsafe { + assert_eq!(BAR as *mut i32, FOO as *mut i32); + } +} From c490a96acb55c80fe730e30aecc95c9098f6251c Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Wed, 5 Jun 2024 15:33:01 +0000 Subject: [PATCH 9/9] 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 70356ae5a842d..7fe6d6da25282 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; @@ -1333,6 +1333,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, @@ -1340,7 +1341,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. // @@ -1353,31 +1354,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 {