Skip to content

Avoid calling queries during query stack printing #112708

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 3 commits into from
Jun 28, 2023

Conversation

flip1995
Copy link
Member

@flip1995 flip1995 commented Jun 16, 2023

This has the side effect, that when Clippy should ICE (during an EarlyPass?) it will fill up the RAM with 2 GB/s and then freezes my Laptop. This is blocking the Clippy sync and might give some people really bad experiences, so this should be merged ASAP.

r? @cjgillot
cc @Zoxc

I only commented this on Zulip. I should've left a comment on the PR as well. My bad.

@flip1995 flip1995 added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jun 16, 2023
@rustbot rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 16, 2023
@cjgillot
Copy link
Contributor

@flip1995 do you have a reproducer to add as a test? We wouldn't want this to come back again.
@Zoxc any idea of what's happening?

@flip1995
Copy link
Member Author

flip1995 commented Jun 16, 2023

I posted a reproducer on Zulip:

  • Add default = ["internal"] to the features in src/tools/clippy/Cargo.toml
  • x.py build src/tools/clippy
  • Run
    LD_LIBRARY_PATH=build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/x86_64-unknown-linux-gnu/lib ./build/x86_64-unknown-linux-gnu/stage1-tools-bin/clippy-driver src/tools/clippy/tests/ui-internal/custom_ice_message.rs
    

This would be automatically tested, if we'd enable the internal feature when testing Clippy.

@Zoxc
Copy link
Contributor

Zoxc commented Jun 16, 2023

For #112522, there's infinite stack recursion (collecting active queries -> running queries to get a query description -> triggers a cycle error -> collecting active queries -> ...). There's probably something similar happening with the Clippy ICE. I have a workaround in #112603.

We should probably not OOM on infinite stack recursion though. The compiler uses stacker for unbounded segmented stacks.

@lukas-code
Copy link
Member

Smaller reproducer:

fn main() {
    #[deny(while_true)]
    while true {}
}
rustc foo.rs -Ztreat-err-as-bug

@flip1995
Copy link
Member Author

Thanks I'll add this test to the PR.

@flip1995
Copy link
Member Author

When I add this test, I now get

running 1 tests
F

failures:

---- [ui] tests/ui/panics/panic-causes-oom-112708.rs stdout ----

error: Error: expected failure status (Some(101)) but received status None.
status: signal: 6 (SIGABRT) (core dumped)
...

because of the issue of the double panic that #112333 tried to fix:

query stack during panic:
thread panicked while processing panic. aborting.

Can I add some test flags, that would prevent that from happening?

@matthiaskrgr
Copy link
Member

Is it because rustc "crashes" inside the test?

There are a couple of tests that crash intentionally, for example:

tests/ui/treat-err-as-bug/delay_span_bug.rs

// compile-flags: -Ztreat-err-as-bug
// failure-status: 101
// error-pattern: aborting due to `-Z treat-err-as-bug=1`
// error-pattern: [trigger_delay_span_bug] triggering a delay span bug
// normalize-stderr-test "note: .*\n\n" -> ""
// normalize-stderr-test "thread 'rustc' panicked.*\n" -> ""
// rustc-env:RUST_BACKTRACE=0

#![feature(rustc_attrs)]

#[rustc_error(delay_span_bug_from_inside_query)]
fn main() {}

@flip1995
Copy link
Member Author

Is it because rustc "crashes" inside the test?

No, that's not the issue. I have the same header as this file. The issue is, that rustc crashes, while crashing. So the failure-status becomes None, as opposed to Some(101), which it should be for an ICE.

@rust-log-analyzer

This comment has been minimized.

@flip1995 flip1995 force-pushed the clippy-freezing-pc-with-ice branch from f486934 to 68ecff3 Compare June 22, 2023 13:16
@flip1995
Copy link
Member Author

@rustbot review

@flip1995
Copy link
Member Author

friendly ping @cjgillot I added a test for it. It also shows the double panic problem, that #112333 tried to solve. It would be great to merge this, so I can do the Clippy sync.

@cjgillot
Copy link
Contributor

Thanks!
@bors r+

@bors
Copy link
Collaborator

bors commented Jun 22, 2023

📌 Commit 68ecff3 has been approved by cjgillot

It is now in the queue for this repository.

@bors 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 Jun 22, 2023
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jun 22, 2023
…ce, r=cjgillot

Revert "Don't hold the active queries lock while calling `make_query`"

This reverts commit fd3d2d4.

This has the side effect, that when Clippy should ICE (during an EarlyPass?) it will fill up the RAM with 2 GB/s and then freezes the PC. I don't know the correct solution, but this is blocking the Clippy sync and might give some people really bad experiences, so this should be reverted ASAP.

Reverts rust-lang#112333

r? `@cjgillot`
cc `@Zoxc`

I only commented this on [Zulip](https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/.60try_print_query_stack.60.20has.20.60ImplicitCtx.60.20during.20.60EarlyPass.60/near/363926180). I should've left a comment on the PR as well. My bad.
@bors
Copy link
Collaborator

bors commented Jun 22, 2023

⌛ Testing commit 68ecff3 with merge 1cc7058e46560a6bb0718104a6f3caad0a5a5d77...

@bors
Copy link
Collaborator

bors commented Jun 23, 2023

💥 Test timed out

@flip1995 flip1995 force-pushed the clippy-freezing-pc-with-ice branch from 4bc718a to b0142f6 Compare June 27, 2023 14:15
@flip1995
Copy link
Member Author

Done. I kept it as 2 commits. One for adding the test and one for the fix. Something something TDD 😁

@oli-obk
Copy link
Contributor

oli-obk commented Jun 27, 2023

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 27, 2023

📌 Commit b0142f6 has been approved by oli-obk

It is now in the queue for this repository.

@bors 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 Jun 27, 2023
@flip1995 flip1995 changed the title Revert "Don't hold the active queries lock while calling make_query" Avoid calling queries during query stack printing Jun 27, 2023
@matthiaskrgr
Copy link
Member

@bors p=10

@bors
Copy link
Collaborator

bors commented Jun 27, 2023

⌛ Testing commit b0142f6 with merge 9c0609f82bb82d2d0cac50833c54f3788680a029...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented Jun 27, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 27, 2023
@oli-obk
Copy link
Contributor

oli-obk commented Jun 28, 2023

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 28, 2023

📌 Commit 7c15779 has been approved by oli-obk

It is now in the queue for this repository.

@bors 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 Jun 28, 2023
@bors
Copy link
Collaborator

bors commented Jun 28, 2023

⌛ Testing commit 7c15779 with merge 8882507...

@bors
Copy link
Collaborator

bors commented Jun 28, 2023

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 8882507 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 28, 2023
@bors bors merged commit 8882507 into rust-lang:master Jun 28, 2023
@rustbot rustbot added this to the 1.72.0 milestone Jun 28, 2023
@flip1995 flip1995 deleted the clippy-freezing-pc-with-ice branch June 28, 2023 12:25
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (8882507): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
3.9% [2.2%, 5.5%] 2
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 3.9% [2.2%, 5.5%] 2

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.8% [-3.2%, -1.1%] 6
Improvements ✅
(secondary)
-5.0% [-8.5%, -2.1%] 12
All ❌✅ (primary) -1.8% [-3.2%, -1.1%] 6

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 662.942s -> 662.642s (-0.05%)

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.