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

cg_llvm: split dwarf support #77117

Merged
merged 10 commits into from
Dec 16, 2020
Merged

Conversation

davidtwco
Copy link
Member

@davidtwco davidtwco commented Sep 23, 2020

cc #34651

This PR adds initial support for Split DWARF to rustc, based on the implementation in Clang.

Current Status

This PR currently has functioning split-dwarf, running rustc with -Zsplit-dwarf=split when compiling a binary will produce a dwp alongside the binary, which contains the linked dwarf objects.

$ rustc -Cdebuginfo=2 -Zsplit-dwarf=split -C save-temps ./foo.rs
$ ls foo*
foo
foo.belfx9afw9cmv8.rcgu.dwo
foo.belfx9afw9cmv8.rcgu.o
foo.foo.7rcbfp3g-cgu.0.rcgu.dwo
foo.foo.7rcbfp3g-cgu.0.rcgu.o
foo.foo.7rcbfp3g-cgu.1.rcgu.dwo
foo.foo.7rcbfp3g-cgu.1.rcgu.o
foo.foo.7rcbfp3g-cgu.2.rcgu.dwo
foo.foo.7rcbfp3g-cgu.2.rcgu.o
foo.foo.7rcbfp3g-cgu.3.rcgu.dwo
foo.foo.7rcbfp3g-cgu.3.rcgu.o
foo.foo.7rcbfp3g-cgu.4.rcgu.dwo
foo.foo.7rcbfp3g-cgu.4.rcgu.o
foo.foo.7rcbfp3g-cgu.5.rcgu.dwo
foo.foo.7rcbfp3g-cgu.5.rcgu.o
foo.foo.7rcbfp3g-cgu.6.rcgu.dwo
foo.foo.7rcbfp3g-cgu.6.rcgu.o
foo.foo.7rcbfp3g-cgu.7.rcgu.dwo
foo.foo.7rcbfp3g-cgu.7.rcgu.o
foo.dwp
foo.rs
$ readelf -wi foo.foo.7rcbfp3g-cgu.0.rcgu.o
# ...
  Compilation Unit @ offset 0x90:
   Length:        0x2c (32-bit)
   Version:       4
   Abbrev Offset: 0x5b
   Pointer Size:  8
 <0><9b>: Abbrev Number: 1 (DW_TAG_compile_unit)
    <9c>   DW_AT_stmt_list   : 0xe8
    <a0>   DW_AT_comp_dir    : (indirect string, offset: 0x13b): /home/david/Projects/rust/rust0
    <a4>   DW_AT_GNU_dwo_name: (indirect string, offset: 0x15b): foo.foo.7rcbfp3g-cgu.0.rcgu.dwo
    <a8>   DW_AT_GNU_dwo_id  : 0x357472a2b032d7b9
    <b0>   DW_AT_low_pc      : 0x0
    <b8>   DW_AT_ranges      : 0x40
    <bc>   DW_AT_GNU_addr_base: 0x0
# ...
To-Do

I've opened this PR as a draft to get feedback and work out how we'd expect rustc to work when Split DWARF is requested. It might be easier to read the PR commit-by-commit.

  • Add error when Split DWARF is requested on platforms where it doesn't make sense.
  • Determine whether or not there should be a single dwo output from rustc, or one per codegen-unit as exists currently.
  • Add tests.
  • Fix single mode - currently single mode doesn't change the invocation of addPassesToEmitFile, which is correct, but it also needs to change the split dwarf path provided to createCompileUnit and createTargetMachine so that it's just the final binary (currently it is still a non-existent dwo file).

r? @nagisa
cc @michaelwoerister @eddyb @alexcrichton @rust-lang/wg-incr-comp

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 23, 2020
@eddyb
Copy link
Member

eddyb commented Sep 24, 2020

Determine whether or not there should be a single dwo output from rustc, or one per codegen-unit as exists currently.

What happens when you compile a Cargo package in debug mode, with some dependencies? Debug mode should default to -Cdebuginfo=2 but will also enable -Cincremental. I expect you'll end up with hundreds of dwo files.

Would be nice if it were possible to combine the files cheaply, as managing one dwo file seems easier (for distros, or even rustup, if we e.g. want to add a rustc-debuginfo component that has at least -Cdebuginfo=1 for better backtraces).

Other than this, this PR is great! I remember how much better using split DWARF was when compiling LLVM in debug mode.

It also reminds me that I want to eventually look at perhaps deduplicating some of the DWARF definitions (which get duplicated through instantiation in separate CGUs), and being able to operate on the dwo separately from the rest of the binary should help.

Copy link
Member

@nagisa nagisa left a comment

Choose a reason for hiding this comment

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

https://gcc.gnu.org/wiki/DebugFission and https://gcc.gnu.org/wiki/DebugFissionDWP have some discussion on merging the dwo files into a single file.

So far what I'm seeing looks good to me!

compiler/rustc_session/src/options.rs Show resolved Hide resolved
}
/// Configuration passed to the function returned by the `target_machine_factory`.
pub struct TargetMachineFactoryConfig<'a> {
/// Split DWARF is enabled in LLVM by checking that `TM.MCOptions.SplitDwarfFile` isn't empty,
Copy link
Member

Choose a reason for hiding this comment

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

Hmm… this probably should not be LLVM-specific, but I don’t yet know how the alternative backends could handle this being set (error out? transparently ignore?)

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've added to this commit to note that non-LLVM backends can ignore this if they don't need it for their own Split DWARF support.

@davidtwco
Copy link
Member Author

davidtwco commented Sep 26, 2020

gcc.gnu.org/wiki/DebugFission and gcc.gnu.org/wiki/DebugFissionDWP have some discussion on merging the dwo files into a single file.

As far as I can tell (having just had a quick look at dwp's source), dwp doesn't support DWARF 5-flavoured Split DWARF, only pre-standard GNU-flavoured Split DWARF. According to #75890, LLVM enables DWARF 5 on some backends by default and it will emit DWARF 5-flavoured Split DWARF by default in those cases, so I'm not sure that we could rely on dwp always working.

@davidtwco davidtwco added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 9, 2020
@bors

This comment has been minimized.

@davidtwco davidtwco force-pushed the issue-34651-split-dwarf branch 3 times, most recently from 0d82e49 to 39cb612 Compare October 14, 2020 18:17
@davidtwco
Copy link
Member Author

davidtwco commented Oct 14, 2020

I've revisited this today and made some good progress - I've discovered that llvm-dwp exists, and so I've just started using that. First, I change bootstrap so that, like rust-lld, we ship rust-llvm-dwp in the sysroot. Next, I use rust-llvm-dwp after linking objects to link together the dwo files into a dwp file. And that's basically it.

There's still some work to be done on this PR but this is a reasonable step forward.

cc @Mark-Simulacrum for some bootstrap changes (see e83d81b at time of writing)

@davidtwco davidtwco requested a review from nagisa October 14, 2020 18:20
@bors

This comment has been minimized.

@davidtwco davidtwco added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 3, 2020
@davidtwco
Copy link
Member Author

davidtwco commented Nov 8, 2020

Pushed another update - added a compare mode that makes debuginfo tests also run with Split DWARF - which identified an issue that I've resolved. That might not be the long-term approach we want for testing Split DWARF, but it worked for now and I'm open to ideas.

Only thing I identified initially that I've yet to do is add an error when this is used on platforms where it wouldn't make sense, but I'm not sure if that's actually something we want - would appreciate feedback on that point.

@davidtwco davidtwco marked this pull request as ready for review November 8, 2020 18:05
Copy link
Member

@nagisa nagisa left a comment

Choose a reason for hiding this comment

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

@bors r+

src/bootstrap/test.rs Show resolved Hide resolved
@nagisa nagisa added relnotes Marks issues that should be documented in the release notes of the next release. and removed relnotes Marks issues that should be documented in the release notes of the next release. labels Nov 15, 2020
@nagisa
Copy link
Member

nagisa commented Nov 15, 2020

@bors rollup=iffy

@davidtwco
Copy link
Member Author

davidtwco commented Nov 15, 2020

Should the flag for this be moved to -C before this lands?

I'd also like @Mark-Simulacrum to review the changes to bootstrap. I'm not sure if there's any downsides to adding a new tool to the sysroot.

With that said, Split DWARF in this PR isn't enabled by default or accessible outside of nightly - so we can still make changes (except for the dwp tool in the sysroot, I suppose).

@nagisa
Copy link
Member

nagisa commented Nov 15, 2020

Nah, move from -Z to -C flag should happen as part of a separate stabilization effort.

With that said, Split DWARF in this PR is enabled by default or accessible outside of nightly

Did you mean not enabled by default?

@Mark-Simulacrum
Copy link
Member

I think the main downside to adding the extra tool is that it increases our dependence on LLVM a bit further - I don't think it is a sufficient concern to block this, though. When we get closer to stabilization we can consider this and perhaps look at alternatives (e.g. a rust alternative to dwp). Otherwise the bootstrap changes seem fine.

This commit makes minor changes to the cranelift backend so that it can
build given changes in cg_ssa for Split DWARF.

Signed-off-by: David Wood <david@davidtw.co>
This commit adds a run-make-fulldeps test which checks that a DWARF
package file is emitted.

Signed-off-by: David Wood <david@davidtw.co>
This commit adds a Split DWARF compare mode to compiletest so that
debuginfo tests are also tested using Split DWARF in split mode (and
manually in single mode).

Signed-off-by: David Wood <david@davidtw.co>
llvm-dwp concatenates `DW_AT_comp_dir` with `DW_AT_GNU_dwo_name` (only
when `DW_AT_comp_dir` exists), which can result in it failing to find
the DWARF object files.

In earlier testing, `DW_AT_comp_dir` wasn't present in the final
object and the current directory was the output directory.

When running tests through compiletest, the working directory of the
compilation is different from output directory and that resulted in
`DW_AT_comp_dir` being in the object file (and set to the current
working directory, rather than the output directory), and
`DW_AT_GNU_dwo_name` being set to the full path (rather than just
the filename), so llvm-dwp was failing.

This commit changes the compilation directory provided to LLVM to match
the output directory, where DWARF objects are output; and ensures that
only the filename is used for `DW_AT_GNU_dwo_name`.

Signed-off-by: David Wood <david@davidtw.co>
@davidtwco
Copy link
Member Author

@bors r=nagisa

@bors
Copy link
Contributor

bors commented Dec 16, 2020

📌 Commit ee073b5 has been approved by nagisa

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 16, 2020
@bors
Copy link
Contributor

bors commented Dec 16, 2020

⌛ Testing commit ee073b5 with merge 2ba7ca2...

@bors
Copy link
Contributor

bors commented Dec 16, 2020

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing 2ba7ca2 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 16, 2020
@bors bors merged commit 2ba7ca2 into rust-lang:master Dec 16, 2020
@rustbot rustbot added this to the 1.50.0 milestone Dec 16, 2020
@davidtwco davidtwco deleted the issue-34651-split-dwarf branch December 16, 2020 16:12
davidtwco added a commit to davidtwco/rust that referenced this pull request Dec 16, 2020
This commit includes the `llvm-dwp` tool in the CI LLVM (which rustc
developers can download instead of building LLVM locally) - `llvm-dwp`
is required by Split DWARF which landed in PR rust-lang#77117.

Signed-off-by: David Wood <david@davidtw.co>
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 16, 2020
…llvm, r=Mark-Simulacrum

bootstrap: include llvm-dwp in CI LLVM

Fixes rust-lang#80086.

This PR includes the `llvm-dwp` tool in the CI LLVM (which rustc developers can download instead of building LLVM locally) - `llvm-dwp` is required by Split DWARF which landed in PR rust-lang#77117.

r? `@Mark-Simulacrum`
@philipc
Copy link
Contributor

philipc commented Dec 31, 2020

@davidtwco Is there a reason this requires -C save-temps? I thought it should automatically save the files if needed. I get errors such as this without it:

$ RUSTFLAGS="-Z split-dwarf=split" cargo +nightly test
...
error: linking dwarf objects with `rust-llvm-dwp` failed: exit code: 1
  |
  = note: "rust-llvm-dwp" "-e" "/home/philipc/code/rust/backtrace-rs/target/debug/build/miniz_oxide-f8a4dfce48798ca5/build_script_build-f8a4dfce48798ca5" "-o" "/home/philipc/code/rust/backtrace-rs/target/debug/build/miniz_oxide-f8a4dfce48798ca5/build_script_build-f8a4dfce48798ca5.dwp"
  = note: 
  = note: error: No such file or directory

@davidtwco
Copy link
Member Author

@philipc -C save-temps shouldn't be required, when llvm-dwp is invoked then it should have all of the .o and .dwo files still available. There could be some strange behaviour when this is used with dependencies or build scripts that I didn't discover. Alternatively, there could be some scenario whereby DW_AT_comp_dir concatenated with DW_AT_GNU_dwo_name isn't a valid path to the dwo file (I ran into this in some circumstances while working on this, but thought that I had resolved the issue). Is it possible for you to share the commands you ran to produce this failure?

@philipc
Copy link
Contributor

philipc commented Dec 31, 2020

The command is as given above (RUSTFLAGS="-Z split-dwarf=split" cargo +nightly test) and I am running this in a clone of the backtrace-rs crate. The nightly I was using was the first one that had this PR in it, but I could try a newer nightly if you think that might help. (By the way, that backtrace-rs test will also fail without rust-lang/backtrace-rs#401.)

Alternatively, there could be some scenario whereby DW_AT_comp_dir concatenated with DW_AT_GNU_dwo_name isn't a valid path to the dwo file

I wouldn't expect a problem like that to be fixed by using -C save-temps though?

@davidtwco
Copy link
Member Author

I wouldn't expect a problem like that to be fixed by using -C save-temps though?

I wouldn't either. I'll take a look at this and see if I can work out what is happening as soon as I can.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.