From 077b8d5c370391d428dbb4d3d037f6e51b1349a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20K=C3=A5re=20Alsaker?= Date: Sun, 16 Mar 2025 23:59:10 +0100 Subject: [PATCH 1/3] Abort in deadlock handler if we fail to get a query map --- compiler/rustc_interface/src/util.rs | 13 +++++++++++-- compiler/rustc_query_impl/src/plumbing.rs | 15 +++++++++------ compiler/rustc_query_system/src/query/job.rs | 2 +- compiler/rustc_query_system/src/query/mod.rs | 2 +- compiler/rustc_query_system/src/query/plumbing.rs | 5 +++-- 5 files changed, 25 insertions(+), 12 deletions(-) diff --git a/compiler/rustc_interface/src/util.rs b/compiler/rustc_interface/src/util.rs index 5cccab893bb35..0904795529e3b 100644 --- a/compiler/rustc_interface/src/util.rs +++ b/compiler/rustc_interface/src/util.rs @@ -192,7 +192,16 @@ pub(crate) fn run_in_thread_pool_with_globals R + Send, // `TyCtxt` TLS reference here. let query_map = current_gcx2.access(|gcx| { tls::enter_context(&tls::ImplicitCtxt::new(gcx), || { - tls::with(|tcx| QueryCtxt::new(tcx).collect_active_jobs()) + tls::with(|tcx| { + let (query_map, complete) = QueryCtxt::new(tcx).collect_active_jobs(); + if !complete { + eprintln!("internal compiler error: failed to get query map in deadlock handler, aborting process"); + // We need to abort here as we failed to resolve the deadlock, + // otherwise the compiler could just hang, + process::abort(); + } + query_map + }) }) }); let query_map = FromDyn::from(query_map); @@ -201,7 +210,7 @@ pub(crate) fn run_in_thread_pool_with_globals R + Send, .name("rustc query cycle handler".to_string()) .spawn(move || { let on_panic = defer(|| { - eprintln!("query cycle handler thread panicked, aborting process"); + eprintln!("internal compiler error: query cycle handler thread panicked, aborting process"); // We need to abort here as we failed to resolve the deadlock, // otherwise the compiler could just hang, process::abort(); diff --git a/compiler/rustc_query_impl/src/plumbing.rs b/compiler/rustc_query_impl/src/plumbing.rs index 2b8457ace8ee7..85abb77e53c7d 100644 --- a/compiler/rustc_query_impl/src/plumbing.rs +++ b/compiler/rustc_query_impl/src/plumbing.rs @@ -79,14 +79,15 @@ impl QueryContext for QueryCtxt<'_> { tls::with_related_context(self.tcx, |icx| icx.query) } - fn collect_active_jobs(self) -> QueryMap { + fn collect_active_jobs(self) -> (QueryMap, bool) { let mut jobs = QueryMap::default(); + let mut complete = true; for collect in super::TRY_COLLECT_ACTIVE_JOBS.iter() { - collect(self.tcx, &mut jobs); + collect(self.tcx, &mut jobs, &mut complete); } - jobs + (jobs, complete) } // Interactions with on_disk_cache @@ -139,7 +140,8 @@ impl QueryContext for QueryCtxt<'_> { } fn depth_limit_error(self, job: QueryJobId) { - let (info, depth) = job.find_dep_kind_root(self.collect_active_jobs()); + // FIXME: `collect_active_jobs` expects no locks to be held, which doesn't hold for this call. + let (info, depth) = job.find_dep_kind_root(self.collect_active_jobs().0); let suggested_limit = match self.recursion_limit() { Limit(0) => Limit(2), @@ -677,7 +679,7 @@ macro_rules! define_queries { } } - pub(crate) fn try_collect_active_jobs<'tcx>(tcx: TyCtxt<'tcx>, qmap: &mut QueryMap) { + pub(crate) fn try_collect_active_jobs<'tcx>(tcx: TyCtxt<'tcx>, qmap: &mut QueryMap, complete: &mut bool) { let make_query = |tcx, key| { let kind = rustc_middle::dep_graph::dep_kinds::$name; let name = stringify!($name); @@ -692,6 +694,7 @@ macro_rules! define_queries { // don't `unwrap()` here, just manually check for `None` and do best-effort error // reporting. if res.is_none() { + *complete = false; tracing::warn!( "Failed to collect active jobs for query with name `{}`!", stringify!($name) @@ -756,7 +759,7 @@ macro_rules! define_queries { // These arrays are used for iteration and can't be indexed by `DepKind`. - const TRY_COLLECT_ACTIVE_JOBS: &[for<'tcx> fn(TyCtxt<'tcx>, &mut QueryMap)] = + const TRY_COLLECT_ACTIVE_JOBS: &[for<'tcx> fn(TyCtxt<'tcx>, &mut QueryMap, &mut bool)] = &[$(query_impl::$name::try_collect_active_jobs),*]; const ALLOC_SELF_PROFILE_QUERY_STRINGS: &[ diff --git a/compiler/rustc_query_system/src/query/job.rs b/compiler/rustc_query_system/src/query/job.rs index 37b305d0a8b50..44f9325dce262 100644 --- a/compiler/rustc_query_system/src/query/job.rs +++ b/compiler/rustc_query_system/src/query/job.rs @@ -588,7 +588,7 @@ pub fn print_query_stack( // state if it was responsible for triggering the panic. let mut count_printed = 0; let mut count_total = 0; - let query_map = qcx.collect_active_jobs(); + let query_map = qcx.collect_active_jobs().0; if let Some(ref mut file) = file { let _ = writeln!(file, "\n\nquery stack during panic:"); diff --git a/compiler/rustc_query_system/src/query/mod.rs b/compiler/rustc_query_system/src/query/mod.rs index 2ed0c810b7516..3728691a6f9a9 100644 --- a/compiler/rustc_query_system/src/query/mod.rs +++ b/compiler/rustc_query_system/src/query/mod.rs @@ -86,7 +86,7 @@ pub trait QueryContext: HasDepContext { /// Get the query information from the TLS context. fn current_query_job(self) -> Option; - fn collect_active_jobs(self) -> QueryMap; + fn collect_active_jobs(self) -> (QueryMap, bool); /// Load a side effect associated to the node in the previous session. fn load_side_effect( diff --git a/compiler/rustc_query_system/src/query/plumbing.rs b/compiler/rustc_query_system/src/query/plumbing.rs index 7578cb5e2aebd..68590854343e2 100644 --- a/compiler/rustc_query_system/src/query/plumbing.rs +++ b/compiler/rustc_query_system/src/query/plumbing.rs @@ -250,8 +250,9 @@ where Q: QueryConfig, Qcx: QueryContext, { - let error = - try_execute.find_cycle_in_stack(qcx.collect_active_jobs(), &qcx.current_query_job(), span); + let (query_map, complete) = qcx.collect_active_jobs(); + assert!(complete, "failed to collect active queries"); + let error = try_execute.find_cycle_in_stack(query_map, &qcx.current_query_job(), span); (mk_cycle(query, qcx, error), None) } From 157008d7117ec9ca13b70c38058639524437112f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20K=C3=A5re=20Alsaker?= Date: Tue, 18 Mar 2025 14:36:28 +0100 Subject: [PATCH 2/3] Update comments --- compiler/rustc_interface/src/util.rs | 5 +++-- compiler/rustc_query_impl/src/plumbing.rs | 2 ++ compiler/rustc_query_system/src/query/plumbing.rs | 3 +++ 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/compiler/rustc_interface/src/util.rs b/compiler/rustc_interface/src/util.rs index 0904795529e3b..249b80bb31d91 100644 --- a/compiler/rustc_interface/src/util.rs +++ b/compiler/rustc_interface/src/util.rs @@ -195,9 +195,10 @@ pub(crate) fn run_in_thread_pool_with_globals R + Send, tls::with(|tcx| { let (query_map, complete) = QueryCtxt::new(tcx).collect_active_jobs(); if !complete { + // There was an unexpected error collecting all active jobs, which we need + // to find cycles to break. + // We want to avoid panicking in the deadlock handler, so we abort instead. eprintln!("internal compiler error: failed to get query map in deadlock handler, aborting process"); - // We need to abort here as we failed to resolve the deadlock, - // otherwise the compiler could just hang, process::abort(); } query_map diff --git a/compiler/rustc_query_impl/src/plumbing.rs b/compiler/rustc_query_impl/src/plumbing.rs index 85abb77e53c7d..301bcac4ca795 100644 --- a/compiler/rustc_query_impl/src/plumbing.rs +++ b/compiler/rustc_query_impl/src/plumbing.rs @@ -79,6 +79,8 @@ impl QueryContext for QueryCtxt<'_> { tls::with_related_context(self.tcx, |icx| icx.query) } + /// Returns a query map representing active query jobs and a bool being false + /// if there was an error constructing the map. fn collect_active_jobs(self) -> (QueryMap, bool) { let mut jobs = QueryMap::default(); let mut complete = true; diff --git a/compiler/rustc_query_system/src/query/plumbing.rs b/compiler/rustc_query_system/src/query/plumbing.rs index 68590854343e2..dc1d6f760faca 100644 --- a/compiler/rustc_query_system/src/query/plumbing.rs +++ b/compiler/rustc_query_system/src/query/plumbing.rs @@ -251,7 +251,10 @@ where Qcx: QueryContext, { let (query_map, complete) = qcx.collect_active_jobs(); + // Ensure there was no errors collecting all active jobs. + // We need the complete map to ensure we find a cycle to break. assert!(complete, "failed to collect active queries"); + let error = try_execute.find_cycle_in_stack(query_map, &qcx.current_query_job(), span); (mk_cycle(query, qcx, error), None) } From 34244c1477e4b485c51872fc3c6d846b4a3c6ffa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20K=C3=A5re=20Alsaker?= Date: Fri, 21 Mar 2025 08:09:42 +0100 Subject: [PATCH 3/3] Address comments --- compiler/rustc_interface/src/util.rs | 17 +++++++------ compiler/rustc_query_impl/src/plumbing.rs | 25 ++++++++++++------- compiler/rustc_query_system/src/query/job.rs | 7 +++++- compiler/rustc_query_system/src/query/mod.rs | 2 +- .../rustc_query_system/src/query/plumbing.rs | 3 +-- 5 files changed, 33 insertions(+), 21 deletions(-) diff --git a/compiler/rustc_interface/src/util.rs b/compiler/rustc_interface/src/util.rs index 249b80bb31d91..333786f0ca38f 100644 --- a/compiler/rustc_interface/src/util.rs +++ b/compiler/rustc_interface/src/util.rs @@ -193,15 +193,16 @@ pub(crate) fn run_in_thread_pool_with_globals R + Send, let query_map = current_gcx2.access(|gcx| { tls::enter_context(&tls::ImplicitCtxt::new(gcx), || { tls::with(|tcx| { - let (query_map, complete) = QueryCtxt::new(tcx).collect_active_jobs(); - if !complete { - // There was an unexpected error collecting all active jobs, which we need - // to find cycles to break. - // We want to avoid panicking in the deadlock handler, so we abort instead. - eprintln!("internal compiler error: failed to get query map in deadlock handler, aborting process"); - process::abort(); + match QueryCtxt::new(tcx).collect_active_jobs() { + Ok(query_map) => query_map, + Err(_) => { + // There was an unexpected error collecting all active jobs, which we need + // to find cycles to break. + // We want to avoid panicking in the deadlock handler, so we abort instead. + eprintln!("internal compiler error: failed to get query map in deadlock handler, aborting process"); + process::abort(); + } } - query_map }) }) }); diff --git a/compiler/rustc_query_impl/src/plumbing.rs b/compiler/rustc_query_impl/src/plumbing.rs index 301bcac4ca795..cafe16ce77b9f 100644 --- a/compiler/rustc_query_impl/src/plumbing.rs +++ b/compiler/rustc_query_impl/src/plumbing.rs @@ -79,17 +79,20 @@ impl QueryContext for QueryCtxt<'_> { tls::with_related_context(self.tcx, |icx| icx.query) } - /// Returns a query map representing active query jobs and a bool being false - /// if there was an error constructing the map. - fn collect_active_jobs(self) -> (QueryMap, bool) { + /// Returns a query map representing active query jobs. + /// It returns an incomplete map as an error if it fails + /// to take locks. + fn collect_active_jobs(self) -> Result { let mut jobs = QueryMap::default(); let mut complete = true; for collect in super::TRY_COLLECT_ACTIVE_JOBS.iter() { - collect(self.tcx, &mut jobs, &mut complete); + if collect(self.tcx, &mut jobs).is_none() { + complete = false; + } } - (jobs, complete) + if complete { Ok(jobs) } else { Err(jobs) } } // Interactions with on_disk_cache @@ -143,7 +146,11 @@ impl QueryContext for QueryCtxt<'_> { fn depth_limit_error(self, job: QueryJobId) { // FIXME: `collect_active_jobs` expects no locks to be held, which doesn't hold for this call. - let (info, depth) = job.find_dep_kind_root(self.collect_active_jobs().0); + let query_map = match self.collect_active_jobs() { + Ok(query_map) => query_map, + Err(query_map) => query_map, + }; + let (info, depth) = job.find_dep_kind_root(query_map); let suggested_limit = match self.recursion_limit() { Limit(0) => Limit(2), @@ -681,7 +688,7 @@ macro_rules! define_queries { } } - pub(crate) fn try_collect_active_jobs<'tcx>(tcx: TyCtxt<'tcx>, qmap: &mut QueryMap, complete: &mut bool) { + pub(crate) fn try_collect_active_jobs<'tcx>(tcx: TyCtxt<'tcx>, qmap: &mut QueryMap) -> Option<()> { let make_query = |tcx, key| { let kind = rustc_middle::dep_graph::dep_kinds::$name; let name = stringify!($name); @@ -696,12 +703,12 @@ macro_rules! define_queries { // don't `unwrap()` here, just manually check for `None` and do best-effort error // reporting. if res.is_none() { - *complete = false; tracing::warn!( "Failed to collect active jobs for query with name `{}`!", stringify!($name) ); } + res } pub(crate) fn alloc_self_profile_query_strings<'tcx>( @@ -761,7 +768,7 @@ macro_rules! define_queries { // These arrays are used for iteration and can't be indexed by `DepKind`. - const TRY_COLLECT_ACTIVE_JOBS: &[for<'tcx> fn(TyCtxt<'tcx>, &mut QueryMap, &mut bool)] = + const TRY_COLLECT_ACTIVE_JOBS: &[for<'tcx> fn(TyCtxt<'tcx>, &mut QueryMap) -> Option<()>] = &[$(query_impl::$name::try_collect_active_jobs),*]; const ALLOC_SELF_PROFILE_QUERY_STRINGS: &[ diff --git a/compiler/rustc_query_system/src/query/job.rs b/compiler/rustc_query_system/src/query/job.rs index 44f9325dce262..f0cb378f471e8 100644 --- a/compiler/rustc_query_system/src/query/job.rs +++ b/compiler/rustc_query_system/src/query/job.rs @@ -588,7 +588,12 @@ pub fn print_query_stack( // state if it was responsible for triggering the panic. let mut count_printed = 0; let mut count_total = 0; - let query_map = qcx.collect_active_jobs().0; + + // Make use of a partial query map if we fail to take locks collecting active queries. + let query_map = match qcx.collect_active_jobs() { + Ok(query_map) => query_map, + Err(query_map) => query_map, + }; if let Some(ref mut file) = file { let _ = writeln!(file, "\n\nquery stack during panic:"); diff --git a/compiler/rustc_query_system/src/query/mod.rs b/compiler/rustc_query_system/src/query/mod.rs index 3728691a6f9a9..0d0c66aa978ef 100644 --- a/compiler/rustc_query_system/src/query/mod.rs +++ b/compiler/rustc_query_system/src/query/mod.rs @@ -86,7 +86,7 @@ pub trait QueryContext: HasDepContext { /// Get the query information from the TLS context. fn current_query_job(self) -> Option; - fn collect_active_jobs(self) -> (QueryMap, bool); + fn collect_active_jobs(self) -> Result; /// Load a side effect associated to the node in the previous session. fn load_side_effect( diff --git a/compiler/rustc_query_system/src/query/plumbing.rs b/compiler/rustc_query_system/src/query/plumbing.rs index dc1d6f760faca..a5ecc50421e77 100644 --- a/compiler/rustc_query_system/src/query/plumbing.rs +++ b/compiler/rustc_query_system/src/query/plumbing.rs @@ -250,10 +250,9 @@ where Q: QueryConfig, Qcx: QueryContext, { - let (query_map, complete) = qcx.collect_active_jobs(); // Ensure there was no errors collecting all active jobs. // We need the complete map to ensure we find a cycle to break. - assert!(complete, "failed to collect active queries"); + let query_map = qcx.collect_active_jobs().expect("failed to collect active queries"); let error = try_execute.find_cycle_in_stack(query_map, &qcx.current_query_job(), span); (mk_cycle(query, qcx, error), None)