Abort in deadlock handler if we fail to get a query map #138581

Zoxc · 2025-03-16T23:37:34Z

Resolving query cycles requires the complete active query map, or it may miss query cycles. We did not check that the map is completely constructed before. If there is some error collecting the map, something has gone wrong already. This adds a check to abort/panic if we fail to construct the complete map.

This can help differentiate errors from the deadlock detected case if constructing query map has errors in practice.

An Option is not used for collect_active_jobs as the panic handler can still make use of a partial map.

rustbot · 2025-03-16T23:37:38Z

r? @petrochenkov

rustbot has assigned @petrochenkov.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

petrochenkov · 2025-03-17T22:28:16Z

Is anybody else familiar with this logic?
The changes look reasonable but I do not understand the details.

Zoxc · 2025-03-17T22:30:08Z

@oli-obk looked at my last PR in this area at least.

petrochenkov · 2025-03-17T22:29:33Z

compiler/rustc_interface/src/util.rs

+                        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,


Why does failing to collect_active_jobs mean that we failed to resolve the deadlock?
Why the compiler can hang in this case?

Deadlock detection depends on obtaining all the unfinished query tasks in the state and collecting them into one QueryMap.

Deadlock detection depends on obtaining all the unfinished query tasks in the state and collecting them into one QueryMap.

@Zoxc Could you add this as a comment to code (preferably with a brief explanation of why it depends on obtaining all the tasks).

petrochenkov · 2025-03-17T22:31:19Z

compiler/rustc_query_system/src/query/plumbing.rs

-    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");


Why is incomplete map insufficient here?
What can happen if we try to find_cycle_in_stack with an incomplete map?

It will just panic if the relevant queries are missing. It's useful to differentiate the errors though.

Could you add this as a comment to code?

petrochenkov · 2025-03-17T22:32:41Z

cc @nnethercote @zetanumbers

nnethercote · 2025-03-18T02:58:14Z

@SparrowLii might be the best person to review?

I think a better description of the motivation and change would help. The PR description says the following:

Resolving query cycles requires the complete active query map. If for some reason it's incomplete, we abort instead.

An Option is not used as the panic handler can still make use of a partial map.

I can't tell if that's describing the current state, or the change being made. It needs a clear description of the problem with the current code, and the change being made, and how that change improves things.

SparrowLii · 2025-03-18T03:20:57Z

compiler/rustc_interface/src/util.rs

+                        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,


Deadlock detection depends on obtaining all the unfinished query tasks in the state and collecting them into one QueryMap.

SparrowLii · 2025-03-18T03:26:25Z

compiler/rustc_query_impl/src/plumbing.rs

@@ -693,7 +695,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) {


Can we just return bool here? I'd like to avoid having a function rely on extra mutable references.

That adds more complexity though.

SparrowLii · 2025-03-18T03:36:06Z

@nnethercote AFAIK, in the previous design, try_collect_active_jobs would have returned an Option<QueryMap> for each query, and then merged all QueryMaps. The current design avoids the cost of merging, but loses the ability to check whether the collection is complete by Option, so the motivation for this PR looks reasonable.

@Zoxc Can you add a description to the PR so that people without the knowledge about try_collect_active_jobs can understand the history?

petrochenkov · 2025-03-18T12:10:21Z

r? @SparrowLii

tjallingt · 2025-03-19T10:54:37Z

compiler/rustc_query_impl/src/plumbing.rs

-    fn collect_active_jobs(self) -> QueryMap {
+    /// 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) {


Suggested change

fn collect_active_jobs(self) -> (QueryMap, bool) {

fn collect_active_jobs(self) -> Result<QueryMap, QueryMap> {

Might be more explicit. When not caring about whether the map is complete or not one can do:

let query_map = match qcx.collect_active_jobs() { Ok(complete_map) => complete_map, Err(incomplete_map) => incomplete_map, };

That does feel a bit odd to me since QueryMap is not an error.

Using QueryMap as an error does force callers of collect_active_jobs to check if it is incomplete or not, the bool can still easily be ignored by accident. Pretty sure i've seen this pattern of Result<T, T> in rustc and elsewhere.

If we use Result<QueryMap, QueryMap>, we should at least do something about the incomplete QueryMap, but we doesn't. I've not had to deal with an incomplete QueryMap in practice.

Zoxc · 2025-03-22T15:56:38Z

@rustbot ready

SparrowLii

LGTM!

SparrowLii · 2025-03-24T06:08:29Z

@bors r+

bors · 2025-03-24T06:08:32Z

📌 Commit 34244c1 has been approved by SparrowLii

It is now in the queue for this repository.

bors · 2025-03-24T06:08:32Z

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#135745 (Recognise new IPv6 non-global range from IETF RFC 9602) - rust-lang#137247 (cg_llvm: Reduce the visibility of types, modules and using declarations in `rustc_codegen_llvm`.) - rust-lang#138317 (privacy: Visit types and traits in impls in type privacy lints) - rust-lang#138581 (Abort in deadlock handler if we fail to get a query map) - rust-lang#138776 (coverage: Separate span-extraction from unexpansion) - rust-lang#138886 (Fix autofix for `self` and `self as …` in `unused_imports` lint) - rust-lang#138924 (Reduce `kw::Empty` usage, part 3) - rust-lang#138929 (Visitors track whether an assoc item is in a trait impl or an inherent impl) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#138581 - Zoxc:abort-handler-if-locked, r=SparrowLii Abort in deadlock handler if we fail to get a query map Resolving query cycles requires the complete active query map, or it may miss query cycles. We did not check that the map is completely constructed before. If there is some error collecting the map, something has gone wrong already. This adds a check to abort/panic if we fail to construct the complete map. This can help differentiate errors from the `deadlock detected` case if constructing query map has errors in practice. An `Option` is not used for `collect_active_jobs` as the panic handler can still make use of a partial map.

rustbot assigned petrochenkov Mar 16, 2025

petrochenkov reviewed Mar 17, 2025

View reviewed changes

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 17, 2025

SparrowLii reviewed Mar 18, 2025

View reviewed changes

rustbot assigned SparrowLii and unassigned petrochenkov Mar 18, 2025

Zoxc mentioned this pull request Mar 18, 2025

Avoiding calling queries when collecting active queries #138672

Open

tjallingt reviewed Mar 19, 2025

View reviewed changes

Zoxc added 3 commits March 21, 2025 07:37

Abort in deadlock handler if we fail to get a query map

077b8d5

Update comments

157008d

Address comments

34244c1

Zoxc force-pushed the abort-handler-if-locked branch from e8ad42d to 34244c1 Compare March 21, 2025 07:26

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 22, 2025

SparrowLii approved these changes Mar 24, 2025

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 24, 2025

matthiaskrgr mentioned this pull request Mar 25, 2025

Rollup of 8 pull requests #138933

Merged

bors merged commit 43297ff into rust-lang:master Mar 25, 2025
6 checks passed

rustbot added this to the 1.87.0 milestone Mar 25, 2025

Zoxc deleted the abort-handler-if-locked branch March 26, 2025 01:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Abort in deadlock handler if we fail to get a query map #138581

Abort in deadlock handler if we fail to get a query map #138581

Zoxc commented Mar 16, 2025 •

edited

Loading

rustbot commented Mar 16, 2025

petrochenkov commented Mar 17, 2025

Zoxc commented Mar 17, 2025

petrochenkov Mar 17, 2025

SparrowLii Mar 18, 2025 •

edited

Loading

petrochenkov Mar 18, 2025

petrochenkov Mar 17, 2025

Zoxc Mar 17, 2025

petrochenkov Mar 18, 2025

petrochenkov commented Mar 17, 2025

nnethercote commented Mar 18, 2025

SparrowLii Mar 18, 2025 •

edited

Loading

SparrowLii Mar 18, 2025

Zoxc Mar 19, 2025

SparrowLii commented Mar 18, 2025 •

edited

Loading

petrochenkov commented Mar 18, 2025

tjallingt Mar 19, 2025

Zoxc Mar 19, 2025

tjallingt Mar 19, 2025

SparrowLii Mar 20, 2025

Zoxc commented Mar 22, 2025

SparrowLii left a comment

SparrowLii commented Mar 24, 2025

bors commented Mar 24, 2025

bors commented Mar 24, 2025

	fn collect_active_jobs(self) -> (QueryMap, bool) {
	fn collect_active_jobs(self) -> Result<QueryMap, QueryMap> {

Abort in deadlock handler if we fail to get a query map #138581

Abort in deadlock handler if we fail to get a query map #138581

Conversation

Zoxc commented Mar 16, 2025 • edited Loading

rustbot commented Mar 16, 2025

petrochenkov commented Mar 17, 2025

Zoxc commented Mar 17, 2025

Choose a reason for hiding this comment

SparrowLii Mar 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov commented Mar 17, 2025

nnethercote commented Mar 18, 2025

SparrowLii Mar 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparrowLii commented Mar 18, 2025 • edited Loading

petrochenkov commented Mar 18, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zoxc commented Mar 22, 2025

SparrowLii left a comment

Choose a reason for hiding this comment

SparrowLii commented Mar 24, 2025

bors commented Mar 24, 2025

bors commented Mar 24, 2025

Zoxc commented Mar 16, 2025 •

edited

Loading

SparrowLii Mar 18, 2025 •

edited

Loading

SparrowLii Mar 18, 2025 •

edited

Loading

SparrowLii commented Mar 18, 2025 •

edited

Loading