core::intrinsics::abort: Terminate with __fastfail on Windows

[Fulgen301]

Currently, std::process::abort aborts the process via __fastfail on Windows, which is the preferred way of terminating processes since Windows 8 as it doesn't go through exception handlers. However, core::intrinsics::abort does not, and terminates by executing an illegal instruction, which causes STATUS_ILLEGAL_INSTRUCTION to be thrown. This PR removes that discrepancy and makes core::intrinsics::abort use __fastfail on Windows.

Implementation-wise, this is currently done by having core::intrinsics::abort call a function in core::os if on Windows and on one of the supported architectures. Unfortunately, this means there is now a function that, depending on the target, isn't an intrinsic inside the intrinsics module, which isn't great. I've chosen to implement it like that anyway to gather feedback on better approaches - my initial idea was to lower it to a inline asm block within rustc_codegen_ssa, as none of the backends has an intrinsic for fastfail and we thus have to manually spell out the assembly anyway, but there didn't seem to be a good reason for manually generating an inline assembly block in MIR intrinsic handling as opposed to just writing it as an asm block in the first place, as has been done inside std.

[rustbot]

Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter
gets adapted for the changes, if necessary.

cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr

[rustbot]

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[RalfJung]

However, core::intrinsics::abort does not, and terminates by executing an illegal instruction,

This is what we do on all targets. It seems odd to do something special here for Windows only. If we want the intrinsic to do something OS-specific, we should make it so that all OSes benefit from that.

It is also rather unprecedented to do anything OS-specific in core. Being OS-agnostic is pretty much the definition of core.

Cc @rust-lang/libs

[ChrisDenton]

Well if you ignore core::ffi. And how intrinsics are implemented for that matter. E.g. is there a practical difference between library code and an intrinsic that causes the compiler to codegen the same thing?

[Fulgen301]

This is what we do on all targets. It seems odd to do something special here for Windows only.

__fastfail is the intrinsic for process abortion on Windows, it being implemented in core as asm! blocks as opposed to backend inline asm or LLVM intrinsic calls is a matter of convenience, cleanliness and backends not having a fastfail intrinsic - we can generate the same asm blocks inside cg_ssa, at the expense of code readability.

If we want the intrinsic to do something OS-specific, we should make it so that all OSes benefit from that.

We have an intrinsic that declares it does something OS-specific by being described as "Aborts the execution of the process.", which is an inherently OS-specific concept and not something the compiler has to or should implement. Here I'd argue that core::intrinsic::abort itself should not exist, and should be split between an abort method in core that selects the best implementation for the current target, and a new core::intrinsic::trap intrinsic that lowers to llvm.trap or the equivalents for other codegen backends, which is the current behavior of abort. That'd need a RFC however.

It is also rather unprecedented to do anything OS-specific in core.

core::os was initially added by the darwin_objc feature, and there is no std dependency on __fastfail since it is an intrinsic that invokes a kernel interrupt.

[RalfJung]

For better or worse, aborting execution is an OS-independent capability in MIR. The intrinsic is just one of the ways it gets invoked; there is also am Abort terminator, and there might be other places in codegen that invoke the same operation. If LLVM cannot generate the right instructions for us, then at the very least we should do something consistent on our side. Is this worth bringing up with LLVM, to fix whatever intrinsic we are using for abort() so that it behaves as intended on Windows?

[ChrisDenton]

The intrinsic is currently implemented in terms of llvm's trap which simply maps to an illegal instruction (ud2 on x86).

I don't think llvm has an abort. At least I couldn't find one.

[RalfJung]

So you would argue that what LLVM does on Windows is a reasonable implementation for "trap", it's just not what we want for "abort"?

[Fulgen301]

Indeed.

[ChrisDenton]

Yes. I would expect an abort to immediately abort the process rather than raising an exception (unless there's no reasonable alternative).

On the other hand raising an exception is a reasonable implementation of trap.

Somewhat related is #149708 but there the exception can only be caught outside of the rust program.

[RalfJung]

Okay.

But then we should use that consistently for all implementations of "abort", not just the intrinsic. We should probably rename the abort function on codegen backends to trap to make it more clear that this is different. And to ensure that the Abort MIR terminator is consistent with intrinsics::abort, we need to do one of

• add a lang item so the compiler can invoke the __fastfail call that is in the library
• generate that call in the codegen backend

Then I would suggest you entirely undo the changes in core/src/intrinsics from this PR. Instead we have the codegen backends do the following to implement the abort intrinsic:

• check if the abort lang item is present; if so, invoke that
• otherwise invoke the trap helper of the codegen backend

[nikic]

I think there may be some unfortunate choices of terminology here, but it's not super obvious to me that this is really a desirable/necessary change in terms of how the abort intrinsic is used. If we take -C panic=immediate-abort as a primary use case, trapping seems like a pretty reasonable thing to do in that context?

Also worth noting that this also sends a SIGILL signal on other OSes, so I'm not sure why Windows specifically needs divergent behavior.

[ChrisDenton]

core::intrinsic::abort is used throughout core the same way std::panic::abort is used in std. I don't think there's any real justification for them to be different?

The panic_abort crate already uses std::process::abort https://github.com/search?q=repo%3Arust-lang%2Frust+__rust_abort+path%3Alibrary%2F&type=code.

And personally I would expect, from a libs pov, for abort to make a best effort to abort the process immediately. The extent to which this is possible of course varies by platform.

[bjorn3]

This entire doc comment only applies to the Windows version of this function. So doc.rust-lang.org would no longer show it.

[bjorn3]

Also worth noting that this also sends a SIGILL signal on other OSes, so I'm not sure why Windows specifically needs divergent behavior.

SIGILL doesn't unwind the stack, while Windows exceptions do unwind the stack through SEH. As such SIGILL only runs an explicitly registered signal handler, while Windows exceptions as I understand it run all __finally and __catch SEH blocks, even when they don't explicitly intent to run on illegal instructions. And SIGILL is safe in the face of a corrupted stack when using a separate stack for the signal handler, while SEH is not safe in that case.

[nikic]

Also worth noting that this also sends a SIGILL signal on other OSes, so I'm not sure why Windows specifically needs divergent behavior.

SIGILL doesn't unwind the stack, while Windows exceptions do unwind the stack through SEH. As such SIGILL only runs an explicitly registered signal handler, while Windows exceptions as I understand it run all __finally and __catch SEH blocks, even when they don't explicitly intent to run on illegal instructions. And SIGILL is safe in the face of a corrupted stack when using a separate stack for the signal handler, while SEH is not safe in that case.

I guess in that case I'd wonder whether this difference in behavior would be a compelling reason to change what llvm.trap does on Windows?

[Fulgen301]

llvm.trap is also used for e.g. ensuring that hitting unreachable code aborts, and it's quite small (on x86, it's a one byte ud2). Unconditionally replacing that with __fastfail, even if desired, would increase that size by at least five times (movzx ecx, 7; int 0x29), and would cause observable behavior differences for all languages using LLVM, including clang's __builtin_trap.

Trapping is much more generic in usage than specifically aborting the process as fast as possible, which is what we want and already do in std::process::abort.

[RalfJung]

Trapping is much more generic in usage than specifically aborting the process as fast as possible, which is what we want and already do in std::process::abort.

That's not the only thing abort usually does though -- if a debugger is attached, it also usually halts execution so one can inspect the program state and backtrace at this point.

Does __fastfail do that?

[bjorn3]

Trapping is much more generic in usage than specifically aborting the process as fast as possible, which is what we want and already do in std::process::abort.

Most direct uses of core::intrinsics::abort() I have seen are as no_std replacement for std::process::abort() in a panic handler.

[ChrisDenton]

That's not the only thing abort usually does though -- if a debugger is attached, it also usually halts execution so one can inspect the program state and backtrace at this point.

Does __fastfail do that?

Yes, see the documentation for __fastfail

If a debugger is present, it's given an opportunity to examine the state of the program before termination.

[RalfJung]

That seems to make it a reasonable call for LLVM to use it as an implementation of trap. Arguably, a trap that unwinds and executes "finish" blocks is... odd.

[cuviper]

What target do the Windows kernel folks use? I'm guessing some external target spec, but if that still sets cfg(windows), are they going to be bothered by __fastfail assembly in core?

I also feel pretty strongly that this should be an implementation detail of the intrinsic (not lib cfg), whether or not that propagates all the way to changing llvm.trap, but I don't think everyone shared that opinion in the libs meeting just now.

[Fulgen301]

They use the MSVC targets.

are they going to be bothered by __fastfail assembly in core?

I at least can't think of a reason, as __fastfail is explicitely supported in kernel mode:

Kernel-mode fast fail requests are implemented by using a dedicated bugcheck code, KERNEL_SECURITY_CHECK_FAILURE (0x139). In both cases, no exception handlers are invoked because the program is expected to be in a corrupted state. If a debugger is present, it's given an opportunity to examine the state of the program before termination.

[ChrisDenton]

One thing libs asked about is how libs-api feels more generally about the core/std split and whether they would consider this to be breaking it. So I'll nominate it for that reason.

[Amanieu]

We discussed this in the @rust-lang/libs-api meeting. We're happy to add this for Windows since both user mode and kernel mode have special handling for this specific instruction sequence which triggers an exception.

We're however not yet committing to a more general policy which would allow things like the Linux version of abort to use syscalls to trigger a SIGABRT instead of SIGILL.

[scottmcm]

The intrinsic is currently implemented in terms of llvm's trap which simply maps to an illegal instruction (ud2 on x86).

What's the binary size consequence of using __fastfail? Most of the things I've seen using intrinsics::abort are doing it in places that intentionally aren't using the more-graceful panic_nounwind.

If it's just 2 instructions instead of one (no call, no linking implications, etc) then sure, let's do this. If it's more, then maybe it should be part of panic_nounwind instead of abort.

[scottmcm]

It's not obvious that this is the right way to change the intrinsic behaviour.

For example, if the goal is to change what abort does, should https://doc.rust-lang.org/nightly/nightly-rustc/rustc_codegen_llvm/builder/struct.GenericBuilder.html#method.abort be the thing that changes instead so that any uses of abort in cg_ssa also do a __fastfail?

[RalfJung]

Yeah I already mentioned that above -- this needs to become a lang item so the compiler can invoke it to implement the Abort MIR terminator and other "native" aborts.

[ChrisDenton]

I think that's categorically different. The compiler inserts a single byte trap in some places where a code path is unreachable. That seems a bit unnecessary but fine as a debugging aid. It's only one byte.

core::intrinsic::abort is different. It's used in core where we would use std::panic::abort in std, except that obviously that's unavailable.

[RalfJung]

I think it would be very strange if some aborts behave different from others. I am not talking about unreachable code. "Native" aborts occur, for instance, when code unwinds out of a drop, or out of an extern "C".

[ChrisDenton]

We already have different aborts with the core and std difference I mentioned. Should the compiler insert calls to abort() on unix platforms?

I do think the compiler misnames what should be called trap.

[RalfJung]

🤷 I don't think it is much complexity, and I don't think it is unnecessary.

[Fulgen301]

For example, if the goal is to change what abort does, should https://doc.rust-lang.org/nightly/nightly-rustc/rustc_codegen_llvm/builder/struct.GenericBuilder.html#method.abort be the thing that changes instead so that any uses of abort in cg_ssa also do a __fastfail?

Doing that would replace every single ud2 instruction of two bytes with a seven byte instruction sequence, and I doubt LLVM would be allowed / smart enough to optimize out the lang item call like it does with unneeded llvm.trap instructions.

[RalfJung]

The truly unreachable ones anyway shouldn't trap, they should tell LLVM that this is unreachable... (#59793)

[Fulgen301]

True, but increasing the size now for until that six-year-old issue is resolved does not sound like a good tradeoff.

[scottmcm]

core::intrinsic::abort is different. It's used in core where we would use std::panic::abort in std, except that obviously that's unavailable.

Can you elaborate on how the library considers them different between abort and panic_nounwind, which is available in core?

[ChrisDenton]

It is just two instructions (no call, no linking implications, etc). It just calls an interrupt handler

mov ecx, FAST_FAIL_FATAL_APP_EXIT
int 0x29

[ChrisDenton]

Suggested change

	core::arch::asm!("int $$0x29", in("ecx") FAST_FAIL_FATAL_APP_EXIT, options(noreturn, nostack));
	core::arch::asm!("int 0x29", in("ecx") FAST_FAIL_FATAL_APP_EXIT, options(noreturn, nostack));

This has been bugging me for years but not enough to PR it. The $$ is entirely redundant (left over from AT&T syntax, I assume).

[bjorn3]

Also mind removing the workaround at

rust/compiler/rustc_codegen_cranelift/src/inline_asm.rs

Lines 47 to 53 in f794a08

	// Used by panic_abort on Windows, but uses a syntax which only happens to work with
	// asm!() by accident and breaks with the GNU assembler as well as global_asm!() for
	// the LLVM backend.
	if template.len() == 1 && template[0] == InlineAsmTemplatePiece::String("int $$0x29".into()) {
	fx.bcx.ins().trap(TrapCode::user(2).unwrap());
	return;
	}

if you apply the above suggestion?