-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
Stabilize -Zdwarf-version
as -Cdwarf-version
#136926
base: master
Are you sure you want to change the base?
Conversation
rustbot has assigned @GuillaumeGomez. Use |
@rfcbot fcp merge |
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! See this document for info about what commands tagged team members can give me. |
r? compiler |
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.
Question (not for this test but just to have an inline comment somewhere instead of having a ton of top-level comments): does -Cdwarf-version
have any interesting interactions with -Csplit-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.
As I understand it, most DWARF features are implemented as extensions on top of the base specification describing the format itself. Split DWARF is technically a DWARF 5 feature but it's just an agreed-upon extension to DWARF itself. So -Cdwarf-version=2 -Csplit-dwarf
will generate DWARF 2 but with the Split DWARF extension "enabled".
As such, I don't think there's any interesting interactions (@davidtwco please correct me if I'm wrong here) other than the fact you can target a lower DWARF standard version and still have this feature enabled (which is the case right now on stable Rust as we target DWARF 4 by default but still support Split 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.
That is effectively correct. As DWARF moves forward, it generally is extended in a way that a debugger that doesn't support it will just be less able to read the debuginfo and produce less useful feedback, not experience a critical error. But I'm only really somewhat familiar with DWARFv4 and DWARFv5. Maybe @tromey, @khuey, or @philipc have more thoughts on how that works out in practice?
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 5 rearranged the compilation unit header so I think any reader had to adapt to even try to scan the contents.
Often a lot of things are shared or overlap though. And DWARF readers are expected to ignore some things they don't understand, like extension tags or unusual attributes or whatnot.
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 is not correct. There is an interaction between the DWARF version and split-debuginfo=[un]packed.
Without -Zdwarf-version=5 rustc/llvm emit the pre-DWARF 5 GNU extension documented at https://gcc.gnu.org/wiki/DebugFission. With -Zdwarf-version=5 rust/llvm emit the standardized DWARF 5 split DWARF from the specification. These two differ in the attributes used, forms, and even some of the ELF section names, and are not directly compatible. Generally tools support both, though.
As far as what the compatibility story is that depends on what you consider a "critical error". Debugging symbols are of course always optional and the debugger can operate without them at all. Since DWARF is versioned on a per-compilation-unit basis, in a binary with heterogeneous DWARF versions in principle a debugger could simply skip the compilation units with the version it doesn't understand while continuing to provide a full debugging experience for the compilation units it does understand.
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.
Thanks, I wasn't aware of the GNU extension! (I mean, I was aware there was a predecessor but not that we actually already emitted it, huh!)
//@ compile-flags: -g --target x86_64-unknown-linux-gnu -C dwarf-version=4 -Copt-level=0 | ||
//@ needs-llvm-components: x86 |
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.
Discussion (re. no indication if DWARF is unsupported):
Currently, passing
-{C,Z} dwarf-version
on targets like*-windows-msvc
does not do anything. It may be preferable to emit a warning alerting the user that the flag has no effect on the target platform. Alternatively, we could emit an error but this could be annoying since it would require the use of target specificRUSTFLAGS
to use the flag correctly (and there isn't a way to target "any platform that uses DWARF" using cfgs).
Could we maybe emit an allow-able warn-by-default if -{C,Z} dwarf-version
is a no-op? It might be surprising or misleading if flag just silently does nothing on unsupported platforms.
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 think it could make sense to generate a warning or informational message in that case but I could also see it just being annoying on Windows/MSVC where this simply doesn't matter. If the user really needs DWARF for some reason, they're going to find out very quickly that DWARF is not used on those targets. For most other deprecated no-opt -C
flags, we generate an unconditional warning (not a lint).
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.
Yeah. It can't be an unconditional warning because you can't silence that and it would be very annoying. I think maybe a remark about this being no-op in the docs would suffice (at the risk of it becoming outdated, but still)?
This option controls the version of DWARF that the compiler emits, on platforms | ||
that use DWARF to encode debug information. It takes one of the following | ||
values: |
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.
Discussion: should Pick the max DWARF version when LTO'ing modules with different versions #136659 be described here, now that this is being proposed for stabilization?
For DWARF experts maybe, is there ever a reason to not want to pick the max DWARF version when LTO-ing modules with different DWARF versions?
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.
@davidtwco, @nikic, @workingjubilee Do any of you have opinions on the LLVM module merge behavior for the DWARF version flag?
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.
trying to generate debuginfo while doing significant LTO seems to have exciting results anyways, so perhaps we should consider heavily caveating our behavior when this is combined with LTO: https://github.com/llvm/llvm-project/issues?q=is%3Aissue%20state%3Aopen%20%20label%3ALTO%20label%3Adebuginfo
that would not be because the choice for the module versions made is bad, though. indeed, "pick highest" seems like the only reasonable option.
//@ compile-flags: -C lto -g -Zdwarf-version=5 | ||
//@ compile-flags: -C lto -g -Cdwarf-version=5 |
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.
Discussion (re. call-for-testing period): should we still explicitly have a call-for-testing period, to invite users to explicitly try to break this flag somehow? It seems not too high risk, but still.
No call-for-testing has been conducted but Rust for Linux has been using this flag without issue.
RfL is an important user group, but they may have their specific ways of exercising this flag. Would we benefit from having more diverse user groups try to break this?
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.
No objection to a call-for-testing period but I think we've gotten most of the interesting user testing done by RfL here as the Linux kernel developer community cares quite a lot about things like the various new compact forms in DWARF 5.
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.
That's good enough for me. I'll leave this unresolved for a brief moment, in case someone else has concerns. But feel free to resolve this (non-blocking).
That could be nice, yeah, as long as it can be allowed (is this one one of those types of warnings that would be currently hard to introduce support for allowing it with the current infrastructure?).
To give some context, and so that you can evaluate how much tested this was: the flag has been there since Linux v6.10 (~9 months); however, the flag is only actually used when someone explicitly picks v4 or v5 (e.g. From a quick look, e.g. Ubuntu seems to build with Having said that, even if there are hidden issues, we would still somehow need a way to pick/force v4 or v5 eventually nevertheless, i.e. if I understand correctly, the biggest concern here is that a way to pick the version is stabilized, but then somehow an issue so fundamental is discovered that Rust realized it cannot allow the user to ever pick the version, right? |
correct. (note this should not be considered a comment on the practicality of this very specific case, as, er, the point is that implementing a warning for anything that falls outside our lint infrastructure situation winds up being kinda case by case). |
Yes. I believe we have an entanglement with DWARF unwind tables (which aren't exactly the same as debuginfo but if there are any version-dependent changes I would be surprised if this doesn't affect them) that may complicate this selection. I don't expect it to be a problem, but maybe I'm simply not curious enough. And I suspect we could respond to that by instead just moving away from DWARF unwind tables somehow. |
I'd be surprised if |
I read and re-read this question a couple times and I'm still unclear what the question here is. Wouldn't we just emit DWARFv5? I assume this probably meant to ask about situations such as Hypotheticals aside, there is one reason why I think it is probably better for us to go for a unified flag to specify the debuginfo format+version: dwarf and codeinfo aren't going to be the only families of debug info format in perpetuity. Consider WebAssembly: we currently emit dwarf, but this is only usefully supported by chromium. There is another competing format out there with a wider support: source maps. And the source map format has a number of revisions/versions too. If we were to implement source maps, we would possibly look at adding a source-map specific flag? But maybe having separate flags is actually beneficial? It enables doing things like |
Yes, you are correct. I've fixed the text to reflect this. Thank you!
If there are reasonable use-cases, I have no objection to supporting them. However, I think we should not create use-cases. It is certainly possible to emit DWARF on Windows and use gdb for debugging (this is what When use-cases are reported, we can always choose to lift specific restrictions at that time.
I agree there are some definite merits to separate flags. If it was easier to target RUSTFLAGS in this case such that |
@workingjubilee I verified that |
I propose stabilizing
-Zdwarf-version
as-Cdwarf-version
. This PR adds a new-Cdwarf-version
flag, leaving the unstable-Z
flag as is to ease the transition period. The-Z
flag will be removed in the future.-Zdwarf-version
stabilization reportWhat is the RFC for this feature and what changes have occurred to the user-facing design since the RFC was finalized?
No RFC/MCP, this flag was added in #98350 and was not deemed large enough to require additional process.
The tracking issue for this feature is #103057.
What behavior are we committing to that has been controversial? Summarize the major arguments pro/con.
None that has been extensively debated but there are a few questions that could have been chosen differently:
What should the flag name be?
The current flag name is very specific to DWARF. Other debuginfo formats exist (msvc's CodeView format or https://en.wikipedia.org/wiki/Stabs) so we could have chosen to generalize the flag name (
-{C,Z} debuginfo-version=dwarf-5
for example). While this would extend cleanly to support formats other than DWARF, there are some downsides to this design. Neither CodeView nor Stabs have specification or format versions so it's not clear what values would be supported beyonddwarf-{2,3,4,5}
orcodeview
. We would also need to take care to ensure the name does not lead users to think they can pick a format other than one supported by the target. For instance, what would--target x86_64-pc-windows-msvc -Cdebuginfo-version=dwarf-5
do?What is the behavior when flag is used on targets that do not support DWARF?
Currently, passing
-{C,Z} dwarf-version
on targets like*-windows-msvc
does not do anything. It may be preferable to emit a warning alerting the user that the flag has no effect on the target platform. Alternatively, we could emit an error but this could be annoying since it would require the use of target specific RUSTFLAGS to use the flag correctly (and there isn't a way to target "any platform that uses DWARF" using cfgs).Does the precompiled standard library potentially using a different version of DWARF a problem?
I don't believe this is an issue as debuggers (and other such tools) already must deal with the possibility that an application uses different DWARF versions across its statically or dynamically linked libraries.
Are there extensions to this feature that remain unstable? How do we know that we are not accidentally committing to those.
No extensions per se, although future DWARF versions could be considered as such. At present, we validate the requested DWARF version is between 2 and 5 (inclusive) so new DWARF versions will not automatically be supported until the validation logic is adjusted.
Summarize the major parts of the implementation and provide links into the code (or to PRs)
rust/compiler/rustc_target/src/spec/mod.rs
Line 2369 in 34a5ea9
-{C,Z} dwarf-version
rust/compiler/rustc_session/src/session.rs
Line 738 in 34a5ea9
rust/compiler/rustc_session/src/session.rs
Lines 1253 to 1258 in 34a5ea9
rust/compiler/rustc_codegen_llvm/src/debuginfo/mod.rs
Line 106 in 34a5ea9
Summarize existing test coverage of this feature
Has a call-for-testing period been conducted? If so, what feedback was received?
No call-for-testing has been conducted but Rust for Linux has been using this flag without issue.
What outstanding bugs in the issue tracker involve this feature? Are they stabilization-blocking?
All reported bugs have been resolved.
Summarize contributors to the feature by name for recognition and assuredness that people involved in the feature agree with stabilization
.debug_pubnames
and.debug_pubtypes
when using DWARF 5 in fix: stop emitting.debug_pubnames
and.debug_pubtypes
#117962 by @weihanglo.dwarf_version
option #135739), fix LLVM warning on LTO with different DWARF versions (Pick the max DWARF version when LTO'ing modules with different versions #136659) and argument validation (Emit an error if-Zdwarf-version=1
is requested #136746) by @wesleywiserWhat FIXMEs are still in the code for that feature and why is it ok to leave them there?
No FIXMEs related to this feature.
What static checks are done that are needed to prevent undefined behavior?
This feature cannot cause undefined behavior.
We ensure the DWARF version is one of the supported values here.
In what way does this feature interact with the reference/specification, and are those edits prepared?
No changes to reference/spec, unstable rustc docs are moved to the stable book as part of the stabilization PR.
Does this feature introduce new expressions and can they produce temporaries? What are the lifetimes of those temporaries?
No.
What other unstable features may be exposed by this feature?
-Zembed-source
requires use of DWARF 5 extensions but has its own feature gate.What is tooling support like for this feature, w.r.t rustdoc, clippy, rust-analzyer, rustfmt, etc.?
No support needed for rustdoc, clippy, rust-analyzer, rustfmt or rustup.
Cargo could expose this as an option in build profiles but I would expect the decision as to what version should be used would be made for the entire crate graph at build time rather than by individual package authors.
cc-rs has support for detecting the presence of
-{C,Z} dwarf-version
inRUSTFLAGS
and providing the corresponding flag to Clang/gcc (rust-lang/cc-rs#1395).Closes #103057