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

All intrinsics are callable in const fn #61495

Closed
RalfJung opened this issue Jun 3, 2019 · 15 comments · Fixed by #67466
Closed

All intrinsics are callable in const fn #61495

RalfJung opened this issue Jun 3, 2019 · 15 comments · Fixed by #67466
Labels
A-const-eval Area: constant evaluation (mir interpretation) A-intrinsics Area: intrinsics C-bug Category: This is a bug.

Comments

@RalfJung
Copy link
Member

RalfJung commented Jun 3, 2019

At some point we lost the check making sure that in const context (const/static items, const fn), you can only call a select few whitelisted intrinsics. Instead, you can now call all of them.

The issue is that the check happening here does not complain when there is an intrinsic in const context (unlike the checks here that complain about other kinds of function calls in const context).

Cc @oli-obk @eddyb

@RalfJung
Copy link
Member Author

RalfJung commented Jun 3, 2019

Note that this is not the same as the problem fixed by #61493: that was about promotion (in a runtime function), the issue here is about calls happening in const context.

@RalfJung
Copy link
Member Author

RalfJung commented Jun 3, 2019

I believe that as far as the "core" intrinsics go, there actually is no issue on stable because the only intrinsic you can directly call on stable is transmute, and that one is specifically feature-gated.

But SIMD intrinsics might be a different story, it might be that those are publicly reexported. Cc @gnzlbg

@gnzlbg
Copy link
Contributor

gnzlbg commented Jun 3, 2019

I think this is only a problem on nightly, MWE (playground):

#![feature(const_fn, core_intrinsics)]
pub const fn bar(a: i32, b: i32) -> (i32, bool) {
    unsafe { core::intrinsics::add_with_overflow(a, b) }
}

Note that, right now, the type of add_with_overflow is:

pub unsafe extern "rust-intrinsic" fn add_with_overflow<T>(
    x: T, 
    y: T
) -> (T, bool)

which is not a const fn.

Being able to call a non-const fn from a const fn feels like a soundness bug on nightly Rust. EDIT: but for "rust-intrinsic" this is debatable. These are always available, the declarations in core::intrinsics aren't really declarations, they aren't #[lang_item]s either, etc. Basically any nightly Rust code that enables the appropriate features can write:

mod foo {
pub unsafe extern "rust-intrinsic" fn add_with_overflow<T>(
    x: T, 
    y: T
) -> (T, bool)
}

and just use foo::add_with_overflow.


But SIMD intrinsics might be a different story, it might be that those are publicly reexported.

I don't think miri currently support evaluating any of the stable SIMD intrinsics, so this bug should not impact stable Rust users. If miri gains support for evaluating some of the stable SIMD intrinsics, it would be bad if that makes them "automatically const fn". Some of the intrinsics have side-effects, and I can imagine wanting to support cargo miri for Rust programs that use them (e.g. by mapping their side-effects to something meaningful in the miri VM), but that does not mean that we want to allow them within const fn. That's something that has to be decided on a 1:1 basis, and the appropriate tool for that would be to actually make their type const fn.

@gnzlbg
Copy link
Contributor

gnzlbg commented Jun 3, 2019

Maybe a temporary way to fix, at least for the ones exposed in core::intrinsics, would be to not expose rust-intrinsics directly , but to expose, e.g.,

mod intrinsics {
  #[inline(always)]
  pub unsafe fn add_with_overflow<T>(x: T, y: T) -> (T, bool) {
    extern "rust-intrinsic" { fn add_with_overflow<T>(x: T, y: T) -> (T, bool); }
    add_with_overflow(x, y)
  } 
}

that would make the MWE above fail to compile because the functions in core::intrinsics aren't really intrinsics, but normal Rust functions, and the const fn check works for those (playground).

@RalfJung
Copy link
Member Author

RalfJung commented Jun 3, 2019

I don't think miri currently support evaluating any of the stable SIMD intrinsics, so this bug should not impact stable Rust users.

The bug is not about accidentally executing intrinsics during CTFE. That's impossible, those intrinsics are not even implemented in rustc. (Miri's implementations live out-of-tree.)

The bug is about accepting code that calls (stable) (SIMD) intrinsics in a const fn.

@gnzlbg
Copy link
Contributor

gnzlbg commented Jun 3, 2019

The bug is about accepting code that calls (stable) (SIMD) intrinsics in a const fn.

So this cannot happen, because we use the pattern I showed here (#61495 (comment)) to implement all stable SIMD intrinsics. That is, all stable SIMD "intrinsics" are not intrinsics, but Rust functions that call intrinsics. The intrinsics themselves are not stable, and the intent is to never stabilize them.

@varkor varkor added the A-intrinsics Area: intrinsics label Jun 3, 2019
@jonas-schievink jonas-schievink added A-const-eval Area: constant evaluation (mir interpretation) C-bug Category: This is a bug. labels Jun 3, 2019
@RalfJung
Copy link
Member Author

RalfJung commented Jun 3, 2019

So this cannot happen, because we use the pattern I showed here (#61495 (comment)) to implement all stable SIMD intrinsics. That is, all stable SIMD "intrinsics" are not intrinsics, but Rust functions that call intrinsics. The intrinsics themselves are not stable, and the intent is to never stabilize them.

I see, makes sense.

So it seems like this bug does not affect stable, but that's mostly luck (e.g., without #57997 this would be observable on stable).

@Centril
Copy link
Contributor

Centril commented Jun 3, 2019

@RalfJung The stable whitelist should be https://doc.rust-lang.org/nightly/nightly-rustc/src/rustc_mir/transform/qualify_min_const_fn.rs.html#378 and I hope that isn't borked.

@RalfJung
Copy link
Member Author

RalfJung commented Jun 4, 2019

That only applies in stable const fn though, not in const.

@RalfJung
Copy link
Member Author

RalfJung commented Aug 10, 2019

This is still an issue: assume is not whitelisted and yet this compiles on nightly:

#![feature(const_fn, core_intrinsics)]
pub const fn bar() {
    unsafe { core::intrinsics::assume(true) }
}

I think #61835 is supposed to fix this.

@RalfJung
Copy link
Member Author

I think #61835 is supposed to fix this.

But that PR got closed, so right now there's noone working on this issue AFAK.

Cc @vertexclique

@vertexclique
Copy link
Member

I will take a look in this week. Closed it because not to be pinged by triage. Currently I am rebasing the master.

@RalfJung
Copy link
Member Author

@vertexclique any update on this?

@RalfJung
Copy link
Member Author

@oli-obk this was resolved by #66275, right?

@oli-obk
Copy link
Contributor

oli-obk commented Dec 15, 2019

Well... yes and no. It is resolved, but I still want to change it so we don't have these lists at all but instead make all intrinsics use rustc_const_stable and rustc_const_unstable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: constant evaluation (mir interpretation) A-intrinsics Area: intrinsics C-bug Category: This is a bug.
Projects
None yet
7 participants