Improved handling for cycles within the query system itself

[jyn514]

View all comments

This PR is somewhat complicated and I don't expect it to merge as-is. Let me start with a motivating example. The following rustc-driver worked fine a week ago:

structDriver;implCallbacksforDriver{fnconfig(&mutself,config:&mut interface::Config){
config.override_queries = Some(|_sess, providers| {
providers.queries.codegen_fn_attrs = |tcx, def_id| {let _ = tcx.hir_crate_items(());(rustc_interface::DEFAULT_QUERY_PROVIDERS.queries.codegen_fn_attrs)(tcx, def_id)};});}}fnmain() -> ExitCode{
rustc_driver::install_ice_hook("https://github.com/ferrocene/ferrocene/issues/new", |_| ());let handler = EarlyDiagCtxt::new(ErrorOutputType::default());
rustc_driver::init_rustc_env_logger(&handler);
rustc_driver::catch_with_exit_code(move || {let args:Vec<String> = std::env::args().collect();
rustc_driver::run_compiler(&args,&mutDriver)})}

Since #153489 merged, it hangs indefinitely trying to compile the following program (reduced from tests/ui/delegation/bad-resolve):

#![feature(fn_delegation)]#![allow(incomplete_features)]traitTrait{typeType;fnbar(){}}structF;implTraitforF{typeType = i32;}structS(F);implTraitforS{
reuse <FasTrait>::bar;}fnmain(){}

I'll step through how this program behaves on each commit in this PR.

---

1. Main branch: Hangs indefinitely with no output.

2. After first commit (adding safeguards to ensure_sufficient_stack):
   a. First, it prints a stacktrace that cuts off early
   b. Then it hangs forever.

3. After calling abort if ensure_sufficient_stack fails:
   a. First, it prints a ~250 frame stacktrace,
   including both the first and last hundred frames.
   b. Then it exits.

4. After adding debug logging, shows the stack trace from above, but first:
   a. Prints a trace of the query execution
   b. Shows the stacktrace from above.

5. After the bug fix: as above, but instead of the 5 million frame backtrace, prints a "more normal" query cycle:

error[E0391]: cycle detected when looking up span for `<impl at bad-resolve.rs:15:1: 15:17>::bar`
|
= note: ...which requires getting HIR ID of `<impl at bad-resolve.rs:15:1: 15:17>::bar`...
= note: ...which again requires looking up span for `<impl at bad-resolve.rs:15:1: 15:17>::bar`, completing the cycle
note: cycle used when computing codegen attributes of `Trait::bar`
--> bad-resolve.rs:6:5
|
6 | fn bar() {}
| ^^^^^^^^

---

Here's an extract of that query trace:

Details

rustc_query_impl::execution::try_execute_query query="hir_crate_items"
...
rustc_query_impl::execution::try_execute_query query="lower_delayed_owner"
...
rustc_query_impl::execution::try_execute_query query="codegen_fn_attrs"
rustc_query_impl::execution::try_execute_query query="hir_crate_items"
0ms INFO rustc_query_impl::execution hit a query cycle evaluating hir_crate_items!
rustc_query_impl::execution::try_execute_query query="def_span"
rustc_query_impl::execution::try_execute_query query="local_def_id_to_hir_id"
rustc_query_impl::execution::try_execute_query query="lower_delayed_owner"
0ms INFO rustc_query_impl::execution hit a query cycle evaluating lower_delayed_owner!
rustc_query_impl::execution::try_execute_query query="def_span"
0ms INFO rustc_query_impl::execution hit a query cycle evaluating def_span!
rustc_query_impl::execution::try_execute_query query="def_span"
...

I feel somewhat strongly that there is a bug here; it should not be possible to get query cycles within def_span.
In particular, note that the first two cycles are ignored altogether in order to report the
third cycle; that seems quite bad!

I'm not sure if the bug is in the implementation of lower_delayed_owner, in the query system, or merely in the fact that lower_delay_owner is a prerequisite of hir_items; I'm opening this PR to get more feedback on that. However, I think the commits here stand on their own.

In #153979 (comment) I mistakenly attributed this to recent refactors in the query system; I no longer think that's true, I think the infinite loop is a latent bug that's been around for years, and the new query cycle is related to lower_delayed_owner, not the query system itself.

cc @petrochenkov@aerooneqq@Zalathar

[rustbot]

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

The list of allowed third-party dependencies may have been modified! You must ensure that any new dependencies have compatible licenses before merging.

cc @davidtwco, @wesleywiser

[rustbot]

r? @JohnTitor

rustbot has assigned @JohnTitor.
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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 69 candidates
• Random selection from 12 candidates

[jyn514]

hm. maybe r? @Zalathar ?

[rustbot]

Zalathar is not on the review rotation at the moment.
They may take a while to respond.

[Zoxc]

Not sure about the query related changes here, but it makes sense to have proper stack limit so we don't OOM with infinite recursion.

[Zoxc]

@jyn514 You could try your test failures with #154389. We have some failing tests on main which that detects and the cause might be similar.

[jyn514]

I think the tests were wrong, not the code. I've pushed a commit updating them; the new cycle errors look much more informative to me.

[jyn514]

Here's a trace of the queries that caused the cycle for /query-cycle-issue-124901.rs:

 rustc_query_impl::execution::try_execute_query query="local_def_id_to_hir_id"
rustc_query_impl::execution::try_execute_query query="hir_crate"
rustc_query_impl::execution::try_execute_query query="early_lint_checks"
rustc_query_impl::execution::try_execute_query query="resolver_for_lowering_raw"
0ms INFO rustc_query_impl::execution hit a query cycle evaluating resolver_for_lowering_raw!
rustc_middle::queries::default_span self=resolver_for_lowering_raw(())
rustc_middle::queries::default_span self=local_def_id_to_hir_id(DefId(0:3 ~ query_cycle_issue_124901[dbc8]::Default))
rustc_middle::queries::default_span self=hir_crate(())
rustc_middle::queries::default_span self=early_lint_checks(())

Previously, default_span(local_def_id_to_hir_id) would call into def_span(def_id), which would call back into local_def_id_to_hir_id, causing a follow-up cycle. I don't think that follow-up cycle is useful to anyone.

[jyn514]

hm, I think this fix is incomplete. hir_span can also call queries (I've verified it calls at least hir_owner_parent_q and opt_hir_owner_nodes). I wonder if the query system should have some special "fallback cycle handler" where def_span can return to choose DUMMY_SP if there's an existing query that's executing. Then we don't have to hard-code all its dependencies in default_span.

[Zoxc]

I think we could check if HIR lowering is done for the DefId by checking if relevant queries have executed and only then try to get the span.

[jyn514]

Hm, I'm not sure how to do this. I found rustc_middle_query::inner::try_get_cached, so I thought I could do something like try_get_cached(TaggedQueryKey::def_span(key)).is_some(). But each query might have a different input type, and I don't know how to get from that other type back to a DefId. I don't see how to do this without modifying the implementation of def_span itself.

[jyn514]

Maybe we need something like QueryKey::def_id()?

[jyn514]

oh lol that already exists

[jyn514]

ok #154389 solves this in a completely different way and I'm not sure my approach makes sense anymore, so I'm just going to drop the last commit.

[Zoxc]

It doesn't fix any query cycles though, just improves the reporting.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[jyn514]

r? @Zoxc

[Zoxc]

It kind of seems like this could be an upstream feature of stacker. cc @nagisa

For 64-bit platforms, we could just skip stacker as we just have fixed size thread stacks again.

[programmerjake]

shouldn't this be using a drop-guard to update the value properly when unwinding?

View changes since the review

[rust-bors]

☔ The latest upstream changes (presumably #160535) made this pull request unmergeable. Please resolve the merge conflicts by rebasing.

[jyn514]

View all comments

This PR is somewhat complicated and I don't expect it to merge as-is. Let me start with a motivating example. The following rustc-driver worked fine a week ago:

structDriver;implCallbacksforDriver{fnconfig(&mutself,config:&mut interface::Config){
config.override_queries = Some(|_sess, providers| {
providers.queries.codegen_fn_attrs = |tcx, def_id| {let _ = tcx.hir_crate_items(());(rustc_interface::DEFAULT_QUERY_PROVIDERS.queries.codegen_fn_attrs)(tcx, def_id)};});}}fnmain() -> ExitCode{
rustc_driver::install_ice_hook("https://github.com/ferrocene/ferrocene/issues/new", |_| ());let handler = EarlyDiagCtxt::new(ErrorOutputType::default());
rustc_driver::init_rustc_env_logger(&handler);
rustc_driver::catch_with_exit_code(move || {let args:Vec<String> = std::env::args().collect();
rustc_driver::run_compiler(&args,&mutDriver)})}

Since #153489 merged, it hangs indefinitely trying to compile the following program (reduced from tests/ui/delegation/bad-resolve):

#![feature(fn_delegation)]#![allow(incomplete_features)]traitTrait{typeType;fnbar(){}}structF;implTraitforF{typeType = i32;}structS(F);implTraitforS{
reuse <FasTrait>::bar;}fnmain(){}

I'll step through how this program behaves on each commit in this PR.

---

1. Main branch: Hangs indefinitely with no output.

2. After first commit (adding safeguards to ensure_sufficient_stack):
   a. First, it prints a stacktrace that cuts off early
   b. Then it hangs forever.

3. After calling abort if ensure_sufficient_stack fails:
   a. First, it prints a ~250 frame stacktrace,
   including both the first and last hundred frames.
   b. Then it exits.

4. After adding debug logging, shows the stack trace from above, but first:
   a. Prints a trace of the query execution
   b. Shows the stacktrace from above.

5. After the bug fix: as above, but instead of the 5 million frame backtrace, prints a "more normal" query cycle:

error[E0391]: cycle detected when looking up span for `<impl at bad-resolve.rs:15:1: 15:17>::bar`
|
= note: ...which requires getting HIR ID of `<impl at bad-resolve.rs:15:1: 15:17>::bar`...
= note: ...which again requires looking up span for `<impl at bad-resolve.rs:15:1: 15:17>::bar`, completing the cycle
note: cycle used when computing codegen attributes of `Trait::bar`
--> bad-resolve.rs:6:5
|
6 | fn bar() {}
| ^^^^^^^^

---

Here's an extract of that query trace:

Details

rustc_query_impl::execution::try_execute_query query="hir_crate_items"
...
rustc_query_impl::execution::try_execute_query query="lower_delayed_owner"
...
rustc_query_impl::execution::try_execute_query query="codegen_fn_attrs"
rustc_query_impl::execution::try_execute_query query="hir_crate_items"
0ms INFO rustc_query_impl::execution hit a query cycle evaluating hir_crate_items!
rustc_query_impl::execution::try_execute_query query="def_span"
rustc_query_impl::execution::try_execute_query query="local_def_id_to_hir_id"
rustc_query_impl::execution::try_execute_query query="lower_delayed_owner"
0ms INFO rustc_query_impl::execution hit a query cycle evaluating lower_delayed_owner!
rustc_query_impl::execution::try_execute_query query="def_span"
0ms INFO rustc_query_impl::execution hit a query cycle evaluating def_span!
rustc_query_impl::execution::try_execute_query query="def_span"
...

I feel somewhat strongly that there is a bug here; it should not be possible to get query cycles within def_span.
In particular, note that the first two cycles are ignored altogether in order to report the
third cycle; that seems quite bad!

I'm not sure if the bug is in the implementation of lower_delayed_owner, in the query system, or merely in the fact that lower_delay_owner is a prerequisite of hir_items; I'm opening this PR to get more feedback on that. However, I think the commits here stand on their own.

In #153979 (comment) I mistakenly attributed this to recent refactors in the query system; I no longer think that's true, I think the infinite loop is a latent bug that's been around for years, and the new query cycle is related to lower_delayed_owner, not the query system itself.

cc @petrochenkov@aerooneqq@Zalathar

[rustbot]

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

The list of allowed third-party dependencies may have been modified! You must ensure that any new dependencies have compatible licenses before merging.

cc @davidtwco, @wesleywiser

[rustbot]

r? @JohnTitor

rustbot has assigned @JohnTitor.
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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 69 candidates
• Random selection from 12 candidates

[jyn514]

hm. maybe r? @Zalathar ?

[rustbot]

Zalathar is not on the review rotation at the moment.
They may take a while to respond.

[Zoxc]

Not sure about the query related changes here, but it makes sense to have proper stack limit so we don't OOM with infinite recursion.

[Zoxc]

@jyn514 You could try your test failures with #154389. We have some failing tests on main which that detects and the cause might be similar.

[jyn514]

I think the tests were wrong, not the code. I've pushed a commit updating them; the new cycle errors look much more informative to me.

[jyn514]

Here's a trace of the queries that caused the cycle for /query-cycle-issue-124901.rs:

 rustc_query_impl::execution::try_execute_query query="local_def_id_to_hir_id"
rustc_query_impl::execution::try_execute_query query="hir_crate"
rustc_query_impl::execution::try_execute_query query="early_lint_checks"
rustc_query_impl::execution::try_execute_query query="resolver_for_lowering_raw"
0ms INFO rustc_query_impl::execution hit a query cycle evaluating resolver_for_lowering_raw!
rustc_middle::queries::default_span self=resolver_for_lowering_raw(())
rustc_middle::queries::default_span self=local_def_id_to_hir_id(DefId(0:3 ~ query_cycle_issue_124901[dbc8]::Default))
rustc_middle::queries::default_span self=hir_crate(())
rustc_middle::queries::default_span self=early_lint_checks(())

Previously, default_span(local_def_id_to_hir_id) would call into def_span(def_id), which would call back into local_def_id_to_hir_id, causing a follow-up cycle. I don't think that follow-up cycle is useful to anyone.

[jyn514]

hm, I think this fix is incomplete. hir_span can also call queries (I've verified it calls at least hir_owner_parent_q and opt_hir_owner_nodes). I wonder if the query system should have some special "fallback cycle handler" where def_span can return to choose DUMMY_SP if there's an existing query that's executing. Then we don't have to hard-code all its dependencies in default_span.

[Zoxc]

I think we could check if HIR lowering is done for the DefId by checking if relevant queries have executed and only then try to get the span.

[jyn514]

Hm, I'm not sure how to do this. I found rustc_middle_query::inner::try_get_cached, so I thought I could do something like try_get_cached(TaggedQueryKey::def_span(key)).is_some(). But each query might have a different input type, and I don't know how to get from that other type back to a DefId. I don't see how to do this without modifying the implementation of def_span itself.

[jyn514]

Maybe we need something like QueryKey::def_id()?

[jyn514]

oh lol that already exists

[jyn514]

ok #154389 solves this in a completely different way and I'm not sure my approach makes sense anymore, so I'm just going to drop the last commit.

[Zoxc]

It doesn't fix any query cycles though, just improves the reporting.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[jyn514]

r? @Zoxc

[Zoxc]

It kind of seems like this could be an upstream feature of stacker. cc @nagisa

For 64-bit platforms, we could just skip stacker as we just have fixed size thread stacks again.

[programmerjake]

shouldn't this be using a drop-guard to update the value properly when unwinding?

View changes since the review

[rust-bors]

☔ The latest upstream changes (presumably #160535) made this pull request unmergeable. Please resolve the merge conflicts by rebasing.

[jyn514]

View all comments

This PR is somewhat complicated and I don't expect it to merge as-is. Let me start with a motivating example. The following rustc-driver worked fine a week ago:

structDriver;implCallbacksforDriver{fnconfig(&mutself,config:&mut interface::Config){
config.override_queries = Some(|_sess, providers| {
providers.queries.codegen_fn_attrs = |tcx, def_id| {let _ = tcx.hir_crate_items(());(rustc_interface::DEFAULT_QUERY_PROVIDERS.queries.codegen_fn_attrs)(tcx, def_id)};});}}fnmain() -> ExitCode{
rustc_driver::install_ice_hook("https://github.com/ferrocene/ferrocene/issues/new", |_| ());let handler = EarlyDiagCtxt::new(ErrorOutputType::default());
rustc_driver::init_rustc_env_logger(&handler);
rustc_driver::catch_with_exit_code(move || {let args:Vec<String> = std::env::args().collect();
rustc_driver::run_compiler(&args,&mutDriver)})}

Since #153489 merged, it hangs indefinitely trying to compile the following program (reduced from tests/ui/delegation/bad-resolve):

#![feature(fn_delegation)]#![allow(incomplete_features)]traitTrait{typeType;fnbar(){}}structF;implTraitforF{typeType = i32;}structS(F);implTraitforS{
reuse <FasTrait>::bar;}fnmain(){}

I'll step through how this program behaves on each commit in this PR.

---

1. Main branch: Hangs indefinitely with no output.

2. After first commit (adding safeguards to ensure_sufficient_stack):
   a. First, it prints a stacktrace that cuts off early
   b. Then it hangs forever.

3. After calling abort if ensure_sufficient_stack fails:
   a. First, it prints a ~250 frame stacktrace,
   including both the first and last hundred frames.
   b. Then it exits.

4. After adding debug logging, shows the stack trace from above, but first:
   a. Prints a trace of the query execution
   b. Shows the stacktrace from above.

5. After the bug fix: as above, but instead of the 5 million frame backtrace, prints a "more normal" query cycle:

error[E0391]: cycle detected when looking up span for `<impl at bad-resolve.rs:15:1: 15:17>::bar`
|
= note: ...which requires getting HIR ID of `<impl at bad-resolve.rs:15:1: 15:17>::bar`...
= note: ...which again requires looking up span for `<impl at bad-resolve.rs:15:1: 15:17>::bar`, completing the cycle
note: cycle used when computing codegen attributes of `Trait::bar`
--> bad-resolve.rs:6:5
|
6 | fn bar() {}
| ^^^^^^^^

---

Here's an extract of that query trace:

Details

rustc_query_impl::execution::try_execute_query query="hir_crate_items"
...
rustc_query_impl::execution::try_execute_query query="lower_delayed_owner"
...
rustc_query_impl::execution::try_execute_query query="codegen_fn_attrs"
rustc_query_impl::execution::try_execute_query query="hir_crate_items"
0ms INFO rustc_query_impl::execution hit a query cycle evaluating hir_crate_items!
rustc_query_impl::execution::try_execute_query query="def_span"
rustc_query_impl::execution::try_execute_query query="local_def_id_to_hir_id"
rustc_query_impl::execution::try_execute_query query="lower_delayed_owner"
0ms INFO rustc_query_impl::execution hit a query cycle evaluating lower_delayed_owner!
rustc_query_impl::execution::try_execute_query query="def_span"
0ms INFO rustc_query_impl::execution hit a query cycle evaluating def_span!
rustc_query_impl::execution::try_execute_query query="def_span"
...

I feel somewhat strongly that there is a bug here; it should not be possible to get query cycles within def_span.
In particular, note that the first two cycles are ignored altogether in order to report the
third cycle; that seems quite bad!

I'm not sure if the bug is in the implementation of lower_delayed_owner, in the query system, or merely in the fact that lower_delay_owner is a prerequisite of hir_items; I'm opening this PR to get more feedback on that. However, I think the commits here stand on their own.

In #153979 (comment) I mistakenly attributed this to recent refactors in the query system; I no longer think that's true, I think the infinite loop is a latent bug that's been around for years, and the new query cycle is related to lower_delayed_owner, not the query system itself.

cc @petrochenkov@aerooneqq@Zalathar

[rustbot]

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

The list of allowed third-party dependencies may have been modified! You must ensure that any new dependencies have compatible licenses before merging.

cc @davidtwco, @wesleywiser

[rustbot]

r? @JohnTitor

rustbot has assigned @JohnTitor.
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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 69 candidates
• Random selection from 12 candidates

[jyn514]

hm. maybe r? @Zalathar ?

[rustbot]

Zalathar is not on the review rotation at the moment.
They may take a while to respond.

[Zoxc]

Not sure about the query related changes here, but it makes sense to have proper stack limit so we don't OOM with infinite recursion.

[Zoxc]

@jyn514 You could try your test failures with #154389. We have some failing tests on main which that detects and the cause might be similar.

[jyn514]

I think the tests were wrong, not the code. I've pushed a commit updating them; the new cycle errors look much more informative to me.

[jyn514]

Here's a trace of the queries that caused the cycle for /query-cycle-issue-124901.rs:

 rustc_query_impl::execution::try_execute_query query="local_def_id_to_hir_id"
rustc_query_impl::execution::try_execute_query query="hir_crate"
rustc_query_impl::execution::try_execute_query query="early_lint_checks"
rustc_query_impl::execution::try_execute_query query="resolver_for_lowering_raw"
0ms INFO rustc_query_impl::execution hit a query cycle evaluating resolver_for_lowering_raw!
rustc_middle::queries::default_span self=resolver_for_lowering_raw(())
rustc_middle::queries::default_span self=local_def_id_to_hir_id(DefId(0:3 ~ query_cycle_issue_124901[dbc8]::Default))
rustc_middle::queries::default_span self=hir_crate(())
rustc_middle::queries::default_span self=early_lint_checks(())

Previously, default_span(local_def_id_to_hir_id) would call into def_span(def_id), which would call back into local_def_id_to_hir_id, causing a follow-up cycle. I don't think that follow-up cycle is useful to anyone.

[jyn514]

hm, I think this fix is incomplete. hir_span can also call queries (I've verified it calls at least hir_owner_parent_q and opt_hir_owner_nodes). I wonder if the query system should have some special "fallback cycle handler" where def_span can return to choose DUMMY_SP if there's an existing query that's executing. Then we don't have to hard-code all its dependencies in default_span.

[Zoxc]

I think we could check if HIR lowering is done for the DefId by checking if relevant queries have executed and only then try to get the span.

[jyn514]

Hm, I'm not sure how to do this. I found rustc_middle_query::inner::try_get_cached, so I thought I could do something like try_get_cached(TaggedQueryKey::def_span(key)).is_some(). But each query might have a different input type, and I don't know how to get from that other type back to a DefId. I don't see how to do this without modifying the implementation of def_span itself.

[jyn514]

Maybe we need something like QueryKey::def_id()?

[jyn514]

oh lol that already exists

[jyn514]

ok #154389 solves this in a completely different way and I'm not sure my approach makes sense anymore, so I'm just going to drop the last commit.

[Zoxc]

It doesn't fix any query cycles though, just improves the reporting.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[jyn514]

r? @Zoxc

[Zoxc]

It kind of seems like this could be an upstream feature of stacker. cc @nagisa

For 64-bit platforms, we could just skip stacker as we just have fixed size thread stacks again.

[programmerjake]

shouldn't this be using a drop-guard to update the value properly when unwinding?

View changes since the review

[rust-bors]

☔ The latest upstream changes (presumably #160535) made this pull request unmergeable. Please resolve the merge conflicts by rebasing.

[jyn514]

View all comments

This PR is somewhat complicated and I don't expect it to merge as-is. Let me start with a motivating example. The following rustc-driver worked fine a week ago:

structDriver;implCallbacksforDriver{fnconfig(&mutself,config:&mut interface::Config){
config.override_queries = Some(|_sess, providers| {
providers.queries.codegen_fn_attrs = |tcx, def_id| {let _ = tcx.hir_crate_items(());(rustc_interface::DEFAULT_QUERY_PROVIDERS.queries.codegen_fn_attrs)(tcx, def_id)};});}}fnmain() -> ExitCode{
rustc_driver::install_ice_hook("https://github.com/ferrocene/ferrocene/issues/new", |_| ());let handler = EarlyDiagCtxt::new(ErrorOutputType::default());
rustc_driver::init_rustc_env_logger(&handler);
rustc_driver::catch_with_exit_code(move || {let args:Vec<String> = std::env::args().collect();
rustc_driver::run_compiler(&args,&mutDriver)})}

Since #153489 merged, it hangs indefinitely trying to compile the following program (reduced from tests/ui/delegation/bad-resolve):

#![feature(fn_delegation)]#![allow(incomplete_features)]traitTrait{typeType;fnbar(){}}structF;implTraitforF{typeType = i32;}structS(F);implTraitforS{
reuse <FasTrait>::bar;}fnmain(){}

I'll step through how this program behaves on each commit in this PR.

---

1. Main branch: Hangs indefinitely with no output.

2. After first commit (adding safeguards to ensure_sufficient_stack):
   a. First, it prints a stacktrace that cuts off early
   b. Then it hangs forever.

3. After calling abort if ensure_sufficient_stack fails:
   a. First, it prints a ~250 frame stacktrace,
   including both the first and last hundred frames.
   b. Then it exits.

4. After adding debug logging, shows the stack trace from above, but first:
   a. Prints a trace of the query execution
   b. Shows the stacktrace from above.

5. After the bug fix: as above, but instead of the 5 million frame backtrace, prints a "more normal" query cycle:

error[E0391]: cycle detected when looking up span for `<impl at bad-resolve.rs:15:1: 15:17>::bar`
|
= note: ...which requires getting HIR ID of `<impl at bad-resolve.rs:15:1: 15:17>::bar`...
= note: ...which again requires looking up span for `<impl at bad-resolve.rs:15:1: 15:17>::bar`, completing the cycle
note: cycle used when computing codegen attributes of `Trait::bar`
--> bad-resolve.rs:6:5
|
6 | fn bar() {}
| ^^^^^^^^

---

Here's an extract of that query trace:

Details

rustc_query_impl::execution::try_execute_query query="hir_crate_items"
...
rustc_query_impl::execution::try_execute_query query="lower_delayed_owner"
...
rustc_query_impl::execution::try_execute_query query="codegen_fn_attrs"
rustc_query_impl::execution::try_execute_query query="hir_crate_items"
0ms INFO rustc_query_impl::execution hit a query cycle evaluating hir_crate_items!
rustc_query_impl::execution::try_execute_query query="def_span"
rustc_query_impl::execution::try_execute_query query="local_def_id_to_hir_id"
rustc_query_impl::execution::try_execute_query query="lower_delayed_owner"
0ms INFO rustc_query_impl::execution hit a query cycle evaluating lower_delayed_owner!
rustc_query_impl::execution::try_execute_query query="def_span"
0ms INFO rustc_query_impl::execution hit a query cycle evaluating def_span!
rustc_query_impl::execution::try_execute_query query="def_span"
...

I feel somewhat strongly that there is a bug here; it should not be possible to get query cycles within def_span.
In particular, note that the first two cycles are ignored altogether in order to report the
third cycle; that seems quite bad!

I'm not sure if the bug is in the implementation of lower_delayed_owner, in the query system, or merely in the fact that lower_delay_owner is a prerequisite of hir_items; I'm opening this PR to get more feedback on that. However, I think the commits here stand on their own.

In #153979 (comment) I mistakenly attributed this to recent refactors in the query system; I no longer think that's true, I think the infinite loop is a latent bug that's been around for years, and the new query cycle is related to lower_delayed_owner, not the query system itself.

cc @petrochenkov@aerooneqq@Zalathar

[rustbot]

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

The list of allowed third-party dependencies may have been modified! You must ensure that any new dependencies have compatible licenses before merging.

cc @davidtwco, @wesleywiser

[rustbot]

r? @JohnTitor

rustbot has assigned @JohnTitor.
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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 69 candidates
• Random selection from 12 candidates

[jyn514]

hm. maybe r? @Zalathar ?

[rustbot]

Zalathar is not on the review rotation at the moment.
They may take a while to respond.

[Zoxc]

Not sure about the query related changes here, but it makes sense to have proper stack limit so we don't OOM with infinite recursion.

[Zoxc]

@jyn514 You could try your test failures with #154389. We have some failing tests on main which that detects and the cause might be similar.

[jyn514]

I think the tests were wrong, not the code. I've pushed a commit updating them; the new cycle errors look much more informative to me.

[jyn514]

Here's a trace of the queries that caused the cycle for /query-cycle-issue-124901.rs:

 rustc_query_impl::execution::try_execute_query query="local_def_id_to_hir_id"
rustc_query_impl::execution::try_execute_query query="hir_crate"
rustc_query_impl::execution::try_execute_query query="early_lint_checks"
rustc_query_impl::execution::try_execute_query query="resolver_for_lowering_raw"
0ms INFO rustc_query_impl::execution hit a query cycle evaluating resolver_for_lowering_raw!
rustc_middle::queries::default_span self=resolver_for_lowering_raw(())
rustc_middle::queries::default_span self=local_def_id_to_hir_id(DefId(0:3 ~ query_cycle_issue_124901[dbc8]::Default))
rustc_middle::queries::default_span self=hir_crate(())
rustc_middle::queries::default_span self=early_lint_checks(())

Previously, default_span(local_def_id_to_hir_id) would call into def_span(def_id), which would call back into local_def_id_to_hir_id, causing a follow-up cycle. I don't think that follow-up cycle is useful to anyone.

[jyn514]

hm, I think this fix is incomplete. hir_span can also call queries (I've verified it calls at least hir_owner_parent_q and opt_hir_owner_nodes). I wonder if the query system should have some special "fallback cycle handler" where def_span can return to choose DUMMY_SP if there's an existing query that's executing. Then we don't have to hard-code all its dependencies in default_span.

[Zoxc]

I think we could check if HIR lowering is done for the DefId by checking if relevant queries have executed and only then try to get the span.

[jyn514]

Hm, I'm not sure how to do this. I found rustc_middle_query::inner::try_get_cached, so I thought I could do something like try_get_cached(TaggedQueryKey::def_span(key)).is_some(). But each query might have a different input type, and I don't know how to get from that other type back to a DefId. I don't see how to do this without modifying the implementation of def_span itself.

[jyn514]

Maybe we need something like QueryKey::def_id()?

[jyn514]

oh lol that already exists

[jyn514]

ok #154389 solves this in a completely different way and I'm not sure my approach makes sense anymore, so I'm just going to drop the last commit.

[Zoxc]

It doesn't fix any query cycles though, just improves the reporting.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[jyn514]

r? @Zoxc

[Zoxc]

It kind of seems like this could be an upstream feature of stacker. cc @nagisa

For 64-bit platforms, we could just skip stacker as we just have fixed size thread stacks again.

[programmerjake]

shouldn't this be using a drop-guard to update the value properly when unwinding?

View changes since the review

[rust-bors]

☔ The latest upstream changes (presumably #160535) made this pull request unmergeable. Please resolve the merge conflicts by rebasing.

[jyn514]

View all comments

This PR is somewhat complicated and I don't expect it to merge as-is. Let me start with a motivating example. The following rustc-driver worked fine a week ago:

structDriver;implCallbacksforDriver{fnconfig(&mutself,config:&mut interface::Config){
config.override_queries = Some(|_sess, providers| {
providers.queries.codegen_fn_attrs = |tcx, def_id| {let _ = tcx.hir_crate_items(());(rustc_interface::DEFAULT_QUERY_PROVIDERS.queries.codegen_fn_attrs)(tcx, def_id)};});}}fnmain() -> ExitCode{
rustc_driver::install_ice_hook("https://github.com/ferrocene/ferrocene/issues/new", |_| ());let handler = EarlyDiagCtxt::new(ErrorOutputType::default());
rustc_driver::init_rustc_env_logger(&handler);
rustc_driver::catch_with_exit_code(move || {let args:Vec<String> = std::env::args().collect();
rustc_driver::run_compiler(&args,&mutDriver)})}

Since #153489 merged, it hangs indefinitely trying to compile the following program (reduced from tests/ui/delegation/bad-resolve):

#![feature(fn_delegation)]#![allow(incomplete_features)]traitTrait{typeType;fnbar(){}}structF;implTraitforF{typeType = i32;}structS(F);implTraitforS{
reuse <FasTrait>::bar;}fnmain(){}

I'll step through how this program behaves on each commit in this PR.

---

1. Main branch: Hangs indefinitely with no output.

2. After first commit (adding safeguards to ensure_sufficient_stack):
   a. First, it prints a stacktrace that cuts off early
   b. Then it hangs forever.

3. After calling abort if ensure_sufficient_stack fails:
   a. First, it prints a ~250 frame stacktrace,
   including both the first and last hundred frames.
   b. Then it exits.

4. After adding debug logging, shows the stack trace from above, but first:
   a. Prints a trace of the query execution
   b. Shows the stacktrace from above.

5. After the bug fix: as above, but instead of the 5 million frame backtrace, prints a "more normal" query cycle:

error[E0391]: cycle detected when looking up span for `<impl at bad-resolve.rs:15:1: 15:17>::bar`
|
= note: ...which requires getting HIR ID of `<impl at bad-resolve.rs:15:1: 15:17>::bar`...
= note: ...which again requires looking up span for `<impl at bad-resolve.rs:15:1: 15:17>::bar`, completing the cycle
note: cycle used when computing codegen attributes of `Trait::bar`
--> bad-resolve.rs:6:5
|
6 | fn bar() {}
| ^^^^^^^^

---

Here's an extract of that query trace:

Details

rustc_query_impl::execution::try_execute_query query="hir_crate_items"
...
rustc_query_impl::execution::try_execute_query query="lower_delayed_owner"
...
rustc_query_impl::execution::try_execute_query query="codegen_fn_attrs"
rustc_query_impl::execution::try_execute_query query="hir_crate_items"
0ms INFO rustc_query_impl::execution hit a query cycle evaluating hir_crate_items!
rustc_query_impl::execution::try_execute_query query="def_span"
rustc_query_impl::execution::try_execute_query query="local_def_id_to_hir_id"
rustc_query_impl::execution::try_execute_query query="lower_delayed_owner"
0ms INFO rustc_query_impl::execution hit a query cycle evaluating lower_delayed_owner!
rustc_query_impl::execution::try_execute_query query="def_span"
0ms INFO rustc_query_impl::execution hit a query cycle evaluating def_span!
rustc_query_impl::execution::try_execute_query query="def_span"
...

I feel somewhat strongly that there is a bug here; it should not be possible to get query cycles within def_span.
In particular, note that the first two cycles are ignored altogether in order to report the
third cycle; that seems quite bad!

I'm not sure if the bug is in the implementation of lower_delayed_owner, in the query system, or merely in the fact that lower_delay_owner is a prerequisite of hir_items; I'm opening this PR to get more feedback on that. However, I think the commits here stand on their own.

In #153979 (comment) I mistakenly attributed this to recent refactors in the query system; I no longer think that's true, I think the infinite loop is a latent bug that's been around for years, and the new query cycle is related to lower_delayed_owner, not the query system itself.

cc @petrochenkov@aerooneqq@Zalathar

[rustbot]

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

The list of allowed third-party dependencies may have been modified! You must ensure that any new dependencies have compatible licenses before merging.

cc @davidtwco, @wesleywiser

[rustbot]

r? @JohnTitor

rustbot has assigned @JohnTitor.
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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 69 candidates
• Random selection from 12 candidates

[jyn514]

hm. maybe r? @Zalathar ?

[rustbot]

Zalathar is not on the review rotation at the moment.
They may take a while to respond.

[Zoxc]

Not sure about the query related changes here, but it makes sense to have proper stack limit so we don't OOM with infinite recursion.

[Zoxc]

@jyn514 You could try your test failures with #154389. We have some failing tests on main which that detects and the cause might be similar.

[jyn514]

I think the tests were wrong, not the code. I've pushed a commit updating them; the new cycle errors look much more informative to me.

[jyn514]

Here's a trace of the queries that caused the cycle for /query-cycle-issue-124901.rs:

 rustc_query_impl::execution::try_execute_query query="local_def_id_to_hir_id"
rustc_query_impl::execution::try_execute_query query="hir_crate"
rustc_query_impl::execution::try_execute_query query="early_lint_checks"
rustc_query_impl::execution::try_execute_query query="resolver_for_lowering_raw"
0ms INFO rustc_query_impl::execution hit a query cycle evaluating resolver_for_lowering_raw!
rustc_middle::queries::default_span self=resolver_for_lowering_raw(())
rustc_middle::queries::default_span self=local_def_id_to_hir_id(DefId(0:3 ~ query_cycle_issue_124901[dbc8]::Default))
rustc_middle::queries::default_span self=hir_crate(())
rustc_middle::queries::default_span self=early_lint_checks(())

Previously, default_span(local_def_id_to_hir_id) would call into def_span(def_id), which would call back into local_def_id_to_hir_id, causing a follow-up cycle. I don't think that follow-up cycle is useful to anyone.

[jyn514]

hm, I think this fix is incomplete. hir_span can also call queries (I've verified it calls at least hir_owner_parent_q and opt_hir_owner_nodes). I wonder if the query system should have some special "fallback cycle handler" where def_span can return to choose DUMMY_SP if there's an existing query that's executing. Then we don't have to hard-code all its dependencies in default_span.

[Zoxc]

I think we could check if HIR lowering is done for the DefId by checking if relevant queries have executed and only then try to get the span.

[jyn514]

Hm, I'm not sure how to do this. I found rustc_middle_query::inner::try_get_cached, so I thought I could do something like try_get_cached(TaggedQueryKey::def_span(key)).is_some(). But each query might have a different input type, and I don't know how to get from that other type back to a DefId. I don't see how to do this without modifying the implementation of def_span itself.

[jyn514]

Maybe we need something like QueryKey::def_id()?

[jyn514]

oh lol that already exists

[jyn514]

ok #154389 solves this in a completely different way and I'm not sure my approach makes sense anymore, so I'm just going to drop the last commit.

[Zoxc]

It doesn't fix any query cycles though, just improves the reporting.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[jyn514]

r? @Zoxc

[Zoxc]

It kind of seems like this could be an upstream feature of stacker. cc @nagisa

For 64-bit platforms, we could just skip stacker as we just have fixed size thread stacks again.

[programmerjake]

shouldn't this be using a drop-guard to update the value properly when unwinding?

View changes since the review

[rust-bors]

☔ The latest upstream changes (presumably #160535) made this pull request unmergeable. Please resolve the merge conflicts by rebasing.

[jyn514]

View all comments

This PR is somewhat complicated and I don't expect it to merge as-is. Let me start with a motivating example. The following rustc-driver worked fine a week ago:

structDriver;implCallbacksforDriver{fnconfig(&mutself,config:&mut interface::Config){
config.override_queries = Some(|_sess, providers| {
providers.queries.codegen_fn_attrs = |tcx, def_id| {let _ = tcx.hir_crate_items(());(rustc_interface::DEFAULT_QUERY_PROVIDERS.queries.codegen_fn_attrs)(tcx, def_id)};});}}fnmain() -> ExitCode{
rustc_driver::install_ice_hook("https://github.com/ferrocene/ferrocene/issues/new", |_| ());let handler = EarlyDiagCtxt::new(ErrorOutputType::default());
rustc_driver::init_rustc_env_logger(&handler);
rustc_driver::catch_with_exit_code(move || {let args:Vec<String> = std::env::args().collect();
rustc_driver::run_compiler(&args,&mutDriver)})}

Since #153489 merged, it hangs indefinitely trying to compile the following program (reduced from tests/ui/delegation/bad-resolve):

#![feature(fn_delegation)]#![allow(incomplete_features)]traitTrait{typeType;fnbar(){}}structF;implTraitforF{typeType = i32;}structS(F);implTraitforS{
reuse <FasTrait>::bar;}fnmain(){}

I'll step through how this program behaves on each commit in this PR.

---

1. Main branch: Hangs indefinitely with no output.

2. After first commit (adding safeguards to ensure_sufficient_stack):
   a. First, it prints a stacktrace that cuts off early
   b. Then it hangs forever.

3. After calling abort if ensure_sufficient_stack fails:
   a. First, it prints a ~250 frame stacktrace,
   including both the first and last hundred frames.
   b. Then it exits.

4. After adding debug logging, shows the stack trace from above, but first:
   a. Prints a trace of the query execution
   b. Shows the stacktrace from above.

5. After the bug fix: as above, but instead of the 5 million frame backtrace, prints a "more normal" query cycle:

error[E0391]: cycle detected when looking up span for `<impl at bad-resolve.rs:15:1: 15:17>::bar`
|
= note: ...which requires getting HIR ID of `<impl at bad-resolve.rs:15:1: 15:17>::bar`...
= note: ...which again requires looking up span for `<impl at bad-resolve.rs:15:1: 15:17>::bar`, completing the cycle
note: cycle used when computing codegen attributes of `Trait::bar`
--> bad-resolve.rs:6:5
|
6 | fn bar() {}
| ^^^^^^^^

---

Here's an extract of that query trace:

Details

rustc_query_impl::execution::try_execute_query query="hir_crate_items"
...
rustc_query_impl::execution::try_execute_query query="lower_delayed_owner"
...
rustc_query_impl::execution::try_execute_query query="codegen_fn_attrs"
rustc_query_impl::execution::try_execute_query query="hir_crate_items"
0ms INFO rustc_query_impl::execution hit a query cycle evaluating hir_crate_items!
rustc_query_impl::execution::try_execute_query query="def_span"
rustc_query_impl::execution::try_execute_query query="local_def_id_to_hir_id"
rustc_query_impl::execution::try_execute_query query="lower_delayed_owner"
0ms INFO rustc_query_impl::execution hit a query cycle evaluating lower_delayed_owner!
rustc_query_impl::execution::try_execute_query query="def_span"
0ms INFO rustc_query_impl::execution hit a query cycle evaluating def_span!
rustc_query_impl::execution::try_execute_query query="def_span"
...

I feel somewhat strongly that there is a bug here; it should not be possible to get query cycles within def_span.
In particular, note that the first two cycles are ignored altogether in order to report the
third cycle; that seems quite bad!

I'm not sure if the bug is in the implementation of lower_delayed_owner, in the query system, or merely in the fact that lower_delay_owner is a prerequisite of hir_items; I'm opening this PR to get more feedback on that. However, I think the commits here stand on their own.

In #153979 (comment) I mistakenly attributed this to recent refactors in the query system; I no longer think that's true, I think the infinite loop is a latent bug that's been around for years, and the new query cycle is related to lower_delayed_owner, not the query system itself.

cc @petrochenkov@aerooneqq@Zalathar

[rustbot]

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

The list of allowed third-party dependencies may have been modified! You must ensure that any new dependencies have compatible licenses before merging.

cc @davidtwco, @wesleywiser

[rustbot]

r? @JohnTitor

rustbot has assigned @JohnTitor.
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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 69 candidates
• Random selection from 12 candidates

[jyn514]

hm. maybe r? @Zalathar ?

[rustbot]

Zalathar is not on the review rotation at the moment.
They may take a while to respond.

[Zoxc]

Not sure about the query related changes here, but it makes sense to have proper stack limit so we don't OOM with infinite recursion.

[Zoxc]

@jyn514 You could try your test failures with #154389. We have some failing tests on main which that detects and the cause might be similar.

[jyn514]

I think the tests were wrong, not the code. I've pushed a commit updating them; the new cycle errors look much more informative to me.

[jyn514]

Here's a trace of the queries that caused the cycle for /query-cycle-issue-124901.rs:

 rustc_query_impl::execution::try_execute_query query="local_def_id_to_hir_id"
rustc_query_impl::execution::try_execute_query query="hir_crate"
rustc_query_impl::execution::try_execute_query query="early_lint_checks"
rustc_query_impl::execution::try_execute_query query="resolver_for_lowering_raw"
0ms INFO rustc_query_impl::execution hit a query cycle evaluating resolver_for_lowering_raw!
rustc_middle::queries::default_span self=resolver_for_lowering_raw(())
rustc_middle::queries::default_span self=local_def_id_to_hir_id(DefId(0:3 ~ query_cycle_issue_124901[dbc8]::Default))
rustc_middle::queries::default_span self=hir_crate(())
rustc_middle::queries::default_span self=early_lint_checks(())

Previously, default_span(local_def_id_to_hir_id) would call into def_span(def_id), which would call back into local_def_id_to_hir_id, causing a follow-up cycle. I don't think that follow-up cycle is useful to anyone.

[jyn514]

hm, I think this fix is incomplete. hir_span can also call queries (I've verified it calls at least hir_owner_parent_q and opt_hir_owner_nodes). I wonder if the query system should have some special "fallback cycle handler" where def_span can return to choose DUMMY_SP if there's an existing query that's executing. Then we don't have to hard-code all its dependencies in default_span.

[Zoxc]

I think we could check if HIR lowering is done for the DefId by checking if relevant queries have executed and only then try to get the span.

[jyn514]

Hm, I'm not sure how to do this. I found rustc_middle_query::inner::try_get_cached, so I thought I could do something like try_get_cached(TaggedQueryKey::def_span(key)).is_some(). But each query might have a different input type, and I don't know how to get from that other type back to a DefId. I don't see how to do this without modifying the implementation of def_span itself.

[jyn514]

Maybe we need something like QueryKey::def_id()?

[jyn514]

oh lol that already exists

[jyn514]

ok #154389 solves this in a completely different way and I'm not sure my approach makes sense anymore, so I'm just going to drop the last commit.

[Zoxc]

It doesn't fix any query cycles though, just improves the reporting.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[jyn514]

r? @Zoxc

[Zoxc]

It kind of seems like this could be an upstream feature of stacker. cc @nagisa

For 64-bit platforms, we could just skip stacker as we just have fixed size thread stacks again.

[programmerjake]

shouldn't this be using a drop-guard to update the value properly when unwinding?

View changes since the review

[rust-bors]

☔ The latest upstream changes (presumably #160535) made this pull request unmergeable. Please resolve the merge conflicts by rebasing.

[jyn514]

View all comments

This PR is somewhat complicated and I don't expect it to merge as-is. Let me start with a motivating example. The following rustc-driver worked fine a week ago:

structDriver;implCallbacksforDriver{fnconfig(&mutself,config:&mut interface::Config){
config.override_queries = Some(|_sess, providers| {
providers.queries.codegen_fn_attrs = |tcx, def_id| {let _ = tcx.hir_crate_items(());(rustc_interface::DEFAULT_QUERY_PROVIDERS.queries.codegen_fn_attrs)(tcx, def_id)};});}}fnmain() -> ExitCode{
rustc_driver::install_ice_hook("https://github.com/ferrocene/ferrocene/issues/new", |_| ());let handler = EarlyDiagCtxt::new(ErrorOutputType::default());
rustc_driver::init_rustc_env_logger(&handler);
rustc_driver::catch_with_exit_code(move || {let args:Vec<String> = std::env::args().collect();
rustc_driver::run_compiler(&args,&mutDriver)})}

Since #153489 merged, it hangs indefinitely trying to compile the following program (reduced from tests/ui/delegation/bad-resolve):

#![feature(fn_delegation)]#![allow(incomplete_features)]traitTrait{typeType;fnbar(){}}structF;implTraitforF{typeType = i32;}structS(F);implTraitforS{
reuse <FasTrait>::bar;}fnmain(){}

I'll step through how this program behaves on each commit in this PR.

---

1. Main branch: Hangs indefinitely with no output.

2. After first commit (adding safeguards to ensure_sufficient_stack):
   a. First, it prints a stacktrace that cuts off early
   b. Then it hangs forever.

3. After calling abort if ensure_sufficient_stack fails:
   a. First, it prints a ~250 frame stacktrace,
   including both the first and last hundred frames.
   b. Then it exits.

4. After adding debug logging, shows the stack trace from above, but first:
   a. Prints a trace of the query execution
   b. Shows the stacktrace from above.

5. After the bug fix: as above, but instead of the 5 million frame backtrace, prints a "more normal" query cycle:

error[E0391]: cycle detected when looking up span for `<impl at bad-resolve.rs:15:1: 15:17>::bar`
|
= note: ...which requires getting HIR ID of `<impl at bad-resolve.rs:15:1: 15:17>::bar`...
= note: ...which again requires looking up span for `<impl at bad-resolve.rs:15:1: 15:17>::bar`, completing the cycle
note: cycle used when computing codegen attributes of `Trait::bar`
--> bad-resolve.rs:6:5
|
6 | fn bar() {}
| ^^^^^^^^

---

Here's an extract of that query trace:

Details

rustc_query_impl::execution::try_execute_query query="hir_crate_items"
...
rustc_query_impl::execution::try_execute_query query="lower_delayed_owner"
...
rustc_query_impl::execution::try_execute_query query="codegen_fn_attrs"
rustc_query_impl::execution::try_execute_query query="hir_crate_items"
0ms INFO rustc_query_impl::execution hit a query cycle evaluating hir_crate_items!
rustc_query_impl::execution::try_execute_query query="def_span"
rustc_query_impl::execution::try_execute_query query="local_def_id_to_hir_id"
rustc_query_impl::execution::try_execute_query query="lower_delayed_owner"
0ms INFO rustc_query_impl::execution hit a query cycle evaluating lower_delayed_owner!
rustc_query_impl::execution::try_execute_query query="def_span"
0ms INFO rustc_query_impl::execution hit a query cycle evaluating def_span!
rustc_query_impl::execution::try_execute_query query="def_span"
...

I feel somewhat strongly that there is a bug here; it should not be possible to get query cycles within def_span.
In particular, note that the first two cycles are ignored altogether in order to report the
third cycle; that seems quite bad!

I'm not sure if the bug is in the implementation of lower_delayed_owner, in the query system, or merely in the fact that lower_delay_owner is a prerequisite of hir_items; I'm opening this PR to get more feedback on that. However, I think the commits here stand on their own.

In #153979 (comment) I mistakenly attributed this to recent refactors in the query system; I no longer think that's true, I think the infinite loop is a latent bug that's been around for years, and the new query cycle is related to lower_delayed_owner, not the query system itself.

cc @petrochenkov@aerooneqq@Zalathar

[rustbot]

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

The list of allowed third-party dependencies may have been modified! You must ensure that any new dependencies have compatible licenses before merging.

cc @davidtwco, @wesleywiser

[rustbot]

r? @JohnTitor

rustbot has assigned @JohnTitor.
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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 69 candidates
• Random selection from 12 candidates

[jyn514]

hm. maybe r? @Zalathar ?

[rustbot]

Zalathar is not on the review rotation at the moment.
They may take a while to respond.

[Zoxc]

Not sure about the query related changes here, but it makes sense to have proper stack limit so we don't OOM with infinite recursion.

[Zoxc]

@jyn514 You could try your test failures with #154389. We have some failing tests on main which that detects and the cause might be similar.

[jyn514]

I think the tests were wrong, not the code. I've pushed a commit updating them; the new cycle errors look much more informative to me.

[jyn514]

Here's a trace of the queries that caused the cycle for /query-cycle-issue-124901.rs:

 rustc_query_impl::execution::try_execute_query query="local_def_id_to_hir_id"
rustc_query_impl::execution::try_execute_query query="hir_crate"
rustc_query_impl::execution::try_execute_query query="early_lint_checks"
rustc_query_impl::execution::try_execute_query query="resolver_for_lowering_raw"
0ms INFO rustc_query_impl::execution hit a query cycle evaluating resolver_for_lowering_raw!
rustc_middle::queries::default_span self=resolver_for_lowering_raw(())
rustc_middle::queries::default_span self=local_def_id_to_hir_id(DefId(0:3 ~ query_cycle_issue_124901[dbc8]::Default))
rustc_middle::queries::default_span self=hir_crate(())
rustc_middle::queries::default_span self=early_lint_checks(())

Previously, default_span(local_def_id_to_hir_id) would call into def_span(def_id), which would call back into local_def_id_to_hir_id, causing a follow-up cycle. I don't think that follow-up cycle is useful to anyone.

[jyn514]

hm, I think this fix is incomplete. hir_span can also call queries (I've verified it calls at least hir_owner_parent_q and opt_hir_owner_nodes). I wonder if the query system should have some special "fallback cycle handler" where def_span can return to choose DUMMY_SP if there's an existing query that's executing. Then we don't have to hard-code all its dependencies in default_span.

[Zoxc]

I think we could check if HIR lowering is done for the DefId by checking if relevant queries have executed and only then try to get the span.

[jyn514]

Hm, I'm not sure how to do this. I found rustc_middle_query::inner::try_get_cached, so I thought I could do something like try_get_cached(TaggedQueryKey::def_span(key)).is_some(). But each query might have a different input type, and I don't know how to get from that other type back to a DefId. I don't see how to do this without modifying the implementation of def_span itself.

[jyn514]

Maybe we need something like QueryKey::def_id()?

[jyn514]

oh lol that already exists

[jyn514]

ok #154389 solves this in a completely different way and I'm not sure my approach makes sense anymore, so I'm just going to drop the last commit.

[Zoxc]

It doesn't fix any query cycles though, just improves the reporting.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[jyn514]

r? @Zoxc

[Zoxc]

It kind of seems like this could be an upstream feature of stacker. cc @nagisa

For 64-bit platforms, we could just skip stacker as we just have fixed size thread stacks again.

[programmerjake]

shouldn't this be using a drop-guard to update the value properly when unwinding?

View changes since the review

[rust-bors]

☔ The latest upstream changes (presumably #160535) made this pull request unmergeable. Please resolve the merge conflicts by rebasing.

[jyn514]

View all comments

This PR is somewhat complicated and I don't expect it to merge as-is. Let me start with a motivating example. The following rustc-driver worked fine a week ago:

structDriver;implCallbacksforDriver{fnconfig(&mutself,config:&mut interface::Config){
config.override_queries = Some(|_sess, providers| {
providers.queries.codegen_fn_attrs = |tcx, def_id| {let _ = tcx.hir_crate_items(());(rustc_interface::DEFAULT_QUERY_PROVIDERS.queries.codegen_fn_attrs)(tcx, def_id)};});}}fnmain() -> ExitCode{
rustc_driver::install_ice_hook("https://github.com/ferrocene/ferrocene/issues/new", |_| ());let handler = EarlyDiagCtxt::new(ErrorOutputType::default());
rustc_driver::init_rustc_env_logger(&handler);
rustc_driver::catch_with_exit_code(move || {let args:Vec<String> = std::env::args().collect();
rustc_driver::run_compiler(&args,&mutDriver)})}

Since #153489 merged, it hangs indefinitely trying to compile the following program (reduced from tests/ui/delegation/bad-resolve):

#![feature(fn_delegation)]#![allow(incomplete_features)]traitTrait{typeType;fnbar(){}}structF;implTraitforF{typeType = i32;}structS(F);implTraitforS{
reuse <FasTrait>::bar;}fnmain(){}

I'll step through how this program behaves on each commit in this PR.

---

1. Main branch: Hangs indefinitely with no output.

2. After first commit (adding safeguards to ensure_sufficient_stack):
   a. First, it prints a stacktrace that cuts off early
   b. Then it hangs forever.

3. After calling abort if ensure_sufficient_stack fails:
   a. First, it prints a ~250 frame stacktrace,
   including both the first and last hundred frames.
   b. Then it exits.

4. After adding debug logging, shows the stack trace from above, but first:
   a. Prints a trace of the query execution
   b. Shows the stacktrace from above.

5. After the bug fix: as above, but instead of the 5 million frame backtrace, prints a "more normal" query cycle:

error[E0391]: cycle detected when looking up span for `<impl at bad-resolve.rs:15:1: 15:17>::bar`
|
= note: ...which requires getting HIR ID of `<impl at bad-resolve.rs:15:1: 15:17>::bar`...
= note: ...which again requires looking up span for `<impl at bad-resolve.rs:15:1: 15:17>::bar`, completing the cycle
note: cycle used when computing codegen attributes of `Trait::bar`
--> bad-resolve.rs:6:5
|
6 | fn bar() {}
| ^^^^^^^^

---

Here's an extract of that query trace:

Details

rustc_query_impl::execution::try_execute_query query="hir_crate_items"
...
rustc_query_impl::execution::try_execute_query query="lower_delayed_owner"
...
rustc_query_impl::execution::try_execute_query query="codegen_fn_attrs"
rustc_query_impl::execution::try_execute_query query="hir_crate_items"
0ms INFO rustc_query_impl::execution hit a query cycle evaluating hir_crate_items!
rustc_query_impl::execution::try_execute_query query="def_span"
rustc_query_impl::execution::try_execute_query query="local_def_id_to_hir_id"
rustc_query_impl::execution::try_execute_query query="lower_delayed_owner"
0ms INFO rustc_query_impl::execution hit a query cycle evaluating lower_delayed_owner!
rustc_query_impl::execution::try_execute_query query="def_span"
0ms INFO rustc_query_impl::execution hit a query cycle evaluating def_span!
rustc_query_impl::execution::try_execute_query query="def_span"
...

I feel somewhat strongly that there is a bug here; it should not be possible to get query cycles within def_span.
In particular, note that the first two cycles are ignored altogether in order to report the
third cycle; that seems quite bad!

I'm not sure if the bug is in the implementation of lower_delayed_owner, in the query system, or merely in the fact that lower_delay_owner is a prerequisite of hir_items; I'm opening this PR to get more feedback on that. However, I think the commits here stand on their own.

In #153979 (comment) I mistakenly attributed this to recent refactors in the query system; I no longer think that's true, I think the infinite loop is a latent bug that's been around for years, and the new query cycle is related to lower_delayed_owner, not the query system itself.

cc @petrochenkov@aerooneqq@Zalathar

[rustbot]

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

The list of allowed third-party dependencies may have been modified! You must ensure that any new dependencies have compatible licenses before merging.

cc @davidtwco, @wesleywiser

[rustbot]

r? @JohnTitor

rustbot has assigned @JohnTitor.
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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 69 candidates
• Random selection from 12 candidates

[jyn514]

hm. maybe r? @Zalathar ?

[rustbot]

Zalathar is not on the review rotation at the moment.
They may take a while to respond.

[Zoxc]

Not sure about the query related changes here, but it makes sense to have proper stack limit so we don't OOM with infinite recursion.

[Zoxc]

@jyn514 You could try your test failures with #154389. We have some failing tests on main which that detects and the cause might be similar.

[jyn514]

I think the tests were wrong, not the code. I've pushed a commit updating them; the new cycle errors look much more informative to me.

[jyn514]

Here's a trace of the queries that caused the cycle for /query-cycle-issue-124901.rs:

 rustc_query_impl::execution::try_execute_query query="local_def_id_to_hir_id"
rustc_query_impl::execution::try_execute_query query="hir_crate"
rustc_query_impl::execution::try_execute_query query="early_lint_checks"
rustc_query_impl::execution::try_execute_query query="resolver_for_lowering_raw"
0ms INFO rustc_query_impl::execution hit a query cycle evaluating resolver_for_lowering_raw!
rustc_middle::queries::default_span self=resolver_for_lowering_raw(())
rustc_middle::queries::default_span self=local_def_id_to_hir_id(DefId(0:3 ~ query_cycle_issue_124901[dbc8]::Default))
rustc_middle::queries::default_span self=hir_crate(())
rustc_middle::queries::default_span self=early_lint_checks(())

Previously, default_span(local_def_id_to_hir_id) would call into def_span(def_id), which would call back into local_def_id_to_hir_id, causing a follow-up cycle. I don't think that follow-up cycle is useful to anyone.

[jyn514]

hm, I think this fix is incomplete. hir_span can also call queries (I've verified it calls at least hir_owner_parent_q and opt_hir_owner_nodes). I wonder if the query system should have some special "fallback cycle handler" where def_span can return to choose DUMMY_SP if there's an existing query that's executing. Then we don't have to hard-code all its dependencies in default_span.

[Zoxc]

I think we could check if HIR lowering is done for the DefId by checking if relevant queries have executed and only then try to get the span.

[jyn514]

Hm, I'm not sure how to do this. I found rustc_middle_query::inner::try_get_cached, so I thought I could do something like try_get_cached(TaggedQueryKey::def_span(key)).is_some(). But each query might have a different input type, and I don't know how to get from that other type back to a DefId. I don't see how to do this without modifying the implementation of def_span itself.

[jyn514]

Maybe we need something like QueryKey::def_id()?

[jyn514]

oh lol that already exists

[jyn514]

ok #154389 solves this in a completely different way and I'm not sure my approach makes sense anymore, so I'm just going to drop the last commit.

[Zoxc]

It doesn't fix any query cycles though, just improves the reporting.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[jyn514]

r? @Zoxc

[Zoxc]

It kind of seems like this could be an upstream feature of stacker. cc @nagisa

For 64-bit platforms, we could just skip stacker as we just have fixed size thread stacks again.

[programmerjake]

shouldn't this be using a drop-guard to update the value properly when unwinding?

View changes since the review

[rust-bors]

☔ The latest upstream changes (presumably #160535) made this pull request unmergeable. Please resolve the merge conflicts by rebasing.