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

error_with_backtrace_outputs_correctly_with_one_source fails on Windows7 #117941

Closed
roblabla opened this issue Nov 15, 2023 · 7 comments · Fixed by #118137
Closed

error_with_backtrace_outputs_correctly_with_one_source fails on Windows7 #117941

roblabla opened this issue Nov 15, 2023 · 7 comments · Fixed by #118137
Assignees
Labels
C-bug Category: This is a bug. P-low Low priority regression-from-stable-to-beta Performance or correctness regression from stable to beta.

Comments

@roblabla
Copy link
Contributor

Since #117089 was merged, the error_with_backtrace_outputs_correctly_with_one_source std test fails with the following error on windows7:

---- error::tests::error_with_backtrace_outputs_correctly_with_one_source stdout ----
thread 'error::tests::error_with_backtrace_outputs_correctly_with_one_source' panicked at library/std/src/../../backtrace/src/dbghelp.rs:168:1:
called `Option::unwrap()` on a `None` value

After some further instrumentation, I was able to find out the source of the problem: Failed to find SymAddrIncludeInlineTrace in dbghelp. This was likely caused by rust-lang/backtrace-rs@c1464fa , which reworked the backtracing code on windows to be faster through the use of newer APIs. Unfortunately, not all of those APIs are available in Windows 7.

Version it worked on

7201240

Version with regression

608e968

@rustbot modify labels: +regression-from-stable-to-nightly -regression-untriaged

@roblabla roblabla added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Nov 15, 2023
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. and removed regression-untriaged Untriaged performance or correctness regression. labels Nov 15, 2023
@lqd
Copy link
Member

lqd commented Nov 15, 2023

cc @wesleywiser #117089 is going to land in 1.75 while rust-lang/compiler-team#651 drops windows <10 support from 1.76.

Though to be clear windows 7 is not particularly well supported today, CI only tests windows 10.

@roblabla
Copy link
Contributor Author

roblabla commented Nov 15, 2023

@lqd While this manifests as a test error, I'm pretty sure there's an actual regression here that can be observed in code, and isn't just a unit test failure. I haven't taken the time to make an actual reproducer, but I think any attempt to print a Backtrace through its Display implementation will fail. I'll try to make some code showing the regression tomorrow.

Regarding windows7 being not well-supported, I'm well-aware of that. I'm trying to improve on this by running the library/std tests on windows7 every night on some (private) infrastructure, and eventually adding a new (tier3) windows7 target once support is officially dropped in 1.76 so we can continue targeting it.

@lqd
Copy link
Member

lqd commented Nov 15, 2023

I agree this is likely a regression, and pinged the author: it’s likely the fact that the PR will not work on windows 7 wasn’t known then, but at the same time it seems it could be accepted if it landed today.

It’s likely the panic could be resolved though, or some of the old code brought back for the new tier3 target.

@wesleywiser wesleywiser self-assigned this Nov 16, 2023
@saethlin saethlin removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Nov 18, 2023
@apiraino
Copy link
Contributor

Unfortunately, not all of those APIs are available in Windows 7.

@roblabla Just to be sure I understand: Do I read correctly that due to the absence of some APIs, the issue won't reproduce on Win10?

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-low

@rustbot rustbot added P-low Low priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Nov 20, 2023
@roblabla
Copy link
Contributor Author

That is correct, the missing APIs were added in dbghelp.dll version 6.2, which I believe comes built into Windows 8+. At least win10 has it for sure.

@roblabla
Copy link
Contributor Author

I just realized this actually affects the 1.75 beta branch as well, this is a regression from stable to beta.

@ChrisDenton ChrisDenton added regression-from-stable-to-beta Performance or correctness regression from stable to beta. and removed regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Nov 21, 2023
@ChrisDenton
Copy link
Member

Once the backtrace PR is merged another PR updating the crate can be merged here with the beta-nominated label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. P-low Low priority regression-from-stable-to-beta Performance or correctness regression from stable to beta.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants