-
-
Notifications
You must be signed in to change notification settings - Fork 14.9k
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.
Outdated
|
||||
| /// only checks whether the function has a `const` modifier | ||||
| fn is_const_evaluatable(tcx: TyCtxt<'tcx, 'tcx>, def_id: DefId) -> bool { | ||||
|
Contributor
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
Contributor
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?
Member
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
Contributor
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?
Member
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
Contributor
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...
Contributor
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
Contributor
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
Member
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
Contributor
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.
Outdated
|
||||
| // top level beforehand. | ||||
| match tcx.fn_sig(def_id).abi() { | ||||
| Abi::RustIntrinsic | | ||||
| Abi::PlatformIntrinsic => { | ||||
| match &tcx.item_name(def_id).as_str()[..] { | ||||
|
Contributor
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
Contributor
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" | ||||
|
Contributor
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) | ||||
|
Contributor
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.
|
||||
| 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
Member
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
Member
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
Contributor
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
Member
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.^^
Contributor
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
Member
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?
Contributor
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.
Contributor
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
Contributor
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.
|
||
| // 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.