From aeda178011775f4a8e16446341fe4774e02d8f5f Mon Sep 17 00:00:00 2001 From: Patrick Walton Date: Thu, 23 May 2013 19:12:16 -0700 Subject: [PATCH] librustc: Redo the unsafe checker and make unsafe methods not callable from safe code --- src/libextra/arc.rs | 18 +- src/libextra/sync.rs | 94 ++++++----- src/librustc/driver/driver.rs | 3 + src/librustc/middle/effect.rs | 154 ++++++++++++++++++ src/librustc/middle/typeck/check/mod.rs | 34 ---- src/librustc/rustc.rc | 1 + src/libstd/comm.rs | 20 ++- src/libstd/os.rs | 24 ++- src/libstd/rt/message_queue.rs | 18 +- src/libstd/rt/work_queue.rs | 34 ++-- src/libstd/task/spawn.rs | 8 +- .../compile-fail/foreign-unsafe-fn-called.rs | 2 +- src/test/compile-fail/foreign-unsafe-fn.rs | 2 +- src/test/compile-fail/forget-init-unsafe.rs | 6 +- .../unsafe-fn-called-from-safe.rs | 2 +- .../compile-fail/unsafe-fn-used-as-value.rs | 4 +- 16 files changed, 288 insertions(+), 136 deletions(-) create mode 100644 src/librustc/middle/effect.rs diff --git a/src/libextra/arc.rs b/src/libextra/arc.rs index 4baa1168b3b..e120f3dd033 100644 --- a/src/libextra/arc.rs +++ b/src/libextra/arc.rs @@ -198,13 +198,15 @@ pub impl MutexARC { */ #[inline(always)] unsafe fn access(&self, blk: &fn(x: &mut T) -> U) -> U { - let state = self.x.get(); - // Borrowck would complain about this if the function were - // not already unsafe. See borrow_rwlock, far below. - do (&(*state).lock).lock { - check_poison(true, (*state).failed); - let _z = PoisonOnFail(&mut (*state).failed); - blk(&mut (*state).data) + unsafe { + let state = self.x.get(); + // Borrowck would complain about this if the function were + // not already unsafe. See borrow_rwlock, far below. + do (&(*state).lock).lock { + check_poison(true, (*state).failed); + let _z = PoisonOnFail(&mut (*state).failed); + blk(&mut (*state).data) + } } } @@ -356,8 +358,8 @@ pub impl RWARC { * access modes, this will not poison the ARC. */ fn read(&self, blk: &fn(x: &T) -> U) -> U { - let state = self.x.get(); unsafe { + let state = self.x.get(); do (*state).lock.read { check_poison(false, (*state).failed); blk(&(*state).data) diff --git a/src/libextra/sync.rs b/src/libextra/sync.rs index 48f34fdf46a..beea3b6f52b 100644 --- a/src/libextra/sync.rs +++ b/src/libextra/sync.rs @@ -100,30 +100,34 @@ fn new_sem_and_signal(count: int, num_condvars: uint) #[doc(hidden)] pub impl Sem { fn acquire(&self) { - let mut waiter_nobe = None; - do (**self).with |state| { - state.count -= 1; - if state.count < 0 { - // Create waiter nobe. - let (WaitEnd, SignalEnd) = comm::oneshot(); - // Tell outer scope we need to block. - waiter_nobe = Some(WaitEnd); - // Enqueue ourself. - state.waiters.tail.send(SignalEnd); + unsafe { + let mut waiter_nobe = None; + do (**self).with |state| { + state.count -= 1; + if state.count < 0 { + // Create waiter nobe. + let (WaitEnd, SignalEnd) = comm::oneshot(); + // Tell outer scope we need to block. + waiter_nobe = Some(WaitEnd); + // Enqueue ourself. + state.waiters.tail.send(SignalEnd); + } + } + // Uncomment if you wish to test for sem races. Not valgrind-friendly. + /* for 1000.times { task::yield(); } */ + // Need to wait outside the exclusive. + if waiter_nobe.is_some() { + let _ = comm::recv_one(waiter_nobe.unwrap()); } - } - // Uncomment if you wish to test for sem races. Not valgrind-friendly. - /* for 1000.times { task::yield(); } */ - // Need to wait outside the exclusive. - if waiter_nobe.is_some() { - let _ = comm::recv_one(waiter_nobe.unwrap()); } } fn release(&self) { - do (**self).with |state| { - state.count += 1; - if state.count <= 0 { - signal_waitqueue(&state.waiters); + unsafe { + do (**self).with |state| { + state.count += 1; + if state.count <= 0 { + signal_waitqueue(&state.waiters); + } } } } @@ -283,17 +287,19 @@ pub impl<'self> Condvar<'self> { /// As signal, but with a specified condvar_id. See wait_on. fn signal_on(&self, condvar_id: uint) -> bool { - let mut out_of_bounds = None; - let mut result = false; - do (**self.sem).with |state| { - if condvar_id < state.blocked.len() { - result = signal_waitqueue(&state.blocked[condvar_id]); - } else { - out_of_bounds = Some(state.blocked.len()); + unsafe { + let mut out_of_bounds = None; + let mut result = false; + do (**self.sem).with |state| { + if condvar_id < state.blocked.len() { + result = signal_waitqueue(&state.blocked[condvar_id]); + } else { + out_of_bounds = Some(state.blocked.len()); + } + } + do check_cvar_bounds(out_of_bounds, condvar_id, "cond.signal_on()") { + result } - } - do check_cvar_bounds(out_of_bounds, condvar_id, "cond.signal_on()") { - result } } @@ -304,20 +310,22 @@ pub impl<'self> Condvar<'self> { fn broadcast_on(&self, condvar_id: uint) -> uint { let mut out_of_bounds = None; let mut queue = None; - do (**self.sem).with |state| { - if condvar_id < state.blocked.len() { - // To avoid :broadcast_heavy, we make a new waitqueue, - // swap it out with the old one, and broadcast on the - // old one outside of the little-lock. - queue = Some(util::replace(&mut state.blocked[condvar_id], - new_waitqueue())); - } else { - out_of_bounds = Some(state.blocked.len()); + unsafe { + do (**self.sem).with |state| { + if condvar_id < state.blocked.len() { + // To avoid :broadcast_heavy, we make a new waitqueue, + // swap it out with the old one, and broadcast on the + // old one outside of the little-lock. + queue = Some(util::replace(&mut state.blocked[condvar_id], + new_waitqueue())); + } else { + out_of_bounds = Some(state.blocked.len()); + } + } + do check_cvar_bounds(out_of_bounds, condvar_id, "cond.signal_on()") { + let queue = queue.swap_unwrap(); + broadcast_waitqueue(&queue) } - } - do check_cvar_bounds(out_of_bounds, condvar_id, "cond.signal_on()") { - let queue = queue.swap_unwrap(); - broadcast_waitqueue(&queue) } } } diff --git a/src/librustc/driver/driver.rs b/src/librustc/driver/driver.rs index 2d2170278ad..b56699927e4 100644 --- a/src/librustc/driver/driver.rs +++ b/src/librustc/driver/driver.rs @@ -262,6 +262,9 @@ pub fn compile_rest(sess: Session, time(time_passes, ~"privacy checking", || middle::privacy::check_crate(ty_cx, &method_map, crate)); + time(time_passes, ~"effect checking", || + middle::effect::check_crate(ty_cx, method_map, crate)); + time(time_passes, ~"loop checking", || middle::check_loop::check_crate(ty_cx, crate)); diff --git a/src/librustc/middle/effect.rs b/src/librustc/middle/effect.rs new file mode 100644 index 00000000000..d2f0a5580ef --- /dev/null +++ b/src/librustc/middle/effect.rs @@ -0,0 +1,154 @@ +// Copyright 2012-2013 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +//! Enforces the Rust effect system. Currently there is just one effect, +/// `unsafe`. + +use middle::ty::{ty_bare_fn, ty_closure, ty_ptr}; +use middle::ty; +use middle::typeck::method_map; +use util::ppaux; + +use syntax::ast::{deref, expr_call, expr_inline_asm, expr_method_call}; +use syntax::ast::{expr_unary, node_id, unsafe_blk, unsafe_fn}; +use syntax::ast; +use syntax::codemap::span; +use syntax::visit::{fk_item_fn, fk_method}; +use syntax::visit; + +#[deriving(Eq)] +enum UnsafeContext { + SafeContext, + UnsafeFn, + UnsafeBlock(node_id), +} + +struct Context { + /// The method map. + method_map: method_map, + /// Whether we're in an unsafe context. + unsafe_context: UnsafeContext, +} + +fn type_is_unsafe_function(ty: ty::t) -> bool { + match ty::get(ty).sty { + ty_bare_fn(ref f) => f.purity == unsafe_fn, + ty_closure(ref f) => f.purity == unsafe_fn, + _ => false, + } +} + +pub fn check_crate(tcx: ty::ctxt, + method_map: method_map, + crate: @ast::crate) { + let context = @mut Context { + method_map: method_map, + unsafe_context: SafeContext, + }; + + let require_unsafe: @fn(span: span, + description: &str) = |span, description| { + match context.unsafe_context { + SafeContext => { + // Report an error. + tcx.sess.span_err(span, + fmt!("%s requires unsafe function or block", + description)) + } + UnsafeBlock(block_id) => { + // OK, but record this. + debug!("effect: recording unsafe block as used: %?", block_id); + let _ = tcx.used_unsafe.insert(block_id); + } + UnsafeFn => {} + } + }; + + let visitor = visit::mk_vt(@visit::Visitor { + visit_fn: |fn_kind, fn_decl, block, span, node_id, _, visitor| { + let is_unsafe_fn = match *fn_kind { + fk_item_fn(_, _, purity, _) => purity == unsafe_fn, + fk_method(_, _, method) => method.purity == unsafe_fn, + _ => false, + }; + + let old_unsafe_context = context.unsafe_context; + if is_unsafe_fn { + context.unsafe_context = UnsafeFn + } + + visit::visit_fn(fn_kind, + fn_decl, + block, + span, + node_id, + (), + visitor); + + context.unsafe_context = old_unsafe_context + }, + + visit_block: |block, _, visitor| { + let old_unsafe_context = context.unsafe_context; + if block.node.rules == unsafe_blk { + context.unsafe_context = UnsafeBlock(block.node.id) + } + + visit::visit_block(block, (), visitor); + + context.unsafe_context = old_unsafe_context + }, + + visit_expr: |expr, _, visitor| { + match expr.node { + expr_method_call(*) => { + let base_type = ty::node_id_to_type(tcx, expr.callee_id); + debug!("effect: method call case, base type is %s", + ppaux::ty_to_str(tcx, base_type)); + if type_is_unsafe_function(base_type) { + require_unsafe(expr.span, + "invocation of unsafe method") + } + } + expr_call(base, _, _) => { + let base_type = ty::node_id_to_type(tcx, base.id); + debug!("effect: call case, base type is %s", + ppaux::ty_to_str(tcx, base_type)); + if type_is_unsafe_function(base_type) { + require_unsafe(expr.span, "call to unsafe function") + } + } + expr_unary(deref, base) => { + let base_type = ty::node_id_to_type(tcx, base.id); + debug!("effect: unary case, base type is %s", + ppaux::ty_to_str(tcx, base_type)); + match ty::get(base_type).sty { + ty_ptr(_) => { + require_unsafe(expr.span, + "dereference of unsafe pointer") + } + _ => {} + } + } + expr_inline_asm(*) => { + require_unsafe(expr.span, "use of inline assembly") + } + _ => {} + } + + visit::visit_expr(expr, (), visitor) + }, + + .. *visit::default_visitor() + }); + + visit::visit_crate(crate, (), visitor) +} + diff --git a/src/librustc/middle/typeck/check/mod.rs b/src/librustc/middle/typeck/check/mod.rs index e700b8760fd..f25d451363b 100644 --- a/src/librustc/middle/typeck/check/mod.rs +++ b/src/librustc/middle/typeck/check/mod.rs @@ -891,21 +891,6 @@ pub impl FnCtxt { infer::mk_subr(self.infcx(), a_is_expected, span, sub, sup) } - fn require_unsafe(&self, sp: span, op: ~str) { - match self.ps.purity { - ast::unsafe_fn => { - // ok, but flag that we used the source of unsafeness - debug!("flagging %? as a used unsafe source", self.ps); - self.tcx().used_unsafe.insert(self.ps.def); - } - _ => { - self.ccx.tcx.sess.span_err( - sp, - fmt!("%s requires unsafe function or block", op)); - } - } - } - fn with_region_lb(@mut self, lb: ast::node_id, f: &fn() -> R) -> R { let old_region_lb = self.region_lb; self.region_lb = lb; @@ -2285,16 +2270,6 @@ pub fn check_expr_with_unifier(fcx: @mut FnCtxt, } ast::deref => { let sty = structure_of(fcx, expr.span, oprnd_t); - match sty { - // deref'ing an unsafe pointer requires that we be in - // an unsafe context - ty::ty_ptr(*) => { - fcx.require_unsafe( - expr.span, - ~"dereference of unsafe pointer"); - } - _ => { /*ok*/ } - } let operand_ty = ty::deref_sty(tcx, &sty, true); match operand_ty { Some(mt) => { @@ -2392,8 +2367,6 @@ pub fn check_expr_with_unifier(fcx: @mut FnCtxt, fcx.write_ty(id, ty_param_bounds_and_ty.ty); } ast::expr_inline_asm(ref ia) => { - fcx.require_unsafe(expr.span, ~"use of inline assembly"); - for ia.inputs.each |&(_, in)| { check_expr(fcx, in); } @@ -3223,13 +3196,6 @@ pub fn ty_param_bounds_and_ty_for_def(fcx: @mut FnCtxt, }; } - ast::def_fn(id, ast::unsafe_fn) | - ast::def_static_method(id, _, ast::unsafe_fn) => { - // Unsafe functions can only be touched in an unsafe context - fcx.require_unsafe(sp, ~"access to unsafe function"); - return ty::lookup_item_type(fcx.ccx.tcx, id); - } - ast::def_fn(id, _) | ast::def_static_method(id, _, _) | ast::def_const(id) | ast::def_variant(_, id) | ast::def_struct(id) => { diff --git a/src/librustc/rustc.rc b/src/librustc/rustc.rc index bdab72c4d73..019ca2a0ed5 100644 --- a/src/librustc/rustc.rc +++ b/src/librustc/rustc.rc @@ -109,6 +109,7 @@ pub mod middle { pub mod privacy; pub mod moves; pub mod entry; + pub mod effect; } pub mod front { diff --git a/src/libstd/comm.rs b/src/libstd/comm.rs index 59eb915c239..adc2c21580b 100644 --- a/src/libstd/comm.rs +++ b/src/libstd/comm.rs @@ -236,20 +236,24 @@ impl SharedChan { impl GenericChan for SharedChan { fn send(&self, x: T) { - let mut xx = Some(x); - do self.ch.with_imm |chan| { - let x = replace(&mut xx, None); - chan.send(x.unwrap()) + unsafe { + let mut xx = Some(x); + do self.ch.with_imm |chan| { + let x = replace(&mut xx, None); + chan.send(x.unwrap()) + } } } } impl GenericSmartChan for SharedChan { fn try_send(&self, x: T) -> bool { - let mut xx = Some(x); - do self.ch.with_imm |chan| { - let x = replace(&mut xx, None); - chan.try_send(x.unwrap()) + unsafe { + let mut xx = Some(x); + do self.ch.with_imm |chan| { + let x = replace(&mut xx, None); + chan.try_send(x.unwrap()) + } } } } diff --git a/src/libstd/os.rs b/src/libstd/os.rs index 15c68efc7cc..9c8c2dbe4d7 100644 --- a/src/libstd/os.rs +++ b/src/libstd/os.rs @@ -861,20 +861,18 @@ pub fn change_dir_locked(p: &Path, action: &fn()) -> bool { fn key(_: Exclusive<()>) { } - let result = unsafe { - global_data_clone_create(key, || { - ~exclusive(()) - }) - }; + unsafe { + let result = global_data_clone_create(key, || { ~exclusive(()) }); - do result.with_imm() |_| { - let old_dir = os::getcwd(); - if change_dir(p) { - action(); - change_dir(&old_dir) - } - else { - false + do result.with_imm() |_| { + let old_dir = os::getcwd(); + if change_dir(p) { + action(); + change_dir(&old_dir) + } + else { + false + } } } } diff --git a/src/libstd/rt/message_queue.rs b/src/libstd/rt/message_queue.rs index eaab9288ac8..fb1a5334646 100644 --- a/src/libstd/rt/message_queue.rs +++ b/src/libstd/rt/message_queue.rs @@ -29,16 +29,20 @@ impl MessageQueue { } pub fn push(&mut self, value: T) { - let value = Cell(value); - self.queue.with(|q| q.push(value.take()) ); + unsafe { + let value = Cell(value); + self.queue.with(|q| q.push(value.take()) ); + } } pub fn pop(&mut self) -> Option { - do self.queue.with |q| { - if !q.is_empty() { - Some(q.shift()) - } else { - None + unsafe { + do self.queue.with |q| { + if !q.is_empty() { + Some(q.shift()) + } else { + None + } } } } diff --git a/src/libstd/rt/work_queue.rs b/src/libstd/rt/work_queue.rs index e9eb663392b..4671a45aaea 100644 --- a/src/libstd/rt/work_queue.rs +++ b/src/libstd/rt/work_queue.rs @@ -29,32 +29,40 @@ pub impl WorkQueue { } fn push(&mut self, value: T) { - let value = Cell(value); - self.queue.with(|q| q.unshift(value.take()) ); + unsafe { + let value = Cell(value); + self.queue.with(|q| q.unshift(value.take()) ); + } } fn pop(&mut self) -> Option { - do self.queue.with |q| { - if !q.is_empty() { - Some(q.shift()) - } else { - None + unsafe { + do self.queue.with |q| { + if !q.is_empty() { + Some(q.shift()) + } else { + None + } } } } fn steal(&mut self) -> Option { - do self.queue.with |q| { - if !q.is_empty() { - Some(q.pop()) - } else { - None + unsafe { + do self.queue.with |q| { + if !q.is_empty() { + Some(q.pop()) + } else { + None + } } } } fn is_empty(&self) -> bool { - self.queue.with_imm(|q| q.is_empty() ) + unsafe { + self.queue.with_imm(|q| q.is_empty() ) + } } } diff --git a/src/libstd/task/spawn.rs b/src/libstd/task/spawn.rs index 5941221821a..56b1fb43ff1 100644 --- a/src/libstd/task/spawn.rs +++ b/src/libstd/task/spawn.rs @@ -159,13 +159,17 @@ struct AncestorList(Option>); // Accessors for taskgroup arcs and ancestor arcs that wrap the unsafety. #[inline(always)] fn access_group(x: &TaskGroupArc, blk: &fn(TaskGroupInner) -> U) -> U { - x.with(blk) + unsafe { + x.with(blk) + } } #[inline(always)] fn access_ancestors(x: &Exclusive, blk: &fn(x: &mut AncestorNode) -> U) -> U { - x.with(blk) + unsafe { + x.with(blk) + } } // Iterates over an ancestor list. diff --git a/src/test/compile-fail/foreign-unsafe-fn-called.rs b/src/test/compile-fail/foreign-unsafe-fn-called.rs index ed8b8088ee4..2f5258aa7f2 100644 --- a/src/test/compile-fail/foreign-unsafe-fn-called.rs +++ b/src/test/compile-fail/foreign-unsafe-fn-called.rs @@ -19,5 +19,5 @@ mod test { fn main() { test::free(); - //~^ ERROR access to unsafe function requires unsafe function or block + //~^ ERROR call to unsafe function requires unsafe function or block } diff --git a/src/test/compile-fail/foreign-unsafe-fn.rs b/src/test/compile-fail/foreign-unsafe-fn.rs index 3633267d02c..fce269ab517 100644 --- a/src/test/compile-fail/foreign-unsafe-fn.rs +++ b/src/test/compile-fail/foreign-unsafe-fn.rs @@ -19,5 +19,5 @@ mod test { fn main() { let x = test::free; - //~^ ERROR access to unsafe function requires unsafe function or block + //~^ ERROR call to unsafe function requires unsafe function or block } diff --git a/src/test/compile-fail/forget-init-unsafe.rs b/src/test/compile-fail/forget-init-unsafe.rs index 25ab28b5fc0..9ad7a178fb1 100644 --- a/src/test/compile-fail/forget-init-unsafe.rs +++ b/src/test/compile-fail/forget-init-unsafe.rs @@ -12,6 +12,6 @@ use std::unstable::intrinsics::{init, forget}; // Test that the `forget` and `init` intrinsics are really unsafe pub fn main() { - let stuff = init::(); //~ ERROR access to unsafe function requires unsafe - forget(stuff); //~ ERROR access to unsafe function requires unsafe -} \ No newline at end of file + let stuff = init::(); //~ ERROR call to unsafe function requires unsafe + forget(stuff); //~ ERROR call to unsafe function requires unsafe +} diff --git a/src/test/compile-fail/unsafe-fn-called-from-safe.rs b/src/test/compile-fail/unsafe-fn-called-from-safe.rs index 864dffea8bf..2ea0f5a4ec9 100644 --- a/src/test/compile-fail/unsafe-fn-called-from-safe.rs +++ b/src/test/compile-fail/unsafe-fn-called-from-safe.rs @@ -13,5 +13,5 @@ unsafe fn f() { return; } fn main() { - f(); //~ ERROR access to unsafe function requires unsafe function or block + f(); //~ ERROR call to unsafe function requires unsafe function or block } diff --git a/src/test/compile-fail/unsafe-fn-used-as-value.rs b/src/test/compile-fail/unsafe-fn-used-as-value.rs index cbfb60f0a32..b5565b4821b 100644 --- a/src/test/compile-fail/unsafe-fn-used-as-value.rs +++ b/src/test/compile-fail/unsafe-fn-used-as-value.rs @@ -13,6 +13,6 @@ unsafe fn f() { return; } fn main() { - let x = f; //~ ERROR access to unsafe function requires unsafe function or block - x(); + let x = f; + x(); //~ ERROR call to unsafe function requires unsafe function or block }