[libc++] Use exclude_from_explicit_instantiation instead of always_inline

[ldionne]

Summary

This commit adopts the exclude_from_explicit_instantiation attribute discussed
at [1] and reviewed in [2] in libc++ to supplant the use of always_inline
for visibility purposes.

This change means that users wanting to link together translation units built
with different versions of libc++’s headers into the same final linked image
MUST define the _LIBCPP_HIDE_FROM_ABI_PER_TU macro to 1 when building those
TUs. Doing otherwise will lead to ODR violations and ABI issues.

[1]: http://lists.llvm.org/pipermail/cfe-dev/2018-August/059024.html
[2]: https://reviews.llvm.org/D51789

[rsmith]

Makes sense to me. Is there some way you can write a test for this? (Maybe applying nm to a linked program?)

[ldionne]

In D52405#1254526, @rsmith wrote:

Makes sense to me. Is there some way you can write a test for this? (Maybe applying nm to a linked program?)

What I had in mind is that we should have a build bot that runs the test suite with/without _LIBCPP_HIDE_FROM_ABI_PER_TU. If all the current libc++ tests link, this seems to give us the most confidence that nothing is broken. Did you have something more specific in mind that you're concerned about? If so, I'm happy to write a test for it, I'd just like to understand what that is.

Also, are we good to go with https://reviews.llvm.org/D51789, which this patch requires?

[rsmith]

In D52405#1254544, @ldionne wrote:

In D52405#1254526, @rsmith wrote:

Makes sense to me. Is there some way you can write a test for this? (Maybe applying nm to a linked program?)

What I had in mind is that we should have a build bot that runs the test suite with/without _LIBCPP_HIDE_FROM_ABI_PER_TU. If all the current libc++ tests link, this seems to give us the most confidence that nothing is broken. Did you have something more specific in mind that you're concerned about? If so, I'm happy to write a test for it, I'd just like to understand what that is.

I'm happy with any testing that works :)

[ldionne]

In D52405#1254572, @rsmith wrote:

In D52405#1254544, @ldionne wrote:

In D52405#1254526, @rsmith wrote:

Makes sense to me. Is there some way you can write a test for this? (Maybe applying nm to a linked program?)

What I had in mind is that we should have a build bot that runs the test suite with/without _LIBCPP_HIDE_FROM_ABI_PER_TU. If all the current libc++ tests link, this seems to give us the most confidence that nothing is broken. Did you have something more specific in mind that you're concerned about? If so, I'm happy to write a test for it, I'd just like to understand what that is.

I'm happy with any testing that works :)

Ok. Well, if I remove __always_inline__ without using exclude_from_explicit_instantiation, I get many link errors in the libc++ test suite. So I'm confident that the fact that libc++ isn't broken is already tested, at least to some extent. Also, the well-functioning of the attribute itself is tested in the Clang patch.

Of course, it's possible that some unexpected thing is going to show up when I merge this in, as is always the case, but I don't easily see what tests I could add to reduce this risk at the moment.

[ldionne]

I'd like to merge this early in the LLVM 8 release cycle to give us plenty of time to catch potential problems with this change. @EricWF, can you take a look?

[EricWF]

LGTM other than the inline comments.

The chrome binary size goes from 810 MB to 756 MB after this patch.

libcxx/include/__config

802

We should use the reserved spelling __exclude_from_explicit_instantiation__.

[ldionne]

Use exclude_from_explicit_instantiation instead of non-reserved variant, per Eric's comment

[ldionne]

In D52405#1277803, @EricWF wrote:

[...]

The chrome binary size goes from 810 MB to 756 MB after this patch.

This is great!

libcxx/docs/ReleaseNotes.rst

 What's New in Libc++ 8.0.0?
 ===========================
 
 New Features
 ------------
 
 API Changes
 -----------
-
+- Starting with LLVM 8.0.0, users that wish to link together translation units
+  built with different versions of libc++’s headers into the same final linked
+  image MUST define the _LIBCPP_HIDE_FROM_ABI_PER_TU macro to 1 when building
+  those translation units. Not defining _LIBCPP_HIDE_FROM_ABI_PER_TU to 1 and
+  linking translation units built with different versions of libc++’s headers
+  together may lead to ODR violations and ABI issues. On the flipside, code
+  size improvements should be expected for everyone not defining the macro.

libcxx/include/__config

 #endif
 
 #if __has_attribute(internal_linkage)
 #  define _LIBCPP_INTERNAL_LINKAGE __attribute__ ((internal_linkage))
 #else
 #  define _LIBCPP_INTERNAL_LINKAGE _LIBCPP_ALWAYS_INLINE
 #endif
 
+#if __has_attribute(exclude_from_explicit_instantiation)
+#  define _LIBCPP_EXCLUDE_FROM_EXPLICIT_INSTANTIATION __attribute__ ((exclude_from_explicit_instantiation))
+#else
+   // Try to approximate the effect of exclude_from_explicit_instantiation
+   // (which is that entities are not assumed to be provided by explicit
+   // template instantitations in the dylib) by always inlining those entities.
+#  define _LIBCPP_EXCLUDE_FROM_EXPLICIT_INSTANTIATION _LIBCPP_ALWAYS_INLINE
+#endif
+
 #ifndef _LIBCPP_HIDE_FROM_ABI_PER_TU
 #  ifndef _LIBCPP_HIDE_FROM_ABI_PER_TU_BY_DEFAULT
 #    define _LIBCPP_HIDE_FROM_ABI_PER_TU 0
 #  else
 #    define _LIBCPP_HIDE_FROM_ABI_PER_TU 1
 #  endif
 #endif
 
 #ifndef _LIBCPP_HIDE_FROM_ABI
 #  if _LIBCPP_HIDE_FROM_ABI_PER_TU
 #    define _LIBCPP_HIDE_FROM_ABI _LIBCPP_HIDDEN _LIBCPP_INTERNAL_LINKAGE
 #  else
-#    define _LIBCPP_HIDE_FROM_ABI _LIBCPP_HIDDEN _LIBCPP_ALWAYS_INLINE
+#    define _LIBCPP_HIDE_FROM_ABI _LIBCPP_HIDDEN _LIBCPP_EXCLUDE_FROM_EXPLICIT_INSTANTIATION
 #  endif
 #endif
 
 #ifdef _LIBCPP_BUILDING_LIBRARY
 #  if _LIBCPP_ABI_VERSION > 1
 #    define _LIBCPP_HIDE_FROM_ABI_AFTER_V1 _LIBCPP_HIDE_FROM_ABI
 #  else
 #    define _LIBCPP_HIDE_FROM_ABI_AFTER_V1