-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Add rust-lldb pretty printing for Path and PathBuf #120557
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
These both work in my local testing, providing human-readable output for For the non-UTF8, I tested with: use std::ffi::OsStr;
use std::os::unix::ffi::OsStrExt;
use std::path::{Path, PathBuf};
fn main() {
let my_pathbuf: PathBuf = OsStr::from_bytes(&[0x66, 0x6f, 0x80, 0x6f]).into();
let my_path: &Path = my_pathbuf.as_path();
println!("{}", my_path.display());
println!("{}", my_pathbuf.display());
} $ ./rust-lldb -o 'break set --file main.rs --line 8' -o run -o 'po my_pathbuf' target/debug/foo
...
(lldb) po my_pathbuf
"fo�o" Or for the path:
I need some feedback on the regex used to identify the
When wouldn't the latter work? Are the capture groups getting used elsewhere that I failed to notice?
Happy to add these in to the regexes I used, just hoping to understand. |
6d6c72b
to
1e77350
Compare
I'm not sure why/whether we need those capture groups, but would prefer to have this continue using them for consistency if nothing else. I think it would also be good to add some tests verifying this functionality works. I think you should be able to look at e.g. tests/debuginfo/rc_arc.rs and mirror that for PathBuf/OsString. @rustbot author |
Sounds great, thanks for feedback. Will work on it.
If they're unnecessary (and confusing), would it make sense to ensure consistency by removing the others? Happy to format mine to match in the meantime, just curious / hoping to learn. It looks like much of this was added by @artemmukhin in 47c26e6 -- do you have any insight regarding the above, specifically:
Tests sound great. Thanks for the tip to check out |
@n8henrie Thanks for reaching out. I recall that when I started working on Rust pretty-printers in 2018, the Rust stdlib was undergoing changes, and entities were shifting between different packages. For instance, in Rust 1.20, we saw movements like For a reference on comprehensive and well-maintained collection of Rust LLDB pretty-printers, I suggest looking into the CodeLLDB. Particularly, the regex patterns used by CodeLLDB are simpler, for instance |
Thank you @artemmukhin! So it sounds like the more complicated regexes were in large part planning ahead for a problem that never manifested and no longer seems to be a concern. They would certainly be a lot more readable if simplified. @Mark-Simulacrum does this change your opinion at all about how I proceed? For example, I'd be happy to match the existing pattern for now and open a separate PR to consider simplifying all of the patterns. Or perhaps just adding a code comment linking to the above explanation for future searchers. |
I think matching for now and filing a separate PR updating all of them is best, perhaps confirming that we have tests for each of them. |
This comment has been minimized.
This comment has been minimized.
I need to learn more about the test infra. For some reason running the debug tests like so: Is not using my modifications to lldb_commands -- I can see in the output that it gets to
I have cleaned ( There seems to be a variety of copies of the file, some which include my previous commit and some which don't: $ fd -u lldb_commands -X rg --files-with-matches Path
./build/aarch64-apple-darwin/stage2/lib/rustlib/etc/lldb_commands
./src/etc/lldb_commands
$ fd -u lldb_commands -X rg --files-without-match Path
./build/aarch64-apple-darwin/stage0/lib/rustlib/etc/lldb_commands
./build/aarch64-apple-darwin/ci-rustc/lib/rustlib/etc/lldb_commands
./build/aarch64-apple-darwin/rustfmt/lib/rustlib/etc/lldb_commands Ah, it looks like the rust/src/tools/compiletest/src/runtest.rs Line 1479 in 8f359be
That seems like an unusual choice. Wouldn't it be better to |
This comment has been minimized.
This comment has been minimized.
According to logic in rust-lang#120557 (comment) the capture groups may be unnecessary and could possibly go with something as simple as `std::path::PathBuf` as done in CodeLLDB: https://github.com/vadimcn/codelldb/blob/05502bf75e4e7878a99b0bf0a7a81bba2922cbe3/formatters/rust.py#L59C56-L59C74 However for consistency will follow the pattern for now and consider updating all of regexes in a future PR.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@Mark-Simulacrum I think I have a little better understanding of the infra and all tests passing now. Based on the helpful context form @artemmukhin, once / if this is merged, I'll open a new PR with simplified regexes that I think will be far less confusing to new contributors such as myself, as it seems some of the capture groups have never been necessary, and others seem no longer necessary. |
☔ The latest upstream changes (presumably #121885) made this pull request unmergeable. Please resolve the merge conflicts. |
Some changes occurred in src/tools/compiletest cc @jieyouxu |
Ugh, some mistakes in my rebase. Will fix. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…crum Add rust-lldb pretty printing for Path and PathBuf Fixes rust-lang#120553 Fixes rust-lang#48462
💔 Test failed - checks-actions |
This comment has been minimized.
This comment has been minimized.
Looks like other PRs may be failing with that same error ( |
you're fine |
…crum Add rust-lldb pretty printing for Path and PathBuf Fixes rust-lang#120553 Fixes rust-lang#48462
This comment has been minimized.
This comment has been minimized.
💔 Test failed - checks-actions |
@bors r+ |
☀️ Test successful - checks-actions |
Finished benchmarking commit (69fa40c): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis 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.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 671.487s -> 670.4s (-0.16%) |
Fixes #120553
Fixes #48462