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

Make raw ptr ops unsafe in const contexts #55009

Merged
merged 2 commits into from
Jan 22, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 11 additions & 2 deletions src/librustc/hir/map/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -455,11 +455,20 @@ impl<'hir> Map<'hir> {
Node::AnonConst(_) => {
BodyOwnerKind::Const
}
Node::Variant(&Spanned { node: VariantKind { data: VariantData::Tuple(..), .. }, .. }) |
Node::StructCtor(..) |
Node::Item(&Item { node: ItemKind::Fn(..), .. }) |
Node::TraitItem(&TraitItem { node: TraitItemKind::Method(..), .. }) |
Node::ImplItem(&ImplItem { node: ImplItemKind::Method(..), .. }) => {
BodyOwnerKind::Fn
}
Node::Item(&Item { node: ItemKind::Static(_, m, _), .. }) => {
BodyOwnerKind::Static(m)
}
// Default to function if it's not a constant or static.
_ => BodyOwnerKind::Fn
Node::Expr(&Expr { node: ExprKind::Closure(..), .. }) => {
BodyOwnerKind::Closure
}
node => bug!("{:#?} is not a body node", node),
Copy link
Member

Choose a reason for hiding this comment

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

This currently breaks Clippy in clippy_lints/src/utils/mod.rs:

pub fn in_constant(cx: &LateContext<'_, '_>, id: NodeId) -> bool {
    let parent_id = cx.tcx.hir().get_parent(id);
    match cx.tcx.hir().body_owner_kind(parent_id) {
        hir::BodyOwnerKind::Fn | hir::BodyOwnerKind::Closure => false,
        hir::BodyOwnerKind::Const | hir::BodyOwnerKind::Static(..) => true,
    }
}

This is because it is called on an enum variant here: clippy_lints/src/non_copy_const.rs. self.get(id) returns Node::Item(Item { node: ItemKind::Enum(..), .. })

Should this function really produce an ICE if the node isn't one of the above. I think the previous approach of just assuming it is a function is also wrong. Maybe another variant for BodyOwnerKind would be better (BodyOwnerKind::Other?)

I would be happy to open a PR if this refactor should be done. Otherwise I'll try to fix this in Clippy.

cc @oli-obk

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could make it return an Option, but I'm not sure if that is desirable. We didn't want to know whether something was a function, closure or something else, we just wanted to know whether it is a constant or static.

In the end I decided to just check for const/static manually: https://github.com/rust-lang/rust-clippy/pull/3685/files

}
}

Expand Down
12 changes: 12 additions & 0 deletions src/librustc/hir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1270,13 +1270,25 @@ pub enum BodyOwnerKind {
/// Functions and methods.
Fn,

/// Closures
Closure,

/// Constants and associated constants.
Const,

/// Initializer of a `static` item.
Static(Mutability),
}

impl BodyOwnerKind {
pub fn is_fn_or_closure(self) -> bool {
match self {
BodyOwnerKind::Fn | BodyOwnerKind::Closure => true,
BodyOwnerKind::Const | BodyOwnerKind::Static(_) => false,
}
}
}

/// A constant (expression) that's not an item or associated item,
/// but needs its own `DefId` for type-checking, const-eval, etc.
/// These are usually found nested inside types (e.g., array lengths)
Expand Down
4 changes: 2 additions & 2 deletions src/librustc/middle/region.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1268,8 +1268,8 @@ impl<'a, 'tcx> Visitor<'tcx> for RegionResolutionVisitor<'a, 'tcx> {

// The body of the every fn is a root scope.
self.cx.parent = self.cx.var_parent;
if let hir::BodyOwnerKind::Fn = self.tcx.hir().body_owner_kind(owner_id) {
self.visit_expr(&body.value);
if self.tcx.hir().body_owner_kind(owner_id).is_fn_or_closure() {
self.visit_expr(&body.value)
} else {
// Only functions have an outer terminating (drop) scope, while
// temporaries in constant initializers may be 'static, but only
Expand Down
5 changes: 1 addition & 4 deletions src/librustc_mir/borrow_check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -163,10 +163,7 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>(
|bd, i| DebugFormatted::new(&bd.move_data().move_paths[i]),
));

let locals_are_invalidated_at_exit = match tcx.hir().body_owner_kind(id) {
hir::BodyOwnerKind::Const | hir::BodyOwnerKind::Static(_) => false,
hir::BodyOwnerKind::Fn => true,
};
let locals_are_invalidated_at_exit = tcx.hir().body_owner_kind(id).is_fn_or_closure();
let borrow_set = Rc::new(BorrowSet::build(
tcx, mir, locals_are_invalidated_at_exit, &mdpe.move_data));

Expand Down
1 change: 1 addition & 0 deletions src/librustc_mir/borrow_check/nll/universal_regions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -476,6 +476,7 @@ impl<'cx, 'gcx, 'tcx> UniversalRegionsBuilder<'cx, 'gcx, 'tcx> {
let closure_base_def_id = tcx.closure_base_def_id(self.mir_def_id);

match tcx.hir().body_owner_kind(self.mir_node_id) {
BodyOwnerKind::Closure |
BodyOwnerKind::Fn => {
let defining_ty = if self.mir_def_id == closure_base_def_id {
tcx.type_of(closure_base_def_id)
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_mir/build/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ pub fn mir_build<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId) -> Mir<'t
let cx = Cx::new(&infcx, id);
let mut mir = if cx.tables().tainted_by_errors {
build::construct_error(cx, body_id)
} else if let hir::BodyOwnerKind::Fn = cx.body_owner_kind {
} else if cx.body_owner_kind.is_fn_or_closure() {
// fetch the fully liberated fn signature (that is, all bound
// types/lifetimes replaced)
let fn_hir_id = tcx.hir().node_to_hir_id(id);
Expand Down
1 change: 1 addition & 0 deletions src/librustc_mir/build/scope.rs
Original file line number Diff line number Diff line change
Expand Up @@ -613,6 +613,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
hir::BodyOwnerKind::Static(_) =>
// No need to free storage in this context.
None,
hir::BodyOwnerKind::Closure |
hir::BodyOwnerKind::Fn =>
Some(self.topmost_scope()),
}
Expand Down
1 change: 1 addition & 0 deletions src/librustc_mir/hair/cx/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ impl<'a, 'gcx, 'tcx> Cx<'a, 'gcx, 'tcx> {
let constness = match body_owner_kind {
hir::BodyOwnerKind::Const |
hir::BodyOwnerKind::Static(_) => hir::Constness::Const,
hir::BodyOwnerKind::Closure |
hir::BodyOwnerKind::Fn => hir::Constness::NotConst,
};

Expand Down
97 changes: 77 additions & 20 deletions src/librustc_mir/transform/check_unsafety.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use rustc_data_structures::sync::Lrc;

use rustc::ty::query::Providers;
use rustc::ty::{self, TyCtxt};
use rustc::ty::cast::CastTy;
use rustc::hir;
use rustc::hir::Node;
use rustc::hir::def_id::DefId;
Expand All @@ -20,6 +21,7 @@ use util;

pub struct UnsafetyChecker<'a, 'tcx: 'a> {
mir: &'a Mir<'tcx>,
const_context: bool,
min_const_fn: bool,
source_scope_local_data: &'a IndexVec<SourceScope, SourceScopeLocalData>,
violations: Vec<UnsafetyViolation>,
Expand All @@ -33,14 +35,20 @@ pub struct UnsafetyChecker<'a, 'tcx: 'a> {

impl<'a, 'gcx, 'tcx> UnsafetyChecker<'a, 'tcx> {
fn new(
const_context: bool,
min_const_fn: bool,
mir: &'a Mir<'tcx>,
source_scope_local_data: &'a IndexVec<SourceScope, SourceScopeLocalData>,
tcx: TyCtxt<'a, 'tcx, 'tcx>,
param_env: ty::ParamEnv<'tcx>,
) -> Self {
// sanity check
if min_const_fn {
assert!(const_context);
}
Self {
mir,
const_context,
min_const_fn,
source_scope_local_data,
violations: vec![],
Expand Down Expand Up @@ -124,29 +132,70 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {
rvalue: &Rvalue<'tcx>,
location: Location)
{
if let &Rvalue::Aggregate(box ref aggregate, _) = rvalue {
match aggregate {
&AggregateKind::Array(..) |
&AggregateKind::Tuple => {}
&AggregateKind::Adt(ref def, ..) => {
match self.tcx.layout_scalar_valid_range(def.did) {
(Bound::Unbounded, Bound::Unbounded) => {},
_ => self.require_unsafe(
"initializing type with `rustc_layout_scalar_valid_range` attr",
"initializing a layout restricted type's field with a value outside \
the valid range is undefined behavior",
UnsafetyViolationKind::GeneralAndConstFn,
),
match rvalue {
Rvalue::Aggregate(box ref aggregate, _) => {
match aggregate {
&AggregateKind::Array(..) |
&AggregateKind::Tuple => {}
&AggregateKind::Adt(ref def, ..) => {
match self.tcx.layout_scalar_valid_range(def.did) {
(Bound::Unbounded, Bound::Unbounded) => {},
_ => self.require_unsafe(
"initializing type with `rustc_layout_scalar_valid_range` attr",
"initializing a layout restricted type's field with a value \
outside the valid range is undefined behavior",
UnsafetyViolationKind::GeneralAndConstFn,
),
}
}
&AggregateKind::Closure(def_id, _) |
&AggregateKind::Generator(def_id, _, _) => {
let UnsafetyCheckResult {
violations, unsafe_blocks
} = self.tcx.unsafety_check_result(def_id);
self.register_violations(&violations, &unsafe_blocks);
}
}
&AggregateKind::Closure(def_id, _) |
&AggregateKind::Generator(def_id, _, _) => {
let UnsafetyCheckResult {
violations, unsafe_blocks
} = self.tcx.unsafety_check_result(def_id);
self.register_violations(&violations, &unsafe_blocks);
},
// casting pointers to ints is unsafe in const fn because the const evaluator cannot
// possibly know what the result of various operations like `address / 2` would be
// pointers during const evaluation have no integral address, only an abstract one
Rvalue::Cast(CastKind::Misc, ref operand, cast_ty)
if self.const_context && self.tcx.features().const_raw_ptr_to_usize_cast => {
let operand_ty = operand.ty(self.mir, self.tcx);
let cast_in = CastTy::from_ty(operand_ty).expect("bad input type for cast");
let cast_out = CastTy::from_ty(cast_ty).expect("bad output type for cast");
match (cast_in, cast_out) {
(CastTy::Ptr(_), CastTy::Int(_)) |
(CastTy::FnPtr, CastTy::Int(_)) => {
self.register_violations(&[UnsafetyViolation {
source_info: self.source_info,
description: Symbol::intern("cast of pointer to int").as_interned_str(),
details: Symbol::intern("casting pointers to integers in constants")
.as_interned_str(),
kind: UnsafetyViolationKind::General,
}], &[]);
},
_ => {},
}
}
// raw pointer and fn pointer operations are unsafe as it is not clear whether one
// pointer would be "less" or "equal" to another, because we cannot know where llvm
// or the linker will place various statics in memory. Without this information the
// result of a comparison of addresses would differ between runtime and compile-time.
Rvalue::BinaryOp(_, ref lhs, _)
if self.const_context && self.tcx.features().const_compare_raw_pointers => {
if let ty::RawPtr(_) | ty::FnPtr(..) = lhs.ty(self.mir, self.tcx).sty {
self.register_violations(&[UnsafetyViolation {
source_info: self.source_info,
description: Symbol::intern("pointer operation").as_interned_str(),
details: Symbol::intern("operations on pointers in constants")
.as_interned_str(),
kind: UnsafetyViolationKind::General,
}], &[]);
}
}
_ => {},
}
self.super_rvalue(rvalue, location);
}
Expand Down Expand Up @@ -484,8 +533,16 @@ fn unsafety_check_result<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId)
};

let param_env = tcx.param_env(def_id);

let id = tcx.hir().as_local_node_id(def_id).unwrap();
let (const_context, min_const_fn) = match tcx.hir().body_owner_kind(id) {
hir::BodyOwnerKind::Closure => (false, false),
hir::BodyOwnerKind::Fn => (tcx.is_const_fn(def_id), tcx.is_min_const_fn(def_id)),
hir::BodyOwnerKind::Const |
hir::BodyOwnerKind::Static(_) => (true, false),
};
let mut checker = UnsafetyChecker::new(
tcx.is_min_const_fn(def_id),
const_context, min_const_fn,
mir, source_scope_local_data, tcx, param_env);
checker.visit_mir(mir);

Expand Down
12 changes: 4 additions & 8 deletions src/librustc_mir/transform/inline.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
//! Inlining pass for MIR functions

use rustc::hir;
use rustc::hir::CodegenFnAttrFlags;
use rustc::hir::def_id::DefId;

Expand Down Expand Up @@ -74,15 +73,12 @@ impl<'a, 'tcx> Inliner<'a, 'tcx> {

// Only do inlining into fn bodies.
let id = self.tcx.hir().as_local_node_id(self.source.def_id).unwrap();
let body_owner_kind = self.tcx.hir().body_owner_kind(id);

if let (hir::BodyOwnerKind::Fn, None) = (body_owner_kind, self.source.promoted) {

if self.tcx.hir().body_owner_kind(id).is_fn_or_closure() && self.source.promoted.is_none() {
for (bb, bb_data) in caller_mir.basic_blocks().iter_enumerated() {
if let Some(callsite) = self.get_valid_function_call(bb,
bb_data,
caller_mir,
param_env) {
bb_data,
caller_mir,
param_env) {
callsites.push_back(callsite);
}
}
Expand Down
1 change: 1 addition & 0 deletions src/librustc_mir/transform/qualify_consts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1152,6 +1152,7 @@ impl MirPass for QualifyAndPromoteConstants {
let id = tcx.hir().as_local_node_id(def_id).unwrap();
let mut const_promoted_temps = None;
let mode = match tcx.hir().body_owner_kind(id) {
hir::BodyOwnerKind::Closure => Mode::Fn,
hir::BodyOwnerKind::Fn => {
if tcx.is_const_fn(def_id) {
Mode::ConstFn
Expand Down
2 changes: 2 additions & 0 deletions src/librustc_mir/util/pretty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -573,6 +573,7 @@ fn write_mir_sig(tcx: TyCtxt, src: MirSource, mir: &Mir, w: &mut dyn Write) -> i
let body_owner_kind = tcx.hir().body_owner_kind(id);
match (body_owner_kind, src.promoted) {
(_, Some(i)) => write!(w, "{:?} in", i)?,
(hir::BodyOwnerKind::Closure, _) |
(hir::BodyOwnerKind::Fn, _) => write!(w, "fn")?,
(hir::BodyOwnerKind::Const, _) => write!(w, "const")?,
(hir::BodyOwnerKind::Static(hir::MutImmutable), _) => write!(w, "static")?,
Expand All @@ -585,6 +586,7 @@ fn write_mir_sig(tcx: TyCtxt, src: MirSource, mir: &Mir, w: &mut dyn Write) -> i
})?;

match (body_owner_kind, src.promoted) {
(hir::BodyOwnerKind::Closure, None) |
(hir::BodyOwnerKind::Fn, None) => {
write!(w, "(")?;

Expand Down
1 change: 1 addition & 0 deletions src/librustc_passes/rvalue_promotion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ impl<'a, 'tcx> CheckCrateVisitor<'a, 'tcx> {
self.in_static = false;

match self.tcx.hir().body_owner_kind(item_id) {
hir::BodyOwnerKind::Closure |
hir::BodyOwnerKind::Fn => self.in_fn = true,
hir::BodyOwnerKind::Static(_) => self.in_static = true,
_ => {}
Expand Down
8 changes: 6 additions & 2 deletions src/test/ui/cast/cast-ptr-to-int-const.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
// gate-test-const_raw_ptr_to_usize_cast

fn main() {
const X: u32 = main as u32; //~ ERROR casting pointers to integers in constants is unstable
const X: u32 = unsafe {
main as u32 //~ ERROR casting pointers to integers in constants is unstable
};
const Y: u32 = 0;
const Z: u32 = &Y as *const u32 as u32; //~ ERROR is unstable
const Z: u32 = unsafe {
&Y as *const u32 as u32 //~ ERROR is unstable
};
}
12 changes: 6 additions & 6 deletions src/test/ui/cast/cast-ptr-to-int-const.stderr
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
error[E0658]: casting pointers to integers in constants is unstable (see issue #51910)
--> $DIR/cast-ptr-to-int-const.rs:4:20
--> $DIR/cast-ptr-to-int-const.rs:5:9
|
LL | const X: u32 = main as u32; //~ ERROR casting pointers to integers in constants is unstable
| ^^^^^^^^^^^
LL | main as u32 //~ ERROR casting pointers to integers in constants is unstable
| ^^^^^^^^^^^
|
= help: add #![feature(const_raw_ptr_to_usize_cast)] to the crate attributes to enable

error[E0658]: casting pointers to integers in constants is unstable (see issue #51910)
--> $DIR/cast-ptr-to-int-const.rs:6:20
--> $DIR/cast-ptr-to-int-const.rs:9:9
|
LL | const Z: u32 = &Y as *const u32 as u32; //~ ERROR is unstable
| ^^^^^^^^^^^^^^^^^^^^^^^
LL | &Y as *const u32 as u32 //~ ERROR is unstable
| ^^^^^^^^^^^^^^^^^^^^^^^
|
= help: add #![feature(const_raw_ptr_to_usize_cast)] to the crate attributes to enable

Expand Down
8 changes: 4 additions & 4 deletions src/test/ui/consts/const-eval/const_raw_ptr_ops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,13 @@
fn main() {}

// unconst and bad, will thus error in miri
const X: bool = &1 as *const i32 == &2 as *const i32; //~ ERROR any use of this value will cause
const X: bool = unsafe { &1 as *const i32 == &2 as *const i32 }; //~ ERROR any use of this
// unconst and fine
const X2: bool = 42 as *const i32 == 43 as *const i32;
const X2: bool = unsafe { 42 as *const i32 == 43 as *const i32 };
// unconst and fine
const Y: usize = 42usize as *const i32 as usize + 1;
const Y: usize = unsafe { 42usize as *const i32 as usize + 1 };
// unconst and bad, will thus error in miri
const Y2: usize = &1 as *const i32 as usize + 1; //~ ERROR any use of this value will cause
const Y2: usize = unsafe { &1 as *const i32 as usize + 1 }; //~ ERROR any use of this
// unconst and fine
const Z: i32 = unsafe { *(&1 as *const i32) };
// unconst and bad, will thus error in miri
Expand Down
16 changes: 8 additions & 8 deletions src/test/ui/consts/const-eval/const_raw_ptr_ops.stderr
Original file line number Diff line number Diff line change
@@ -1,20 +1,20 @@
error: any use of this value will cause an error
--> $DIR/const_raw_ptr_ops.rs:6:1
|
LL | const X: bool = &1 as *const i32 == &2 as *const i32; //~ ERROR any use of this value will cause
| ^^^^^^^^^^^^^^^^------------------------------------^
| |
| "pointer arithmetic or comparison" needs an rfc before being allowed inside constants
LL | const X: bool = unsafe { &1 as *const i32 == &2 as *const i32 }; //~ ERROR any use of this
| ^^^^^^^^^^^^^^^^^^^^^^^^^------------------------------------^^^
| |
| "pointer arithmetic or comparison" needs an rfc before being allowed inside constants
|
= note: #[deny(const_err)] on by default

error: any use of this value will cause an error
--> $DIR/const_raw_ptr_ops.rs:12:1
|
LL | const Y2: usize = &1 as *const i32 as usize + 1; //~ ERROR any use of this value will cause
| ^^^^^^^^^^^^^^^^^^-----------------------------^
| |
| "pointer arithmetic or comparison" needs an rfc before being allowed inside constants
LL | const Y2: usize = unsafe { &1 as *const i32 as usize + 1 }; //~ ERROR any use of this
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^-----------------------------^^^
| |
| "pointer arithmetic or comparison" needs an rfc before being allowed inside constants

error: any use of this value will cause an error
--> $DIR/const_raw_ptr_ops.rs:16:1
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/consts/min_const_fn/cmp_fn_pointers.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
const fn cmp(x: fn(), y: fn()) -> bool { //~ ERROR function pointers in const fn are unstable
x == y
unsafe { x == y }
}

fn main() {}
Loading