Generate DW_AT_RUST_short_backtrace attributes for DISubprogram nodes by jyn514 · Pull Request #123683 · llvm/llvm-project (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation20 Commits1 Checks3 Files changed

Conversation

This file contains hidden or 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

[ Show hidden characters]({{ revealButtonHref }})

jyn514

The Rust standard library has two styles for printing backtraces at runtime:

  1. Full backtraces. These work in the obvious way.
  2. Short backtraces. These filter out "unimportant" frames that are likely not related to the developer's bug. For example, frames like __libc_start_main, _Unwind_Resume, and rust runtime internals like std::rt::lang_start are filtered out.

Currently, the Rust runtime determines "unimportant" frames by looking directly at un-mangled symbol names of generated functions. This is not extensible, and involves a state machine that requires the frames to be present at runtime; in particular the frames must be marked noinline, impeding optimizations.

This extends LLVM to encode short backtrace metadata in DWARF debuginfo. It currently does not attempt to add debuginfo to PDB, which was not designed to be extensible.

See rust-lang/compiler-team#818 and the rust zulip for more background.

cc @adrian-prantl and @dwblaikie, i talked with @dianqk and he suggested consulting you about whether this is the best approach.

@github-actions GitHub Actions

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

dianqk

enum class ShortBacktraceAttr {
SkipFrame = 0,
StartShortBacktrace = 1,
EndShortBacktrace = 2,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add some documents? I'm not clear why StartShortBacktrace and EndShortBacktrace cannot be replaced by SkipFrame? IIUC, we can add SkipFrame annotation to all frames/functions that between StartShortBacktrace and EndShortBacktrace.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few cases where this cannot be replaced by annotating individual frames. Consider these three:

  1. Frames before main are not controlled by the compiler. To skip frames before main, we have to annotate it with StartShortBacktrace, and it has no corresponding EndShortBacktrace pair.
  2. The compiler may not control all frames in a given section of the call graph; for example when using a shared object library, or when linking object files from cross-language translation units.
  3. The compiler may not statically know the callgraph. I am not super familiar with how dataflow analysis works, but my impression is that things like vtables and jump tables mean the callgraph is not fully known until runtime.

Rust currently handles this with a define void @__rust_start_short_backtrace() noinline function, and it works ok. But you mentioned that you want this to be extensible to other languages and tools. And in that case they probably don't want to hardcode __rust in the name, nor be forced to use frames instead of debuginfo.

I am happy to document the three cases above inline in this enum.

std::optional parsedShortBacktrace;
switch (shortBacktrace.Val) {
case -1:
parsedShortBacktrace = std::nullopt;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This requires a debug assertion.

@@ -605,6 +605,10 @@ HANDLE_DW_AT(0x3b28, BORLAND_Delphi_ABI, 0, BORLAND)
HANDLE_DW_AT(0x3b29, BORLAND_Delphi_return, 0, BORLAND)
HANDLE_DW_AT(0x3b30, BORLAND_Delphi_frameptr, 0, BORLAND)
HANDLE_DW_AT(0x3b31, BORLAND_closure, 0, BORLAND)
// Rust extensions.
HANDLE_DW_AT(0x3c00, RUST_short_backtrace, 0, RUST)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for me, this can be a LLVM project extension which should benefit for other languages or lld.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am happy for other languages or tools to use this (in particular i would love for llvm-symbolizer to just handle these frames without needing rust-specific patches). But I worry about making it an LLVM vendor extension - I also want other backends, like rustc_codegen_cranelift and codegen_gcc, to be able to use this.

cc @bjorn3 @antoyo do you have opinions about whether short backtraces should be a Rust DWARF extension or an LLVM extension?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is nothing stopping other backends from implementing an LLVM vendor extension. Both LLVM and cg_clif implement a bunch of GNU vendor extensions too.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar enough with this to have an opinion.

I don't know what LLVM extensions are: does that mean this wouldn't be in DWARF?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DWARF has a general mechanism for vendor extensions. No matter what we do, this won't be in the DWARF 5 standard, which has already been published. But if it's used widely enough, there's a chance it makes it into DWARF 6 or 7. I don't think that depends very much on the naming of the attribute though.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have much of an opinion here, so choose whatever you think makes more sense overall.

@github-actions GitHub Actions

✅ With the latest revision this PR passed the C/C++ code formatter.

@jyn514

…odes

The Rust standard library has two styles for printing backtraces at runtime:

  1. Full backtraces. These work in the obvious way.
  2. Short backtraces. These filter out "unimportant" frames that are likely not related to the developer's bug. For example, frames like __libc_start_main, _Unwind_Resume, and rust runtime internals like std::rt::lang_start are filtered out.

Currently, the Rust runtime determines "unimportant" frames by looking directly at un-mangled symbol names of generated functions. This is not extensible, and involves a state machine that requires the frames to be present at runtime; in particular the frames must be marked noinline, impeding optimizations.

This extends LLVM to encode short backtrace metadata in DWARF debuginfo. It currently does not attempt to add debuginfo to PDB, which was not designed to be extensible.

Other languages are allowed and encouraged to use this new debuginfo for their backtraces. The Rust runtime does not distinguish between producers of the debuginfo; any function with this debuginfo will be respected when printing backtraces. I hope in the future to extend llvm-symbolizer to have a short backtrace printing mode.

See rust-lang/compiler-team#818 for more background.

@dwblaikie

Might be good to have a design discussion on discourse before finalizing this. Easier to discuss the design separately from the implementation (& the implementation probably gets split up into a few patches).

I'd endorse the LLVM_* naming (though I understand that may make it harder to get GCC adoption, though I'm not sure if RUST_* is any more likely, maybe a bit).

I wonder about the naming and carried value of this attribute. For instance, the StartFrame/EndFrame feature, if I'm understanding it correctly, seems like maybe it could be skipped if we're emitting DWARF (& instead the skip attribute could be placed on all the calls between StartFrame and EndFrame?).

@jyn514

For instance, the StartFrame/EndFrame feature, if I'm understanding it correctly, seems like maybe it could be skipped if we're emitting DWARF (& instead the skip attribute could be placed on all the calls between StartFrame and EndFrame?).

does #123683 (comment) address your concerns?

i’m happy to post on https://discourse.llvm.org/ but i’m not entirely sure what the process is - do you want me to just say “here is my current design, feel free to leave design comments”?

@dwblaikie

For instance, the StartFrame/EndFrame feature, if I'm understanding it correctly, seems like maybe it could be skipped if we're emitting DWARF (& instead the skip attribute could be placed on all the calls between StartFrame and EndFrame?).

does #123683 (comment) address your concerns?

Ah, sorry I missed that. Didn't mean to have you repeat yourself.

I think a design discussion will make it more clear what these different features do and how they might be generalized.

i’m happy to post on https://discourse.llvm.org/ but i’m not entirely sure what the process is - do you want me to just say “here is my current design, feel free to leave design comments”?

Yep, pretty much that. Maybe some worked examples of behavior (given this code, with these attributes in these places, this is the raw backtrace and this is the attribute-influenced backtrace).

Maybe an attribute with "skip, don't skip, do whatever the caller did" would be the tool, perhaps - but let's see the worked examples in a post and hash it out there.

@jyn514

@dwblaikie

i posted this as a thread on https://discourse.llvm.org/t/adding-short-backtrace-debuginfo/84187/1 but it got hidden automatically :/ i think because i added hyperlinks.

hmm, what do you mean by hidden? hyperlinks shouldn't themselves be a problem... But could try again without the hyperlinks/leave them in this post, or we can try adding them in follow-up comments...

@jyn514

This comment was marked as outdated.

@jyn514

The conversation there seems stalled. What can I do to move this forward?

Reviewers

@antoyo antoyo antoyo left review comments

@dianqk dianqk dianqk left review comments

@bjorn3 bjorn3 bjorn3 left review comments

@adrian-prantl adrian-prantl Awaiting requested review from adrian-prantl

@dwblaikie dwblaikie Awaiting requested review from dwblaikie