This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Make __num_get_float hidden
ClosedPublic

Authored by smeenai on Nov 27 2016, 3:20 PM.

Details

Summary

It's an internal function and shouldn't be exported. It's also a source
of discrepancy in the published ABI list; these symbols aren't exported
for me on CentOS 7 or Ubuntu 16.04, leading to spurious check-cxx-abilist
failures.

Diff Detail

Repository
rL LLVM

Event Timeline

smeenai updated this revision to Diff 79362.Nov 27 2016, 3:20 PM
smeenai retitled this revision from to [libc++] Make __num_get_float hidden.
smeenai updated this object.
smeenai added reviewers: EricWF, mclow.lists.
smeenai added a subscriber: cfe-commits.

I'm having some second thoughts about this. Visibility for template functions makes my head spin :/ Is there a general policy we've been following for these? I didn't find much just scanning through other definitions.

EricWF edited edge metadata.Dec 5 2016, 1:17 AM

I'm having some second thoughts about this. Visibility for template functions makes my head spin :/ Is there a general policy we've been following for these? I didn't find much just scanning through other definitions.

I would suggest adding the inline keyword so that implicit instantiation are marked hidden when -fvisibility-inlines-hidden. That should be sufficient to hide the implicit instantiations in libc++.so.

I'm having some second thoughts about this. Visibility for template functions makes my head spin :/ Is there a general policy we've been following for these? I didn't find much just scanning through other definitions.

I would suggest adding the inline keyword so that implicit instantiation are marked hidden when -fvisibility-inlines-hidden. That should be sufficient to hide the implicit instantiations in libc++.so.

I thought about that as well. It's slightly less intrusive, since it would only change the visibility for clients using -fvisibility-inlines-hidden. It also seems like a more roundabout way of going about hiding these symbols though, and we definitely don't want the compiler using the inline as a hint to actually perform inlining for this particular function (from what I understand, inline is mostly ignored as far as inlining goes, but it might have some effect on the compiler's cost models or something).

I feel like, if the goal is to have these symbols hidden (and I think they should be), annotating the visibility explicitly is a better approach than marking as inline. What are your thoughts on that?

Pinging above question.

EricWF accepted this revision.Dec 23 2016, 10:26 PM
EricWF edited edge metadata.
This revision is now accepted and ready to land.Dec 23 2016, 10:26 PM
This revision was automatically updated to reflect the committed changes.