From b0f6c29b4f10bafa59723714161a393a204f9c13 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Wed, 9 Oct 2013 21:16:51 -0700 Subject: [PATCH 1/2] Use the result of privacy for reachability This fixes a bug in which the visibility rules were approximated by reachability, but forgot to cover the case where a 'pub use' reexports a private item. This fixes the commit by instead using the results of the privacy pass of the compiler to create the initial working set of the reachability pass. This may have the side effect of increasing the size of metadata, but it's difficult to avoid for correctness purposes sadly. Closes #9790 --- src/librustc/driver/driver.rs | 10 +- src/librustc/middle/privacy.rs | 13 +- src/librustc/middle/reachable.rs | 188 +++++------------- src/librustc/middle/trans/base.rs | 1 - .../auxiliary/reexport-should-still-link.rs | 15 ++ .../run-pass/reexport-should-still-link.rs | 18 ++ 6 files changed, 98 insertions(+), 147 deletions(-) create mode 100644 src/test/auxiliary/reexport-should-still-link.rs create mode 100644 src/test/run-pass/reexport-should-still-link.rs diff --git a/src/librustc/driver/driver.rs b/src/librustc/driver/driver.rs index 4b6d2ea50801a..0c896007fc3ba 100644 --- a/src/librustc/driver/driver.rs +++ b/src/librustc/driver/driver.rs @@ -263,9 +263,10 @@ pub fn phase_3_run_analysis_passes(sess: Session, method_map, ty_cx)); let maps = (external_exports, last_private_map); - time(time_passes, "privacy checking", maps, |(a, b)| - middle::privacy::check_crate(ty_cx, &method_map, &exp_map2, - a, b, crate)); + let exported_items = + time(time_passes, "privacy checking", maps, |(a, b)| + middle::privacy::check_crate(ty_cx, &method_map, &exp_map2, + a, b, crate)); time(time_passes, "effect checking", (), |_| middle::effect::check_crate(ty_cx, method_map, crate)); @@ -300,7 +301,8 @@ pub fn phase_3_run_analysis_passes(sess: Session, let reachable_map = time(time_passes, "reachability checking", (), |_| - reachable::find_reachable(ty_cx, method_map, crate)); + reachable::find_reachable(ty_cx, method_map, exp_map2, + &exported_items)); time(time_passes, "lint checking", (), |_| lint::check_crate(ty_cx, crate)); diff --git a/src/librustc/middle/privacy.rs b/src/librustc/middle/privacy.rs index fb4b76c7c916e..6f52ba5db4e10 100644 --- a/src/librustc/middle/privacy.rs +++ b/src/librustc/middle/privacy.rs @@ -31,6 +31,10 @@ use syntax::visit::Visitor; type Context<'self> = (&'self method_map, &'self resolve::ExportMap2); +// A set of all nodes in the ast which can be considered "publicly exported" in +// the sense that they are accessible from anywhere in any hierarchy. +pub type ExportedItems = HashSet; + // This visitor is used to determine the parent of all nodes in question when it // comes to privacy. This is used to determine later on if a usage is actually // valid or not. @@ -137,7 +141,7 @@ impl<'self> Visitor<()> for ParentVisitor<'self> { // This visitor is used to determine which items of the ast are embargoed, // otherwise known as not exported. struct EmbargoVisitor<'self> { - exported_items: &'self mut HashSet, + exported_items: &'self mut ExportedItems, exp_map2: &'self resolve::ExportMap2, path_all_public: bool, } @@ -239,7 +243,7 @@ struct PrivacyVisitor<'self> { curitem: ast::NodeId, // Results of previous analyses necessary for privacy checking. - exported_items: &'self HashSet, + exported_items: &'self ExportedItems, method_map: &'self method_map, parents: &'self HashMap, external_exports: resolve::ExternalExports, @@ -785,7 +789,7 @@ pub fn check_crate(tcx: ty::ctxt, exp_map2: &resolve::ExportMap2, external_exports: resolve::ExternalExports, last_private_map: resolve::LastPrivateMap, - crate: &ast::Crate) { + crate: &ast::Crate) -> ExportedItems { let mut parents = HashMap::new(); let mut exported_items = HashSet::new(); @@ -802,7 +806,7 @@ pub fn check_crate(tcx: ty::ctxt, { // Initialize the exported items with resolve's id for the "root crate" // to resolve references to `super` leading to the root and such. - exported_items.insert(0); + exported_items.insert(ast::CRATE_NODE_ID); let mut visitor = EmbargoVisitor { exported_items: &mut exported_items, exp_map2: exp_map2, @@ -824,4 +828,5 @@ pub fn check_crate(tcx: ty::ctxt, }; visit::walk_crate(&mut visitor, crate, ()); } + return exported_items; } diff --git a/src/librustc/middle/reachable.rs b/src/librustc/middle/reachable.rs index 796855434505c..3c78c86d43603 100644 --- a/src/librustc/middle/reachable.rs +++ b/src/librustc/middle/reachable.rs @@ -17,11 +17,13 @@ use middle::ty; use middle::typeck; +use middle::privacy; +use middle::resolve; use std::hashmap::HashSet; use syntax::ast::*; use syntax::ast_map; -use syntax::ast_util::def_id_of_def; +use syntax::ast_util::{def_id_of_def, is_local}; use syntax::attr; use syntax::parse::token; use syntax::visit::Visitor; @@ -71,15 +73,6 @@ fn trait_method_might_be_inlined(trait_method: &trait_method) -> bool { } } -// The context we're in. If we're in a public context, then public symbols are -// marked reachable. If we're in a private context, then only trait -// implementations are marked reachable. -#[deriving(Clone, Eq)] -enum PrivacyContext { - PublicContext, - PrivateContext, -} - // Information needed while computing reachability. struct ReachableContext { // The type context. @@ -92,108 +85,8 @@ struct ReachableContext { // A worklist of item IDs. Each item ID in this worklist will be inlined // and will be scanned for further references. worklist: @mut ~[NodeId], -} - -struct ReachableVisitor { - reachable_symbols: @mut HashSet, - worklist: @mut ~[NodeId], -} - -impl Visitor for ReachableVisitor { - - fn visit_item(&mut self, item:@item, privacy_context:PrivacyContext) { - - match item.node { - item_fn(*) => { - if privacy_context == PublicContext { - self.reachable_symbols.insert(item.id); - } - if item_might_be_inlined(item) { - self.worklist.push(item.id) - } - } - item_struct(ref struct_def, _) => { - match struct_def.ctor_id { - Some(ctor_id) if - privacy_context == PublicContext => { - self.reachable_symbols.insert(ctor_id); - } - Some(_) | None => {} - } - } - item_enum(ref enum_def, _) => { - if privacy_context == PublicContext { - for variant in enum_def.variants.iter() { - self.reachable_symbols.insert(variant.node.id); - } - } - } - item_impl(ref generics, ref trait_ref, _, ref methods) => { - // XXX(pcwalton): We conservatively assume any methods - // on a trait implementation are reachable, when this - // is not the case. We could be more precise by only - // treating implementations of reachable or cross- - // crate traits as reachable. - - let should_be_considered_public = |method: @method| { - (method.vis == public && - privacy_context == PublicContext) || - trait_ref.is_some() - }; - - // Mark all public methods as reachable. - for &method in methods.iter() { - if should_be_considered_public(method) { - self.reachable_symbols.insert(method.id); - } - } - - if generics_require_inlining(generics) { - // If the impl itself has generics, add all public - // symbols to the worklist. - for &method in methods.iter() { - if should_be_considered_public(method) { - self.worklist.push(method.id) - } - } - } else { - // Otherwise, add only public methods that have - // generics to the worklist. - for method in methods.iter() { - let generics = &method.generics; - let attrs = &method.attrs; - if generics_require_inlining(generics) || - attributes_specify_inlining(*attrs) || - should_be_considered_public(*method) { - self.worklist.push(method.id) - } - } - } - } - item_trait(_, _, ref trait_methods) => { - // Mark all provided methods as reachable. - if privacy_context == PublicContext { - for trait_method in trait_methods.iter() { - match *trait_method { - provided(method) => { - self.reachable_symbols.insert(method.id); - self.worklist.push(method.id) - } - required(_) => {} - } - } - } - } - _ => {} - } - - if item.vis == public && privacy_context == PublicContext { - visit::walk_item(self, item, PublicContext) - } else { - visit::walk_item(self, item, PrivateContext) - } - } - + // Known reexports of modules + exp_map2: resolve::ExportMap2, } struct MarkSymbolVisitor { @@ -256,31 +149,17 @@ impl Visitor<()> for MarkSymbolVisitor { impl ReachableContext { // Creates a new reachability computation context. - fn new(tcx: ty::ctxt, method_map: typeck::method_map) - -> ReachableContext { + fn new(tcx: ty::ctxt, method_map: typeck::method_map, + exp_map2: resolve::ExportMap2) -> ReachableContext { ReachableContext { tcx: tcx, method_map: method_map, reachable_symbols: @mut HashSet::new(), worklist: @mut ~[], + exp_map2: exp_map2, } } - // Step 1: Mark all public symbols, and add all public symbols that might - // be inlined to a worklist. - fn mark_public_symbols(&self, crate: &Crate) { - let reachable_symbols = self.reachable_symbols; - let worklist = self.worklist; - - let mut visitor = ReachableVisitor { - reachable_symbols: reachable_symbols, - worklist: worklist, - }; - - - visit::walk_crate(&mut visitor, crate, PublicContext); - } - // Returns true if the given def ID represents a local item that is // eligible for inlining and false otherwise. fn def_id_represents_local_inlined_item(tcx: ty::ctxt, def_id: DefId) @@ -352,6 +231,19 @@ impl ReachableContext { } } + fn propagate_mod(&self, id: NodeId) { + match self.exp_map2.find(&id) { + Some(l) => { + for reexport in l.iter() { + if reexport.reexport && is_local(reexport.def_id) { + self.worklist.push(reexport.def_id.node); + } + } + } + None => {} + } + } + // Step 2: Mark all symbols that the symbols on the worklist touch. fn propagate(&self) { let mut visitor = self.init_visitor(); @@ -373,6 +265,18 @@ impl ReachableContext { item_fn(_, _, _, _, ref search_block) => { 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 + // we won't find them in the ast_map, so this is where + // we deal with publicly re-exported items instead. + item_mod(*) => { self.propagate_mod(item.id); } + // These are normal, nothing reachable about these + // inherently and their children are already in the + // worklist + item_struct(*) | item_impl(*) | item_static(*) | + item_enum(*) | item_ty(*) | item_trait(*) | + item_foreign_mod(*) => {} _ => { self.tcx.sess.span_bug(item.span, "found non-function item \ @@ -382,10 +286,8 @@ impl ReachableContext { } Some(&ast_map::node_trait_method(trait_method, _, _)) => { match *trait_method { - required(ref ty_method) => { - self.tcx.sess.span_bug(ty_method.span, - "found required method in \ - worklist?!") + required(*) => { + // Keep going, nothing to get exported } provided(ref method) => { visit::walk_block(&mut visitor, &method.body, ()) @@ -395,6 +297,10 @@ impl ReachableContext { Some(&ast_map::node_method(ref method, _, _)) => { visit::walk_block(&mut visitor, &method.body, ()) } + // Nothing to recurse on for these + Some(&ast_map::node_foreign_item(*)) | + Some(&ast_map::node_variant(*)) | + Some(&ast_map::node_struct_ctor(*)) => {} Some(_) => { let ident_interner = token::get_ident_interner(); let desc = ast_map::node_id_to_str(self.tcx.items, @@ -404,6 +310,9 @@ impl ReachableContext { worklist: {}", desc)) } + None if search_item == CRATE_NODE_ID => { + self.propagate_mod(search_item); + } None => { self.tcx.sess.bug(format!("found unmapped ID in worklist: \ {}", @@ -429,7 +338,8 @@ impl ReachableContext { pub fn find_reachable(tcx: ty::ctxt, method_map: typeck::method_map, - crate: &Crate) + 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`, @@ -442,11 +352,13 @@ pub fn find_reachable(tcx: ty::ctxt, // 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); + let reachable_context = ReachableContext::new(tcx, method_map, exp_map2); - // Step 1: Mark all public symbols, and add all public symbols that might - // be inlined to a worklist. - reachable_context.mark_public_symbols(crate); + // Step 1: Seed the worklist with all nodes which were found to be public as + // a result of the privacy pass + for &id in exported_items.iter() { + reachable_context.worklist.push(id); + } // Step 2: Mark all symbols that the symbols on the worklist touch. reachable_context.propagate(); diff --git a/src/librustc/middle/trans/base.rs b/src/librustc/middle/trans/base.rs index 025d1a18bd7e5..d0085afb3a7a2 100644 --- a/src/librustc/middle/trans/base.rs +++ b/src/librustc/middle/trans/base.rs @@ -2535,7 +2535,6 @@ pub fn get_item_val(ccx: @mut CrateContext, id: ast::NodeId) -> ValueRef { let (v, inlineable) = consts::const_expr(ccx, expr); ccx.const_values.insert(id, v); let mut inlineable = inlineable; - exprt = true; unsafe { let llty = llvm::LLVMTypeOf(v); diff --git a/src/test/auxiliary/reexport-should-still-link.rs b/src/test/auxiliary/reexport-should-still-link.rs new file mode 100644 index 0000000000000..9d5464e652651 --- /dev/null +++ b/src/test/auxiliary/reexport-should-still-link.rs @@ -0,0 +1,15 @@ +// 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. + +pub use foo::bar; + +mod foo { + pub fn bar() {} +} diff --git a/src/test/run-pass/reexport-should-still-link.rs b/src/test/run-pass/reexport-should-still-link.rs new file mode 100644 index 0000000000000..ed5c3941c3657 --- /dev/null +++ b/src/test/run-pass/reexport-should-still-link.rs @@ -0,0 +1,18 @@ +// 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:reexport-should-still-link.rs +// xfail-fast windows doesn't like extern mod + +extern mod foo(name = "reexport-should-still-link"); + +fn main() { + foo::bar(); +} From caf7b678dd2f07918b47120aa73a1bca51d12da1 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Thu, 10 Oct 2013 06:00:51 -0700 Subject: [PATCH 2/2] Add `pub` to all the codegen tests Otherwise the test function is internalized and LLVM will most likely optimize it out. --- src/test/codegen/iterate-over-array.rs | 2 +- src/test/codegen/scalar-function-call.rs | 2 +- src/test/codegen/single-return-value.rs | 2 +- src/test/codegen/small-dense-int-switch.rs | 2 +- src/test/codegen/stack-alloc-string-slice.rs | 2 +- src/test/codegen/static-method-call-multi.rs | 10 +++++----- src/test/codegen/static-method-call.rs | 2 +- src/test/codegen/virtual-method-call-struct-return.rs | 2 +- src/test/codegen/virtual-method-call.rs | 2 +- 9 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/test/codegen/iterate-over-array.rs b/src/test/codegen/iterate-over-array.rs index e3bd7f5521628..b2cbd8821e465 100644 --- a/src/test/codegen/iterate-over-array.rs +++ b/src/test/codegen/iterate-over-array.rs @@ -9,7 +9,7 @@ // except according to those terms. #[no_mangle] -fn test(x: &[int]) -> int { +pub fn test(x: &[int]) -> int { let mut y = 0; let mut i = 0; while (i < x.len()) { diff --git a/src/test/codegen/scalar-function-call.rs b/src/test/codegen/scalar-function-call.rs index ca30fc2bd7b86..b95d6b03288ea 100644 --- a/src/test/codegen/scalar-function-call.rs +++ b/src/test/codegen/scalar-function-call.rs @@ -13,6 +13,6 @@ fn foo(x: int) -> int { } #[no_mangle] -fn test() { +pub fn test() { let _x = foo(10); } diff --git a/src/test/codegen/single-return-value.rs b/src/test/codegen/single-return-value.rs index e6eb9a2be72cb..948809a632679 100644 --- a/src/test/codegen/single-return-value.rs +++ b/src/test/codegen/single-return-value.rs @@ -9,6 +9,6 @@ // except according to those terms. #[no_mangle] -fn test() -> int { +pub fn test() -> int { 5 } diff --git a/src/test/codegen/small-dense-int-switch.rs b/src/test/codegen/small-dense-int-switch.rs index c6bab3da958c1..d75bc5209fd25 100644 --- a/src/test/codegen/small-dense-int-switch.rs +++ b/src/test/codegen/small-dense-int-switch.rs @@ -9,7 +9,7 @@ // except according to those terms. #[no_mangle] -fn test(x: int, y: int) -> int { +pub fn test(x: int, y: int) -> int { match x { 1 => y, 2 => y*2, diff --git a/src/test/codegen/stack-alloc-string-slice.rs b/src/test/codegen/stack-alloc-string-slice.rs index b776f5a46a721..188ee246bf32c 100644 --- a/src/test/codegen/stack-alloc-string-slice.rs +++ b/src/test/codegen/stack-alloc-string-slice.rs @@ -9,6 +9,6 @@ // except according to those terms. #[no_mangle] -fn test() { +pub fn test() { let _x = "hello"; } diff --git a/src/test/codegen/static-method-call-multi.rs b/src/test/codegen/static-method-call-multi.rs index 9cb011a49f82a..efac93692f679 100644 --- a/src/test/codegen/static-method-call-multi.rs +++ b/src/test/codegen/static-method-call-multi.rs @@ -19,10 +19,10 @@ impl Struct { } #[no_mangle] -fn test(a: &Struct, - b: &Struct, - c: &Struct, - d: &Struct, - e: &Struct) -> int { +pub fn test(a: &Struct, + b: &Struct, + c: &Struct, + d: &Struct, + e: &Struct) -> int { a.method(b.method(c.method(d.method(e.method(1))))) } diff --git a/src/test/codegen/static-method-call.rs b/src/test/codegen/static-method-call.rs index a1cf6d1dda774..79fb9d8aa2933 100644 --- a/src/test/codegen/static-method-call.rs +++ b/src/test/codegen/static-method-call.rs @@ -19,6 +19,6 @@ impl Struct { } #[no_mangle] -fn test(s: &Struct) -> int { +pub fn test(s: &Struct) -> int { s.method() } diff --git a/src/test/codegen/virtual-method-call-struct-return.rs b/src/test/codegen/virtual-method-call-struct-return.rs index 86d737118d62f..20bda755f3728 100644 --- a/src/test/codegen/virtual-method-call-struct-return.rs +++ b/src/test/codegen/virtual-method-call-struct-return.rs @@ -18,6 +18,6 @@ trait Trait { } #[no_mangle] -fn test(t: &Trait) -> int { +pub fn test(t: &Trait) -> int { t.method().a } diff --git a/src/test/codegen/virtual-method-call.rs b/src/test/codegen/virtual-method-call.rs index 66dd41984a7ce..513a299cc63b6 100644 --- a/src/test/codegen/virtual-method-call.rs +++ b/src/test/codegen/virtual-method-call.rs @@ -13,6 +13,6 @@ trait Trait { } #[no_mangle] -fn test(t: &Trait) -> int { +pub fn test(t: &Trait) -> int { t.method() }