Implement new gdb/lldb pretty-printers #60826
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
artemmukhin
force-pushed
the
new-dbg-pretty-printers
branch
from
1eba6e9
to
03d864d
Compare
This comment has been minimized.
This comment has been minimized.
|
@ortem I think the pretty printer tests need to be updated to match the format of the IntelliJ printers. See the example line below for one of the errors in the log: |
|
It seems like the failures are due to the pretty printer not displaying the type name, empty structs being presented as |
artemmukhin
force-pushed
the
new-dbg-pretty-printers
branch
from
03d864d
to
52be2f9
Compare
This comment has been minimized.
This comment has been minimized.
artemmukhin
force-pushed
the
new-dbg-pretty-printers
branch
from
52be2f9
to
9fe612b
Compare
This comment has been minimized.
This comment has been minimized.
artemmukhin
force-pushed
the
new-dbg-pretty-printers
branch
from
9fe612b
to
94ce04e
Compare
This comment has been minimized.
This comment has been minimized.
artemmukhin
force-pushed
the
new-dbg-pretty-printers
branch
from
94ce04e
to
00a5a89
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
New pretty-printers use regex to choose a specific provider based on the type name. For example, it is expected that Currently, pretty-printers tests are failed because during these tests |
|
This seems great, but I think I'm really not a good choice for reviewer. |
|
Visiting for triage. Not sure who'd be an adequate reviewer for this. Anyone from @rust-lang/compiler? |
|
☔ The latest upstream changes (presumably #61827) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Ah, great to know that this PR. Existed. I was considering tweaking the current scripts, but they are going to be replaced? |
Mark-Simulacrum
added
S-inactive
and removed
S-waiting-on-review
|
I'm going to tentatively reassign this to @michaelwoerister but in some sense I suspect we just don't have reviewer capacity for this. cc @eddyb as well, since you're listed as an expert in debuginfo (though that's a little different to this). It may also be that this sort of PR needs to go through some design thinking on internals before being landed in this repository. |
|
I could be mistaken, but I think that these scripts effectively have no owner at this point. I think i'd be ok r+'ing this PR, though I'm not sure how many tests we have and so forth so there's definitely risk. It'd be great if somebody (maybe @ortem!) wants to try to take ownership of those scripts and keep maintaining, them, though! |
|
One concern I have with the PR is that I didn't see any tests for the new functionality -- @ortem do you think you'd be able to add some tests that show the new support for
|
|
In any case, the PR will need to be rebased. If that happens, I'm inclined to say r=me -- even without tests, though more tests would be great! (@eddyb mentioned that debuginfo tests may be broken) |
|
@ortem After/if this lands in nightly, could you make a post on the forums to tell people to try them out, just so we can catch any issues that might arise from them? Thanks! |
artemmukhin
force-pushed
the
new-dbg-pretty-printers
branch
from
00a5a89
to
9b8e08d
Compare
artemmukhin
force-pushed
the
new-dbg-pretty-printers
branch
from
d182144
to
3255a4a
Compare
|
@nikic Thank you! |
artemmukhin
force-pushed
the
new-dbg-pretty-printers
branch
from
3255a4a
to
8048354
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
artemmukhin
force-pushed
the
new-dbg-pretty-printers
branch
from
8048354
to
039324d
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
artemmukhin
force-pushed
the
new-dbg-pretty-printers
branch
from
039324d
to
2b0ae47
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
artemmukhin
force-pushed
the
new-dbg-pretty-printers
branch
from
2b0ae47
to
de2dd3c
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
☔ The latest upstream changes (presumably #71147) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@ortem can you resolve the conflicts and fix the failing test? thanks |
Dylan-DPC-zz
added
S-waiting-on-author
joelpalmer
added
S-waiting-on-author
|
Ping from Triage: @ortem closing due to inactivity. Please reopen with conflicts resolved. Thanks for the PR. |
|
Sorry for a long wait, @joelpalmer could you please reopen this PR? I've fixed the conflicts and I'm fixing failed tests right now, so I need this PR to run tests on macOS. |
|
@ortem Unfortunately it's not possible to reopen this PR, because the branch has been force-pushed in the meantime, and GitHub doesn't like that. You'll have to open a new one. |
|
If you force-push the commit from when you closed the PR, you can re-open it again. Just make sure to save your current commit on a different branch. |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jun 15, 2020
…elix Implement new gdb/lldb pretty-printers Reopened rust-lang#60826 This PR replaces current gdb and lldb pretty-printers with new ones that were originally written for [IntelliJ Rust](https://github.com/intellij-rust/intellij-rust/tree/master/prettyPrinters). The current state of lldb pretty-printers is poor, because [they don't use synthetic children](rust-lang#55586 (comment)). When I started to reimplement lldb pretty-printers with synthetic children support, I've found current version strange and hard to support. I think `debugger_pretty_printers_common.py` is overkill, so I got rid of it. The new pretty-printers have to support all types supported by current pretty-printers, and also support `Rc`, `Arc`, `Cell`, `Ref`, `RefCell`, `RefMut`, `HashMap`, `HashSet`. Fixes rust-lang#56252
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR replaces current gdb and lldb pretty-printers with new ones which were originally written for IntelliJ Rust.
The current state of lldb pretty-printers is poor, because they don't use synthetic children. When I started to reimplement lldb pretty-printers with synthetic children support, I've found current version strange and hard to support. I think
debugger_pretty_printers_common.pyis overkill, so I got rid of it.The new pretty-printers have to support all types supported by current pretty-printers, and also support
Rc,Arc,Cell,Ref,RefCell,RefMut,HashMap,HashSet.Fixes #56252