-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Open code the __failfast intrinsic for rtabort! on windows #31519
Conversation
intrinsics::abort compiles down to an illegal instruction, which on Unix-like platforms causes the process to be killed with SIGILL. A more appropriate way to kill the process would be SIGABRT; this indicates better that the runtime has explicitly aborted, rather than some kind of compiler bug or architecture mismatch that SIGILL might indicate. For rtassert!, replace this with libc::abort. libc::abort raises SIGABRT, but is defined to do so in such a way that it will terminate the process even if SIGABRT is currently masked or caught by a signal handler that returns. On non-Unix platforms, retain the existing behavior. On Windows we prefer to avoid depending on the C runtime, and we need a fallback for any other platforms that may be defined. An alternative on Windows would be to call TerminateProcess, but this seems less essential than switching to using SIGABRT on Unix-like platforms, where it is common for the process-killing signal to be printed out or logged. This is a [breaking-change] for any code that depends on the exact signal raised to abort a process via rtabort! cc rust-lang#31273 cc rust-lang#31333
cc @lambda |
This is untested... |
As described https://msdn.microsoft.com/en-us/library/dn774154.aspx This is a Windows 8+ mechanism for terminating the process quickly, which degrades to either an access violation or bugcheck in older versions. I'm not sure this is better the the current mechanism of terminating with an illegal instruction, but we recently converted unix to terminate more correctly with SIGABORT, and this *seems* more correct for windows. [breaking-change]
I'd be a little wary about picking one of the many ways to exit a process on Windows to do something like this. Do other applications/frameworks call this function as well? |
Also from an offline conversation from @brson, we concluded that using |
On Windows 7 and older Illegal instruction leads to exiting with If someone wants to check for return codes that indicate the process crashing in some way rather than exiting cleanly, they just need to check that the exit code is in the range 0xC0000000 - 0xFFFFFFFF. Also for the record |
Another point to make is that the CRT |
I personally think that we should either:
Doing something like this |
Like @retep998 mentions above, the technique used by You can reference the exact implementation of how to throw an uncatchable exception in the CRT sources in the Windows SDK (C:\Program Files (x86)\Windows Kits\10\Source\10.0.10586.0\ucrt). Throwing an uncatchable exception boils down to calling |
So in some sense it's incorrect to abort with illegal instruction because some other party might catch it and attempt to recover, and we really want to bail. I don't imagine there's any useful case for catching a rust abort and attempting to recover. |
Ah ok, to ensure we throw an uncatchable exception sounds like akin to using I can't help but feel that this is a little severe, however, in terms of exiting. Taking a closer look at the "abort points" in the standard library
I think that's all the situations, although I wouldn't bet on it. Of them, the categories are:
Of those, none of them are really hold-the-phone-stop-the-world-literally-nothing-can-progress I think except maybe stack overflow. Things like panicking while panicking, OOM, or stack unwinding returning, we basically just want to make sure that control is never returned back to us. Using a super-uncatchable exception may be the idiomatic thing to do here though? I guess it's what applications that In my opinion if an application is generically catching illegal instructions and attempting to return control arbitrarily back to the point of the illegal instruction, that's just flat out wrong and there's nothing we can do about that. So I guess my takeaway is that it sounds like Windows is mirroring Unix here in the concept of an "uncatchable exception". I'm not sure if the severity is worth it, but it seems idiomatic so far? What do others think? In terms of technical stuff I'd prefer to avoid duplicating the |
Ah I guess one thing I also wanted to point out was that we're pretty bad about |
Closing this due to inactivity for now, but we can of course resubmit if this looks like the right thing to do later on |
Open code the __failfast intrinsic for rtabort! on windows
Based on #31457
As described https://msdn.microsoft.com/en-us/library/dn774154.aspx
This is a Windows 8+ mechanism for terminating the process quickly,
which degrades to either an access violation or bugcheck in older versions.
I'm not sure this is better the the current mechanism of terminating
with an illegal instruction, but we recently converted unix to
terminate more correctly with SIGABORT, and this seems more correct
for windows.
I'm not convinced this is the right thing to do, since it will make it harder to detect runtime aborts on windows - presumably the different exit modes for different windows versions produce different exit codes. Since this is a breaking change we really only want to do it once.
r? @alexcrichton
cc @retep998