[libc++] Introduce _LIBCPP_HIDE_FROM_ABI to replace _LIBCPP_INLINE_VISIBILITY

[ldionne]

Summary

This commit introduces a new macro, _LIBCPP_HIDE_FROM_ABI, whose goal is to
mark functions that shouldn't be part of libc++'s ABI. It marks the functions
as being hidden for dylib visibility purposes, and as having internal linkage
using Clang's attribute((internal_linkage)) when available, and
always_inline otherwise.

It replaces _LIBCPP_INLINE_VISIBILITY, which was always using always_inline
to achieve similar goals, but suffered from debuggability and code size problems.
The full proposal, along with more background information, can be found here:

http://lists.llvm.org/pipermail/cfe-dev/2018-July/058419.html

This commit does not rename uses of _LIBCPP_INLINE_VISIBILITY to
_LIBCPP_HIDE_FROM_ABI: this wide reaching but mechanical change can
be done later when we've confirmed we're happy with the new macro.

In the future, it would be nice if we could optionally allow dropping
any internal_linkage or always_inline attribute, which could result
in code size improvements. However, this is currently impossible for
reasons explained here: http://lists.llvm.org/pipermail/cfe-dev/2018-July/058450.html

[ldionne]

libcxx/include/__config

767

Is this acceptable?

[mclow.lists]

libcxx/include/__config

764

We are (were) trying to keep separate sections for each compiler in __config

The clang section runs from line 321 - 483
The GCC section runs from line 484 - 574
The MSVC section runs from about line 575 - 606.
The IBM section runs from line 607 - 636.

I see there's a bunch of cleanup I need to do there after the 7.0 branch.

767

Depends on whether or not we know of any supported compilers that don't do this.

[ldionne]

Split the definition of _LIBCPP_ALWAYS_INLINE into compiler-specific sections
of __config, per Marshall's comment.

[ldionne]

Ping. I would like to move forward with this so I can tackle the remaining problems, i.e. _LIBCPP_EXTERN_TEMPLATE_INLINE_VISIBILITY and the ability to not have internal_linkage at all. Are we waiting on anything specific before we can move forward with this?

[dexonsmith]

FWIW this LGTM, but since Marshall already commented you'll need his sign-off as well.

[mclow.lists]

This looks good to me.

libcxx/include/__config

673–674

This is one of the bits I'll be cleaning up post-7.0 branch.

[EricWF]

LGTM. Assuming this doesn't break the ABI list checks.

I talked to @ldionne about if we wanted to land this right before 7.0, but I think we can. If we have to disable the internal linkage change, we can at least keep the new naming scheme and start transitioning to it.

libcxx/docs/DesignDocs/VisibilityMacros.rst

41

Should we re-document _LIBCPP_ALWAYS_INLINE and _LIBCPP_INTERNAL_LINKAGE?

libcxx/include/__config

762–778

Lets keep these comments in the design docs and not the headers. We want to keep our header size down.

[ldionne]

In D49240#1177382, @EricWF wrote:

LGTM. Assuming this doesn't break the ABI list checks.

I talked to @ldionne about if we wanted to land this right before 7.0, but I think we can. If we have to disable the internal linkage change, we can at least keep the new naming scheme and start transitioning to it.

I'd like to get input from @mclow.lists and @dexonsmith about whether we should move forward with this before we branch 7.0, or wait after the release. This is a wide reaching change so I want to share responsibility ;-)

[dexonsmith]

In D49240#1177386, @ldionne wrote:

In D49240#1177382, @EricWF wrote:

LGTM. Assuming this doesn't break the ABI list checks.

I talked to @ldionne about if we wanted to land this right before 7.0, but I think we can. If we have to disable the internal linkage change, we can at least keep the new naming scheme and start transitioning to it.

I'd like to get input from @mclow.lists and @dexonsmith about whether we should move forward with this before we branch 7.0, or wait after the release. This is a wide reaching change so I want to share responsibility ;-)

I think we should do it now.

[ldionne]

Note: I resolved Eric's comments before pushing.

[thakis]

When we updated out clang bundle in chromium (which includes libc++ headers), our ios simulator bots regressed debug info size by ~50% due to this commit (https://bugs.chromium.org/p/chromium/issues/detail?id=872926#c13). Is that expected?

[ldionne]

In D49240#1195125, @thakis wrote:

When we updated out clang bundle in chromium (which includes libc++ headers), our ios simulator bots regressed debug info size by ~50% due to this commit (https://bugs.chromium.org/p/chromium/issues/detail?id=872926#c13). Is that expected?

No, this is quite surprising. What happened with the size of the Release builds? We should investigate, perhaps this change exposed something in Clang's debug info generation.

[rnk]

In D49240#1195237, @ldionne wrote:

In D49240#1195125, @thakis wrote:

When we updated out clang bundle in chromium (which includes libc++ headers), our ios simulator bots regressed debug info size by ~50% due to this commit (https://bugs.chromium.org/p/chromium/issues/detail?id=872926#c13). Is that expected?

No, this is quite surprising. What happened with the size of the Release builds? We should investigate, perhaps this change exposed something in Clang's debug info generation.

It looks like the increase is entirely from more symbols in the symbol table. It's not a problem with clang's debug info. It would help a lot if ld64 de-duplicated strings in the symbol table, if that's possible.

$ bloaty after_base_unittests -- before_base_unittests 
     VM SIZE                                 FILE SIZE
 --------------                           --------------
  +103% +51.4Mi String Table              +51.4Mi  +103%
  +105% +17.6Mi Symbol Table              +17.6Mi  +105%
   +49%  +827Ki Code Signature             +827Ki   +49%
   +92%  +263Ki Function Start Addresses   +263Ki   +92%
   +15% +4.50Ki __TEXT,__unwind_info      +4.50Ki   +15%
   +74% +3.52Ki Table of Non-instructions +3.52Ki   +74%
  -0.0%    -128 __TEXT,__const               -128  -0.0%
  -2.0%    -136 [__TEXT]                     -136  -2.0%
  [ = ]       0 [Unmapped]                   -480  -3.4%
  -1.0%    -516 __TEXT,__gcc_except_tab      -516  -1.0%
 -39.3% -1.07Ki [__LINKEDIT]                   +4   +33%
  -9.8% -5.61Mi __TEXT,__text             -5.61Mi  -9.8%
   +50% +64.5Mi TOTAL                     +64.5Mi   +50%

[ldionne]

In D49240#1195723, @rnk wrote:

In D49240#1195237, @ldionne wrote:

In D49240#1195125, @thakis wrote:

When we updated out clang bundle in chromium (which includes libc++ headers), our ios simulator bots regressed debug info size by ~50% due to this commit (https://bugs.chromium.org/p/chromium/issues/detail?id=872926#c13). Is that expected?

No, this is quite surprising. What happened with the size of the Release builds? We should investigate, perhaps this change exposed something in Clang's debug info generation.

It looks like the increase is entirely from more symbols in the symbol table. It's not a problem with clang's debug info. It would help a lot if ld64 de-duplicated strings in the symbol table, if that's possible.

[...]

Ah, thanks a lot for taking a look! Yes, this makes a lot of sense, since now we're not inlining everything anymore. So the code size is actually smaller (-9.8%), but there's more symbols because more functions are emitted. In this case, I would say this is expected, if unfortunate. Also, a similar effect would probably be witnessed if Chromium were to change their standard library to libstdc++, for example, since libstdc++ does not abuse inlining like libc++ used to.

[rnk]

In D49240#1195733, @ldionne wrote:

Ah, thanks a lot for taking a look! Yes, this makes a lot of sense, since now we're not inlining everything anymore. So the code size is actually smaller (-9.8%), but there's more symbols because more functions are emitted. In this case, I would say this is expected, if unfortunate. Also, a similar effect would probably be witnessed if Chromium were to change their standard library to libstdc++, for example, since libstdc++ does not abuse inlining like libc++ used to.

I think if we used libstdc++, the situation would be much better because the inline functions would be linkonce_odr, and there would be far fewer symbols in the symbol table. We'd get code size wins from deduplicating the code, and symbol table size wins from dropping duplicate long mangled names.

[hans]

The reason we noticed this was that it caused a *50 GB* size increase of the build output on our buildbots, which was enough to cause infrastructure problems.

This change was also committed shortly before the 7.0 branch, so it's part of the 7.0.0 release candidates.

Should we back it out until a solution is found?

[ldionne]

In D49240#1196878, @hans wrote:

The reason we noticed this was that it caused a *50 GB* size increase of the build output on our buildbots, which was enough to cause infrastructure problems.

This change was also committed shortly before the 7.0 branch, so it's part of the 7.0.0 release candidates.

Should we back it out until a solution is found?

The thing is -- there's no solution without changing the guarantees that libc++ make. Today, libc++ guarantees that you can link TUs that were built with different versions of libc++ together. If we remove that guarantee, then we can use linkonce_odr and solve the problem that Chromium is having.

Is that guarantee something that Chromium is relying upon?

[hans]

In D49240#1197052, @ldionne wrote:

In D49240#1196878, @hans wrote:

The reason we noticed this was that it caused a *50 GB* size increase of the build output on our buildbots, which was enough to cause infrastructure problems.

This change was also committed shortly before the 7.0 branch, so it's part of the 7.0.0 release candidates.

Should we back it out until a solution is found?

The thing is -- there's no solution without changing the guarantees that libc++ make. Today, libc++ guarantees that you can link TUs that were built with different versions of libc++ together. If we remove that guarantee, then we can use linkonce_odr and solve the problem that Chromium is having.

Is that guarantee something that Chromium is relying upon?

I'm not sure (thakis can probably answer better), but isn't it enough to link against some system libraries that might use libc++ for this to be true?

The previous solution, using inlining, while not very elegant didn't have this giant binary size problem, so maybe it was better?

What should we put in the release notes, that folks should expect significantly larger binaries using the 7.0.0 version?

[ldionne]

In D49240#1197149, @hans wrote:

In D49240#1197052, @ldionne wrote:

In D49240#1196878, @hans wrote:

The reason we noticed this was that it caused a *50 GB* size increase of the build output on our buildbots, which was enough to cause infrastructure problems.

This change was also committed shortly before the 7.0 branch, so it's part of the 7.0.0 release candidates.

Should we back it out until a solution is found?

The thing is -- there's no solution without changing the guarantees that libc++ make. Today, libc++ guarantees that you can link TUs that were built with different versions of libc++ together. If we remove that guarantee, then we can use linkonce_odr and solve the problem that Chromium is having.

Is that guarantee something that Chromium is relying upon?

I'm not sure (thakis can probably answer better), but isn't it enough to link against some system libraries that might use libc++ for this to be true?

One would have to link statically against a system library that was statically linked against libc++ -- I don't think this happens, at least not on Darwin AFAICT. As soon as we cross a dylib boundary, we're safe because those symbols hidden from the ABI are not exported from the dylib. The only problem is when distributing static archives using different versions of libc++, where we don't want presumably ODR-equivalent symbols to be deduplicated (because they might not actually be ODR-equivalent).

The previous solution, using inlining, while not very elegant didn't have this giant binary size problem, so maybe it was better?

Just to be clear, it's only about the number of symbols, not actual code size.

What should we put in the release notes, that folks should expect significantly larger binaries using the 7.0.0 version?

If the current state is not acceptable, we should roll back the change and only put it in once we _also_ know how to allow ODR-deduplicating across TUs. We don't have a solution for that right now -- this was a follow-up step that was planned in the future since this one was semantics-changing. If the current state is acceptable, we can put in the release notes that significant symbol size increases should be expected using LLVM 7.0.0.

[ldionne]

I opened a straw man proposal to fix this at https://reviews.llvm.org/D50652.

libcxx/docs/DesignDocs/VisibilityMacros.rst

**_LIBCPP_OVERRIDABLE_FUNC_VIS**
   overridden locally. On non-Windows, this is equivalent to `_LIBCPP_FUNC_VIS`.
   This macro is applied to all `operator new` and `operator delete` overloads.
 
   **Windows Behavior**: Any symbol marked `dllimport` cannot be overridden
   locally, since `dllimport` indicates the symbol should be bound to a separate
   DLL. All `operator new` and `operator delete` overloads are required to be
   locally overridable, and therefore must not be marked `dllimport`. On Windows,
   this macro therefore expands to `__declspec(dllexport)` when building the
   library and has an empty definition otherwise.
 
-**_LIBCPP_INLINE_VISIBILITY**
+**_LIBCPP_HIDE_FROM_ABI**
   Mark a function as not being part of the ABI of any final linked image that
   uses it, and also as being internal to each TU that uses that function. In
   other words, the address of a function marked with this attribute is not
   guaranteed to be the same across translation units.
 
 **_LIBCPP_TYPE_VIS**
   Mark a type's typeinfo, vtable and members as having default visibility.
   This attribute cannot be used on class templates.

libcxx/include/__config

 #define _LIBCPP_FUNC_VIS            _LIBCPP_DLL_VIS
 #define _LIBCPP_EXTERN_VIS          _LIBCPP_DLL_VIS
 #define _LIBCPP_EXCEPTION_ABI       _LIBCPP_DLL_VIS
 #define _LIBCPP_HIDDEN
 #define _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS
 #define _LIBCPP_TEMPLATE_VIS
 #define _LIBCPP_ENUM_VIS
 
 #if defined(_LIBCPP_COMPILER_MSVC)
-#  define _LIBCPP_INLINE_VISIBILITY __forceinline
 #  define _LIBCPP_EXTERN_TEMPLATE_INLINE_VISIBILITY __forceinline
 #else
-#  define _LIBCPP_INLINE_VISIBILITY __attribute__ ((__always_inline__))
 #  define _LIBCPP_EXTERN_TEMPLATE_INLINE_VISIBILITY __attribute__ ((__always_inline__))
 #endif
 
 #endif // defined(_LIBCPP_OBJECT_FORMAT_COFF)
 
 #ifndef _LIBCPP_HIDDEN
 #  if !defined(_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS)
 #    define _LIBCPP_HIDDEN __attribute__ ((__visibility__("hidden")))

 #    define _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS
 #  endif
 #endif
 
 #ifndef _LIBCPP_CLASS_TEMPLATE_INSTANTIATION_VIS
 #define _LIBCPP_CLASS_TEMPLATE_INSTANTIATION_VIS
 #endif
 
-#ifndef _LIBCPP_INLINE_VISIBILITY
-#  if !defined(_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS)
-#    define _LIBCPP_INLINE_VISIBILITY __attribute__ ((__visibility__("hidden"), __always_inline__))
-#  else
-#    define _LIBCPP_INLINE_VISIBILITY __attribute__ ((__always_inline__))
-#  endif
+#if __has_attribute(__always_inline__)
+#  define _LIBCPP_ALWAYS_INLINE __attribute__ ((__always_inline__))
+#elif _LIBCPP_COMPILER_MSVC
+#  define _LIBCPP_ALWAYS_INLINE __forceinline
+#else
+#  error "This compiler doesn't support any form of always_inline attribute"
+#endif
+
+// _LIBCPP_INTERNAL_LINKAGE tries its best to give internal linkage (i.e.
+// the `static` keyword in C) to the function it is applied to. When the
+// `internal_linkage` attribute is not available, we fall back to forcing
+// the function to be inlined, which approximates internal linkage since
+// an externally visible symbol is never generated for that function.
+#if __has_attribute(internal_linkage)
+#  define _LIBCPP_INTERNAL_LINKAGE __attribute__ ((internal_linkage))
+#else
+#  define _LIBCPP_INTERNAL_LINKAGE _LIBCPP_ALWAYS_INLINE
+#endif
+
+#ifndef _LIBCPP_HIDE_FROM_ABI
+#  define _LIBCPP_HIDE_FROM_ABI _LIBCPP_HIDDEN _LIBCPP_INTERNAL_LINKAGE
 #endif
 
+// Just so we can migrate to _LIBCPP_HIDE_FROM_ABI gradually.
+#define _LIBCPP_INLINE_VISIBILITY _LIBCPP_HIDE_FROM_ABI
+
 #ifndef _LIBCPP_EXTERN_TEMPLATE_INLINE_VISIBILITY
 #  if !defined(_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS)
 #    define _LIBCPP_EXTERN_TEMPLATE_INLINE_VISIBILITY __attribute__((__visibility__("default"), __always_inline__))
 #  else
 #    define _LIBCPP_EXTERN_TEMPLATE_INLINE_VISIBILITY __attribute__((__always_inline__))
 #  endif
 #endif