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

link.exe: Don't embed full path to PDB file in binary. #121297

Merged
merged 3 commits into from
Mar 15, 2024

Conversation

michaelwoerister
Copy link
Member

@michaelwoerister michaelwoerister commented Feb 19, 2024

This PR makes rustc unconditionally pass /PDBALTPATH:%_PDB% to MSVC-style linkers, causing the linker to only embed the filename of the PDB in the binary instead of the full path. This will help implement the trim-paths RFC for *-msvc targets.

Passing /PDBALTPATH:%_PDB% to the linker is already done by many projects that need reproducible builds and debugger's should still be able to find the PDB if it is in the same directory as the binary.

r? @ghost

Fixes #87825

@rustbot rustbot added 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 19, 2024
@michaelwoerister michaelwoerister added A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. WG-debugging Working group: Bad Rust debugging experiences F-trim-paths Feature: trim-paths and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 19, 2024
@michaelwoerister
Copy link
Member Author

#122306 updated backtrace-rs to a version that includes rust-lang/backtrace-rs#584, so this is unblocked now.

@rustbot ping windows

r? compiler

@rustbot rustbot added the O-windows Operating system: Windows label Mar 12, 2024
@rustbot
Copy link
Collaborator

rustbot commented Mar 12, 2024

Hey Windows Group! This bug has been identified as a good "Windows candidate".
In case it's useful, here are some instructions for tackling these sorts of
bugs. Maybe take a look?
Thanks! <3

cc @albertlarsan68 @arlosi @ChrisDenton @danielframpton @gdr-at-ms @kennykerr @luqmana @lzybkr @nico-abram @retep998 @sivadeilra @wesleywiser

@michaelwoerister michaelwoerister removed the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Mar 12, 2024
@michaelwoerister
Copy link
Member Author

@bors rollup=never since this touching an *-msvc-only run-make test.

@kennykerr
Copy link
Contributor

Looks good!

@lcnr
Copy link
Contributor

lcnr commented Mar 12, 2024

r? compiler

@rustbot rustbot assigned nnethercote and unassigned lcnr Mar 12, 2024
@wesleywiser
Copy link
Member

Thanks @michaelwoerister for fixing this and landing that fix to backtrace-rs to unblock it!

@bors r+

@bors
Copy link
Contributor

bors commented Mar 12, 2024

📌 Commit c37a824 has been approved by wesleywiser

It is now in the queue for this repository.

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Mar 12, 2024
@workingjubilee
Copy link
Member

However, backtrace-rs currently does not find the PDB. Consequently, this PR is blocked until rust-lang/backtrace-rs#584 is merged.

Merged and backtrace has been updated, so this can land, yep.

@bors
Copy link
Contributor

bors commented Mar 12, 2024

⌛ Testing commit c37a824 with merge 4026a87...

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 12, 2024
…=wesleywiser

link.exe: Don't embed full path to PDB file in binary.

This PR makes `rustc` unconditionally pass `/PDBALTPATH:%_PDB%` to MSVC-style linkers, causing the linker to only embed the filename of the PDB in the binary instead of the full path. This will help implement the [trim-paths RFC](rust-lang#111540) for `*-msvc` targets.

Passing `/PDBALTPATH:%_PDB%` to the linker is already done by many projects that need reproducible builds and [debugger's should still be able to find the PDB](https://learn.microsoft.com/cpp/build/reference/pdbpath) if it is in the same directory as the binary.

r? `@ghost`

Fixes rust-lang#87825
@bors
Copy link
Contributor

bors commented Mar 12, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 12, 2024
@michaelwoerister
Copy link
Member Author

r? @wesleywiser

Let's see if this is the same i686 backtracing issue already encountered in rust-lang/backtrace-rs#584, where backtracing was pretty flaky without frame pointers enabled.

@michaelwoerister
Copy link
Member Author

@bors r=wesleywiser

@bors
Copy link
Contributor

bors commented Mar 13, 2024

📌 Commit d3af77c has been approved by wesleywiser

It is now in the queue for this repository.

@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 Mar 13, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 13, 2024
…=wesleywiser

link.exe: Don't embed full path to PDB file in binary.

This PR makes `rustc` unconditionally pass `/PDBALTPATH:%_PDB%` to MSVC-style linkers, causing the linker to only embed the filename of the PDB in the binary instead of the full path. This will help implement the [trim-paths RFC](rust-lang#111540) for `*-msvc` targets.

Passing `/PDBALTPATH:%_PDB%` to the linker is already done by many projects that need reproducible builds and [debugger's should still be able to find the PDB](https://learn.microsoft.com/cpp/build/reference/pdbpath) if it is in the same directory as the binary.

r? `@ghost`

Fixes rust-lang#87825
@bors
Copy link
Contributor

bors commented Mar 13, 2024

⌛ Testing commit d3af77c with merge 3f79023...

@bors
Copy link
Contributor

bors commented Mar 13, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 13, 2024
@michaelwoerister
Copy link
Member Author

I modified the test code in 0a094ba, so that unwinding will more reliably produce a stack trace that actually contains frames from the test. Unwinding not always working is a pre-existing issue on i686-pc-windows-msvc and unrelated to the change in this PR.

@wesleywiser
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Mar 14, 2024

📌 Commit 0a094ba has been approved by wesleywiser

It is now in the queue for this repository.

@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 Mar 14, 2024
@bors
Copy link
Contributor

bors commented Mar 15, 2024

⌛ Testing commit 0a094ba with merge c5b5713...

@bors
Copy link
Contributor

bors commented Mar 15, 2024

☀️ Test successful - checks-actions
Approved by: wesleywiser
Pushing c5b5713 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 15, 2024
@bors bors merged commit c5b5713 into rust-lang:master Mar 15, 2024
12 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Mar 15, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c5b5713): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
2.2% [2.2%, 2.2%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.8% [-4.8%, -1.0%] 14
Improvements ✅
(secondary)
-2.5% [-5.7%, -0.5%] 52
All ❌✅ (primary) -2.5% [-4.8%, 2.2%] 15

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 671.321s -> 670.605s (-0.11%)
Artifact size: 311.51 MiB -> 311.47 MiB (-0.01%)

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.) F-trim-paths Feature: trim-paths merged-by-bors This PR was explicitly merged by bors. O-windows Operating system: Windows 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. WG-debugging Working group: Bad Rust debugging experiences
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--remap-path-prefix doesn't map paths to .pdb files, even in release mode
9 participants