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.

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

Only retain no_mangle static symbols across LTO #29676

Closed
wants to merge 2 commits into from

Conversation

arcnmx
Copy link
Contributor

@arcnmx arcnmx commented Nov 7, 2015

Follow-up to #29656

r? @alexcrichton

@alexcrichton
Copy link
Member

Hm ideally this would have the same logic as the function that actually decides the symbol name which not only handles #[no_mangle] but also #[exported_name] apparently. It also handles a few things for other lang items but those are pretty safe to ignore.

@arcnmx
Copy link
Contributor Author

arcnmx commented Nov 9, 2015

Oh! For some reason I thought #[link_name = "..."] only applied to external symbols. Will take a look.

@arcnmx
Copy link
Contributor Author

arcnmx commented Nov 11, 2015

@alexcrichton I made changes to account for export_name as well as #[linkage = "external"]. Also included are changes when LTO'ing an executable as well, which would previously only consider extern fns to be reachable.

@@ -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.

@bors
Copy link
Contributor

bors commented Nov 26, 2015

☔ The latest upstream changes (presumably #30043) made this pull request unmergeable. Please resolve the merge conflicts.

@steveklabnik
Copy link
Member

What's the status of this PR?

@arcnmx
Copy link
Contributor Author

arcnmx commented Dec 31, 2015

I've been busy, but it's on my todo list once I get some space to actually be able to compile things again.

@alexcrichton
Copy link
Member

Closing due to inactivity for now, but feel free to resubmit with a rebase!

@arcnmx
Copy link
Contributor Author

arcnmx commented Jan 11, 2016

o I was going to work on it today >_>

@alexcrichton
Copy link
Member

Oh no worries! I'll take a look at #30830

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants