From 5ec118383b39f00589dc72d01a0f510c1ed0511f Mon Sep 17 00:00:00 2001 From: Huon Wilson Date: Thu, 20 Feb 2014 23:22:45 +1100 Subject: [PATCH] rustc: avoid compiler generated `unsafe` blocks leaking. Previously an `unsafe` block created by the compiler (like those in the formatting macros) would be "ignored" if surrounded by `unsafe`, that is, the internal unsafety would be being legitimised by the external block: unsafe { println!("...") } =(expansion)=> unsafe { ... unsafe { ... } } And the code in the inner block would be using the outer block, making it considered used (and the inner one considered unused). This patch forces the compiler to create a new unsafe context for compiler generated blocks, so that their internal unsafety doesn't escape to external blocks. Fixes #12418. --- src/librustc/middle/effect.rs | 27 +++++++++++++++---- src/librustc/middle/trans/base.rs | 16 +++++------ src/librustc/middle/typeck/check/mod.rs | 4 +-- src/libstd/comm/shared.rs | 16 +++++------ src/libstd/comm/stream.rs | 14 +++++----- ...unsafe-around-compiler-generated-unsafe.rs | 17 ++++++++++++ 6 files changed, 59 insertions(+), 35 deletions(-) create mode 100644 src/test/compile-fail/unsafe-around-compiler-generated-unsafe.rs diff --git a/src/librustc/middle/effect.rs b/src/librustc/middle/effect.rs index 2a40c8148fd..9f239098e81 100644 --- a/src/librustc/middle/effect.rs +++ b/src/librustc/middle/effect.rs @@ -106,11 +106,28 @@ fn visit_fn(&mut self, fn_kind: &visit::FnKind, fn_decl: &ast::FnDecl, fn visit_block(&mut self, block: &ast::Block, _:()) { let old_unsafe_context = self.unsafe_context; - let is_unsafe = match block.rules { - ast::UnsafeBlock(..) => true, ast::DefaultBlock => false - }; - if is_unsafe && self.unsafe_context == SafeContext { - self.unsafe_context = UnsafeBlock(block.id) + match block.rules { + ast::DefaultBlock => {} + ast::UnsafeBlock(source) => { + // By default only the outermost `unsafe` block is + // "used" and so nested unsafe blocks are pointless + // (the inner ones are unnecessary and we actually + // warn about them). As such, there are two cases when + // we need to create a new context, when we're + // - outside `unsafe` and found a `unsafe` block + // (normal case) + // - inside `unsafe` but found an `unsafe` block + // created internally to the compiler + // + // The second case is necessary to ensure that the + // compiler `unsafe` blocks don't accidentally "use" + // 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) + } + } } visit::walk_block(self, block, ()); diff --git a/src/librustc/middle/trans/base.rs b/src/librustc/middle/trans/base.rs index dc8e786fba2..daa58cc8ae2 100644 --- a/src/librustc/middle/trans/base.rs +++ b/src/librustc/middle/trans/base.rs @@ -924,11 +924,9 @@ pub fn invoke<'a>( } if need_invoke(bcx) { - unsafe { - debug!("invoking {} at {}", llfn, bcx.llbb); - for &llarg in llargs.iter() { - debug!("arg: {}", llarg); - } + debug!("invoking {} at {}", llfn, bcx.llbb); + for &llarg in llargs.iter() { + debug!("arg: {}", llarg); } let normal_bcx = bcx.fcx.new_temp_block("normal-return"); let landing_pad = bcx.fcx.get_landing_pad(); @@ -946,11 +944,9 @@ pub fn invoke<'a>( attributes); return (llresult, normal_bcx); } else { - unsafe { - debug!("calling {} at {}", llfn, bcx.llbb); - for &llarg in llargs.iter() { - debug!("arg: {}", llarg); - } + debug!("calling {} at {}", llfn, bcx.llbb); + for &llarg in llargs.iter() { + debug!("arg: {}", llarg); } match call_info { diff --git a/src/librustc/middle/typeck/check/mod.rs b/src/librustc/middle/typeck/check/mod.rs index 177eba5aa1b..0d6efc3a5f9 100644 --- a/src/librustc/middle/typeck/check/mod.rs +++ b/src/librustc/middle/typeck/check/mod.rs @@ -992,9 +992,7 @@ fn anon_regions(&self, span: Span, count: uint) impl FnCtxt { pub fn tag(&self) -> ~str { - unsafe { - format!("{}", self as *FnCtxt) - } + format!("{}", self as *FnCtxt) } pub fn local_ty(&self, span: Span, nid: ast::NodeId) -> ty::t { diff --git a/src/libstd/comm/shared.rs b/src/libstd/comm/shared.rs index 0884c46ee84..444f2d14dba 100644 --- a/src/libstd/comm/shared.rs +++ b/src/libstd/comm/shared.rs @@ -486,14 +486,12 @@ pub fn abort_selection(&mut self, _was_upgrade: bool) -> bool { #[unsafe_destructor] impl Drop for Packet { fn drop(&mut self) { - unsafe { - // Note that this load is not only an assert for correctness about - // disconnection, but also a proper fence before the read of - // `to_wake`, so this assert cannot be removed with also removing - // the `to_wake` assert. - assert_eq!(self.cnt.load(atomics::SeqCst), DISCONNECTED); - assert_eq!(self.to_wake.load(atomics::SeqCst), 0); - assert_eq!(self.channels.load(atomics::SeqCst), 0); - } + // Note that this load is not only an assert for correctness about + // disconnection, but also a proper fence before the read of + // `to_wake`, so this assert cannot be removed with also removing + // the `to_wake` assert. + assert_eq!(self.cnt.load(atomics::SeqCst), DISCONNECTED); + assert_eq!(self.to_wake.load(atomics::SeqCst), 0); + assert_eq!(self.channels.load(atomics::SeqCst), 0); } } diff --git a/src/libstd/comm/stream.rs b/src/libstd/comm/stream.rs index 0bacf1e1d28..4eac22b813d 100644 --- a/src/libstd/comm/stream.rs +++ b/src/libstd/comm/stream.rs @@ -471,13 +471,11 @@ pub fn abort_selection(&mut self, #[unsafe_destructor] impl Drop for Packet { fn drop(&mut self) { - unsafe { - // Note that this load is not only an assert for correctness about - // disconnection, but also a proper fence before the read of - // `to_wake`, so this assert cannot be removed with also removing - // the `to_wake` assert. - assert_eq!(self.cnt.load(atomics::SeqCst), DISCONNECTED); - assert_eq!(self.to_wake.load(atomics::SeqCst), 0); - } + // Note that this load is not only an assert for correctness about + // disconnection, but also a proper fence before the read of + // `to_wake`, so this assert cannot be removed with also removing + // the `to_wake` assert. + assert_eq!(self.cnt.load(atomics::SeqCst), DISCONNECTED); + assert_eq!(self.to_wake.load(atomics::SeqCst), 0); } } diff --git a/src/test/compile-fail/unsafe-around-compiler-generated-unsafe.rs b/src/test/compile-fail/unsafe-around-compiler-generated-unsafe.rs new file mode 100644 index 00000000000..2aefdd213bb --- /dev/null +++ b/src/test/compile-fail/unsafe-around-compiler-generated-unsafe.rs @@ -0,0 +1,17 @@ +// Copyright 2014 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. + +// issue #12418 + +#[deny(unused_unsafe)]; + +fn main() { + unsafe { println!("foo"); } //~ ERROR unnecessary `unsafe` +} -- GitLab