Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Ensure that THIR unsafety check is done before stealing it #115012

Merged
merged 1 commit into from
Aug 24, 2023

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Aug 20, 2023

This ensures that THIR unsafety check is done before stealing it by running it on the typeck root instead of on a closure, which does nothing.

Fixes #111520

@rustbot
Copy link
Collaborator

rustbot commented Aug 20, 2023

r? @b-naber

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added 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 Aug 20, 2023
@@ -56,7 +56,8 @@ pub(crate) fn closure_saved_names_of_captured_variables<'tcx>(
/// Construct the MIR for a given `DefId`.
fn mir_build(tcx: TyCtxt<'_>, def: LocalDefId) -> Body<'_> {
// Ensure unsafeck and abstract const building is ran before we steal the THIR.
tcx.ensure_with_value().thir_check_unsafety(def);
tcx.ensure_with_value()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated to this PR, but why is this ensure_with_value 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ensure verifies that the dependency is green, without computing anything if not required.
This means that a query that is ensured may need to be computed later if some code needs the result value.

ensure_with_value verifies that the result is available, computed or loadable from on-disk cache.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm.. this means that query side effects can be emitted twice? Once when marking a query green and once when executing the query later to recover the value. I guess we need to make sure that's benign.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is ok for diagnostics, as they are deduplicated. But this is a blocker for replaying any other side-effect. I've been trying to fix this to re-create DefId, but I'm not satisfied by my design.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an option to turn it off though. There's also extra logic for the parallel compiler to avoid emitting side effects multiple times, which is kind of unnecessary then.

How does DefId relate to this?

Representing side effects as forcible queries would avoid this issue, but does require work to support forcing arbitrary keys.

@cjgillot
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Aug 22, 2023

📌 Commit 4170ca4 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 Aug 22, 2023
@bors
Copy link
Contributor

bors commented Aug 22, 2023

⌛ Testing commit 4170ca4 with merge ba7e4ce028604a12acc8fc2632661f115083bff3...

@rust-log-analyzer
Copy link
Collaborator

The job aarch64-gnu failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
[RUSTC-TIMING] rustc_metadata test:false 62.101
   Compiling rustc_borrowck v0.0.0 (/checkout/compiler/rustc_borrowck)
[RUSTC-TIMING] rustc_traits test:false 15.571
   Compiling rustc_hir_typeck v0.1.0 (/checkout/compiler/rustc_hir_typeck)
##[error]The runner has received a shutdown signal. This can happen when the runner service is stopped, or a manually started runner is canceled.
##[group]Clock drift check
##[group]Clock drift check
Session terminated, killing shell... ...killed.
time="2023-08-23T00:02:03Z" level=error msg="error waiting for container: unexpected EOF"
##[error]The operation was canceled.
Cleaning up orphan processes

@bors
Copy link
Contributor

bors commented Aug 23, 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 Aug 23, 2023
@Zoxc
Copy link
Contributor Author

Zoxc commented Aug 23, 2023

Retry? Looks to be some CI issue?

@cjgillot
Copy link
Contributor

@bors retry

@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 Aug 23, 2023
@bors
Copy link
Contributor

bors commented Aug 24, 2023

⌛ Testing commit 4170ca4 with merge c9db1f8...

@bors
Copy link
Contributor

bors commented Aug 24, 2023

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing c9db1f8 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 24, 2023
@bors bors merged commit c9db1f8 into rust-lang:master Aug 24, 2023
11 checks passed
@rustbot rustbot added this to the 1.74.0 milestone Aug 24, 2023
@Zoxc Zoxc deleted the thir-check-root branch August 24, 2023 02:27
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c9db1f8): 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)
5.8% [5.8%, 5.8%] 1
Regressions ❌
(secondary)
3.9% [3.0%, 4.7%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 5.8% [5.8%, 5.8%] 1

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)
4.6% [3.9%, 5.0%] 6
Regressions ❌
(secondary)
2.0% [2.0%, 2.0%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 4.6% [3.9%, 5.0%] 6

Binary size

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

Bootstrap: 635.876s -> 636.596s (0.11%)
Artifact size: 347.03 MiB -> 347.02 MiB (-0.00%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

ICE: parallel compiler: attempted to read from stolen value: rustc_middle::thir::Thir : -Zthir-unsafeck=yes
8 participants