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

Tracking Issue for debugger_visualizer #95939

Closed
3 of 5 tasks
wesleywiser opened this issue Apr 11, 2022 · 25 comments · Fixed by #108668
Closed
3 of 5 tasks

Tracking Issue for debugger_visualizer #95939

wesleywiser opened this issue Apr 11, 2022 · 25 comments · Fixed by #108668
Assignees
Labels
A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. F-debugger_visualizer `#![feature(debugger_visualizer)]` finished-final-comment-period The final comment period is finished for this PR / Issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@wesleywiser
Copy link
Member

wesleywiser commented Apr 11, 2022

This is a tracking issue for RFC 3191.
The feature gate for the issue is #![feature(debugger_visualizer)].

About tracking issues

Tracking issues are used to record the overall progress of implementation.
They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Steps

Unresolved Questions

No unresolved questions at this time.

Implementation history

@wesleywiser wesleywiser added A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) T-lang Relevant to the language team, which will review and decide on the PR/issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC F-debugger_visualizer `#![feature(debugger_visualizer)]` labels Apr 11, 2022
@ridwanabdillahi
Copy link
Contributor

@rustbot claim

@ridwanabdillahi

This comment was marked as resolved.

@madsmtm
Copy link
Contributor

madsmtm commented Aug 10, 2022

What's the plan for LLDB? It seems like the design of LLDB is slightly different, they err on the side of "the binary cannot be trusted", so automatically loading these kind of things is not really supported - but maybe it would be possible to provide when using rust-lldb?

@pravic
Copy link
Contributor

pravic commented Jan 12, 2023

Why

#![debugger_visualizer(natvis_file = "../Foo.natvis")] 

in Rust code and not

debug_visualizer = "foo.natvis"

in Cargo.toml?

@madsmtm
Copy link
Contributor

madsmtm commented Jan 14, 2023

Probably because the desire is to allow the attribute directly on types in the future.

@gibbyfree
Copy link

@rustbot claim

@gibbyfree
Copy link

Request for Stabilization

I'd like to stabilize the debugger_visualizer attribute. A stabilization PR has been submitted as #108668.

Summary

The following feature will be stabilized:

  • The debugger_visualizer attribute:
#![feature(debugger_visualizer)]
#![debugger_visualizer(natvis_file = "foo.natvis")] 
#![debugger_visualizer(gdb_script_file = "foo.py")] 
struct Foo { 

}

Documentation

I've submitted the following PR:

Tests

You can find test cases in:

Real World Usage

NatVis definitions have been added to various popular open-source crates:

Unresolved Issues

No issues are known at this time.

@madsmtm
Copy link
Contributor

madsmtm commented Mar 6, 2023

I feel like we should figure out the plan for LLDB (or at least rust-lldb) before stabilizing this attribute - it's hard to change after-the-fact if we find that something else is needed to support that.

@gibbyfree
Copy link

I feel like we should figure out the plan for LLDB (or at least rust-lldb) before stabilizing this attribute - it's hard to change after-the-fact if we find that something else is needed to support that.

That's a valid point. It might be worth investigating how LLDB handles embedded pretty printers and whether it has support for them. Since the repo already has LLDB debugger tests, we can consider adding tests that target LLDB, try embedding pretty printers and see if they are handled in a similar way as GDB.

That said, I am not sure that this is worth delaying stabilization. At this point, the unstable status of this feature is a barrier to adoption in the Rust community. The out-of-the-box value of this feature relies on crate owners developing their own visualization definitions / pretty printers, and there has been some pushback from crate owners who would rather not support an unstable feature. (See rust-lang/regex#849)

It seems reasonable to me that LLDB support can go through its own stabilization process at a later time. I'm open to further discussion, though.

@wesleywiser
Copy link
Member Author

Thanks for writing up the stabilization report @gibbyfree! Overall this looks good to me. I think we should spend a little time on this point right now though:

It might be worth investigating how LLDB handles embedded pretty printers and whether it has support for them.

If LLDB doesn't support embedded pretty printers, then the overall question is moot and we can go ahead and stabilize the feature for the debuggers which do support this. If LLDB does have support, then I think the information we'll want to know is "how are the pretty printers embedded?" and "how do we enable them automatically?".

Depending on those answers, we can decide if it's important to do that work now (if it is actually possible) or defer until a later time.

@gibbyfree
Copy link

Thanks for taking a look @wesleywiser :) I've spent some time in the last week investigating LLDB's pretty printing capabilities.

Usage of debugger_visualizer's natvis_file and gdb_script_file embeds content onto the .pdb/.elf generated by the compiler. This enables crate owners to add debugger visualizations to their crate, which can be utilized by the crate's users without any manual configuration on their end. This scenario is one of debugger_visualizer's primary motivations.

Like GDB, LLDB supports pretty printing of custom types with Python scripts. Unlike GDB, LLDB is incapable of modifying symbol file metadata. This is an important distinction.

We might still be able to improve Rust's LLDB debugging experience. LLDB enables auto-loading of pretty printers by setting an environment variable, so printers can be registered in one's local environment without manual configuration. This is a bit similar to how debugger_visualizer auto-loads printers into GDB. But again, this only affects the local machine and does not embed any data in the debug binary itself.

It may be worth discussing how we choose to add support for more debuggers in the future. Maybe the "embedded" aspect isn't a hard requirement for this feature. Still, I'm inclined to think that that discussion has little bearing on the stabilization of debugger_visualizer's Natvis and GDB support.

This feature has been designed for straight-forward extension, and its current implementation shouldn't block anyone interested in hacking on LLDB support - there were slightly different approaches to supporting Natvis and GDB, and I assume that implementing support for other debuggers will require different approaches too.

That said, I'm not an LLDB expert 😅 I'd love to be educated if I'm mistaken about any of this.

@DianaNites
Copy link

DianaNites commented Mar 28, 2023

Usage of debugger_visualizer's natvis_file and gdb_script_file embeds content onto the .pdb/.elf generated by the compiler. This enables crate owners to add debugger visualizations to their crate, which can be utilized by the crate's users without any manual configuration on their end. This scenario is one of debugger_visualizer's primary motivations.

Is it safe to have untrusted debug visualizers? Can they be malicious?

I did a quick search through this thread and the RFC and its thread for "trust" and "security" and didnt see any discussion of this, but even though Rust already has stuff like proc macros and build.rs, those are only at build time, whereas debug visualizers embedded in a binary could have a greater reach and potential for concern for anyone debugging a Rust binary.

@wesleywiser
Copy link
Member Author

Thanks for investigating @gibbyfree! It sounds like no matter what, we won't be able to provide exactly the same experience in lldb as we do in gdb or WinDbg/VS. We could collect lldb scripts into a well known path relative to the output binary in the target folder and teach rust-lldb to look for this or perhaps a hypothetical cargo debug command could set up lldb with the appropriate environment variables but I don't think any of that work needs to block stabilization of this feature as it currently exists as none of that would imply changes to gdb pretty printers or the Natvis support.

@DianaNites good question! There are two different kinds of debugger visualizers supported currently: Natvis and gdb pretty printers.

Natvis is a declarative language that only allows you to provide an alternative description of a type. The number of supported operations is very small and I think the most an adversarial visualizer could do would be to attempt to obscure or mislead the debugger user regarding some value in the debugger. Users are able to turn off the visualization either global or per value in supported debuggers so I don't believe there's a realistic path of attack via this avenue.

Gdb pretty printers are Python scripts that run inside the embedded Python interpreter in gdb. This is an optional feature of gdb that most large distros seem to enable. I believe (but have not tested) that scripts are not sandboxed in any sort of way. As such, they could access resources on the machine hosting the debugger. gdb itself provides mechanisms to enable or disable autoloading of embedded debugger scripts. I think for most users, this is likely not an additional path of attack as they have already compiled attacker-controlled code (which could execute malicious code on their machine via proc macros or build.rs as you pointed out) and they have also run the binary which contains attacker-controlled code as well. For users with additional security needs, gdb should be configured to not autoload scripts but this is not a Rust specific concern.

Does that make sense?

@wesleywiser
Copy link
Member Author

I think this is ready for stabilization.

@gibbyfree wrote a stabilization report and the associated PR is #108668.

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Apr 4, 2023

Team member @wesleywiser has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Apr 4, 2023
@nikomatsakis
Copy link
Contributor

@rfcbot fcp reviewed -- better debugger experience ftw!

@petrochenkov
Copy link
Contributor

I still don't like how the visualizer file is looked up (based on attr.span, rust-lang/rfcs#3191 (comment)), but the same problem is shared by macros like include_str and I don't expect #[debugger_visualizer] to be used "creatively" enough to prevent a fix in the future.

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Apr 14, 2023
@rfcbot
Copy link

rfcbot commented Apr 14, 2023

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Apr 24, 2023
@rfcbot
Copy link

rfcbot commented Apr 24, 2023

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue May 1, 2023
…zer, r=wesleywiser

Stabilize debugger_visualizer

This stabilizes the `debugger_visualizer` attribute (rust-lang#95939).

* Marks the `debugger_visualizer` feature as `accepted`.
* Marks the `debugger_visualizer` attribute as `ungated`.
* Deletes feature gate test, removes feature gate from other tests.

Closes rust-lang#95939
@bors bors closed this as completed in f379a58 May 2, 2023
@ehuss
Copy link
Contributor

ehuss commented May 6, 2023

I'm concerned about the stabilization of this feature in its current state. I filed a few issues (see F-debugger_visualizer `#![feature(debugger_visualizer)]` ). It is almost unusable for the purpose of authoring visualizers since you can't modify a file without needing to delete your build directory and starting over. I would recommend looking at either fixing those issues or considering to revert the stabilization, since otherwise I think it would provide a poor first impression.

@lcnr lcnr added the I-compiler-nominated Nominated for discussion during a compiler team meeting. label May 7, 2023
@lcnr
Copy link
Contributor

lcnr commented May 7, 2023

nominating for t-compiler meeting due to the above concern. if we're able to remove this asynchronously before then we can always remove the nomination.

@lcnr lcnr reopened this May 7, 2023
@michaelwoerister
Copy link
Member

I agree that these issue are a blocking concern for the feature. If I'm reading the schedule right, we have until May 26 to either fix the issue or revert stabilization.

@michaelwoerister
Copy link
Member

Thanks for opening the issues, @ehuss! I've assigned myself to do an initial investigation.

@apiraino
Copy link
Contributor

apiraino commented May 18, 2023

Discussed in T-compiler meeting on Zulip

Linking #111641 that should address this.

@rustbot label -I-compiler-nominated

@rustbot rustbot removed the I-compiler-nominated Nominated for discussion during a compiler team meeting. label May 25, 2023
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label May 25, 2023
@Mark-Simulacrum
Copy link
Member

Closing this tracking issue -- I believe the feature is now stabilized, and the concerns raised were addressed.

@dtolnay dtolnay removed the S-tracking-needs-to-bake Status: The implementation is "complete" but it needs time to bake. label Apr 5, 2024
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.) C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. F-debugger_visualizer `#![feature(debugger_visualizer)]` finished-final-comment-period The final comment period is finished for this PR / Issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.