-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Organize intrinsics const evaluability checks #61835
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
Changes from 4 commits
0c2a54d
49fdb57
9f6ef64
21a5963
da6cc10
9484246
34951a9
5354f7f
c41c07b
54127de
07d1419
ab96b18
9328ee0
63cc638
e62acd4
3bc84dd
7ba0a57
ddaf56d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -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; | ||||
|
||||
|
@@ -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 { | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Shouldn't we just allow them to be that syntactically? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It would be a lot of work
Well... if there's an implementation in the miri-engine, then they can do whatever unstable fun they want I guess? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. And that might subvert properties that There was a problem hiding this comment. Choose a reason for hiding this commentThe 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... There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: Line 197 in 605ea9d
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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()[..] { | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we change this to match on a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Oh, this isn't actually exported in |
||||
| "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" | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. rename |
||||
} else if let hir::Node::Ctor(_) = node { | ||||
oli-obk marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
true | ||||
} else { | ||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah dang I might have mixed this up again with There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 @ecstatic-morse and I will be removing the copy from There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.^^ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh yeah this PR can land at any time, and I think There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good question for @ecstatic-morse. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I missed that this was baked into |
||
if !is_const_fn { | ||
return true; | ||
} | ||
} | ||
_ => return true, | ||
|
@@ -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; | ||
} | ||
|
@@ -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 | ||
|
Uh oh!
There was an error while loading. Please reload this page.