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 13 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 @@ -935,6 +935,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
50 changes: 47 additions & 3 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,57 @@ impl<'tcx> TyCtxt<'tcx> {


pub fn provide(providers: &mut Providers<'_>) {
/// only checks whether the function has a `const` modifier
fn is_const_intrinsic(tcx: TyCtxt<'_>, def_id: DefId) -> bool {
// Const evaluability whitelist is here to check evaluability at the
// 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: TyCtxt<'_>, 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(hir_id);
if let Some(fn_like) = FnLikeNode::from_node(node) {
fn_like.constness() == hir::Constness::Const

if is_const_intrinsic(tcx, def_id) {
false
vertexclique marked this conversation as resolved.
Show resolved Hide resolved
} else if let Some(fn_like) = FnLikeNode::from_node(node) {
(fn_like.constness() == hir::Constness::Const)
} else if let hir::Node::Ctor(_) = node {
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
true
} else {
Expand Down
70 changes: 9 additions & 61 deletions src/librustc_mir/transform/qualify_consts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -528,51 +528,12 @@ impl Qualif for IsNotPromotable {
let fn_ty = callee.ty(cx.body, cx.tcx);
match fn_ty.kind {
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"
| "wrapping_add"
| "wrapping_sub"
| "wrapping_mul"
| "unchecked_shl"
| "unchecked_shr"
| "rotate_left"
| "rotate_right"
| "add_with_overflow"
| "sub_with_overflow"
| "mul_with_overflow"
| "saturating_add"
| "saturating_sub"
| "transmute"
| "simd_insert"
| "simd_extract"
=> 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 @@ -1324,21 +1285,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 @@ -1349,7 +1295,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
4 changes: 0 additions & 4 deletions src/libsyntax/feature_gate/active.rs
Original file line number Diff line number Diff line change
Expand Up @@ -429,10 +429,6 @@ declare_features! (
/// Allows using `#[doc(keyword = "...")]`.
(active, doc_keyword, "1.28.0", Some(51315), 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
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
#![feature(core_intrinsics)]
fn main() {
let x: &'static usize =
&std::intrinsics::size_of::<i32>(); //~ ERROR temporary value dropped while borrowed
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
error[E0716]: temporary value dropped while borrowed
--> $DIR/const-eval-intrinsic-promotion.rs:4:10
|
LL | let x: &'static usize =
| -------------- type annotation requires that borrow lasts for `'static`
LL | &std::intrinsics::size_of::<i32>();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ creates a temporary which is freed while still in use
LL | }
| - temporary value is freed at the end of this statement

error: aborting due to previous error

For more information about this error, try `rustc --explain E0716`.