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

No backtrace on windows with current rustc stable #122857

Closed
TheBlueMatt opened this issue Mar 21, 2024 · 43 comments
Closed

No backtrace on windows with current rustc stable #122857

TheBlueMatt opened this issue Mar 21, 2024 · 43 comments
Labels
A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) O-windows-msvc Toolchain: MSVC, Operating system: Windows P-critical Critical priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-debugging Working group: Bad Rust debugging experiences

Comments

@TheBlueMatt
Copy link

TheBlueMatt commented Mar 21, 2024

Currently rustc beta in our CI is giving us backtraces like this with simple cargo tests (with Cargo.toml set for opt-level = 1):

0: <unknown>
   1: <unknown>
   2: <unknown>
   3: <unknown>
   4: <unknown>
   5: <unknown>
   6: <unknown>
   7: <unknown>
   8: <unknown>
   9: <unknown>
  10: <unknown>
  11: <unknown>
  12: <unknown>
  13: <unknown>
  14: BaseThreadInitThunk
  15: RtlUserThreadStart
@ChrisDenton
Copy link
Member

ChrisDenton commented Mar 21, 2024

Hm, I wonder if that's due to a change in debug information being generated. Does it work if you use debuginfo="line-tables-only"? EDIT: Oh the Cargo.toml would be debug = "line-tables-only"

@TheBlueMatt
Copy link
Author

TheBlueMatt commented Mar 21, 2024

I tried RUSTFLAGS="-C debuginfo=line-tables-only" cargo test --features backtrace and it seems to still fail with the same backtrace.

@workingjubilee
Copy link
Member

@TheBlueMatt What about nightly?

@ChrisDenton
Copy link
Member

The reasons I can think of for a lack of symbols is:

  • debug info is not being generated
  • debug info is being removed somehow
  • the pdb file is not being found for some reason
  • there's a bug in the dbghelp code

The last case I think is the most unlikely because that should show up in our CI. The other cases are more likely caused by either a rustc or CI issue, in which case it should be reported on the rust-lang/rust repository. A possibly relevant PR I see is #121297

@workingjubilee
Copy link
Member

Oh yeah, the beta is a really recent nightly, right.

@workingjubilee
Copy link
Member

Confirmed in rust-lang/backtrace-rs#594 that our CI for Windows is now broken, likely due to rustc changes. Transferring this to rust-lang/rust accordingly.

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Mar 22, 2024
@workingjubilee workingjubilee transferred this issue from rust-lang/backtrace-rs Mar 22, 2024
@workingjubilee workingjubilee added O-windows Operating system: Windows A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-debugging Working group: Bad Rust debugging experiences O-windows-msvc Toolchain: MSVC, Operating system: Windows regression-from-stable-to-beta Performance or correctness regression from stable to beta. and removed O-windows Operating system: Windows labels Mar 22, 2024
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Mar 22, 2024
@workingjubilee
Copy link
Member

cc @michaelwoerister sorry but it seems the combination of everything actually merging together did in fact break backtraces!

@workingjubilee
Copy link
Member

workingjubilee commented Mar 22, 2024

For note it has not been confirmed in a beyond-a-shadow-of-a-doubt way that #121297 is the guilty commit.

@workingjubilee workingjubilee added the E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc label Mar 22, 2024
@saethlin saethlin added T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Mar 22, 2024
@workingjubilee workingjubilee added regression-untriaged Untriaged performance or correctness regression. and removed regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Mar 22, 2024
@workingjubilee
Copy link
Member

workingjubilee commented Mar 22, 2024

hmm. the remarks in rust-lang/backtrace-rs#594 (comment) removes my confidence this is a stable->beta regression and not a stable->stable regression? could it have been rust-lang/cargo#13257 instead? cc @Kobzol

people were relying on these backtraces working. but should they have not been? and how do PDBs work anyways?

@ChrisDenton
Copy link
Member

Also cc @wesleywiser, as I'm wondering what the semantics of strip="debuginfo" should be. Currently it's treated the same as strip="symbols".

@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-high

@rustbot rustbot added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Mar 22, 2024
@Kobzol
Copy link
Contributor

Kobzol commented Mar 23, 2024

I tried to cook up something in rust-lang/cargo#13630, but I'm not sure if it's the right approach.

@ChrisDenton
Copy link
Member

@michaelwoerister It seems like #115120 will take time to design but there is perhaps an interim solution that's simpler and easier to backport. We could simply make strip = "debuginfo" the same as strip = "none" with the rationale that, whatever strip = "debuginfo" does or does not do, it should not be stripping symbols from anywhere.

@Kobzol
Copy link
Contributor

Kobzol commented Mar 24, 2024

An alternative to rust-lang/cargo#13630 that could fix the problem could be something like this.

@ChrisDenton
Copy link
Member

I think I'd suggest something more like this. This would lead to more consistent behaviour. We could additionally skip including natvis but I'm more "meh" on that, especially if the eventual intent is for strip to have no effect on external files.

@michaelwoerister
Copy link
Member

I think it is clear by now that we don't want -Cstrip to touch PDB files or other separate debuginfo. So I think that @ChrisDenton's proposal is a move in the right direction.

However, given that the current documentation of -Cstrip mentions debuginfo not being copied to separate files, I think we need an FCP to change that behavior, which we know can take a long time.

That being the case, I think we should temporarily revert Cargo's defaults when targeting *-windows-msvc as @Kobzol proposes in rust-lang/cargo#13630.

@ChrisDenton
Copy link
Member

I created #123031 if you wanted to consider an fcp. In the meantime, backporting @Kobzol's proposal sounds good to me!

bors added a commit to rust-lang/cargo that referenced this issue Mar 26, 2024
Do not strip debuginfo by default for MSVC

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

stable cargo backport is up #123105

taiki-e referenced this issue in nextest-rs/nextest Mar 27, 2024
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.
@VorpalBlade
Copy link

Now that I think about it, that can actually be a bit problematic, since -Cstrip apparently overrides -Cdebug, and since Cargo uses -Cstrip, it cannot be easily overridden by RUSTFLAGS (OTOH, things that can be configured in a profile probably shouldn't be set in the flags...).

@Kobzol This might impact building packages for Linux distros too. I know Arch Linux build system sets RUSTFLAGS expecting it to take effect just like CFLAGS etc does. It is exposed in the user config at /etc/makepkg.conf too. I would expect other distros to be impacted as well.

@Kobzol
Copy link
Contributor

Kobzol commented Mar 27, 2024

You can still overwrite the defaut by setting RUSTFLAGS="-Cstrip=none".

@apiraino
Copy link
Contributor

I will remove the t-compiler nomination since a decision to fix this in cargo was reached way faster, so I guess there is not a lot to discuss right now :-)

@rustbot label -I-compiler-nominated

@rustbot rustbot removed the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Mar 28, 2024
@ChrisDenton
Copy link
Member

Discussing what rustc should do has moved to #115120 (comment)

@retep998
Copy link
Member

Windows MSVC has debuginfo in a separate file, and thus stripping provides no benefits to binary size. Given generating the .pdb file does not take that much time, and how useful they are for backtraces, you should thus always default to generating the .pdb on Windows MSVC regardless of debug vs release.

This is something that was already figured out years before but I guess people just forget after a while.

@ChrisDenton
Copy link
Member

The "striping" behaviour of rustc has existed for four years now a6c2f73. I don't think Cargo was necessarily wrong to use strip cross-platform but rustc should have treated it as a no-op because, as you say, there's nothing to strip from the binary.

@retep998
Copy link
Member

retep998 commented Mar 28, 2024

And I think that PR from four years ago was wrong to do so. Someone brought up the reason that people might want to not emit PDB files on Windows (#71825 (comment)), but if you look into the linked issue (#67012) you'll see that they actually just didn't want the full path to the PDB file embedded into the binary. However, that reason was solved by a different PR fairly recently (#121297). I even stated in that issue (#67012) that Rust always emitting a PDB file was intended behavior, and yet that wasn't considered by the people in the split implementation PR! As in an earlier PR we intentionally made the conscious decision to emit /DEBUG unconditionally (#28505), and yet nobody thought to ask why or consult a Windows subject matter expert about it.

Thus we should stop having strip affect PDB generation on Windows MSVC at all. It was wrong to do in the first place, but it's not too late to fix it.

@m-ou-se m-ou-se removed the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Apr 3, 2024
@workingjubilee
Copy link
Member

workingjubilee commented Apr 4, 2024

This was fixed in:

Nightly already landed the original cargo fix. Nightly should also be landing #115120 sometime soon (for very expansive definitions of "soon") which would allow us to revert the cargo workaround at our leisure.

@apiraino
Copy link
Contributor

can this be closed now?

@TheBlueMatt
Copy link
Author

Our CI started working again, so I assume so 🤷‍♂️

@lqd
Copy link
Member

lqd commented Apr 15, 2024

Fixes have landed on stable, beta, and nightly. Closing.

@lqd lqd closed this as completed Apr 15, 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.) O-windows-msvc Toolchain: MSVC, Operating system: Windows P-critical Critical priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. 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

No branches or pull requests