Skip to content

Don't require unsafe blocks for Share mut items #13348

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

Closed
wants to merge 3 commits into from
Closed
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
1 change: 1 addition & 0 deletions src/libgreen/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,7 @@ impl SchedPool {
///
/// This will configure the pool according to the `config` parameter, and
/// initially run `main` inside the pool of schedulers.
#[allow(unused_unsafe)] // NOTE: Remove after next snapshot (and the unsafe block)
pub fn new(config: PoolConfig) -> SchedPool {
static mut POOL_ID: AtomicUint = INIT_ATOMIC_UINT;

Expand Down
2 changes: 2 additions & 0 deletions src/liblog/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,7 @@ pub fn log(level: u32, args: &fmt::Arguments) {
/// safely
#[doc(hidden)]
#[inline(always)]
#[allow(unused_unsafe)] // NOTE: Remove after next snapshot (and the unsafe block)
pub fn log_level() -> u32 { unsafe { LOG_LEVEL } }

/// Replaces the task-local logger with the specified logger, returning the old
Expand All @@ -227,6 +228,7 @@ pub fn set_logger(logger: ~Logger:Send) -> Option<~Logger:Send> {
/// logging. This is the second layer of defense about determining whether a
/// module's log statement should be emitted or not.
#[doc(hidden)]
#[allow(unused_unsafe)] // NOTE: Remove after next snapshot (and the unsafe block)
pub fn mod_enabled(level: u32, module: &str) -> bool {
static mut INIT: Once = ONCE_INIT;
unsafe { INIT.doit(init); }
Expand Down
7 changes: 6 additions & 1 deletion src/librustc/middle/check_static.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,12 @@
//
// Rules Enforced Elsewhere:
// - It's not possible to take the address of a static item with unsafe interior. This is enforced
// by borrowck::gather_loans
// by `rustc::middle::borrowck::gather_loans`
//
// Mutable static items:
// In general, it's not allowed to access mutable static items outside an unsafe block. However,
// taking an immutable address of a mutable static items whose type implements `Share` is considered
// a safe operation. This is enforced by `rustc::middle::effect`

use middle::ty;

Expand Down
135 changes: 101 additions & 34 deletions src/librustc/middle/effect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,11 @@ use util::ppaux;

use syntax::ast;
use syntax::codemap::Span;
use syntax::print::pprust;
use syntax::visit;
use syntax::visit::Visitor;

#[deriving(Eq)]
#[deriving(Eq, Clone)]
enum UnsafeContext {
SafeContext,
UnsafeFn,
Expand All @@ -35,18 +36,26 @@ fn type_is_unsafe_function(ty: ty::t) -> bool {
}
}

#[deriving(Eq, Clone)]
struct EffectEnv {
/// Whether we're in an unsafe context.
unsafe_context: UnsafeContext,

/// Whether mut static usage should
/// be forbidden regardless.
allow_share: bool,
}

struct EffectCheckVisitor<'a> {
tcx: &'a ty::ctxt,

/// The method map.
method_map: MethodMap,
/// Whether we're in an unsafe context.
unsafe_context: UnsafeContext,
}

impl<'a> EffectCheckVisitor<'a> {
fn require_unsafe(&mut self, span: Span, description: &str) {
match self.unsafe_context {
fn require_unsafe(&mut self, span: Span, env: &EffectEnv, description: &str) {
match env.unsafe_context {
SafeContext => {
// Report an error.
self.tcx.sess.span_err(span,
Expand Down Expand Up @@ -77,11 +86,18 @@ impl<'a> EffectCheckVisitor<'a> {
_ => {}
}
}

fn expr_is_mut_static(&mut self, e: &ast::Expr) -> bool {
match self.tcx.def_map.borrow().find(&e.id) {
Some(&ast::DefStatic(_, true)) => true,
_ => false
}
}
}

impl<'a> Visitor<()> for EffectCheckVisitor<'a> {
impl<'a> Visitor<EffectEnv> for EffectCheckVisitor<'a> {
fn visit_fn(&mut self, fn_kind: &visit::FnKind, fn_decl: &ast::FnDecl,
block: &ast::Block, span: Span, node_id: ast::NodeId, _:()) {
block: &ast::Block, span: Span, node_id: ast::NodeId, env: EffectEnv) {

let (is_item_fn, is_unsafe_fn) = match *fn_kind {
visit::FkItemFn(_, _, fn_style, _) =>
Expand All @@ -91,20 +107,19 @@ impl<'a> Visitor<()> for EffectCheckVisitor<'a> {
_ => (false, false),
};

let old_unsafe_context = self.unsafe_context;
let mut env = env;
Copy link
Member

Choose a reason for hiding this comment

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

Write mut env: EffectEnv in the function signature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right (facepalm)


if is_unsafe_fn {
self.unsafe_context = UnsafeFn
env.unsafe_context = UnsafeFn;
} else if is_item_fn {
self.unsafe_context = SafeContext
env.unsafe_context = SafeContext;
}

visit::walk_fn(self, fn_kind, fn_decl, block, span, node_id, ());

self.unsafe_context = old_unsafe_context
visit::walk_fn(self, fn_kind, fn_decl, block, span, node_id, env);
}

fn visit_block(&mut self, block: &ast::Block, _:()) {
let old_unsafe_context = self.unsafe_context;
fn visit_block(&mut self, block: &ast::Block, env: EffectEnv) {
let mut env = env;
match block.rules {
ast::DefaultBlock => {}
ast::UnsafeBlock(source) => {
Expand All @@ -123,79 +138,131 @@ impl<'a> Visitor<()> for EffectCheckVisitor<'a> {
// external blocks (e.g. `unsafe { println("") }`,
// expands to `unsafe { ... unsafe { ... } }` where
// the inner one is compiler generated).
if self.unsafe_context == SafeContext || source == ast::CompilerGenerated {
self.unsafe_context = UnsafeBlock(block.id)
if env.unsafe_context == SafeContext || source == ast::CompilerGenerated {
env.unsafe_context = UnsafeBlock(block.id);
}
}
}

visit::walk_block(self, block, ());

self.unsafe_context = old_unsafe_context
visit::walk_block(self, block, env);
}

fn visit_expr(&mut self, expr: &ast::Expr, _:()) {
fn visit_expr(&mut self, expr: &ast::Expr, env: EffectEnv) {
let mut env = env;
debug!("visit_expr(expr={}, allow_share={})",
pprust::expr_to_str(expr), env.allow_share);
match expr.node {
ast::ExprMethodCall(_, _, _) => {
ast::ExprMethodCall(_, _, ref args) => {
let method_call = MethodCall::expr(expr.id);
let base_type = self.method_map.borrow().get(&method_call).ty;
debug!("effect: method call case, base type is {}",
ppaux::ty_to_str(self.tcx, base_type));
if type_is_unsafe_function(base_type) {
self.require_unsafe(expr.span,
self.require_unsafe(expr.span, &env,
"invocation of unsafe method")
}

// This is a method call, hence we just check the first
// expression in the call args which corresponds `Self`
if self.expr_is_mut_static(*args.get(0)) {
Copy link
Member

Choose a reason for hiding this comment

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

What about (some_static_mut).bar() (with the parens)?

let adj_ty = ty::expr_ty_adjusted(self.tcx, *args.get(0),
&*self.method_map.borrow());
match ty::get(adj_ty).sty {
ty::ty_rptr(_, mt) if mt.mutbl == ast::MutMutable => {
self.require_unsafe(expr.span, &env,
"mutable borrow of mutable static");
}
_ => {}
}
}

env.allow_share = true;
}
ast::ExprIndex(base, index) => {
self.visit_expr(base, env.clone());

// It is safe to access share static mut
// in index expressions.
env.allow_share = true;
return self.visit_expr(index, env);
}
ast::ExprCall(base, _) => {
let base_type = ty::node_id_to_type(self.tcx, base.id);
debug!("effect: call case, base type is {}",
ppaux::ty_to_str(self.tcx, base_type));
if type_is_unsafe_function(base_type) {
self.require_unsafe(expr.span, "call to unsafe function")
self.require_unsafe(expr.span, &env, "call to unsafe function")
}

env.allow_share = true;
}
ast::ExprUnary(ast::UnDeref, base) => {
let base_type = ty::node_id_to_type(self.tcx, base.id);
debug!("effect: unary case, base type is {}",
ppaux::ty_to_str(self.tcx, base_type));
match ty::get(base_type).sty {
ty::ty_ptr(_) => {
self.require_unsafe(expr.span,
self.require_unsafe(expr.span, &env,
"dereference of unsafe pointer")
}
_ => {}
}
}
ast::ExprAssign(base, _) | ast::ExprAssignOp(_, base, _) => {
self.check_str_index(base);
ast::ExprAssign(lhs, rhs) | ast::ExprAssignOp(_, lhs, rhs) => {
self.check_str_index(lhs);

debug!("assign(rhs={}, lhs={})",
pprust::expr_to_str(rhs),
pprust::expr_to_str(lhs))

env.allow_share = true;
self.visit_expr(rhs, env.clone());

// we want to ignore `Share` statics
Copy link
Member

Choose a reason for hiding this comment

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

What about

static mut SHARABLE: *mut int = ...;

*SOME_SHARE = 1

static mut ANOTHER_SHARE: ... = ...;

*ANOTHER_SHARE.returns_mut_ref_safely() = 1;

It's a little weird having indexing-on-the-LHS-of-assign validated by code that's well removed from the assignment code.

// *just* in the LHS of the assignment.
env.allow_share = false;
return self.visit_expr(lhs, env);
}
ast::ExprAddrOf(ast::MutImmutable, _) => {
env.allow_share = true;
}
ast::ExprAddrOf(ast::MutMutable, base) => {
if self.expr_is_mut_static(base) {
self.require_unsafe(expr.span, &env,
"mutable borrow of mutable static");
}

self.check_str_index(base);
env.allow_share = true;
}
ast::ExprInlineAsm(..) => {
self.require_unsafe(expr.span, "use of inline assembly")
self.require_unsafe(expr.span, &env, "use of inline assembly")
}
ast::ExprPath(..) => {
match ty::resolve_expr(self.tcx, expr) {
ast::DefStatic(_, true) => {
self.require_unsafe(expr.span, "use of mutable static")
if self.expr_is_mut_static(expr) {
let ety = ty::node_id_to_type(self.tcx, expr.id);
if !env.allow_share || !ty::type_is_sharable(self.tcx, ety) {
self.require_unsafe(expr.span, &env, "this use of mutable static");
}
_ => {}
}
}
_ => {}
}

visit::walk_expr(self, expr, ());
visit::walk_expr(self, expr, env);
}
}

pub fn check_crate(tcx: &ty::ctxt, method_map: MethodMap, krate: &ast::Crate) {
let mut visitor = EffectCheckVisitor {
tcx: tcx,
method_map: method_map,
};

let env = EffectEnv{
allow_share: false,
unsafe_context: SafeContext,
};

visit::walk_crate(&mut visitor, krate, ());
visit::walk_crate(&mut visitor, krate, env);
}
1 change: 1 addition & 0 deletions src/librustc/middle/trans/debuginfo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2307,6 +2307,7 @@ fn cache_id_for_type(t: ty::t) -> uint {

// Used to avoid LLVM metadata uniquing problems. See `create_struct_stub()` and
// `prepare_enum_metadata()`.
#[allow(unused_unsafe)] // NOTE: Remove after next snapshot (and the unsafe block)
fn generate_unique_type_id(prefix: &'static str) -> ~str {
unsafe {
static mut unique_id_counter: atomics::AtomicUint = atomics::INIT_ATOMIC_UINT;
Expand Down
4 changes: 4 additions & 0 deletions src/librustc/middle/ty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1989,6 +1989,10 @@ pub fn type_is_sendable(cx: &ctxt, t: ty::t) -> bool {
type_contents(cx, t).is_sendable(cx)
}

pub fn type_is_sharable(cx: &ctxt, t: ty::t) -> bool {
type_contents(cx, t).is_sharable(cx)
}

pub fn type_interior_is_unsafe(cx: &ctxt, t: ty::t) -> bool {
type_contents(cx, t).interior_unsafe()
}
Expand Down
1 change: 1 addition & 0 deletions src/libstd/io/tempfile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ impl TempDir {
/// deleted once the returned wrapper is destroyed.
///
/// If no directory can be created, None is returned.
#[allow(unused_unsafe)] // NOTE: Remove after next snapshot (and the unsafe block)
pub fn new_in(tmpdir: &Path, suffix: &str) -> Option<TempDir> {
if !tmpdir.is_absolute() {
return TempDir::new_in(&os::make_absolute(tmpdir), suffix);
Expand Down
2 changes: 2 additions & 0 deletions src/libstd/io/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ macro_rules! iotest (
)

/// Get a port number, starting at 9600, for use in tests
#[allow(unused_unsafe)] // NOTE: Remove after next snapshot (and the unsafe block)
pub fn next_test_port() -> u16 {
static mut next_offset: AtomicUint = INIT_ATOMIC_UINT;
unsafe {
Expand All @@ -62,6 +63,7 @@ pub fn next_test_port() -> u16 {
}

/// Get a temporary path which could be the location of a unix socket
#[allow(unused_unsafe)] // NOTE: Remove after next snapshot (and the unsafe block)
pub fn next_test_unix() -> Path {
static mut COUNT: AtomicUint = INIT_ATOMIC_UINT;
// base port and pid are an attempt to be unique between multiple
Expand Down
2 changes: 2 additions & 0 deletions src/libstd/os.rs
Original file line number Diff line number Diff line change
Expand Up @@ -781,12 +781,14 @@ static mut EXIT_STATUS: AtomicInt = INIT_ATOMIC_INT;
*
* Note that this is not synchronized against modifications of other threads.
*/
#[allow(unused_unsafe)] // NOTE: Remove after next snapshot (and the unsafe block)
pub fn set_exit_status(code: int) {
unsafe { EXIT_STATUS.store(code, SeqCst) }
}

/// Fetches the process's current exit code. This defaults to 0 and can change
/// by calling `set_exit_status`.
#[allow(unused_unsafe)] // NOTE: Remove after next snapshot (and the unsafe block)
pub fn get_exit_status() -> int {
unsafe { EXIT_STATUS.load(SeqCst) }
}
Expand Down
1 change: 1 addition & 0 deletions src/libstd/rt/backtrace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ pub use self::imp::write;

// For now logging is turned off by default, and this function checks to see
// whether the magical environment variable is present to see if it's turned on.
#[allow(unused_unsafe)] // NOTE: Remove after next snapshot (and the unsafe block)
pub fn log_enabled() -> bool {
static mut ENABLED: atomics::AtomicInt = atomics::INIT_ATOMIC_INT;
unsafe {
Expand Down
1 change: 1 addition & 0 deletions src/libstd/rt/bookkeeping.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ use unstable::mutex::{StaticNativeMutex, NATIVE_MUTEX_INIT};
static mut TASK_COUNT: atomics::AtomicUint = atomics::INIT_ATOMIC_UINT;
static mut TASK_LOCK: StaticNativeMutex = NATIVE_MUTEX_INIT;

#[allow(unused_unsafe)] // NOTE: Remove after next snapshot (and the unsafe block)
pub fn increment() {
let _ = unsafe { TASK_COUNT.fetch_add(1, atomics::SeqCst) };
}
Expand Down
4 changes: 4 additions & 0 deletions src/libstd/rt/env.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ static mut MIN_STACK: uint = 2 * 1024 * 1024;
static mut MAX_CACHED_STACKS: uint = 10;
static mut DEBUG_BORROW: bool = false;

#[allow(unused_unsafe)] // NOTE: Remove after next snapshot (and the unsafe block)
pub fn init() {
unsafe {
match os::getenv("RUST_MIN_STACK") {
Expand All @@ -43,14 +44,17 @@ pub fn init() {
}
}

#[allow(unused_unsafe)] // NOTE: Remove after next snapshot (and the unsafe block)
pub fn min_stack() -> uint {
unsafe { MIN_STACK }
}

#[allow(unused_unsafe)] // NOTE: Remove after next snapshot (and the unsafe block)
pub fn max_cached_stacks() -> uint {
unsafe { MAX_CACHED_STACKS }
}

#[allow(unused_unsafe)] // NOTE: Remove after next snapshot (and the unsafe block)
pub fn debug_borrow() -> bool {
unsafe { DEBUG_BORROW }
}
2 changes: 2 additions & 0 deletions src/libstd/rt/local_ptr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,7 @@ pub mod compiled {

/// Check whether there is a thread-local pointer installed.
#[inline(never)] // see comments above
#[allow(unused_unsafe)] // NOTE: Remove after next snapshot (and the unsafe block)
pub fn exists() -> bool {
unsafe {
RT_TLS_PTR.is_not_null()
Expand Down Expand Up @@ -367,6 +368,7 @@ pub mod native {
#[inline]
#[cfg(not(test))]
#[allow(visible_private_types)]
#[allow(unused_unsafe)] // NOTE: Remove after next snapshot (and the unsafe block)
pub fn maybe_tls_key() -> Option<tls::Key> {
unsafe {
// NB: This is a little racy because, while the key is
Expand Down
Loading