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

Register .dwp files as debuginfo artifacts so they get uplifted #11384

Closed
wants to merge 2 commits into from

Conversation

Gankra
Copy link
Contributor

@Gankra Gankra commented Nov 16, 2022

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:

[profile.dist]
inherits = "release"
split-debuginfo = "packed"
debug = true

Step 2: Run cargo build --profile=dist --message-format=json

Before:

  • .dwp file doesn't end up in the top level of target/dist/
  • json message doesn't include .dwp file in files for the binary's build

After:

  • Both of those things now happen, establishing parity.

Additional information

I am uncertain on:

  • exactly under what conditions this file type should get pushed (currently "not windows-msvc AND not apple")
  • what the right default is for the hyphen-renaming flag
  • whether moving (copying?) the file will break any existing tooling anyone has written (unclear to me if any exists)

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.
@rustbot
Copy link
Collaborator

rustbot commented Nov 16, 2022

r? @weihanglo

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 16, 2022
@Gankra
Copy link
Contributor Author

Gankra commented Nov 16, 2022

See rust-lang/rust#98051 (comment) for some additional context on "wait what the hell is a dwp file"

@Gankra
Copy link
Contributor Author

Gankra commented Nov 16, 2022

I guess cc @davidtwco since this feature seems to be your project?

@Gankra
Copy link
Contributor Author

Gankra commented Nov 16, 2022

Ah interesting, seems there's some extra work to do to avoid conflicting names

@Gankra
Copy link
Contributor Author

Gankra commented Nov 16, 2022

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.

warning: output filename collision.

The targets should have unique names.
Consider changing their names to be unique or compiling them separately.

Colliding filename is: D:/a/cargo/cargo/target/tmp/cit/t340/foo/target/debug/foo.dwp

This may become a hard error in the future; see <https://github.com/rust-lang/cargo/issues/6313>.
The bin target foo in package foo v1.0.0 (D:/a/cargo/cargo/target/tmp/cit/t340/foo) has the same output filename as the lib target foo in package foo v1.0.0 (D:/a/cargo/cargo/target/tmp/cit/t340/foo).

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:

binary cdylib binary debuginfo cdylib debuginfo
macos mypackage libmypackage.dylib mypackage.dSYM libmypackage.dylib.dSYM
linux mypackage libmypackage.so mypackage.dwp libmypackage.dwp
msvc mypackage.exe mypackage.dll mypackage.pdb 💥 mypackage.pdb 💥
mingw ? ? mypackage.dwp 💥 mypackage.dwp 💥

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:

  1. accept it: disable the test on mingw too
  2. ignore it: don't uplift dwp files on mingw
  3. fix it: make mingw's output naming format more robust

I imagine there are Horrible Implications to 3 because things love assuming paths/names. Also that's probably rustc's problem?

@davidtwco
Copy link
Member

I don't have a strong opinion but I'd probably lean towards doing whatever it is we do for pdb files on MSVC (as far as I can tell, that's the "accept it" option).

@Gankra
Copy link
Contributor Author

Gankra commented Nov 21, 2022

I have pushed up the "accept it" option -- CI now passes.

Copy link
Member

@weihanglo weihanglo left a 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
Copy link
Member

@weihanglo weihanglo Nov 23, 2022

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.

@weihanglo weihanglo added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 30, 2022
@weihanglo
Copy link
Member

Friendly ping @Gankra. Just checking in to see if you are still interested in adding some tests and comments for this. I guess @khuey is willing to help if you don't have time :)

@khuey
Copy link
Contributor

khuey commented Jan 13, 2023

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.

@weihanglo
Copy link
Member

Close in favor of #11572. Thanks for the initial work and analysis!

@weihanglo weihanglo closed this Jan 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants