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

Do not strip debuginfo by default for MSVC #13630

Merged
merged 2 commits into from Mar 26, 2024
Merged

Do not strip debuginfo by default for MSVC #13630

merged 2 commits into from Mar 26, 2024

Conversation

Kobzol
Copy link
Contributor

@Kobzol Kobzol commented Mar 23, 2024 •
edited

This PR disables the logic which enables debuginfo stripping by default in release mode (#13257) for MSVC, since it causes problems there (rust-lang/rust#122857).

I'm not sure if this is the correct way to gate the logic on a specific target triple.

The root of the issue is that -Cstrip=debuginfo currently behaves like -Cstrip=symbols on MSVC, which causes the original optimization to break backtraces on Windows. Ultimately, this should probably be fixed in rustc, but that might take some further design work, therefore a faster solution would be to just special case this in Cargo for now, and remove the special case later, once it gets fixed on the rustc side.

Related issue: rust-lang/rust#122857

@rustbot
Copy link
Collaborator

rustbot commented Mar 23, 2024

r? @epage

rustbot has assigned @epage.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 23, 2024
@Kobzol Kobzol mentioned this pull request Mar 23, 2024
Closed
weihanglo
weihanglo reviewed Mar 23, 2024
View reviewed changes
Copy link
Member

@weihanglo weihanglo left a comment •
edited

Choose a reason for hiding this comment

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

If this is intended to be a stable backport, you need to file a PR against rust-1.77.0 branch.

@Kobzol
Copy link
Contributor Author

Kobzol commented Mar 23, 2024

This should also be fixed/changed in nightly, I'm not sure if a different fix will appear that soon. So I thought that this should first go into master, and only then be backported into stable.

@ehuss
Copy link
Contributor

ehuss commented Mar 23, 2024

Is this intended to be temporary? It's hard to follow from rust-lang/rust#122857 if this is actually an issue in rustc.

@Kobzol
Copy link
Contributor Author

Kobzol commented Mar 23, 2024

I modified the PR description to add more context.

@workingjubilee
Copy link

Is this intended to be temporary? It's hard to follow from rust-lang/rust#122857 if this is actually an issue in rustc.

This is an overeager cargo behavior that, when combined with an inconsistent rustc behavior, leads to compounded incorrectness.

@workingjubilee
Copy link

workingjubilee commented Mar 23, 2024 •
edited

To be clear, it is my opinion, speaking mostly from my (brief) tenure as backtrace-rs maint:

  • Kobzol's implementation for cargo should have been fine.
  • rustc's behavior verges on simply incoherent, currently.
  • sorting the rustc fix out will likely take more than a week or two.
  • I can't realistically test release-build backtrace-rs on Windows nightly or beta right now because Windows release-build backtraces are effectively dead on nightly so whatever workaround I use won't reflect anything anyone except a scant handful of rustc/cargo devs know to do.
  • therefore, because the cargo + rustc behaviors lead to absurd incorrectness when combined, one should be walked back so that the current nightly/beta can be tested for a reasonable amount of time, and we already established the rustc fix isn't landing tomorrow or even by Wednesday.

@Kobzol Kobzol force-pushed the msvc-disable-release-strip branch from de87e7c to ab285e9 Compare March 24, 2024 08:39
@Kobzol Kobzol force-pushed the msvc-disable-release-strip branch from 647dbab to 53a0dc4 Compare March 25, 2024 22:42
@weihanglo
Copy link
Member

Thanks!

@bors r+

@bors
Copy link
Collaborator

bors commented Mar 26, 2024

📌 Commit 53a0dc4 has been approved by weihanglo

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 26, 2024
@bors
Copy link
Collaborator

bors commented Mar 26, 2024

⌛ Testing commit 53a0dc4 with merge fa619a9...

@weihanglo weihanglo added the beta-nominated Nominated to backport to the beta branch. label Mar 26, 2024
@bors
Copy link
Collaborator

bors commented Mar 26, 2024

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing fa619a9 to master...

@bors bors merged commit fa619a9 into rust-lang:master Mar 26, 2024
21 checks passed
@taiki-e taiki-e mentioned this pull request Mar 26, 2024
Merged
@Kobzol Kobzol deleted the msvc-disable-release-strip branch March 26, 2024 06:34
@Muscraft Muscraft mentioned this pull request Mar 26, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 26, 2024
@bors
Auto merge of rust-lang#123081 - Muscraft:update-cargo, r=Muscraft,wo…
b13a71a 
…rkingjubilee

Update cargo

5 commits in a510712d05c6c98f987af24dd73cdfafee8922e6..499a61ce7a0fc6a72040084862a68b2603e770e8
2024-03-25 03:45:54 +0000 to 2024-03-26 04:17:04 +0000
- fix: do not borrow shell across registry query (rust-lang/cargo#13647)
- Do not strip debuginfo by default for MSVC (rust-lang/cargo#13630)
- chore(deps): update msrv (rust-lang/cargo#13577)
- Fix doc collision for lib/bin with a dash in the inferred name. (rust-lang/cargo#13640)
- refactor: Make lint names snake_case (rust-lang/cargo#13635)

r? ghost
@rustbot rustbot added this to the 1.79.0 milestone Mar 26, 2024
This was referenced Mar 26, 2024
Merged
Merged
bors added a commit that referenced this pull request Mar 26, 2024
@bors
Auto merge of #13654 - weihanglo:rust-1.77.0-backport, r=epage
e52e360 
[stable-1.77] Do not strip debuginfo by default for MSVC

Stable backports:

- #13630

In order to make CI pass, the following PRs are also cherry-picked:

-
bors added a commit that referenced this pull request Mar 26, 2024
@bors
Auto merge of #13653 - weihanglo:rust-1.78.0-backport, r=epage
54d8815 
[beta-1.78] Do not strip debuginfo by default for MSVC

Beta backports:

- #13630

In order to make CI pass, the following PRs are also cherry-picked:

-
@michaelwoerister michaelwoerister mentioned this pull request Mar 27, 2024
Closed
taiki-e referenced this pull request in nextest-rs/nextest Mar 27, 2024
@sunshowers
[meta] set strip = none for release builds
d4f982b 
Restore behavior changed in 4e259a7, because
it turns out that `strip = "debuginfo"` still strips too much.

This was observed with the cargo-nextest 0.9.68-rc.1 binary on illumos, where
`pstack` showed lots of ??? marks.

Part of #1345.
@michaelwoerister michaelwoerister mentioned this pull request Mar 28, 2024
Merged
@filnet filnet mentioned this pull request Mar 28, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ehuss ehuss ehuss left review comments

@workingjubilee workingjubilee workingjubilee left review comments

@weihanglo weihanglo weihanglo approved these changes

Assignees

@epage epage

Labels
beta-nominated Nominated to backport to the beta branch. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.79.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@Kobzol @rustbot @ehuss @workingjubilee @weihanglo @bors @epage