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

Organize intrinsics const evaluability checks #61835

Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/libcore/intrinsics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -913,6 +913,7 @@ extern "rust-intrinsic" {
/// }
/// ```
#[stable(feature = "rust1", since = "1.0.0")]
#[rustc_const_unstable(feature = "const_transmute")]
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
pub fn transmute<T, U>(e: T) -> U;

/// Returns `true` if the actual type given as `T` requires drop
Expand Down
45 changes: 43 additions & 2 deletions src/librustc/ty/constness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use crate::hir::def_id::DefId;
use crate::hir;
use crate::ty::TyCtxt;
use syntax_pos::symbol::{sym, Symbol};
use rustc_target::spec::abi::Abi;
use crate::hir::map::blocks::FnLikeNode;
use syntax::attr;

Expand Down Expand Up @@ -68,14 +69,54 @@ impl<'tcx> TyCtxt<'tcx, 'tcx> {


pub fn provide<'tcx>(providers: &mut Providers<'tcx>) {
vertexclique marked this conversation as resolved.
Show resolved Hide resolved
/// only checks whether the function has a `const` modifier
fn is_const_evaluatable(tcx: TyCtxt<'tcx, 'tcx>, def_id: DefId) -> bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not have this as a separate function, but rather use it before the usage of FnLikeNode, so intrinsics' definitions aren't checked for const fn (as they syntactically cannot be const fn).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(as they syntactically cannot be const fn).

Shouldn't we just allow them to be that syntactically?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't you say you tried that once and it did not work?

Also, that would allow nightly users to declare intrinsics as const. There might be ways to cause unsoundness there. Wouldn't that be a problem?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't you say you tried that once and it did not work?

It would be a lot of work

Also, that would allow nightly users to declare intrinsics as const. There might be ways to cause unsoundness there. Wouldn't that be a problem?

Well... if there's an implementation in the miri-engine, then they can do whatever unstable fun they want I guess?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. And that might subvert properties that const fn otherwise have.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only consider this a problem in libstd. If you are using nightly features you can already brick properties that we aren't sure about yet, so...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of these days we should change intrinsics so they don't abuse extern "..." {...} anymore and then we can have const fn transmute or w/e. As for not allowing const fn on intrinsics that shouldn't be, I suppose that should go in rustc_typeck's list of intrinsic signatures.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could just move to making them lang items and having libcore implement them by recursing on themselves and then have the compiler overwrite them. Then they'd be regular functions (and we used this trick on one intrinsic already:

unsafe fn real_drop_in_place<T: ?Sized>(to_drop: &mut T) {
)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would make it harder to figure out that a function is an intrinsic. Also real_drop_in_place actually codegens to real functions, while intrinsics always inline their code at the caller site. Another difference between intrinsics and (lang item) functions is that intrinsics cant be turned into function pointers, while (lang item) functions can be.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oli-obk sigh we don't have to re-use lang items, we can have a different mechanism (but also attribute-based) for them. I've been meaning to do this for ages but @nikomatsakis wanted an RFC which I never got around to writing, oops.

// Intrinsics promotion whitelist is here to check const evaluability at the
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
// top level beforehand.
match tcx.fn_sig(def_id).abi() {
Abi::RustIntrinsic |
Abi::PlatformIntrinsic => {
match &tcx.item_name(def_id).as_str()[..] {
Copy link
Contributor

@ecstatic-morse ecstatic-morse Nov 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we change this to match on a Symbol instead? This version is needlessly slow.

Copy link
Contributor

@ecstatic-morse ecstatic-morse Nov 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, @_nnethercote has been trying to remove queries that are easy to compute to improve performance. I think this could just be a function.

Oh, this isn't actually exported in providers. My bad.

| "size_of"
| "min_align_of"
| "needs_drop"
| "type_id"
| "bswap"
| "bitreverse"
| "ctpop"
| "cttz"
| "cttz_nonzero"
| "ctlz"
| "ctlz_nonzero"
| "overflowing_add"
| "overflowing_sub"
| "overflowing_mul"
| "unchecked_shl"
| "unchecked_shr"
| "rotate_left"
| "rotate_right"
| "add_with_overflow"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wrapping intrinsics seem to be missing from this list. The errors mean the check is working! Awesome! Please add anything to this list that is needed to get the tests passing

| "sub_with_overflow"
| "mul_with_overflow"
| "saturating_add"
| "saturating_sub"
| "transmute"
=> true,

_ => false
}
}
_ => false
}
}

/// Checks whether the function has a `const` modifier and intrinsics can be promotable in it
fn is_const_fn_raw<'tcx>(tcx: TyCtxt<'tcx, 'tcx>, def_id: DefId) -> bool {
let hir_id = tcx.hir().as_local_hir_id(def_id)
.expect("Non-local call to local provider is_const_fn");

let node = tcx.hir().get_by_hir_id(hir_id);
if let Some(fn_like) = FnLikeNode::from_node(node) {
fn_like.constness() == hir::Constness::Const
(fn_like.constness() == hir::Constness::Const) || is_const_evaluatable(tcx, def_id)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename is_const_evaluatable to is_const_intrinsic and check it first thing in is_const_fn_raw. If it's true, return true. (as suggested above in /~https://github.com/rust-lang/rust/pull/61835/files#r294078191)

} else if let hir::Node::Ctor(_) = node {
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
true
} else {
Expand Down
68 changes: 9 additions & 59 deletions src/librustc_mir/transform/qualify_consts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -489,49 +489,12 @@ impl Qualif for IsNotPromotable {
let fn_ty = callee.ty(cx.body, cx.tcx);
match fn_ty.sty {
ty::FnDef(def_id, _) => {
match cx.tcx.fn_sig(def_id).abi() {
Abi::RustIntrinsic |
Abi::PlatformIntrinsic => {
assert!(!cx.tcx.is_const_fn(def_id));
match &cx.tcx.item_name(def_id).as_str()[..] {
| "size_of"
| "min_align_of"
| "needs_drop"
| "type_id"
| "bswap"
| "bitreverse"
| "ctpop"
| "cttz"
| "cttz_nonzero"
| "ctlz"
| "ctlz_nonzero"
| "overflowing_add"
| "overflowing_sub"
| "overflowing_mul"
| "unchecked_shl"
| "unchecked_shr"
| "rotate_left"
| "rotate_right"
| "add_with_overflow"
| "sub_with_overflow"
| "mul_with_overflow"
| "saturating_add"
| "saturating_sub"
| "transmute"
=> return true,

_ => {}
}
}
_ => {
let is_const_fn =
cx.tcx.is_const_fn(def_id) ||
cx.tcx.is_unstable_const_fn(def_id).is_some() ||
cx.is_const_panic_fn(def_id);
if !is_const_fn {
return true;
}
}
let is_const_fn =
cx.tcx.is_const_fn(def_id) ||
cx.tcx.is_unstable_const_fn(def_id).is_some() ||
cx.is_const_panic_fn(def_id);
Comment on lines +532 to +535
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this IsNotPromotable? Shouldn't that check the rustc_promotable attribute instead of allowing all const fn?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah dang I might have mixed this up again with IsNotImplicitlyPromotable...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this can't go wrong anymore, without triggering a mismatch with promote_consts' copy of the logic (although we should make sure this PR updates both properly), which would ICE.

@ecstatic-morse and I will be removing the copy from qualify_consts in a week or two (maybe we just need to wait for beta to branch?), so IsNot(Implicitly) Promotable is going away soon!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just hope we get the intrinsic thing landed as part of this refactor, it's been 4 months or so.^^

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah this PR can land at any time, and I think promote_consts even has the right code already, without another copy of the list of intrinsics.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, but what about the new const checking pass? Does that still allow calling intrinsics?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good question for @ecstatic-morse.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reserved a spot for these checks here. I think we just need to call the is_const_intrinsic from that spot? I dunno if this PR makes additional changes to the validation logic though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed that this was baked into in_const_fn now (see below), I think this PR just needs to remove that return along with the FIXME above it.

if !is_const_fn {
return true;
}
}
_ => return true,
Expand Down Expand Up @@ -1251,21 +1214,6 @@ impl<'a, 'tcx> Visitor<'tcx> for Checker<'a, 'tcx> {
Abi::PlatformIntrinsic => {
assert!(!self.tcx.is_const_fn(def_id));
match &self.tcx.item_name(def_id).as_str()[..] {
// special intrinsic that can be called diretly without an intrinsic
// feature gate needs a language feature gate
"transmute" => {
if self.mode.requires_const_checking() {
// const eval transmute calls only with the feature gate
if !self.tcx.features().const_transmute {
emit_feature_err(
&self.tcx.sess.parse_sess, sym::const_transmute,
self.span, GateIssue::Language,
&format!("The use of std::mem::transmute() \
is gated in {}s", self.mode));
}
}
}

name if name.starts_with("simd_shuffle") => {
is_shuffle = true;
}
Expand All @@ -1276,7 +1224,9 @@ impl<'a, 'tcx> Visitor<'tcx> for Checker<'a, 'tcx> {
}
}
_ => {
// In normal functions no calls are feature-gated.
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
// Only in non-const functions it is perfectly fine to call any
// function, even ones whose constness is unstable. Everywhere else
// we need to check the appropriate feature gates.
if self.mode.requires_const_checking() {
let unleash_miri = self
.tcx
Expand Down
3 changes: 0 additions & 3 deletions src/libsyntax/feature_gate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -489,9 +489,6 @@ declare_features! (
// This will likely be removed prior to stabilization of async/await.
(active, await_macro, "1.28.0", Some(50547), None),

// Allows reinterpretation of the bits of a value of one type as another type during const eval.
(active, const_transmute, "1.29.0", Some(53605), None),

// Allows using `try {...}` expressions.
(active, try_blocks, "1.29.0", Some(31436), None),

Expand Down