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

Use debuginfo for short backtrace printing #818

Open
1 of 3 tasks
jyn514 opened this issue Dec 8, 2024 · 2 comments
Open
1 of 3 tasks

Use debuginfo for short backtrace printing #818

jyn514 opened this issue Dec 8, 2024 · 2 comments
Labels
final-comment-period The FCP has started, most (if not all) team members are in agreement major-change A proposal to make a major change to rustc T-compiler Add this label so rfcbot knows to poll the compiler team

Comments

@jyn514
Copy link
Member

jyn514 commented Dec 8, 2024

Proposal

The problem

This MCP is intended to solve rust-lang/rust#124586 in an extensible way.

Currently, short backtrace printing (the part of the default panic handler in std that hides frames like lang_start and std::panicking) works like this:

  • std defines functions named __rust_{start,end}_short_backtrace which are marked with inline(never).
    • Actually, rustc defines these too, to hide frames that come from inside the query system. In this case the "start/end" names are reversed, because the backtrace is defaulting to on, instead of the runtime where it defaults to off until the start frame is encountered.
  • the default panic handler walks up the frames and uses them to toggle whether printing is enabled for the current frame.

This has two drawbacks:

  1. because the functions are marked with inline(never), they form an optimization barrier, because most of LLVM's optimizations are hamstrung when they can't do inlining.
  2. the state is global across all functions in the callgraph, not local to the functions being annotated.

both of these make me reluctant to add more start/end function pairs, because it will hurt performance for everyone using rust and also it's hard to be sure that the panic state machine is working as expected.

Note these drawbacks only to new __rust_{start,end} frames. in std itself we only call these functions once, so we don't care about performance, and we are reasonably confident the existing state machine is correct for those two frames.

My proposed solution

  • Add new #[rustc_{start,end}_short_backtrace] attributes. Use them everywhere except the two existing functions.
  • Represent rustc_{start,end}_backtrace pairs with debuginfo instead of frames. In other words, any callgraph that goes A -> start_short_backtrace -> B will now go directly A -> B, and B will be annotated with debuginfo marking it appropriately.
  • Change the panic handler to, in addition to inspecting the name of the symbol, inspect the debuginfo for the current frame.

Additionally, expose a new #[rustc_skip_short_backtrace] attribute. When applied to a function, this generates debuginfo that says to skip that frame only, but not to change the state of the panic handler state machine. When applied to a module, this generates debuginfo that says to skip any function defined in that module (e.g. DWARF will use DW_TAG_namespace).

Note that this allows other languages besides Rust to read and write functions marked as skippable, as long as their compiler understands the format we use for the debuginfo.

Drawbacks

  • It's more complicated.
  • Doing this for DWARF is mostly straightforward, but PDB files are ... challenging. In the worst case, we could have two different code paths, one for debuginfo formats which support this metadata and one for formats which don't. I don't know much about PDB so this may be simpler than I am expecting.

Alternatives

  1. The same as above, but we remove __rust_start_short_backtrace altogether and only use the new attributes. This will regress anyone who was using those frames outside std. It also will break if debuginfo is disabled.
  2. The same as above, but only the new #[rustc_skip_short_backtrace] attribute is represented with debuginfo; start/end attributes are still represented as inline(never) frames. This avoids missing start/end frames when debuginfo is disabled, but has a slight performance hit anywhere a start/end pair appears.
  3. Use a builtin macro to expand #[rustc_skip_short_backtrace] to a static injected into std that contains a slice of function names (or addresses). Continue using frames for start/end pairs. This is platform independent, but prevents the new attribute from being used anywhere outside std(/alloc/core). Has a small performance hit for start/end pairs. Does not allow other languages to mark frames as skippable, or to read the list of skippable frames.
  4. Inject the static from 2. just before link time, instead of when building std. This allows using the attribute outside std, but makes dynamic linking hard. No performance hit. Does not allow other languages to mark frames as skippable.
    • One possible approach is to use a separate object file for this static, instead of combining it with any other CGU. If multiple dynamic libs are linked into a Rust binary, the compiler merges the two object files into a single symbol. If rustc does not control the final link, the external build system is responsible for linking in the object files, similar to how #[global_allocator] works today.

Future work

  • Consider whether we want to stabilize these attributes.

Mentors or Reviewers

@bjorn3, @saethlin

cc @rust-lang/libs

Process

The main points of the Major Change Process are as follows:

  • File an issue describing the proposal.
  • A compiler team member or contributor who is knowledgeable in the area can second by writing @rustbot second.
    • Finding a "second" suffices for internal changes. If however, you are proposing a new public-facing feature, such as a -C flag, then full team check-off is required.
    • Compiler team members can initiate a check-off via @rfcbot fcp merge on either the MCP or the PR.
  • Once an MCP is seconded, the Final Comment Period begins. If no objections are raised after 10 days, the MCP is considered approved.

You can read more about Major Change Proposals on forge.

Comments

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

@jyn514 jyn514 added major-change A proposal to make a major change to rustc T-compiler Add this label so rfcbot knows to poll the compiler team labels Dec 8, 2024
@rustbot
Copy link
Collaborator

rustbot commented Dec 8, 2024

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

Concerns or objections to the proposal should be discussed on Zulip and formally registered here by adding a comment with the following syntax:

@rfcbot concern reason-for-concern 
<description of the concern> 

Concerns can be lifted with:

@rfcbot resolve reason-for-concern 

See documentation at https://forge.rust-lang.org

cc @rust-lang/compiler

@rustbot rustbot added the to-announce Announce this issue on triage meeting label Dec 8, 2024
@saethlin
Copy link
Member

@rustbot second

@rustbot rustbot added the final-comment-period The FCP has started, most (if not all) team members are in agreement label Dec 14, 2024
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period The FCP has started, most (if not all) team members are in agreement major-change A proposal to make a major change to rustc T-compiler Add this label so rfcbot knows to poll the compiler team
Projects
None yet
Development

No branches or pull requests

4 participants