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.

Sign up for GitHub

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

Jump to bottom

Ignore -C strip on MSVC #115120

Merged
merged 1 commit into from
Apr 22, 2024
Merged

Ignore -C strip on MSVC #115120

merged 1 commit into from
Apr 22, 2024

Conversation

icedrocket
Copy link
Contributor

@icedrocket icedrocket commented Aug 22, 2023 •
edited
Loading

tl;dr - Define -Cstrip to only ever affect the binary; no other build artifacts.

This is necessary to improve cross-platform behavior consistency: if someone wanted debug information to be contained only in separate files on all platforms, they would set -Cstrip=symbols and -Csplit-debuginfo=packed, but this would result in no PDB files on MSVC.

Resolves #114215

@rustbot
Copy link
Collaborator

rustbot commented Aug 22, 2023

r? @b-naber

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

@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 Aug 22, 2023
@icedrocket
Copy link
Contributor Author

icedrocket commented Aug 23, 2023 •
edited
Loading

There may be cases where someone doesn't want a PDB file to be created. One way around this is to prevent a PDB file from being created when split-debuginfo=off.

@b-naber
Copy link
Contributor

b-naber commented Aug 27, 2023

One way around this is to prevent a PDB file from being created when split-debuginfo is none

I don't understand what you're trying to say here, do you mean that you could prevent a PDB file from being created on MSVC by using split-debuginfo=off? off is not supported on MSVC, so there wouldn't be a way to prevent a PDB file from being created if we ignore strip afaict.

@icedrocket
Copy link
Contributor Author

icedrocket commented Aug 27, 2023 •
edited
Loading

Currently, the value of the strip option determines whether or not a PDB file is created, and I'm proposing to remove that condition or use a different one. The title of the pull request is just my main proposal.

Yes, MSVC doesn't actually support split-debuginfo=off, but we can use it as a way to prevent the PDB file from being created. I think the strip option should only be applied to the executable itself, not to a separate debug file, as it would break cross-platform behavior consistency.

@b-naber
Copy link
Contributor

b-naber commented Aug 27, 2023

I think it would make sense to treat split-debuginfo that way on msvc. That would likely require an mcp though and would also be a prerequisite for landing this pr.

@icedrocket icedrocket mentioned this pull request Aug 28, 2023
Closed
3 tasks
@apiraino
Copy link
Contributor

apiraino commented Oct 5, 2023

earmarking this PR, waiting on that MCP seconding

@rustbot author s-waiting-on-mcp

@rustbot rustbot 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 5, 2023
@apiraino apiraino added the S-waiting-on-MCP Status: PR has a compiler MCP and is waiting for the compiler MCP to complete. label Oct 5, 2023
@Dylan-DPC Dylan-DPC removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Nov 5, 2023
@rustbot rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Nov 7, 2023
@rustbot
Copy link
Collaborator

rustbot commented Nov 7, 2023

These commits modify compiler targets.
(See the Target Tier Policy.)

This PR changes config.example.toml. If appropriate, please also update CONFIG_CHANGE_HISTORY in src/bootstrap/src/lib.rs and change-id in config.example.toml.

@icedrocket icedrocket changed the title debuginfo: ignore strip on msvc Use split-debuginfo for PDB generation on MSVC Nov 7, 2023
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@icedrocket
Copy link
Contributor Author

r? @bjorn3

@rustbot rustbot assigned bjorn3 and unassigned b-naber Nov 7, 2023
@bors

This comment was marked as resolved.

Sign in to view
@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Nov 8, 2023
@icedrocket
Copy link
Contributor Author

@rustbot review

@rustbot rustbot 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 9, 2023
@retep998
Copy link
Member

retep998 commented Mar 28, 2024 •
edited
Loading

As for the split-debuginfo option, that simply does not even make sense to ask as a question on Windows MSVC.

As I understand it on MSVC there is still the option to only emit just enough debuginfo in the pdb file to lookup the actual debuginfo in the original object files. This would correspond to -Csplit-debuginfo=unpacked.

Are you referring to /DEBUG:FASTLINK which was added in VS 2017? If so, it's already no longer recommended to use. (https://learn.microsoft.com/en-us/cpp/build/reference/debug-generate-debug-info?view=msvc-170)

Since Visual Studio 2019, /DEBUG:FULL linking times have improved significantly, and /DEBUG:FASTLINK isn't always faster than /DEBUG:FULL. Since /DEBUG:FASTLINK no longer provides large build time improvements and results in a slower debugging experience versus /DEBUG:FULL, this option is no longer recommended.

@bjorn3
Copy link
Member

bjorn3 commented Mar 28, 2024

Are you referring to /DEBUG:FASTLINK which was added in VS 2017?

Yeah

If so, it's already no longer recommended to use.

I see

@icedrocket icedrocket changed the title Limit -C strip on MSVC Ignore -C strip on MSVC Mar 28, 2024
@michaelwoerister
Copy link
Member

Thanks for updating the PR, @icedrocket!

For the record, I don't think any of the new comments are in conflict with the FCP (which only proposes to ignore -Cstrip when targeting MSVC). Please let us know if there are any concerns with the FCP or the current version (95adb6d) of the PR.

@bors

This comment was marked as resolved.

Sign in to view
@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Apr 9, 2024
@rfcbot
Copy link

rfcbot commented Apr 9, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Apr 19, 2024
@rfcbot
Copy link

rfcbot commented Apr 19, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@michaelwoerister
Copy link
Member

Thanks for keeping at this, @icedrocket!

@bors r rollup=never

@bors
Copy link
Contributor

bors commented Apr 22, 2024

📌 Commit e82f46a has been approved by michaelwoerister

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 Apr 22, 2024
@bors
Copy link
Contributor

bors commented Apr 22, 2024

⌛ Testing commit e82f46a with merge 7f2fc33...

@bors
Copy link
Contributor

bors commented Apr 22, 2024

☀️ Test successful - checks-actions
Approved by: michaelwoerister
Pushing 7f2fc33 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 22, 2024
@bors bors merged commit 7f2fc33 into rust-lang:master Apr 22, 2024
12 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Apr 22, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (7f2fc33): 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.4% [2.4%, 2.4%] 1
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 2.4% [2.4%, 2.4%] 1

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.419s -> 670.936s (-0.07%)
Artifact size: 315.41 MiB -> 315.45 MiB (0.01%)

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Apr 29, 2024
@weihanglo
Copy link
Member

weihanglo commented Jun 13, 2024 •
edited
Loading

Since this is in 1.79.0 already, should we revert this PR in Cargo?

weihanglo added a commit to weihanglo/cargo that referenced this pull request Jun 13, 2024
@weihanglo
Revert "Auto merge of rust-lang#13630 - Kobzol:msvc-disable-release-s…
4f002a5 
…trip, r=weihanglo"

This reverts commit fa619a9, reversing
changes made to 1f6857d.

See also <rust-lang/rust#115120>
@weihanglo weihanglo mentioned this pull request Jun 13, 2024
Merged
bors added a commit to rust-lang/cargo that referenced this pull request Jun 13, 2024
@bors
Auto merge of #14061 - weihanglo:revert-13630, r=epage
ebb6a36 
Revert #13630 as rustc ignores `-C strip` on MSVC

This reverts commit fa619a9, reversing changes made to 1f6857d.

See also <rust-lang/rust#115120>.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjorn3 bjorn3 Awaiting requested review from bjorn3

Assignees

@davidtwco davidtwco

Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. 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. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.79.0
Development

Successfully merging this pull request may close these issues.

Inconsistent behavior across platform of split-debuginfo