Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Only retain no_mangle static symbols across LTO #29676

Closed
wants to merge 2 commits into from
Closed

Only retain no_mangle static symbols across LTO #29676

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c59d89f
Only retain no_mangle static symbols across LTO
arcnmx Nov 7, 2015
70b49d8
Rework the extern detection
arcnmx Nov 11, 2015
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
10 changes: 2 additions & 8 deletions src/librustc/metadata/csearch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,9 +344,9 @@ pub fn is_const_fn(cstore: &cstore::CStore, did: DefId) -> bool {
decoder::is_const_fn(&*cdata, did.index)
}

pub fn is_static(cstore: &cstore::CStore, did: DefId) -> bool {
pub fn is_extern_item(cstore: &cstore::CStore, did: DefId, tcx: &ty::ctxt) -> bool {
let cdata = cstore.get_crate_data(did.krate);
decoder::is_static(&*cdata, did.index)
decoder::is_extern_item(&*cdata, did.index, tcx)
}

pub fn is_impl(cstore: &cstore::CStore, did: DefId) -> bool {
Expand Down Expand Up @@ -381,12 +381,6 @@ pub fn is_default_impl(cstore: &cstore::CStore, impl_did: DefId) -> bool {
decoder::is_default_impl(&*cdata, impl_did.index)
}

pub fn is_extern_fn(cstore: &cstore::CStore, did: DefId,
tcx: &ty::ctxt) -> bool {
let cdata = cstore.get_crate_data(did.krate);
decoder::is_extern_fn(&*cdata, did.index, tcx)
}

pub fn closure_kind<'tcx>(tcx: &ty::ctxt<'tcx>, def_id: DefId) -> ty::ClosureKind {
assert!(!def_id.is_local());
let cdata = tcx.sess.cstore.get_crate_data(def_id.krate);
Expand Down
39 changes: 20 additions & 19 deletions src/librustc/metadata/decoder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1425,11 +1425,28 @@ pub fn is_const_fn(cdata: Cmd, id: DefIndex) -> bool {
}
}

pub fn is_static(cdata: Cmd, id: DefIndex) -> bool {
let item_doc = cdata.lookup_item(id);
match item_family(item_doc) {
pub fn is_extern_item(cdata: Cmd, id: DefIndex, tcx: &ty::ctxt) -> bool {
let item_doc = match cdata.get_item(id) {
Some(doc) => doc,
None => return false,
};
let applicable = match item_family(item_doc) {
ImmStatic | MutStatic => true,
Fn => {
let ty::TypeScheme { generics, ty } = get_type(cdata, id, tcx);
let no_generics = generics.types.is_empty();
match ty.sty {
ty::TyBareFn(_, fn_ty) if fn_ty.abi != abi::Rust => return no_generics,
_ => no_generics,
}
},
_ => false,
};

if applicable {
attr::contains_extern_indicator(&get_attributes(item_doc))
} else {
false
}
}

Expand Down Expand Up @@ -1549,22 +1566,6 @@ pub fn get_imported_filemaps(metadata: &[u8]) -> Vec<codemap::FileMap> {
}).collect()
}

pub fn is_extern_fn(cdata: Cmd, id: DefIndex, tcx: &ty::ctxt) -> bool {
let item_doc = match cdata.get_item(id) {
Some(doc) => doc,
None => return false,
};
if let Fn = item_family(item_doc) {
let ty::TypeScheme { generics, ty } = get_type(cdata, id, tcx);
generics.types.is_empty() && match ty.sty {
ty::TyBareFn(_, fn_ty) => fn_ty.abi != abi::Rust,
_ => false,
}
} else {
false
}
}

pub fn closure_kind(cdata: Cmd, closure_id: DefIndex) -> ty::ClosureKind {
let closure_doc = cdata.lookup_item(closure_id);
let closure_kind_doc = reader::get_doc(closure_doc, tag_items_closure_kind);
Expand Down
18 changes: 9 additions & 9 deletions src/librustc/middle/reachable.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,16 +235,16 @@ impl<'a, 'tcx> ReachableContext<'a, 'tcx> {
fn propagate_node(&mut self, node: &ast_map::Node,
search_item: ast::NodeId) {
if !self.any_library {
// If we are building an executable, then there's no need to flag
// anything as external except for `extern fn` types. These
// functions may still participate in some form of native interface,
// but all other rust-only interfaces can be private (they will not
// participate in linkage after this product is produced)
// If we are building an executable, only explicitly extern
// types need to be exported.
if let ast_map::NodeItem(item) = *node {
if let hir::ItemFn(_, _, _, abi, _, _) = item.node {
if abi != abi::Rust {
self.reachable_symbols.insert(search_item);
}
let reachable = if let hir::ItemFn(_, _, _, abi, _, _) = item.node {
abi != abi::Rust
} else {
false
};
if reachable || attr::contains_extern_indicator(&item.attrs) {
self.reachable_symbols.insert(search_item);
}
}
} else {
Expand Down
3 changes: 1 addition & 2 deletions src/librustc_trans/trans/base.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2875,8 +2875,7 @@ pub fn trans_crate<'tcx>(tcx: &ty::ctxt<'tcx>,
sess.cstore.iter_crate_data(|cnum, _| {
let syms = csearch::get_reachable_ids(&sess.cstore, cnum);
reachable_symbols.extend(syms.into_iter().filter(|did| {
csearch::is_extern_fn(&sess.cstore, *did, shared_ccx.tcx()) ||
csearch::is_static(&sess.cstore, *did)
csearch::is_extern_item(&sess.cstore, *did, shared_ccx.tcx())
}).map(|did| {
csearch::get_symbol(&sess.cstore, did)
}));
Expand Down
9 changes: 9 additions & 0 deletions src/libsyntax/attr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,15 @@ pub fn find_export_name_attr(diag: &SpanHandler, attrs: &[Attribute]) -> Option<
})
}

pub fn contains_extern_indicator(attrs: &[Attribute]) -> bool {
contains_name(attrs, "no_mangle") ||
contains_name(attrs, "export_name") ||
Copy link
Member

Choose a reason for hiding this comment

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

This may want to use find_export_name_attr instead to consolidate the logic for the detection of "export_name".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only thing is that method takes a SpanHandler since it tries to actually parse the inner value. Should I add that to this method signature?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that's fine to just add another argument.

first_attr_value_str_by_name(attrs, "linkage").map(|v| match &*v {
Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine to not check for this as even if the linkage is external it still doesn't imply that the function itself is not mangled (e.g. linkage is orthogonal to mangling)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Linkage is indeed orthogonal, but it's actually the part that truly defines reachability. Changing the symbol from its default mangled name only implies an intent to have the symbol exist outside of Rust. linkage makes it explicit! And then there's also link_section, which I should probably add a check for as well...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My definition is basically... If I have a custom linker setup (whether linker script, or compiling a library to be used by C, etc) and -C lto breaks it, that's a bug! The exception being relying on unspecified behaviour, like perhaps naming a specific mangled symbol. And all of these attributes can do that: no_mangle, export_name, linkage, link_section.

I can probably safely remove the check for the specific type of linkage (external), at least. Though I suppose, assuming my unspecified naming assumption is correct, linkage cannot make a symbol reachable on its own, and must be combined with one of the other attributes. So it could probably go then!

Copy link
Member

Choose a reason for hiding this comment

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

I think that we may want to tackle one issue at a time here rather than trying to dive into it all at once, let's separate out these linkage concerns for perhaps another PR?

"external" | "extern_weak" => true,
_ => false,
}).unwrap_or(false)
}

#[derive(Copy, Clone, PartialEq)]
pub enum InlineAttr {
None,
Expand Down