Page MenuHomePhabricator

[libc++] Introduce _LIBCPP_HIDE_FROM_ABI to replace _LIBCPP_INLINE_VISIBILITY
ClosedPublic

Authored by ldionne on Jul 12 2018, 8:08 AM.

Details

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

Diff Detail

Repository
rL LLVM

Event Timeline

ldionne created this revision.Jul 12 2018, 8:08 AM
ldionne planned changes to this revision.Jul 12 2018, 8:08 AM
ldionne added inline comments.Jul 12 2018, 8:08 AM
libcxx/include/__config
767 ↗(On Diff #155185)

Is this acceptable?

mclow.lists added inline comments.
libcxx/include/__config
764 ↗(On Diff #155185)

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 ↗(On Diff #155185)

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

ldionne updated this revision to Diff 156131.Jul 18 2018, 12:40 PM
ldionne marked 3 inline comments as done.

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

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?

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

mclow.lists accepted this revision.Jul 23 2018, 11:49 AM

This looks good to me.

libcxx/include/__config
681 ↗(On Diff #156131)

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

This revision is now accepted and ready to land.Jul 23 2018, 11:49 AM
EricWF accepted this revision.Jul 26 2018, 2:47 PM

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 ↗(On Diff #156131)

Should we re-document _LIBCPP_ALWAYS_INLINE and _LIBCPP_INTERNAL_LINKAGE?

libcxx/include/__config
770 ↗(On Diff #156131)

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

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 accepted this revision.Jul 26 2018, 3:02 PM

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 marked 2 inline comments as done.Jul 27 2018, 5:40 AM
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.

Note: I resolved Eric's comments before pushing.

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?

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 added a subscriber: rnk.Aug 10 2018, 12:07 PM

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%
In D49240#1195723, @rnk 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 added a comment.Aug 10 2018, 3:19 PM

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 added a subscriber: hans.Aug 13 2018, 2:59 AM

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 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 added a comment.Aug 13 2018, 7:05 AM

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?

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.

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