Made `std::hint::unreachable_unchecked` a const fn. #70324

q2p · 2020-03-23T16:23:44Z

rust-highfive · 2020-03-23T16:23:47Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @oli-obk (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

jonas-schievink · 2020-03-23T16:43:10Z

You probably also want to add a test that executes the intrinsic to initialize a const item

Centril · 2020-03-23T17:01:27Z

Including a test case where we trigger UB would also be good I think.

cc @rust-lang/wg-const-eval

tesuji · 2020-03-23T17:16:18Z

What's the use case for constifying this function?
Does never type signal the compiler early and making constifying needlessly?

RalfJung · 2020-03-23T17:17:19Z

Including a test case where we trigger UB would also be good I think.

Yeah, and for that to work you'll need to actually implement the intrinsic as raising an unreachable error. Intrinsic implementations live in this file.

Turns out we forgot about that in Miri. ;)

q2p · 2020-03-24T09:47:01Z

Including a test case where we trigger UB would also be good I think.

I don't know how to write a test for an undefined behavior. Should function be called when initialising a const value, or at runtime? How should assertion or stderr output look like?

Yeah, and for that to work you'll need to actually implement the intrinsic as raising an unreachable error.

Sorry, but I don't know how to do that.

RalfJung · 2020-03-24T09:47:37Z

src/test/ui/consts/const_unsafe_unreachable.rs

+const unsafe fn f_std_intrinsics_unreachable(x: u8) -> u8 {
+    match x {
+        17 => 22,
+        _ => std::intrinsics::unreachable(),


std::hint::unreachable_unchecked is a thing wrapper around the intrinsic, I do not see the point of also testing the intrinsic directly? I don't think we usually do that.

The test @Centril and me asked for was a case where unreachable_unchecked is used incorrectly, where UB is triggered by actually reaching that function. Like, f_std_hint_unreachable(23).

RalfJung · 2020-03-24T09:49:02Z

Sorry, but I don't know how to do that.

You need to add a new arm in this match on sym::unreachable that then does throw_ub!(Unreachable).

q2p · 2020-03-24T14:19:51Z

I have added an arm to the match inside of librustc_mir/interpret/intrinsics.rs as RalfJung asked.

...
sym::unreachable_code => throw_ub!(Unreachable),
...

But I have doubts that it is a right way, because everything else in the file calls throw_ub_format! macro and not throw_ub!.

Also this change, didn't have any effect on output when running the UB test.

When I tried to implement a test where function calls unreachable_unchecked(), it prints out:

...
error: any use of this value will cause an error
  --> /media/p2a/rust/src/libcore/hint.rs:52:5
   |
LL |     intrinsics::unreachable()
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^
   |     |
   |     "calling intrinsic `unreachable`" needs an rfc before being allowed inside constants
   |     inside call to `std::hint::unreachable_unchecked` at /media/p2a/rust/src/test/ui/consts/const_unsafe_unreachable_ub.rs:8:14
...

Which references libcore/hint.rs:52:5 and says that "calling intrinsic unreachable" needs an rfc.
So I'm not sure if this is a desired error message or how to normalize this stderr.

Clearly I can't make this PR, because I have no idea what kind of checks / code should be implemented for intrinsics::unreachable() to have a meaningful error message on incorrect use, or how to prevent the incorrect use.

And I don't want to endlessly keep bothering people with questions, so I'm gonna close this PR.

rust-highfive · 2020-03-24T14:20:57Z

Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2020-03-24T14:20:22.5746953Z ========================== Starting Command Output ===========================
2020-03-24T14:20:22.5752655Z [command]/bin/bash --noprofile --norc /home/vsts/work/_temp/816a5193-ecf1-4dcc-ba10-09e4a9aeda1f.sh
2020-03-24T14:20:22.5753173Z 
2020-03-24T14:20:22.5758066Z ##[section]Finishing: Disable git automatic line ending conversion
2020-03-24T14:20:22.5780154Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/70324/merge to s
2020-03-24T14:20:22.5784007Z Task         : Get sources
2020-03-24T14:20:22.5784333Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-03-24T14:20:22.5784667Z Version      : 1.0.0
2020-03-24T14:20:22.5784886Z Author       : Microsoft
---
2020-03-24T14:20:23.7317355Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-03-24T14:20:23.7329132Z ##[command]git config gc.auto 0
2020-03-24T14:20:23.7372322Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-03-24T14:20:23.7377921Z ##[command]git config --get-all http.proxy
2020-03-24T14:20:23.7389360Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/70324/merge:refs/remotes/pull/70324/merge
2020-03-24T14:20:24.7170978Z fatal: couldn't find remote ref refs/pull/70324/merge
2020-03-24T14:20:24.7868585Z ##[warning]Git fetch failed with exit code 128, back off 3.531 seconds before retry.
2020-03-24T14:20:27.5344676Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/70324/merge:refs/remotes/pull/70324/merge
2020-03-24T14:20:28.1671503Z fatal: couldn't find remote ref refs/pull/70324/merge
2020-03-24T14:20:28.2365632Z ##[warning]Git fetch failed with exit code 128, back off 6.155 seconds before retry.
2020-03-24T14:20:34.3290906Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/70324/merge:refs/remotes/pull/70324/merge
2020-03-24T14:20:34.9298282Z fatal: couldn't find remote ref refs/pull/70324/merge
2020-03-24T14:20:34.9768859Z ##[error]Git fetch failed with exit code: 128
2020-03-24T14:20:34.9783606Z ##[section]Finishing: Checkout rust-lang/rust@refs/pull/70324/merge to s
2020-03-24T14:20:35.0216542Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/70324/merge to s
2020-03-24T14:20:35.0222384Z Task         : Get sources
2020-03-24T14:20:35.0222717Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-03-24T14:20:35.0223051Z Version      : 1.0.0
2020-03-24T14:20:35.0223270Z Author       : Microsoft
2020-03-24T14:20:35.0223270Z Author       : Microsoft
2020-03-24T14:20:35.0223624Z Help         : [More Information](https://go.microsoft.com/fwlink/?LinkId=798199)
2020-03-24T14:20:35.0224022Z ==============================================================================
2020-03-24T14:20:35.3690945Z ##[section]Finishing: Checkout rust-lang/rust@refs/pull/70324/merge to s
2020-03-24T14:20:35.3802772Z Cleaning up task key
2020-03-24T14:20:35.3805159Z Start cleaning up orphan processes.
2020-03-24T14:20:35.3985764Z ##[section]Finishing: Finalize Job
2020-03-24T14:20:35.4031405Z ##[section]Finishing: Linux x86_64-gnu-tools

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

RalfJung · 2020-03-24T15:11:58Z

But you're almost there. ;)

But I have doubts that it is a right way, because everything else in the file calls throw_ub_format! macro and not throw_ub!.

Yeah that's okay. We have specific error constants for common errors, everything else uses the _format variant. Since we have a variant for "entered unreachable code", that is okay to use.

But your mistake is that you used unreachable_code, when the intrinsic is called unreachable. That also explains why you are getting the wrong error message.

If sym::unreachable does not exist, you have to add it here:

rust/src/librustc_span/symbol.rs

Line 783 in 1add455

unreachable_code,

Make unreachable_unchecked a const fn

fb6a7e9

rust-highfive assigned oli-obk Mar 23, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 23, 2020

RalfJung mentioned this pull request Mar 23, 2020

Implement "unreachable" intrinsic rust-lang/miri#1254

Closed

Added std::intrinsics::unreachable() test.

b2d1c5a

RalfJung reviewed Mar 24, 2020

View reviewed changes

q2p added 2 commits March 24, 2020 21:11

Added unreachable_code branch to the intrinsics match

196a481

Added a test for unreachable_unchecked that causes UB.

73a9540

q2p closed this Mar 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Made `std::hint::unreachable_unchecked` a const fn. #70324

Made `std::hint::unreachable_unchecked` a const fn. #70324

q2p commented Mar 23, 2020

rust-highfive commented Mar 23, 2020

jonas-schievink commented Mar 23, 2020

Centril commented Mar 23, 2020

tesuji commented Mar 23, 2020

RalfJung commented Mar 23, 2020 •

edited

Loading

q2p commented Mar 24, 2020

RalfJung Mar 24, 2020 •

edited

Loading

RalfJung commented Mar 24, 2020

q2p commented Mar 24, 2020

rust-highfive commented Mar 24, 2020

RalfJung commented Mar 24, 2020

Made std::hint::unreachable_unchecked a const fn. #70324

Made std::hint::unreachable_unchecked a const fn. #70324

Conversation

q2p commented Mar 23, 2020

rust-highfive commented Mar 23, 2020

jonas-schievink commented Mar 23, 2020

Centril commented Mar 23, 2020

tesuji commented Mar 23, 2020

RalfJung commented Mar 23, 2020 • edited Loading

q2p commented Mar 24, 2020

RalfJung Mar 24, 2020 • edited Loading

Choose a reason for hiding this comment

RalfJung commented Mar 24, 2020

q2p commented Mar 24, 2020

rust-highfive commented Mar 24, 2020

RalfJung commented Mar 24, 2020

Made `std::hint::unreachable_unchecked` a const fn. #70324

Made `std::hint::unreachable_unchecked` a const fn. #70324

RalfJung commented Mar 23, 2020 •

edited

Loading

RalfJung Mar 24, 2020 •

edited

Loading