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

Expose `core::intrinsics::unreachable` in stable wrapper #1385

Closed
bluss opened this Issue Nov 28, 2015 · 18 comments

Comments

Projects
None yet
9 participants
@bluss
Copy link

bluss commented Nov 28, 2015

Best alternative is crate unreachable today, but it may not be able to guarantee exactly the same semantics.

Important for: low level code, performance.

@ticki

This comment has been minimized.

Copy link
Contributor

ticki commented Nov 28, 2015

Note that it cannot be safe, since the compiler cannot be sure at compile time that it's actually unreachable. But, it's really needed for the sake of performance, since it would allow certain optimizations due to LLVM being sure that it's unwinding safe (which is not the case with unreachable!). So 👍 from here.

@DemiMarie

This comment has been minimized.

Copy link

DemiMarie commented Dec 17, 2015

What about inserting a call to intrinsics::breakpoint instead? That raises a SIGTRAP (useful when debugging), corresponds to 1 machine instruction, and never unwinds. It is also safe.

@bluss

This comment has been minimized.

Copy link
Author

bluss commented Dec 17, 2015

Sounds like it doesn't have the same use case, doesn't help for optimization.

@ticki

This comment has been minimized.

Copy link
Contributor

ticki commented Dec 17, 2015

@bluss It does not. It's essentially just a less fancy version of unreachable!().

@glaebhoerl

This comment has been minimized.

Copy link
Contributor

glaebhoerl commented Dec 17, 2015

If we do expose this we should probably give it a name, such as assume_unreachable, that hints at the difference.

@arielb1

This comment has been minimized.

Copy link
Contributor

arielb1 commented Dec 21, 2015

@bluss

Matching on an empty enum is the official way to generate an unreachable.

@ticki

This comment has been minimized.

Copy link
Contributor

ticki commented Dec 21, 2015

@arielb1 No, it's not. It doesn't help doing any of the optimizations, that core::intrinsics::unreachable do allow.

@bluss

This comment has been minimized.

Copy link
Author

bluss commented Dec 21, 2015

@ticki It does have the same effect on optimizations (experience from my projects), see the crate unreachable linked in the issue. It does have the same effect right now.

@arielb1 It would be super cool if you could review the current stable rust impl of unreachable in said crate: https://github.com/reem/rust-unreachable/blob/master/src/lib.rs#L20-L25

My intention is really to put this on safe ground however we do it. Codegen regressions happen. Isn't it much cleaner to just expose the unreachable intrinsic? Rust is low level oriented and has no reason to keep the cool tools locked away. unreachable is another of those intrinsics that does not burden a Rust implementation to support, it can be a noop (but ideally not).

@arielb1

This comment has been minimized.

Copy link
Contributor

arielb1 commented Dec 21, 2015

@bluss

The implementation is fine, although it puts a bit too much stress on the inliner. A simple match unsafe { *(1 as *const Void) } {} is guaranteed to reduce to basically an unreachable instruction.

@bluss

This comment has been minimized.

Copy link
Author

bluss commented Dec 21, 2015

Cool! @reem see above, we can improve this!

bluss added a commit to bluss/odds that referenced this issue Jan 13, 2016

Implement a simpler version of `unreachable` locally
Use aatch's advice on just using a pointer cast to create a Void pointer
that we can match on.

See issue rust-lang/rfcs#1385

Drop dependency on unreachable and void crates.
@ubsan

This comment has been minimized.

Copy link
Contributor

ubsan commented Feb 5, 2016

@arielb1 no it's not. It's just guaranteed undefined behavior. Why would we want to encourage a hack, versus just having an intrinsic for it?

@arielb1

This comment has been minimized.

Copy link
Contributor

arielb1 commented Feb 6, 2016

I don't see it as much of a hack. From an operational perspective, all forms of UB are the same. From a performance standpoint, we probably want to document things like that anyway.

@ubsan

This comment has been minimized.

Copy link
Contributor

ubsan commented Feb 6, 2016

@arielb1 I mean yes, it's true, all forms of UB are "the same". However, the current solution is far less obvious. unreachable() makes it really, really obvious that you're never gonna reach that code; match *(0x1 as *const Void) { }: I have absolutely no idea what this means, coming from anybody who hasn't been programming Rust for a long time.

edited to not deref a null pointer.

@bluss

This comment has been minimized.

Copy link
Author

bluss commented Feb 6, 2016

dereferencing 0x1 or 0x0 ought to have possibly different implications here, just a sidenote.

@ubsan

This comment has been minimized.

Copy link
Contributor

ubsan commented Feb 6, 2016

@bluss it's the same as *(0x0 as *const ()), which is totally okay. There's nothing special about the null pointer, we just use it as a marker for references and such (afaiui).

@main--

This comment has been minimized.

Copy link

main-- commented Feb 7, 2016

There's nothing special about the null pointer

@ubsan Note that at least in C/C++ (and Rust probably inherits this via LLVM) the null pointer is very much special in that dereferencing it is UB by definition. Example

@ubsan

This comment has been minimized.

Copy link
Contributor

ubsan commented Feb 7, 2016

@main-- ah, I was mistaken. I was going off of something like http://en.chys.info/2010/04/null-can-be-a-valid-address/

@nrc nrc added the T-libs label Aug 19, 2016

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Oct 7, 2018

@Centril Centril closed this Oct 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.