Use libc::abort, not intrinsics::abort, in rtabort! #31457
Conversation
|
r? @brson (rust_highfive has picked a reviewer for you, use r? to override) |
|
As discussed in #31333, it may be more appropriate to abort using
This only turns |
|
The proper mechanism on Windows is actually |
|
Alternatively we can call |
|
lgtm |
|
@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. |
|
Here is the Windows implementation. unsafe {
asm!("int $$0x29" :: "{ecx}"(7) ::: volatile); // 7 is FAST_FAIL_FATAL_APP_EXIT
std::intrinsics::unreachable();
} |
|
@bors r+ I'll submit a windows implementation in a bit. |
|
📌 Commit de250e5 has been approved by |
brson
added
the
relnotes
|
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
lambda
force-pushed
the
rtabort-use-libc-abort
branch
from
de250e5
to
cfc3865
Compare
|
@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. |
|
I'm still of the opinion that |
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