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.

Sign up for GitHub

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

Jump to bottom

Add a new Rust attribute to support embedding debugger visualizers #91779

Merged
merged 2 commits into from
May 5, 2022
Merged

Add a new Rust attribute to support embedding debugger visualizers #91779

merged 2 commits into from
May 5, 2022

Conversation

ridwanabdillahi
Copy link
Contributor

@ridwanabdillahi ridwanabdillahi commented Dec 11, 2021 •
edited
Loading

Implemented this RFC to add support for embedding debugger visualizers into a PDB.

Added a new attribute #[debugger_visualizer] and updated the CrateMetadata to store debugger visualizers for crate dependencies.

RFC: rust-lang/rfcs#3191

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @michaelwoerister (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 11, 2021
@ridwanabdillahi ridwanabdillahi mentioned this pull request Dec 11, 2021
Closed
@bjorn3
Copy link
Member

bjorn3 commented Dec 12, 2021

How should this handle reproducible builds?

@camelid camelid added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 13, 2021
@ridwanabdillahi
Copy link
Contributor Author

How should this handle reproducible builds?

I do not believe this is supported with my current set of changes. I've added support for this in Cargo via fingerprinting but perhaps it would be best to add this in rustc as well via the same mechanism? Thoughts?

@bors
Copy link
Contributor

bors commented Jan 1, 2022

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

@bors
Copy link
Contributor

bors commented Jan 14, 2022

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

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@ridwanabdillahi ridwanabdillahi changed the title Add support for embedding Natvis files into the PDBs generated when using the MSVC toolchain Add support for new Rust attribute to support embedding debugger visualizers Mar 8, 2022
@ridwanabdillahi ridwanabdillahi changed the title Add support for new Rust attribute to support embedding debugger visualizers Add a new Rust attribute to support embedding debugger visualizers Mar 8, 2022
@ridwanabdillahi ridwanabdillahi mentioned this pull request Mar 14, 2022
Merged
@bors
Copy link
Contributor

bors commented Mar 27, 2022

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

@michaelwoerister
Copy link
Member

I'll take a closer look at this shortly, now that the RFC has been merged.

michaelwoerister
michaelwoerister reviewed Apr 12, 2022
View reviewed changes
Copy link
Member

@michaelwoerister michaelwoerister left a comment

Choose a reason for hiding this comment

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

Thanks again for the PR, @ridwanabdillahi! I did a first pass over the changes and left some comments.

compiler/rustc_codegen_ssa/src/back/link.rs Outdated Show resolved Hide resolved
compiler/rustc_middle/src/query/mod.rs Show resolved Hide resolved
compiler/rustc_metadata/src/rmeta/encoder.rs Outdated Show resolved Hide resolved
compiler/rustc_span/src/lib.rs Outdated Show resolved Hide resolved
compiler/rustc_span/src/lib.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_ssa/src/back/link.rs Outdated Show resolved Hide resolved
compiler/rustc_passes/src/debugger_visualizer.rs Outdated Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented May 5, 2022

📌 Commit 791bef5 has been approved by michaelwoerister

@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 May 5, 2022
@bors
Copy link
Contributor

bors commented May 5, 2022

⌛ Testing commit 791bef5 with merge a7d6768...

@bors
Copy link
Contributor

bors commented May 5, 2022

☀️ Test successful - checks-actions
Approved by: michaelwoerister
Pushing a7d6768 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 5, 2022
@bors bors merged commit a7d6768 into rust-lang:master May 5, 2022
@rustbot rustbot added this to the 1.62.0 milestone May 5, 2022
@bors bors mentioned this pull request May 5, 2022
Merged
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (a7d6768): comparison url.

Summary:

  • Primary benchmarks: 😿 relevant regressions found
  • Secondary benchmarks: 😿 relevant regressions found
Regressions 😿
(primary)		 Regressions 😿
(secondary)		 Improvements 🎉
(primary)		 Improvements 🎉
(secondary)		 All 😿 🎉
(primary)
count1 88 52 0 0 88
mean2 0.4% 0.5% N/A N/A 0.4%
max 0.9% 1.0% N/A N/A 0.9%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

Footnotes

  1. number of relevant changes ↩

  2. the arithmetic mean of the percent change ↩

@rustbot rustbot added the perf-regression Performance regression. label May 5, 2022
@ridwanabdillahi ridwanabdillahi deleted the natvis branch May 5, 2022 17:29
@michaelwoerister
Copy link
Member

Hm, this seems to introduce a noticeable performance regression. We'll have to take a look if we can reduce that.

@ridwanabdillahi
Copy link
Contributor Author

Yes I've started looking into this. Should I go ahead and open an issue for this as well?

@michaelwoerister
Copy link
Member

Yes, opening an issue is a good idea. Thanks, @ridwanabdillahi!

@ridwanabdillahi ridwanabdillahi mentioned this pull request May 6, 2022
Closed
@ridwanabdillahi
Copy link
Contributor Author

I created a new issue to dig into the above perf regression.

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label May 6, 2022
@ridwanabdillahi ridwanabdillahi mentioned this pull request May 6, 2022
Closed
@ridwanabdillahi ridwanabdillahi mentioned this pull request May 13, 2022
Merged
@ridwanabdillahi ridwanabdillahi mentioned this pull request May 27, 2022
Closed
5 tasks
@ridwanabdillahi
Copy link
Contributor Author

Tracking issue for the feature implemented by this PR: rust-lang/rust#95939

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 28, 2022
@GuillaumeGomez
Rollup merge of rust-lang#97028 - ridwanabdillahi:pretty-printer, r=m…
b6821ad 
…ichaelwoerister

Add support for embedding pretty printers via `#[debugger_visualizer]` attribute

Initial support for [RFC 3191](rust-lang/rfcs#3191) in PR rust-lang#91779 was scoped to supporting embedding NatVis files using a new attribute. This PR implements the pretty printer support as stated in the RFC mentioned above.

This change includes embedding pretty printers in the `.debug_gdb_scripts` just as the pretty printers for rustc are embedded today. Also added additional tests for embedded pretty printers. Additionally cleaned up error checking so all error checking is done up front regardless of the current target.

RFC: rust-lang/rfcs#3191
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 28, 2022
@GuillaumeGomez
Rollup merge of rust-lang#97028 - ridwanabdillahi:pretty-printer, r=m…
239287f 
…ichaelwoerister

Add support for embedding pretty printers via `#[debugger_visualizer]` attribute

Initial support for [RFC 3191](rust-lang/rfcs#3191) in PR rust-lang#91779 was scoped to supporting embedding NatVis files using a new attribute. This PR implements the pretty printer support as stated in the RFC mentioned above.

This change includes embedding pretty printers in the `.debug_gdb_scripts` just as the pretty printers for rustc are embedded today. Also added additional tests for embedded pretty printers. Additionally cleaned up error checking so all error checking is done up front regardless of the current target.

RFC: rust-lang/rfcs#3191
@gibbyfree gibbyfree mentioned this pull request Mar 2, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjorn3 bjorn3 bjorn3 left review comments

@petrochenkov petrochenkov petrochenkov left review comments

@michaelwoerister michaelwoerister michaelwoerister left review comments

Assignees

@michaelwoerister michaelwoerister

Labels
F-debugger_visualizer `#![feature(debugger_visualizer)]` merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. 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
Milestone
1.62.0
Development

Successfully merging this pull request may close these issues.
10 participants
@ridwanabdillahi @rust-highfive @bjorn3 @bors @rust-log-analyzer @michaelwoerister @rust-timer @petrochenkov @camelid @rustbot