-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Register .dwp files as debuginfo artifacts so they get uplifted #11384
Conversation
With this change linux split-debuginfo=packed builds will place the final .dwp in the top level target dir alongside the binary and report it as a file in the json messages. This puts it on equal footing with .pdb and .dysm files.
r? @weihanglo (rustbot has picked a reviewer for you, use r? to override) |
See rust-lang/rust#98051 (comment) for some additional context on "wait what the hell is a dwp file" |
I guess cc @davidtwco since this feature seems to be your project? |
Ah interesting, seems there's some extra work to do to avoid conflicting names |
Ok so it seems like i've kicked a really cursed bee's nest here. I don't have a proper mingw setup right now, so I'm kinda piecing things together from logs and other platforms.
I'm running afoul of this test which is a regression test for #9325 / #9418. It builds a package with a binary and cdylib target of the same name. Originally this was trying to catch problems with .d files, but now it's finding issues with debug files, which is alluded to in the original issues. The naming scheme we are using for .dwp files on "x64 windows gnu" (mingw?) is such that the binary and cdylib get the same name for their .dwp file and things explode. I believe an analogous thing already happens on msvc windows, as discussed in #9325. Hence why the test is disabled for that platform. Testing on platforms I do have available:
So it seems like this is a continuation of the woes on msvc that result from its naming scheme. However the test only fails now because mingw I guess previously didn't have a debuginfo file to collide? A few options:
I imagine there are Horrible Implications to 3 because things love assuming paths/names. Also that's probably rustc's problem? |
I don't have a strong opinion but I'd probably lean towards doing whatever it is we do for |
I have pushed up the "accept it" option -- CI now passes. |
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.
Thank you for posting this PR! It makes sense to me generally. Could we add some tests similar to uplift_dsym_of_bin_on_mac
and uplift_pdb_of_bin_on_windows
?
Re your questions:
- exactly under what conditions this file type should get pushed (currently "not windows-msvc AND not apple")
It seems that rustc calls thorin in any case if it is not on Windows or macOS. I would expect we are following such a pattern. And this PR just did what I expect 😆.
- what the right default is for the hyphen-renaming flag
I believe it depends on how thorin works and how rustc uses it. If I understand correctly, rustc simply swaps the extension to .dwp
under the name of the given executable, and then to writes data into that file. I interpret it as “we don't need to rename”.
From DWARF v5 spec, it also says “The package file is typically placed in the same directory as the application, and is given the same name with a “.dwp” extension.”.
- whether moving (copying?) the file will break any existing tooling anyone has written (unclear to me if any exists)
People currently debug with .dwp
need to find that file under target/debug/deps/<exe>-<meta>.dwp
. I don't aware of any case this PR will break the debugging workflow. It also seems fine to uplift for consistency across platforms.
prefix: prefix.clone(), | ||
flavor: FileFlavor::DebugInfo, | ||
crate_type: Some(crate_type), | ||
// Currently no known reason to prefer one naming scheme |
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.
Maybe we could have a comment with a link to https://dwarfstd.org/doc/DWARF5.pdf or rustc implementation as a reference, like what .pdb
and .dSYM
do.
I left more detailed comments in the other PR but the values in the table above for linux/cdylib debuginfo and mingw/[binary|cdylib] debuginfo are wrong (in the sense that the table accurately describes what rustc outputs but rustc is outputting the wrong thing). In order for debuggers to find the dwp automatically the filename needs to be exactly the binary filename + ".dwp". For the same reason, the hyphen renaming flag needs to match the binary, not be false unconditionally. |
Close in favor of #11572. Thanks for the initial work and analysis! |
What does this PR try to resolve?
With this change linux split-debuginfo=packed builds will place the final .dwp in the top level target dir alongside the binary and report it as a file in the json messages. This puts it on equal footing with .pdb and .dysm files.
How should we test and review this PR?
Step 0: be on linux with the latest rustc stable release.
Step 1: Add this profile to any project that emits a binary:
Step 2: Run
cargo build --profile=dist --message-format=json
Before:
target/dist/
files
for the binary's buildAfter:
Additional information
I am uncertain on: