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

Stabilize -Zdwarf-version as -Cdwarf-version #136926

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wesleywiser
Copy link
Member

@wesleywiser wesleywiser commented Feb 12, 2025

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 report

What 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:

  1. 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 beyond dwarf-{2,3,4,5} or codeview. 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?

  2. 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).

  3. 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)

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

What 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 in RUSTFLAGS and providing the corresponding flag to Clang/gcc (rust-lang/cc-rs#1395).


Closes #103057

@rustbot
Copy link
Collaborator

rustbot commented Feb 12, 2025

r? @GuillaumeGomez

rustbot has assigned @GuillaumeGomez.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 12, 2025
@wesleywiser wesleywiser added -Zdwarf-version Unstable option: set dwarf version and removed A-run-make Area: port run-make Makefiles to rmake.rs labels Feb 12, 2025
@wesleywiser
Copy link
Member Author

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Feb 12, 2025

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.

@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 Feb 12, 2025
@GuillaumeGomez
Copy link
Member

r? compiler

@wesleywiser wesleywiser added the A-rust-for-linux Relevant for the Rust-for-Linux project label Feb 12, 2025
@jieyouxu jieyouxu added the A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) label Feb 12, 2025
Copy link
Member

@jieyouxu jieyouxu Feb 12, 2025

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?

Copy link
Member Author

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).

Copy link
Member

@workingjubilee workingjubilee Feb 12, 2025

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?

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Member

@workingjubilee workingjubilee Feb 13, 2025

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!)

Comment on lines +3 to 4
//@ compile-flags: -g --target x86_64-unknown-linux-gnu -C dwarf-version=4 -Copt-level=0
//@ needs-llvm-components: x86
Copy link
Member

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 specific RUSTFLAGS 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.

Copy link
Member Author

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).

Copy link
Member

@jieyouxu jieyouxu Feb 12, 2025

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)?

Comment on lines +115 to +117
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:
Copy link
Member

@jieyouxu jieyouxu Feb 12, 2025

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?

Copy link
Member Author

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?

Copy link
Member

@workingjubilee workingjubilee Feb 12, 2025

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
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

@jieyouxu jieyouxu Feb 12, 2025

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).

@ojeda
Copy link
Contributor

ojeda commented Feb 12, 2025

2. 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.

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?).

No call-for-testing has been conducted but Rust for Linux has been using this flag without issue.

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. CONFIG_DEBUG_INFO_DWARF5=y). In particular, there are two other choices in the kernel configuration where the flag is not passed: no debug information and "Rely on the toolchain's implicit default DWARF version". Thus it is unclear how often developers/users are actually picking e.g. v5 explicitly.

From a quick look, e.g. Ubuntu seems to build with CONFIG_DEBUG_INFO_DWARF5=y (and Rust enabled), so that is something. However, given there is not much Rust code in use, it is possible nobody is actually debugging the Rust-related code with v5, for instance.

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?

@workingjubilee
Copy link
Member

workingjubilee commented Feb 12, 2025

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?).

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).

@workingjubilee
Copy link
Member

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?

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.

@Noratrieb
Copy link
Member

I'd be surprised if .eh_frame cal frame information (CFI) depended in any way on this flag (but haven't tested that). It's already subtly different from standard dwarf and part of the platform ABI.
(and I don't think we can ever move away from it because again, platform ABI and we want to be compatible, but that's besides the point here).

@nagisa
Copy link
Member

nagisa commented Feb 14, 2025

For instance, what would --target x86_64-unknown-linux-gnu -Cdebuginfo-version=dwarf-5 do?

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 x86_64-pc-windows-msvc+DWARFv5? In that case I still feel like there are genuine possible future use-cases that make sense here. It is perfectly possible to run e.g. gdb on windows and if dwarf is not currently supported there, conceptually there is very little that would prevent enabling this. Similarly, if there was a need for it, I imagine it wouldn't be too onerous to extend LLVM to pick the debuginfo format independently of the compilation target.


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 env RUSTFLAGS='-Cdwarf-version=5 -Csourcemap-version=3 cargo build which then possibly invokes a build.rs which builds some wasm/js... The appropriate version flag would apply to only specific parts of the build process...

@petrochenkov petrochenkov added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 16, 2025
@wesleywiser
Copy link
Member Author

I assume this probably meant to ask about situations such as x86_64-pc-windows-msvc+DWARFv5?

Yes, you are correct. I've fixed the text to reflect this. Thank you!

In that case I still feel like there are genuine possible future use-cases that make sense here. It is perfectly possible to run e.g. gdb on windows and if dwarf is not currently supported there, conceptually there is very little that would prevent enabling this.

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 *-windows-gnu does after all). I'm not aware of any reason to do this for the *-msvc targets whose purpose is to conform closely to the "native" Microsoft C(++) toolchain. Likewise, there's no reason I can think of to generate CodeView records on Linux or macOS.

When use-cases are reported, we can always choose to lift specific restrictions at that time.

But maybe having separate flags is actually beneficial? It enables doing things like env RUSTFLAGS='-Cdwarf-version=5 -Csourcemap-version=3 cargo build which then possibly invokes a build.rs which builds some wasm/js... The appropriate version flag would apply to only specific parts of the build process...

I agree there are some definite merits to separate flags. If it was easier to target RUSTFLAGS in this case such that -Cdebuginfo-version=dwarf-5 only applied to build targets with DWARF support and -Cdebuginfo-version=sourcemap-2 could apply to wasm targets, etc. then consolidating into one flag would make a lot of sense.

@Noratrieb
Copy link
Member

@workingjubilee I verified that -Zdwarf-version indeed has zero impact on .eh_frame unwind tables, by compiling a binary with different dwarf versions (and -Zbuild-std of course), and the two .eh_frame sections were exactly the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-Zdwarf-version Unstable option: set dwarf version A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) A-rust-for-linux Relevant for the Rust-for-Linux project disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tracking issue for -Zdwarf-version