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

Apply path remapping to DW_AT_GNU_dwo_name when producing split DWARF #91566

Merged
merged 8 commits into from
Dec 18, 2021

Conversation

cbeuw
Copy link
Contributor

@cbeuw cbeuw commented Dec 5, 2021

--remap-path-prefix doesn't apply to paths to .o (in case of packed) or .dwo (in case of unpacked) files in DW_AT_GNU_dwo_name. GCC also has this bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91888

@rust-highfive
Copy link
Collaborator

r? @oli-obk

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 5, 2021
@oli-obk
Copy link
Contributor

oli-obk commented Dec 5, 2021

cc @davidtwco I saw you mention dwp on zulip recently

@davidtwco
Copy link
Member

davidtwco commented Dec 5, 2021

I don't have time to do a full review right now (will take a closer look tomorrow), but we could probably do this without breaking packaging:

As rustc invokes the dwarf packaging utility (rather than that being part of a separate build process), we can just always provide the paths to the object files directly rather than just rely on the path computed in the packaging utility from DW_AT_comp_dir and DW_AT_GNU_dwo_name (and for dependencies, we look inside the rlib).

@davidtwco
Copy link
Member

r? @davidtwco

@rust-highfive rust-highfive assigned davidtwco and unassigned oli-obk Dec 5, 2021
Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

Implementation looks good to me, are there other changes you want to make (the PR is marked WIP)?

@cbeuw
Copy link
Contributor Author

cbeuw commented Dec 6, 2021

I marked it as draft because I didn't really know if this will work and I wanted to see the Linux CI run results (I'm on Windows so can't really test DWARF properly).

I'll remove WIP and I'm happy to merge as-is once bors is happy too

@cbeuw cbeuw changed the title [WIP] Apply path remapping to DW_AT_GNU_dwo_name when producing split DWARF Apply path remapping to DW_AT_GNU_dwo_name when producing split DWARF Dec 6, 2021
@cbeuw cbeuw marked this pull request as ready for review December 6, 2021 14:03
@rust-log-analyzer

This comment has been minimized.

@cbeuw
Copy link
Contributor Author

cbeuw commented Dec 6, 2021

Turns out my test actually wasn't doing anything and DW_AT_GNU_dwo_name still isn't properly remapped.

This acutally came up before #82074. Will look into it later

@rust-log-analyzer

This comment has been minimized.

@cbeuw

This comment has been minimized.

@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 9, 2021
@cbeuw

This comment has been minimized.

@cbeuw
Copy link
Contributor Author

cbeuw commented Dec 11, 2021

I misunderstood how MCOptions.SplitDwarfFile is used in LLVM. It does control what will be emitted into DW_AT_[GNU_]dwo_name, but it can be remapped because it's not used as a real path if pass in a separate a filestream when we create an emit file pass, which we do here:

with_codegen(tm, llmod, config.no_builtins, |cpm| {
write_output_file(
diag_handler,
tm,
cpm,
llmod,
&obj_out,
dwo_out,
llvm::FileType::ObjectFile,
&cgcx.prof,
)
})?;

And eventually

if (DwoPath) {
raw_fd_ostream DOS(DwoPath, EC, sys::fs::OF_None);
EC.clear();
if (EC)
ErrorInfo = EC.message();
if (ErrorInfo != "") {
LLVMRustSetLastError(ErrorInfo.c_str());
return LLVMRustResult::Failure;
}
buffer_ostream DBOS(DOS);
unwrap(Target)->addPassesToEmitFile(*PM, BOS, &DBOS, FileType, false);
PM->run(*unwrap(M));
} else {
.

So the LLVM change is not needed and the current solution works

@cbeuw cbeuw requested a review from davidtwco December 11, 2021 11:32
Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

Looks good to me, comment or two about some changes but broadly what I'd expect.

compiler/rustc_codegen_llvm/src/back/write.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_llvm/src/back/write.rs Outdated Show resolved Hide resolved
@davidtwco
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Dec 16, 2021

📌 Commit 5e481d0 has been approved by davidtwco

@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, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 17, 2021
Apply path remapping to DW_AT_GNU_dwo_name when producing split DWARF

`--remap-path-prefix` doesn't apply to paths to `.o` (in case of packed) or `.dwo` (in case of unpacked) files in `DW_AT_GNU_dwo_name`. GCC also has this bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91888
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 17, 2021
Apply path remapping to DW_AT_GNU_dwo_name when producing split DWARF

`--remap-path-prefix` doesn't apply to paths to `.o` (in case of packed) or `.dwo` (in case of unpacked) files in `DW_AT_GNU_dwo_name`. GCC also has this bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91888
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 18, 2021
…askrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#91566 (Apply path remapping to DW_AT_GNU_dwo_name when producing split DWARF)
 - rust-lang#91926 (Remove `in_band_lifetimes` from `rustc_metadata`)
 - rust-lang#91931 (Remove `in_band_lifetimes` from `rustc_codegen_llvm`)
 - rust-lang#92024 (rustc_codegen_llvm: Give each codegen unit a unique DWARF name on all platforms, not just Apple ones.)
 - rust-lang#92037 (Use a const ParamEnv when in default_method_body_is_const)
 - rust-lang#92047 (Set `RUST_BACKTRACE=0` when running location-detail tests)
 - rust-lang#92050 (Add a space and 2 grave accents )

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 1c42199 into rust-lang:master Dec 18, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 18, 2021
@cbeuw cbeuw deleted the remap-dwo-name branch December 18, 2021 21:19
@jieyouxu jieyouxu added A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) A-reproducibility Area: Reproducible / deterministic builds labels Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) A-reproducibility Area: Reproducible / deterministic builds S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

9 participants