Skip to content

whitelist platforms where panicking should work #1241

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

Merged
merged 3 commits into from
Mar 21, 2020

Conversation

RalfJung
Copy link
Member

@CAD97 proposed trying to get a better error for failed panics on Windows.

Could you test if this works for you?

@RalfJung
Copy link
Member Author

Hm no this will probably be too late, actually... I think the RwLock is acquired earlier. We probably need to abort already in panic_impl.

@RalfJung
Copy link
Member Author

I moved the check further "up", i.e. it is now checking earlier and hopefully should no longer reach the bad RwLock on Windows.

@CAD97
Copy link

CAD97 commented Mar 19, 2020

I would test it but I'm having trouble getting miri to build from my (windows) machine. Whichever path I'm trying I fail to find rustc_apfloat. (Yes, I have used rustup-toolchain-install-master to be compiling on master, and have tried just running ./miri build in Git Bash.)

@RalfJung
Copy link
Member Author

Well, let's see if this test passes then.
@bors r+

@bors
Copy link
Contributor

bors commented Mar 19, 2020

📌 Commit 0ae6288 has been approved by RalfJung

@bors
Copy link
Contributor

bors commented Mar 19, 2020

⌛ Testing commit 0ae6288 with merge efcf1ac...

bors added a commit that referenced this pull request 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
Copy link
Contributor

bors commented Mar 19, 2020

💔 Test failed - status-appveyor

@RalfJung
Copy link
Member Author

Nope... there's no "hooked" function on Windows that we can intercept to get a proper error. :/

@bjorn3
Copy link
Member

bjorn3 commented Mar 19, 2020

I would test it but I'm having trouble getting miri to build from my (windows) machine. Whichever path I'm trying I fail to find rustc_apfloat.

Is the rustc-dev component installed?

@CAD97
Copy link

CAD97 commented Mar 19, 2020

Ah, that's almost certainly it.

@bjorn3
Copy link
Member

bjorn3 commented Mar 19, 2020

Filled rust-lang/rust#70157 for a better error message.

@RalfJung
Copy link
Member Author

All right I think this covers all entry points. Let's see what CI says.
@CAD97 could you also test this?

@bors r+

@bors
Copy link
Contributor

bors commented Mar 21, 2020

📌 Commit bde3111 has been approved by RalfJung

@bors
Copy link
Contributor

bors commented Mar 21, 2020

⌛ Testing commit bde3111 with merge ff9f866...

bors added a commit that referenced this pull request 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
Copy link
Contributor

bors commented Mar 21, 2020

💔 Test failed - status-appveyor

@RalfJung
Copy link
Member Author

@bors r+

@bors
Copy link
Contributor

bors commented Mar 21, 2020

📌 Commit 7a10c9d has been approved by RalfJung

bors added a commit that referenced this pull request 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
Copy link
Contributor

bors commented Mar 21, 2020

⌛ Testing commit 7a10c9d with merge 9ef2ef0...

@bors
Copy link
Contributor

bors commented Mar 21, 2020

💔 Test failed - status-appveyor

@RalfJung
Copy link
Member Author

@bors r+

@bors
Copy link
Contributor

bors commented Mar 21, 2020

📌 Commit 5c09047 has been approved by RalfJung

@bors
Copy link
Contributor

bors commented Mar 21, 2020

⌛ Testing commit 5c09047 with merge b2605d8...

@bors
Copy link
Contributor

bors commented Mar 21, 2020

☀️ Test successful - checks-travis, status-appveyor
Approved by: RalfJung
Pushing b2605d8 to master...

@bors bors merged commit b2605d8 into rust-lang:master Mar 21, 2020
@RalfJung RalfJung deleted the dont-panic branch March 21, 2020 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants