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

Add support for panics on Windows #1059

Closed
Aaron1011 opened this issue Nov 17, 2019 · 9 comments · Fixed by #1461
Closed

Add support for panics on Windows #1059

Aaron1011 opened this issue Nov 17, 2019 · 9 comments · Fixed by #1461
Labels
A-panics Area: affects panics and unwinding A-shims Area: This affects the external function shims A-windows Area: affects only Windows targets C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement

Comments

@Aaron1011
Copy link
Member

Aaron1011 commented Nov 17, 2019

Several tests are currently disabled on Windows, due to Miri's lack of support for a number of Windows API functions:

  • GetProcAddress
  • GetModuleHandleW
  • AcquireSRWLockShared

Implementing GetProcAddress and GetModuleHandleW will require us to extend our current handling of dlsym to support Windows. This is currently blocked on #1225 rust-lang/rust#66470 rust-lang/rust#69326, since we need the ability to read wide strings.

@RalfJung RalfJung added A-shims Area: This affects the external function shims A-windows Area: affects only Windows targets C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement labels Nov 17, 2019
Aaron1011 added a commit to Aaron1011/miri that referenced this issue Nov 19, 2019
We should re-enable this once
rust-lang#1059 is fixed
@RalfJung RalfJung added the A-panics Area: affects panics and unwinding label Nov 19, 2019
@RalfJung
Copy link
Member

RalfJung commented Mar 11, 2020

The alternative (which avoids having to implement read-write locks for Windows) is to patch libstd so that RwLock, like Mutex, falls back to EnterCriticalSection when SRWLock is not available.

That would also have the added benefit of making panicking work again on Windows XP (rust-lang/rust#34538).

@CAD97
Copy link

CAD97 commented Mar 18, 2020

At the very least, it'd be great to have a better, more appropriate error message than "thread panicked while processing panic. aborting.". I thought I had a very different problem when I saw that message under miri, when actually everything was working properly, it's just that on Windows miri does not implement panics yet.

I was trying to use miri to implement leak check tests in the face of panics, but because of this I had to fall back to the (probably better, because it works for cargo test) approach of tracking new/drop calls manually.

@RalfJung
Copy link
Member

If you have any suggestions for how we could get a better error, I'm all ears. The error is emitted by libstd, not by Miri.

Once rust-lang/rust#69729 lands, you should be able to use --target x86_64-unknown-linux-gnu on Windows, and then panics should work fine.

@RalfJung
Copy link
Member

@CAD97 but in your repo, doesn't it run Miri on ubuntu-latest? How is this Windows-only issue a problem?

@CAD97
Copy link

CAD97 commented Mar 19, 2020

Yeah, CI is running on Ubuntu, but I also am running tests locally, which is a lot lower latency 😝

I'm not exactly certain how best to get a better error message for Windows targets. Didn't miri used to say something along the lines of "panicking not supported"? I'd expect that message still on the Windows target, rather than for the panic to start but than starting the panic but then failing to handle it.

@RalfJung
Copy link
Member

Ah, good idea... see #1241. Testing would be welcome, as I don't have a Windows machine to try this on. :)

bors added a commit that referenced this issue Mar 19, 2020
whitelist platforms where panicking should work

@CAD97 [proposed](#1059 (comment)) trying to get a better error for failed panics on Windows.

Could you test if this works for you?
bors added a commit that referenced this issue Mar 21, 2020
whitelist platforms where panicking should work

@CAD97 [proposed](#1059 (comment)) trying to get a better error for failed panics on Windows.

Could you test if this works for you?
bors added a commit that referenced this issue Mar 21, 2020
whitelist platforms where panicking should work

@CAD97 [proposed](#1059 (comment)) trying to get a better error for failed panics on Windows.

Could you test if this works for you?
bors added a commit that referenced this issue Mar 21, 2020
whitelist platforms where panicking should work

@CAD97 [proposed](#1059 (comment)) trying to get a better error for failed panics on Windows.

Could you test if this works for you?
@RalfJung
Copy link
Member

With #1249, another alternative for Windows users is to pass --target x86_64-unknown-linux-gnu -- cross-interpretation like that should work fine now, and unwinding is supported for Linux targets.

@RalfJung RalfJung changed the title Add support for unwinding on Windows Add support for panics on Windows Apr 11, 2020
@RalfJung
Copy link
Member

RalfJung commented Apr 11, 2020

Solving this by implementing the "shared RW lock" Windows methods would likely also solve #1302.

@RalfJung
Copy link
Member

@CAD97

I was trying to use miri to implement leak check tests in the face of panics

Note that Miri currently does not check for leaks on Windows, see #1302.

bors added a commit that referenced this issue May 21, 2020
prepare Dlsym system for dynamic symbols on Windows

This makes progress towards #1059.
bors added a commit that referenced this issue May 21, 2020
prepare Dlsym system for dynamic symbols on Windows

This makes progress towards #1059.
bors added a commit that referenced this issue Jun 28, 2020
Implement rwlocks on Windows

Fixes #1059
@bors bors closed this as completed in 2dfa6c1 Jun 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-panics Area: affects panics and unwinding A-shims Area: This affects the external function shims A-windows Area: affects only Windows targets C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants