From 681fda016979786da5d60a5c6d7fcf5c6c649c86 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Thu, 31 Oct 2013 20:47:23 -0700 Subject: [PATCH] Reduce the aggressiveness of reachability Previously, all functions called by a reachable function were considered reachable, but this is only the case if the original function was possibly inlineable (if it's type generic or #[inline]-flagged). --- src/librustc/middle/reachable.rs | 118 +++++++++++++---------- src/librustc/middle/trans/base.rs | 10 +- src/test/auxiliary/linkage-visibility.rs | 36 +++++++ src/test/run-pass/linkage-visibility.rs | 20 ++++ 4 files changed, 130 insertions(+), 54 deletions(-) create mode 100644 src/test/auxiliary/linkage-visibility.rs create mode 100644 src/test/run-pass/linkage-visibility.rs diff --git a/src/librustc/middle/reachable.rs b/src/librustc/middle/reachable.rs index f65eeee49d510..840e3081656c0 100644 --- a/src/librustc/middle/reachable.rs +++ b/src/librustc/middle/reachable.rs @@ -50,6 +50,7 @@ fn item_might_be_inlined(item: @ast::item) -> bool { } match item.node { + ast::item_impl(ref generics, _, _, _) | ast::item_fn(_, _, _, ref generics, _) => { generics_require_inlining(generics) } @@ -64,6 +65,25 @@ fn ty_method_might_be_inlined(ty_method: &ast::TypeMethod) -> bool { generics_require_inlining(&ty_method.generics) } +fn method_might_be_inlined(tcx: ty::ctxt, method: &ast::method, + impl_src: ast::DefId) -> bool { + if attributes_specify_inlining(method.attrs) || + generics_require_inlining(&method.generics) { + return true + } + if is_local(impl_src) { + match tcx.items.find(&impl_src.node) { + Some(&ast_map::node_item(item, _)) => item_might_be_inlined(item), + Some(*) | None => { + tcx.sess.span_bug(method.span, "impl did is not an item") + } + } + } else { + tcx.sess.span_bug(method.span, "found a foreign impl as a parent of a \ + local method") + } +} + // Returns true if the given trait method must be inlined because it may be // monomorphized or it was marked with `#[inline]`. fn trait_method_might_be_inlined(trait_method: &ast::trait_method) -> bool { @@ -100,50 +120,54 @@ impl Visitor<()> for MarkSymbolVisitor { fn visit_expr(&mut self, expr:@ast::Expr, _:()) { - match expr.node { - ast::ExprPath(_) => { - let def = match self.tcx.def_map.find(&expr.id) { - Some(&def) => def, - None => { - self.tcx.sess.span_bug(expr.span, - "def ID not in def map?!") - } - }; + match expr.node { + ast::ExprPath(_) => { + let def = match self.tcx.def_map.find(&expr.id) { + Some(&def) => def, + None => { + self.tcx.sess.span_bug(expr.span, + "def ID not in def map?!") + } + }; - let def_id = def_id_of_def(def); + let def_id = def_id_of_def(def); + if ReachableContext:: + def_id_represents_local_inlined_item(self.tcx, def_id) { + self.worklist.push(def_id.node) + } + self.reachable_symbols.insert(def_id.node); + } + ast::ExprMethodCall(*) => { + match self.method_map.find(&expr.id) { + Some(&typeck::method_map_entry { + origin: typeck::method_static(def_id), + _ + }) => { if ReachableContext:: - def_id_represents_local_inlined_item(self.tcx, - def_id) { - self.worklist.push(def_id.node) - } + def_id_represents_local_inlined_item( + self.tcx, + def_id) { + self.worklist.push(def_id.node) + } self.reachable_symbols.insert(def_id.node); } - ast::ExprMethodCall(*) => { - match self.method_map.find(&expr.id) { - Some(&typeck::method_map_entry { - origin: typeck::method_static(def_id), - _ - }) => { - if ReachableContext:: - def_id_represents_local_inlined_item( - self.tcx, - def_id) { - self.worklist.push(def_id.node) - } - self.reachable_symbols.insert(def_id.node); - } - Some(_) => {} - None => { - self.tcx.sess.span_bug(expr.span, - "method call expression \ + Some(_) => {} + None => { + self.tcx.sess.span_bug(expr.span, + "method call expression \ not in method map?!") - } - } } - _ => {} } + } + _ => {} + } - visit::walk_expr(self, expr, ()) + visit::walk_expr(self, expr, ()) + } + + fn visit_item(&mut self, _item: @ast::item, _: ()) { + // Do not recurse into items. These items will be added to the worklist + // and recursed into manually if necessary. } } @@ -263,8 +287,11 @@ impl ReachableContext { Some(&ast_map::node_item(item, _)) => { match item.node { ast::item_fn(_, _, _, _, ref search_block) => { - visit::walk_block(&mut visitor, search_block, ()) + if item_might_be_inlined(item) { + visit::walk_block(&mut visitor, search_block, ()) + } } + // Our recursion into modules involves looking up their // public reexports and the destinations of those // exports. Privacy will put them in the worklist, but @@ -331,8 +358,10 @@ impl ReachableContext { } } } - Some(&ast_map::node_method(ref method, _, _)) => { - visit::walk_block(&mut visitor, &method.body, ()) + Some(&ast_map::node_method(method, did, _)) => { + if method_might_be_inlined(self.tcx, method, did) { + visit::walk_block(&mut visitor, &method.body, ()) + } } // Nothing to recurse on for these Some(&ast_map::node_foreign_item(*)) | @@ -378,17 +407,6 @@ pub fn find_reachable(tcx: ty::ctxt, exp_map2: resolve::ExportMap2, exported_items: &privacy::ExportedItems) -> @mut HashSet { - // XXX(pcwalton): We only need to mark symbols that are exported. But this - // is more complicated than just looking at whether the symbol is `pub`, - // because it might be the target of a `pub use` somewhere. For now, I - // think we are fine, because you can't `pub use` something that wasn't - // exported due to the bug whereby `use` only looks through public - // modules even if you're inside the module the `use` appears in. When - // this bug is fixed, however, this code will need to be updated. Probably - // the easiest way to fix this (although a conservative overapproximation) - // is to have the name resolution pass mark all targets of a `pub use` as - // "must be reachable". - let reachable_context = ReachableContext::new(tcx, method_map, exp_map2); // Step 1: Seed the worklist with all nodes which were found to be public as diff --git a/src/librustc/middle/trans/base.rs b/src/librustc/middle/trans/base.rs index 2b7ed59b489c6..2482a7d2fcc26 100644 --- a/src/librustc/middle/trans/base.rs +++ b/src/librustc/middle/trans/base.rs @@ -2469,7 +2469,7 @@ pub fn get_item_val(ccx: @mut CrateContext, id: ast::NodeId) -> ValueRef { match val { Some(v) => v, None => { - let mut exprt = false; + let mut foreign = false; let item = ccx.tcx.items.get_copy(&id); let val = match item { ast_map::node_item(i, pth) => { @@ -2582,7 +2582,6 @@ pub fn get_item_val(ccx: @mut CrateContext, id: ast::NodeId) -> ValueRef { get_item_val()"); } ast::provided(m) => { - exprt = true; register_method(ccx, id, pth, m) } } @@ -2594,7 +2593,7 @@ pub fn get_item_val(ccx: @mut CrateContext, id: ast::NodeId) -> ValueRef { ast_map::node_foreign_item(ni, abis, _, pth) => { let ty = ty::node_id_to_type(ccx.tcx, ni.id); - exprt = true; + foreign = true; match ni.node { ast::foreign_item_fn(*) => { @@ -2688,7 +2687,10 @@ pub fn get_item_val(ccx: @mut CrateContext, id: ast::NodeId) -> ValueRef { } }; - if !exprt && !ccx.reachable.contains(&id) { + // foreign items (extern fns and extern statics) don't have internal + // linkage b/c that doesn't quite make sense. Otherwise items can + // have internal linkage if they're not reachable. + if !foreign && !ccx.reachable.contains(&id) { lib::llvm::SetLinkage(val, lib::llvm::InternalLinkage); } diff --git a/src/test/auxiliary/linkage-visibility.rs b/src/test/auxiliary/linkage-visibility.rs new file mode 100644 index 0000000000000..ab3539ebf6f54 --- /dev/null +++ b/src/test/auxiliary/linkage-visibility.rs @@ -0,0 +1,36 @@ +// Copyright 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. + +use std::unstable::dynamic_lib::DynamicLibrary; + +#[no_mangle] +pub fn foo() { bar(); } + +pub fn foo2() { + fn bar2() { + bar(); + } + bar2(); +} + +#[no_mangle] +fn bar() { } + +#[no_mangle] +fn baz() { } + +pub fn test() { + let lib = DynamicLibrary::open(None).unwrap(); + unsafe { + assert!(lib.symbol::("foo").is_ok()); + assert!(lib.symbol::("baz").is_err()); + assert!(lib.symbol::("bar").is_err()); + } +} diff --git a/src/test/run-pass/linkage-visibility.rs b/src/test/run-pass/linkage-visibility.rs new file mode 100644 index 0000000000000..dff45a2ffad94 --- /dev/null +++ b/src/test/run-pass/linkage-visibility.rs @@ -0,0 +1,20 @@ +// Copyright 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. + +// aux-build:linkage-visibility.rs +// xfail-fast windows doesn't like aux-build + +extern mod foo(name = "linkage-visibility"); + +fn main() { + foo::test(); + foo::foo2::(); + foo::foo(); +}