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

debuginfo: Fix DW_AT_containing_type vtable debuginfo regression #93503

Merged

Conversation

michaelwoerister
Copy link
Member

@michaelwoerister michaelwoerister commented Jan 31, 2022

This PR brings back the DW_AT_containing_type attribute for vtables after it has accidentally been removed in #89597.

It also implements a more accurate description of vtables. Instead of describing them as an array of void pointers, the compiler will now emit a struct type description with a field for each entry of the vtable.

r? @wesleywiser

This PR should fix issue #93164.
The PR is blocked on #93154 because both of them modify the codegen/debug-vtable.rs test case.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 31, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 31, 2022
@michaelwoerister michaelwoerister added the A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) label Jan 31, 2022
@bors
Copy link
Contributor

bors commented Feb 2, 2022

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

…accidentally been

removed in rust-lang#89597.

Also describe vtables as structs with a field for each entry.
@michaelwoerister michaelwoerister force-pushed the fix-vtable-holder-debuginfo-regression branch from 1b29d98 to fc7f419 Compare February 3, 2022 09:11
@michaelwoerister
Copy link
Member Author

This is ready for review now.

compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs Outdated Show resolved Hide resolved
(format!("__method{}", index), void_pointer_type_debuginfo)
}
ty::VtblEntry::TraitVPtr(_) => {
(format!("__super_trait_ptr{}", index), void_pointer_type_debuginfo)
Copy link
Member

Choose a reason for hiding this comment

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

This points to the super trait's vtable? It would be good (eventually) if we could set the type correctly here. I'm not sure if we might need to walk the tree of vtables for some reason ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, there is definitely room for improvement here. Types of the fn pointers as well. I'll add a comment about the super trait pointer.

compiler/rustc_codegen_ssa/src/debuginfo/type_names.rs Outdated Show resolved Hide resolved
@wesleywiser
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Feb 8, 2022

📌 Commit ed21805 has been approved by wesleywiser

@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 Feb 8, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 9, 2022
…debuginfo-regression, r=wesleywiser

debuginfo: Fix DW_AT_containing_type vtable debuginfo regression

This PR brings back the `DW_AT_containing_type` attribute for vtables after it has accidentally been removed in rust-lang#89597.

It also implements a more accurate description of vtables. Instead of describing them as an array of void pointers, the compiler will now emit a struct type description with a field for each entry of the vtable.

r? `@wesleywiser`

This PR should fix issue rust-lang#93164.
~~The PR is blocked on rust-lang#93154 because both of them modify the `codegen/debug-vtable.rs` test case.~~
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 10, 2022
…askrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#91443 (Better suggestions when user tries to collect into an unsized `[_]`)
 - rust-lang#91504 (`#[used(linker)]` attribute)
 - rust-lang#93503 (debuginfo: Fix DW_AT_containing_type vtable debuginfo regression)
 - rust-lang#93753 (Complete removal of #[main] attribute from compiler)
 - rust-lang#93799 (Fix typo in `std::fmt` docs)
 - rust-lang#93813 (Make a few cleanup MIR passes public)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 6d40850 into rust-lang:master Feb 10, 2022
@rustbot rustbot added this to the 1.60.0 milestone Feb 10, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 25, 2022
…pemap, r=wesleywiser

debuginfo: Simplify TypeMap used during LLVM debuginfo generation.

This PR simplifies the TypeMap that is used in `rustc_codegen_llvm::debuginfo::metadata`. It was unnecessarily complicated because it was originally implemented when types were not yet normalized before codegen. So it did it's own normalization and kept track of multiple unnormalized types being mapped to a single unique id.

This PR is based on rust-lang#93503, which is not merged yet.

The PR also removes the arena used for allocating string ids and instead uses `InlinableString` from the [inlinable_string](https://crates.io/crates/inlinable_string) crate. That might not be the best choice, since that crate does not seem to be very actively maintained. The [flexible-string](https://crates.io/crates/flexible-string) crate would be an alternative.

r? `@ghost`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) 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.

5 participants