Uh oh!
There was an error while loading. Please reload this page.
Use libc::abort, not intrinsics::abort, in rtabort! - #31457
Conversation
rust-highfive
commented
Feb 6, 2016
r? @brson (rust_highfive has picked a reviewer for you, use r? to override) |
lambda
commented
Feb 6, 2016
As discussed in #31333, it may be more appropriate to abort using
This only turns |
retep998
commented
Feb 6, 2016
The proper mechanism on Windows is actually |
retep998
commented
Feb 7, 2016
Alternatively we can call |
alexcrichton
commented
Feb 7, 2016
lgtm |
lambda
commented
Feb 7, 2016
@retep998 Yeah, looks like one of those might be appropriate. Since I don't have a Windows box and I don't know how the current mechanism or those behave on Windows, it's probably more appropriate for someone else who does have such access to send a patch for switching to those on Windows if appropriate. |
retep998
commented
Feb 8, 2016
Here is the Windows implementation. unsafe{asm!("int $$0x29"::"{ecx}"(7)::: volatile);// 7 is FAST_FAIL_FATAL_APP_EXIT
std::intrinsics::unreachable();} |
brson
commented
Feb 9, 2016
@bors r+ I'll submit a windows implementation in a bit. |
bors
commented
Feb 9, 2016
📌 Commit de250e5 has been approved by |
brson
commented
Feb 9, 2016
alexcrichton
commented
May 21, 2016
Hey @lambda, sorry for the delay! With the landing of #32900 we're already starting to diverge in terms of what abort means on each platform, so I think it's fine now to land this for the Unix side of things and then we can fixup the Windows side later. It seems like it's actually somewhat difficult to find a canonical way on Windows that indicates "the runtime wanted to abort" that works everywhere! Would you be ok updating this patch with two pieces?
|
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
de250e5 to
cfc3865Comparelambda
commented
May 23, 2016
@alexcrichton Should be done in my most recent push. I also rebased the Windows patch from @brson from #31519 in another branch in my repo, in case we want to also apply that. I could push that to this PR if we want to merge both at once, or I could push that separately after this lands. I'm reasonably convinced by the arguments from @retep998 that that's the right thing to do on Windows, but I'm not a Windows user and it's been a while since I've done serious Windows development, so I will defer to those with more experience on that platform. |
retep998
commented
May 23, 2016
I'm still of the opinion that |
alexcrichton
commented
May 23, 2016
bors
commented
May 23, 2016
Use libc::abort, not intrinsics::abort, in rtabort! 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 #31273 cc #31333
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 #31273
cc #31333