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

Open code the __failfast intrinsic for rtabort! on windows #31519

Closed
wants to merge 2 commits into from

Conversation

brson
Copy link
Contributor

@brson brson commented Feb 9, 2016

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

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
@brson
Copy link
Contributor Author

brson commented Feb 9, 2016

cc @lambda

@brson
Copy link
Contributor Author

brson commented Feb 9, 2016

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

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?

@alexcrichton
Copy link
Member

Also from an offline conversation from @brson, we concluded that using intrinsics::abort may not be all that bad because it's cross-platform and cross-architecture, and if you specifically want to detect any particular runtime abort scenario you'd still have to scrape stderr as opposed to looking at the exit code.

@retep998
Copy link
Member

retep998 commented Feb 9, 2016

On Windows 7 and older __fastfail will just cause some sort of access violation or trap which will kill the process, exactly on par with using an illegal instruction to kill the process. On Windows 8 or newer however it is an unrecoverable exception that informs Windows to immediately kill the process without even letting exception handlers run. Also it provides a much nicer message when debugging such a process. Just look at how nice it is!

Illegal instruction leads to exiting with STATUS_ILLEGAL_INSTRUCTION.
__fastfail leads to exiting with STATUS_STACK_BUFFER_OVERRUN on Windows 10.

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 TerminateProcess is considered a clean exit and not a crash, lets the user specify an exit code, and also doesn't trigger debuggers. For these reasons it shouldn't be used for aborts that you want to be noticed.

@retep998
Copy link
Member

Another point to make is that the CRT abort also uses __fastfail to kill the process so there is precedent.

@alexcrichton
Copy link
Member

I personally think that we should either:

  1. Just stick to using intrinsics::abort everywhere, it's simple, triggers the right behavior in terms of debuggers, core dumps, etc.
  2. Use abort on Unix and intrinsics::abort on Windows. The only real reason I see to do this is that abort apparently does things like unregister signal handlers which may be useful at some point in the future.

Doing something like this __fastfail thing seems basically equivalent to intrinsics::abort but just fishing for something Windows-specific because we may do something Unix-specific as well. It doesn't look like there's necessarily a concrete reason to do so?

@ras0219-msft
Copy link

Like @retep998 mentions above, the technique used by abort() in the uCRT on Windows is to use __fastfail for 8+ and to throw an uncatchable exception on previous Windows versions. The need for the uncatchable exception is because on Windows you can use SEH to catch things like Invalid Instructions and Access Violations. If you aren't concerned about SEH on 7-, then simply using __fastfail will do the right thing in the future and will do "close enough" on 7-.

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 SetUnhandledExceptionFilter(0) and then UnhandledExceptionFilter(...) with the right struct.

@brson
Copy link
Contributor Author

brson commented Feb 11, 2016

The need for the uncatchable exception is because on Windows you can use SEH to catch things like Invalid Instructions and Access Violations

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.

@alexcrichton
Copy link
Member

Ah ok, to ensure we throw an uncatchable exception sounds like akin to using abort on Unix which unmasks all signals and resets handlers, and that sounds like some good concrete motivation to me!

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

  • There's a few in src/libstd/panicking.rs which don't use rtabort! and explicitly use intrinsics::abort. These are used to detect situations like panicking-while-panicking or panicking-while-processing-a-panic.
  • On Unix we use rtabort! when handling stack overflow (which funnels here), on Windows we do not abort but just print and let Windows take care of the rest.
  • On OOM we call intrinsics::abort for both Unix and Windows
  • We use rtabort! all over the place when unwinding if an "unwind the stack" function ended up returning.

I think that's all the situations, although I wouldn't bet on it. Of them, the categories are:

  • panics while panicking
  • stack overflow
  • out-of-memory
  • stack unwinding ended up returning

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 assert on Unix end up doing (and perhaps Windows as well).

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 dumb_print and using asm!, but those are pretty minor.

@alexcrichton
Copy link
Member

Ah I guess one thing I also wanted to point out was that we're pretty bad about intrinsics::abort vs rtabort!, so this also won't catch everything in libstd today

@alexcrichton
Copy link
Member

Closing this due to inactivity for now, but we can of course resubmit if this looks like the right thing to do later on

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.

5 participants