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

Fix ElaborateBoxDerefs on debug varinfo #128572

Merged

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Aug 2, 2024

Slightly simplifies the ElaborateBoxDerefs pass to fix cases where it was applying the wrong projections to debug var infos containing places that deref boxes.

From what I can tell1, we don't actually have any tests (or code anywhere, really) that exercise debug x => *(...: Box<T>), and it's very difficult to trigger this in surface Rust, so I wrote a custom MIR test.

What happens is that the pass was turning *(SOME_PLACE: Box<T>) into *(*((((SOME_PLACE).0: Unique<T>).0: NonNull<T>).0: *const T)) in debug var infos. In particular, notice the double deref, which was wrong.

This is the root cause of #128554, so this PR fixes #128554 as well. The reason that async closures was affected is because of the way that we compute the ByMove body, which resulted in *(...: Box<T>) in debug var info. But this really has nothing to do with async closures.

Footnotes

  1. Validated by literally replacing the if elem == PlaceElem::Deref && base_ty.is_box() { ... } innards with a panic!(), which compiled all of stage2 without panicking.

@rustbot
Copy link
Collaborator

rustbot commented Aug 2, 2024

r? @jieyouxu

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

@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 2, 2024
@rustbot
Copy link
Collaborator

rustbot commented Aug 2, 2024

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt


let (unique_ty, nonnull_ty, ptr_ty) =
build_ptr_tys(tcx, base_ty.boxed_ty(), unique_did, nonnull_did);

new_projections.extend_from_slice(&base.projection[last_deref..]);
Copy link
Member Author

@compiler-errors compiler-errors Aug 2, 2024

Choose a reason for hiding this comment

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

I think the problem here has to do with the indexing of this slice (and the other [last_deref..] below), but I think it was pretty difficult to follow so I simplified the logic to just build up the projections more simply, one at a time.

@jieyouxu
Copy link
Member

jieyouxu commented Aug 2, 2024

Rerolling since I'm not familiar with this. r? mir

@rustbot rustbot assigned saethlin and unassigned jieyouxu Aug 2, 2024
@saethlin
Copy link
Member

we don't actually have any tests (or code anywhere, really) that exercise debug x => *(...: Box<T>)

Originally I was a bit surprised by this then I started looking at our debuginfo test suite more and now I'm not surprised at all. It's a Christmas Miracle that debuggers work as well with Rust as they do.

@bors r+ rollup=iffy

@bors
Copy link
Contributor

bors commented Aug 10, 2024

📌 Commit 2e52d61 has been approved by saethlin

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 10, 2024
@bors
Copy link
Contributor

bors commented Aug 10, 2024

⌛ Testing commit 2e52d61 with merge 730d5d4...

@bors
Copy link
Contributor

bors commented Aug 10, 2024

☀️ Test successful - checks-actions
Approved by: saethlin
Pushing 730d5d4 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 10, 2024
@bors bors merged commit 730d5d4 into rust-lang:master Aug 10, 2024
7 checks passed
@rustbot rustbot added this to the 1.82.0 milestone Aug 10, 2024
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
  PR_MESSAGE: Automation to keep dependencies in `Cargo.lock` current.
following is the output from `cargo update`:
  COMMIT_MESSAGE: cargo update 
##[endgroup]
info: syncing channel updates for 'beta-2024-07-26-x86_64-unknown-linux-gnu'
info: latest update on 2024-07-26, rust version 1.81.0-beta.2 (08328a323 2024-07-25)
info: downloading component 'rust-std'
info: downloading component 'rustc'
info: installing component 'cargo'
info: installing component 'rust-std'
info: installing component 'rust-std'
info: installing component 'rustc'

  beta-2024-07-26-x86_64-unknown-linux-gnu installed - rustc 1.81.0-beta.2 (08328a323 2024-07-25)

info: using existing install for 'beta-2024-07-26-x86_64-unknown-linux-gnu'
info: default toolchain set to 'beta-2024-07-26-x86_64-unknown-linux-gnu'

  beta-2024-07-26-x86_64-unknown-linux-gnu unchanged - rustc 1.81.0-beta.2 (08328a323 2024-07-25)

##[group]Run cargo update 2>&1 | sed '/crates.io index/d' | tee -a cargo_update.log
cargo update 2>&1 | sed '/crates.io index/d' | tee -a cargo_update.log
---
    Updating zerocopy v0.7.34 -> v0.7.35
    Updating zerocopy-derive v0.7.34 -> v0.7.35
    Updating zerovec v0.10.2 -> v0.10.4
    Updating zerovec-derive v0.10.2 -> v0.10.3
note: pass `--verbose` to see 89 unchanged dependencies behind latest
##[group]Run echo -e "\nrustbook dependencies:" >> cargo_update.log
echo -e "\nrustbook dependencies:" >> cargo_update.log
cargo update --manifest-path src/tools/rustbook 2>&1 | sed '/crates.io index/d' | tee -a cargo_update.log

@compiler-errors
Copy link
Member Author

wtf

@saethlin
Copy link
Member

@saethlin
Copy link
Member

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (730d5d4): 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 (secondary 1.5%)

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)
5.5% [5.5%, 5.5%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.5% [-2.5%, -2.5%] 1
All ❌✅ (primary) - - 0

Cycles

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

Binary size

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

Bootstrap: 756.781s -> 757.993s (0.16%)
Artifact size: 339.30 MiB -> 339.27 MiB (-0.01%)

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 in async closure that takes ref to boxed dyn trait
7 participants