Stabilize `intrinsics::abort` in `core`

[japaric]

I would like to see intrinsics::abort stabilized in some form as it's useful in some cases to have
panicking behavior (and alloc_error) = abort (minimizes code size and the abort exception / signal
can be handled elsewhere or in a upper abstraction layer).

As intrinsics::abort lowers to a trap instruction on most architectures (*) I propose we add a
safe wrapper around it in core::arch::$ARCH for the architectures where it does.

(*) On most but not on all. For example, On MSP430 it lowers to a call to the abort function
which results in a linker error unless that symbol is defined somewhere in the dependency graph.

(Related: There's std::process::abort which claims to be a safe wrapper around intrinsics::abort
but if you at its implementation it is, in fact, not just a wrapper around intrinsics::abort but
something completely different.)

@rust-lang/libs what would be needed to move this forward? An full RFC, or just a PR + FCP?

cc @Amanieu @glandium

[steveklabnik]

I've also found it strange that this doesn't exist in core; big 👍 from me.

[glandium]

I'm all for it, but I'm wary about core::arch. AIUI, there is, purposefully, no platform-specific code in libcore at the moment. A specific module for that seems like opening the door to more.

[japaric]

@glandium core::arch already exists on stable. There are architecture specific APIs in it for SIMD instructions.

[Amanieu]

I'm not a big fan of the way intrinsics::abort is used at the moment: it just crashes the program with an obscure error message (Illegal instruction). IMO the proper way to handle this is to use something like #[abort_implementation] to allow customizing the behavior of aborts per platform.

Then again, this is a lot of work for something that is very rarely used (compared to normal panics). It might not be worth the trouble.

[alexcrichton]

Sorry for the delay in getting to this @japaric, but FWIW I'd personally be ok with a PR to add this to an unstable (eventually to-be-stable) location in libstd and then later have a stabilization PR. This doesn't seem like it's RFC-grade to me

[japaric]

@Amanieu this proposal is about stabilizing a way to generate a trap / abort instruction. Your comment seems to be about a user customizable process::abort that's available in core.

@alexcrichton sounds good! The main question here is where in core to put this. IMO, it makes sense to put this in coresimd / core::arch::$ARCH and name it trap, instead of abort, as this is a stable wrapper for the llvm.trap intrinsic that only produces a trap instruction for some architectures (e.g. it doesn't do that for MSP430). Could I get yours and @gnzlbg's opinions on that location?

[gnzlbg]

@japaric I'd put it in core::intrinsics first since the llvm intrinsic it forwards to is "portable", as in, it can be called on all architectures, although it might not do what you expect on all of them. We can just point people here to the llvm docs.

I propose we add a safe wrapper around it in core::arch::$ARCH for the architectures where it does.

So suppose we would put it in core::intrinsics and stabilize it. Would we still need to add those safe wrappers to core::arch or could one be built in a stable third-party crate ? Even if it can be built in a third party crate, it might still make sense to expose a safe wrapper to it somewhere in core, but we don't have to solve that problem right now, and maybe with some experience about how to write that in a crate and use it, we can put a better solution in core later.

[japaric]

I'd put it in core::intrinsics first since the llvm intrinsic it forwards to is "portable"

It's already in intrinsics: core::intrinsics::abort but there's no precedent for stabilizing any API in there ... Hold on, the core::intrinsics module is unstable but e.g. core::intrinsics::transmute and a few other intrinsics are stable -- that's weird: core::intrinsics::transmute(..) is stable but the more idiomatic use core::intrinsics; intrinsics::transmute(..) requires a feature gate.

Yeah, I'm not sure stuff in core::intrinsics is supposed to stabilized. core::intrinsics::transmute may only be stable as a byproduct of stabilizing it as a re-export in core::mem.

I propose we add a safe wrapper around it

This was a typo, i think. I probably meant a stable wrapper around the llvm.trap intrinsic. Whether it's safe or not is not too important (though I don't see a reason why abort should be unsafe); the important part is that it becomes a stable API.

So suppose we would put it in core::intrinsics and stabilize it.

If that's OK then, yeah, we don't need anything else in core::arch, but like I said above I don't think we are supposed to tell people to use core::intrinsics on stable. My proposal is assuming that core::intrinsics is not the right place to stabilize this API in.

[japaric]

And as a data point: for these architectures llvm.trap lowers to an
instruction: ARM{,BE}, AArch64, i686, mips{,64,64el}, powerpc{,64,64le},
nvptx64, sparc64, x86_64 and wasm32.

For MSP430 and RISCV32 llvm.trap lowers to a call to the abort function,
which is not defined in core or compiler_builtins so it can cause a linker
error.

AFAIK, there's no precedent for a stable API in core that can produce linker
errors when compiled for some architectures. This is way I'd prefer not to make
a stable llvm.trap available to all targets.

[gnzlbg]

Do clang and GCC export these somehow? I've used a __builtin_trap intrinsic in C and C++, but in some platforms that actually lowered to 1 / 0...

We could re-export it in core::arch for all architectures in which it does something meaningful too. But @alexcrichton maybe the libs team should discuss what's the plan for core::intrinsics, whether its for unstable stuff only, or whether parts of it could be stabilized. If not, where could we put this?

cc @eddyb @sunfishcode does Cretonne have a trap intrinsic? If so, does it do the same thing as llvm.trap? Also, where would you put this?

[SimonSapin]

Libs team hat on: I’d rather not stabilize the intrinsics module. I view it as an implementation detail that (ideally) could be removed entirely.

In the past, when stabilizing an intrinsic, we’ve added a re-export or wrapper in another (stable) module.

That core::intrinsics::transmute(..) works on stable is a bug: rust-lang/rust#15702. We clarified in #2405 that the stability promise does not extend to features that are accidentally available on the stable channel.

[nagisa]

For MSP430 and RISCV32 llvm.trap lowers to a call to the abort function, which is not defined in core or compiler_builtins so it can cause a linker error.

First, lowering into abort() function is just a "default" that generic lowering code in LLVM performs for targets that do not implement lowering of their own. At least RISCV has a dedicated trap instruction in its ISA, so that’s just a shortcoming of the backend that can readily be fixed.

Additionally, there is a function attribute called trap-func-name that when specified will cause all @llvm.trap()s in that function to lower into that specific function call instead, making it possible for us to easily make an override for targets that for some reason do not implement lowering themselves.

Libs team hat on: I’d rather not stabilize the intrinsics module. I view it as an implementation detail that (ideally) could be removed entirely.

Think of intrinsics of a name that could potentially be given up for functions like this (and implementation details becoming private/moving elsewhere). For unreachable specifically, we ended up deciding to create a std::hint module, despite my foresight that it will want to live alongside abort/trap. Since trap does not fit into hint, we can put this function somewhere else, sure. Except there are few or no places where it would fit in thematically, so we might end up creating yet another module that will likely contain just this function.

[japaric]

First, lowering into abort() function is just a "default" that generic lowering code in LLVM performs for targets that do not implement lowering of their own. (..)

Are you suggesting that we "polyfill" llvm.trap in rustc for targets that don't have a dedicated trap instruction? What should such function do when targeting MSP430? It seems to (but I'm no expert) that 16-bit and 8-bit architecture use all their opcode space so there's no undefined opcode that we could use here.

Also, with this approach we would have to take care not only of MSP430 but also of every new architecture that we add in the future and that has this problem (possibly, AVR).

[alexcrichton]

I don't personally have a lot of thoughts about where this should go or what to name it, but I agree with @SimonSapin that we should avoid the intrinsics module