-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Generate DW_AT_RUST_short_backtrace
attributes for DISubprogram
nodes
#123683
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
base: main
Are you sure you want to change the base?
Conversation
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 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. |
enum class ShortBacktraceAttr { | ||
SkipFrame = 0, | ||
StartShortBacktrace = 1, | ||
EndShortBacktrace = 2, |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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:
- 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 correspondingEndShortBacktrace
pair. - 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.
- 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<ShortBacktraceAttr> parsedShortBacktrace; | ||
switch (shortBacktrace.Val) { | ||
case -1: | ||
parsedShortBacktrace = std::nullopt; |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
9502d69
to
4470abc
Compare
✅ With the latest revision this PR passed the C/C++ code formatter. |
4470abc
to
35ca294
Compare
…odes The Rust standard library has two [styles](https://doc.rust-lang.org/stable/std/panic/enum.BacktraceStyle.html) 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. - Add a new `enum ShortBacktraceAttr { SkipFrame, StartFrame, EndFrame }`. StartFrame and EndFrame correspond to the existing `__rust_start_short_backtrace` and `__rust_end_short_backtrace` symbols. SkipFrame currently has no analogue. Each of these values is mutually exclusive with the others, and generating multiple for the same function is a logical error. - Update all callsites of `DISubprogram::getImpl` to pass in a `std::optional<ShortBacktraceAttr>` - Emit `ShortBacktraceAttr` when creating DWARF debuginfo. Note that this also generates debuginfo in more cases than before: When using line-tables-only debuginfo, LLVM attempts to skip functions with 0 lexical scopes. But some shims in the Rust standard library, such as `std::ops::Fn::call`, are shims that never appear in Rust source code but still appear in the backtrace. Generate info for these functions if they have a short backtrace attribute, so that they are correctly skipped in backtraces. - Parse and serialize the new attribute in .ll files - Add a regression test
35ca294
to
1d66e6b
Compare
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?). |
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”? |
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.
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. |
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... |
This comment was marked as outdated.
This comment was marked as outdated.
The conversation there seems stalled. What can I do to move this forward? |
The Rust standard library has two styles for printing backtraces at runtime:
__libc_start_main
,_Unwind_Resume
, and rust runtime internals likestd::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.
enum ShortBacktraceAttr { SkipFrame, StartFrame, EndFrame }
. StartFrame and EndFrame correspond to the existing__rust_start_short_backtrace
and__rust_end_short_backtrace
symbols. SkipFrame currently has no analogue. Each of these values is mutually exclusive with the others, and generating multiple for the same function is a logical error.DISubprogram::getImpl
to pass in astd::optional<ShortBacktraceAttr>
ShortBacktraceAttr
when creating DWARF debuginfo. Note that this also generates debuginfo in more cases than before: When using line-tables-only debuginfo, LLVM attempts to skip functions with 0 lexical scopes. But some shims in the Rust standard library, such asstd::ops::Fn::call
, are shims that never appear in Rust source code but still appear in the backtrace. Generate info for these functions if they have a short backtrace attribute, so that they are correctly skipped in backtraces.cc @adrian-prantl and @dwblaikie, i talked with @dianqk and he suggested consulting you about whether this is the best approach.