This is an archive of the discontinued LLVM Phabricator instance.

Remove visibility attributes from out-of-class method definitions in iostreams.
ClosedPublic

Authored by eugenis on Nov 5 2015, 6:02 PM.

Details

Summary

No point in pretending that these methods are hidden - they are
actually exported from libc++.so. Extern template declarations make
them part of libc++ ABI.

This patch does not change libc++.so export list (at least on Linux).

Diff Detail

Repository
rL LLVM

Event Timeline

eugenis updated this revision to Diff 39452.Nov 5 2015, 6:02 PM
eugenis retitled this revision from to Remove visibility attributes from out-of-class method definitions in iostreams..
eugenis updated this object.
eugenis added reviewers: mclow.lists, EricWF.
eugenis set the repository for this revision to rL LLVM.
eugenis added a subscriber: cfe-commits.
eugenis updated this revision to Diff 39750.Nov 9 2015, 2:05 PM

Applied the same change to <sstream>. No idea how I missed it.

EricWF edited edge metadata.Nov 9 2015, 2:08 PM

No changes are needed in `<string>`?

<string> is an interesting case.
There are no cases of visibility attribute present on an out-of-class definition but missing on an in-class declaration, so nothing needs to be done for a switch to internal_linkage.

Setting hidden visibility on an "extern template" method is probably a bad practice. By itself, it would break everything: extern template promises that this method is available externally, and hidden visibility kinda invalidates this promise. But is libc++ it is always accompanied by another attribute which sidesteps the issue: always_inline (if inlining happends) and internal_linkage (in all cases) emit method body in the caller's translation unit.

So, nothing would break if we leave <string> unchanged. Removing the attributes would add 254 exports to libc++ and not really achieve anything. Well, it could help with the link failure on compilers that do not support internal_linkage AND not always inline always_inline - are there any like this?

Hi,

Have you had a chance to look at this?

Does the inline keyword have any effect when it's on function definitions that are externally instantiated?

Does the inline keyword have any effect when it's on function definitions that are externally instantiated?

I could not detect any difference in behavior with or without inline keyword.
Remove it?

EricWF accepted this revision.Dec 9 2015, 3:06 PM
EricWF edited edge metadata.

LGTM. I built the library before this patch and then diffed the symbols after this change. No symbols have been removed or added. LGTM.

This revision is now accepted and ready to land.Dec 9 2015, 3:06 PM

Does the inline keyword have any effect when it's on function definitions that are externally instantiated?

I could not detect any difference in behavior with or without inline keyword.
Remove it?

Actually, remove the inline breaks tests, because now the method is declared hidden (in-class), so template instantiation in libc++.so produces a hidden symbol.

As an alternative, we could remove both "inline" and the in-class hidden attribute.

Does the inline keyword have any effect when it's on function definitions that are externally instantiated?

I could not detect any difference in behavior with or without inline keyword.
Remove it?

Actually, remove the inline breaks tests, because now the method is declared hidden (in-class), so template instantiation in libc++.so produces a hidden symbol.

As an alternative, we could remove both "inline" and the in-class hidden attribute.

So, we can not remove "inline" because without it always_inline does not seem to have any effect.
We can remove always_inline from the declarations, but that would add a few more exported symbols to libc++.

I'll land this change as is, if you don't mind.

EricWF added a comment.Dec 9 2015, 3:37 PM

Does the inline keyword have any effect when it's on function definitions that are externally instantiated?

I could not detect any difference in behavior with or without inline keyword.
Remove it?

Actually, remove the inline breaks tests, because now the method is declared hidden (in-class), so template instantiation in libc++.so produces a hidden symbol.

As an alternative, we could remove both "inline" and the in-class hidden attribute.

That change would be an ABI break. This change is perfect as-is.

eugenis closed this revision.Dec 9 2015, 3:45 PM

r255177
Thanks for the review!