-
Notifications
You must be signed in to change notification settings - Fork 255
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
Use the Rust project's existing formatter #997
Comments
Yeah, I've considered doing this... However, up until this got merged, vanilla lldb was lacking support for Rust enums, so I had to carry Rust-specific patches in my fork of lldb, and this was not compatible with rust-lldb formatters.
I gotta warn you that I'm rather fond of my own implementation, so if by consolidating you mean basing implementation on rust-lldb formatters, I'll probably decline that patch. Going in the other direction might also be met with pushback: the rust-lldb formatters do not handle peculiarities of the -windows-msvc debug info, since Rust devs expect those users to be using a debugger based on WinDbg. |
Vadim: I was speaking with Vladimir Makaev (the person who landed Rust enum support in lldb) and some folks who work on LLDB (Greg Clayton) about upstreaming your formatters into either lldb itself or rust-lldb. Two questions:
|
@davidbarsky I'm happy to help implementing decoding for enums |
@VladimirMakaev At the moment, CodeLLDB ships a custom version of liblldb, which includes Tom Tromey's implementation of TypeSystem for Rust. This allows LLDB to understand DWARF discriminated unions natively. However, in the face of constant TypeSystem API changes, porting this patch forward has proven quite time-consuming, so I am considering dropping it in favor of just using your patch (with appropriate synthetic provider), even though this causes some regressions compared to the previous implementation (e.g. i32 type appearing in Rust code as "int", etc). BTW, I had a thought in the process of implementing the new enum formatter: it'd be nice if Rust enum types were tagged with a specific keyword - this would allow matching them with a regex, instead of attaching a "wildcard" synth provider to all types and determining whether it's one of the enum types dynamically. Rustc does this for msvc ABI by changing type names to @davidbarsky |
Am I correct in thinking that this is where the bulk of the TypeSystem for Rust that you mentioned lives? If so, I recall @clayborg being pretty receptive to (and was, in fact, asking!) for a Rust TypeSystem implementation to live in-tree with LLDB. If you don't want to do it, I'm pretty sure that I (or @VladimirMakaev, if he'd like to!) would be happy to try porting over the
Ah, gotcha. I didn't realize the scope of the changes, since I was able to successfully load of some of your synthetic formatters (e.g., for |
Yeah, that's the one.
IIRC, he also wanted a firm promise to maintain and improve it afterwards. Personally, I don't have the bandwidth to take upon such a commitment, but if you feel like it, you're welcome to have a go. |
Rust already has built-in support for creating and enabling a "Rust" category with LLDB, see
rust-lldb
.I think it'd make sense for CodeLLDB to defer to that in some way, both reduce the maintenance burden on your project, and to increase the quality of both
rust-lldb
and CodeLLDB. I can think of two ways to do so:rustc --print sysroot
and find the LLDB files, likerust-lldb
is doing nowOption 1 would tie the debugging symbols to the current Rust version that the user is using, which is kinda nice since we'd automatically support all Rust versions, but also a bit troublesome since it's not ensured that makes the fact that python files are present is a part of Rust's stability promise. Also might be difficult to actually determine the Rust version that the user's binary was compiled with, not sure it's worth it.
Option 2 would be the easiest.
Note: I'll admit that Rust's builtin formatter is a bit lacking compared to your formatter, I'd volunteer to do the work of consolidating them if you would be on board with this?
The text was updated successfully, but these errors were encountered: