[PATCH] D49240: [libc++] Introduce _LIBCPP_HIDE_FROM_ABI to replace _LIBCPP_INLINE_VISIBILITY
ldionne added a comment. I opened a straw man proposal to fix this at https://reviews.llvm.org/D50652. Repository: rCXX libc++ https://reviews.llvm.org/D49240 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49240: [libc++] Introduce _LIBCPP_HIDE_FROM_ABI to replace _LIBCPP_INLINE_VISIBILITY
ldionne added a comment. In https://reviews.llvm.org/D49240#1197149, @hans wrote: > In https://reviews.llvm.org/D49240#1197052, @ldionne wrote: > > > In https://reviews.llvm.org/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. Repository: rCXX libc++ https://reviews.llvm.org/D49240 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49240: [libc++] Introduce _LIBCPP_HIDE_FROM_ABI to replace _LIBCPP_INLINE_VISIBILITY
hans added a comment. In https://reviews.llvm.org/D49240#1197052, @ldionne wrote: > In https://reviews.llvm.org/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? Repository: rCXX libc++ https://reviews.llvm.org/D49240 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49240: [libc++] Introduce _LIBCPP_HIDE_FROM_ABI to replace _LIBCPP_INLINE_VISIBILITY
ldionne added a comment. In https://reviews.llvm.org/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? Repository: rCXX libc++ https://reviews.llvm.org/D49240 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49240: [libc++] Introduce _LIBCPP_HIDE_FROM_ABI to replace _LIBCPP_INLINE_VISIBILITY
hans added a comment. 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? Repository: rCXX libc++ https://reviews.llvm.org/D49240 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49240: [libc++] Introduce _LIBCPP_HIDE_FROM_ABI to replace _LIBCPP_INLINE_VISIBILITY
rnk added a comment. In https://reviews.llvm.org/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. Repository: rCXX libc++ https://reviews.llvm.org/D49240 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49240: [libc++] Introduce _LIBCPP_HIDE_FROM_ABI to replace _LIBCPP_INLINE_VISIBILITY
ldionne added a comment. In https://reviews.llvm.org/D49240#1195723, @rnk wrote: > In https://reviews.llvm.org/D49240#1195237, @ldionne wrote: > > > In https://reviews.llvm.org/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. Repository: rCXX libc++ https://reviews.llvm.org/D49240 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49240: [libc++] Introduce _LIBCPP_HIDE_FROM_ABI to replace _LIBCPP_INLINE_VISIBILITY
rnk added a comment. In https://reviews.llvm.org/D49240#1195237, @ldionne wrote: > In https://reviews.llvm.org/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% Repository: rCXX libc++ https://reviews.llvm.org/D49240 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49240: [libc++] Introduce _LIBCPP_HIDE_FROM_ABI to replace _LIBCPP_INLINE_VISIBILITY
ldionne added a comment. In https://reviews.llvm.org/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. Repository: rCXX libc++ https://reviews.llvm.org/D49240 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49240: [libc++] Introduce _LIBCPP_HIDE_FROM_ABI to replace _LIBCPP_INLINE_VISIBILITY
thakis added a comment. 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? Repository: rCXX libc++ https://reviews.llvm.org/D49240 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49240: [libc++] Introduce _LIBCPP_HIDE_FROM_ABI to replace _LIBCPP_INLINE_VISIBILITY
ldionne added a comment. Note: I resolved Eric's comments before pushing. Repository: rCXX libc++ https://reviews.llvm.org/D49240 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49240: [libc++] Introduce _LIBCPP_HIDE_FROM_ABI to replace _LIBCPP_INLINE_VISIBILITY
This revision was automatically updated to reflect the committed changes.
Closed by commit rCXX338122: [libc++] Introduce _LIBCPP_HIDE_FROM_ABI to
replace _LIBCPP_INLINE_VISIBILITY (authored by ldionne, committed by ).
Herald added a subscriber: cfe-commits.
Changed prior to commit:
https://reviews.llvm.org/D49240?vs=156131&id=157671#toc
Repository:
rCXX libc++
https://reviews.llvm.org/D49240
Files:
docs/DesignDocs/VisibilityMacros.rst
include/__config
Index: docs/DesignDocs/VisibilityMacros.rst
===
--- docs/DesignDocs/VisibilityMacros.rst
+++ docs/DesignDocs/VisibilityMacros.rst
@@ -40,7 +40,7 @@
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
@@ -155,6 +155,22 @@
versioning namespace. This allows throwing and catching some exception types
between libc++ and libstdc++.
+**_LIBCPP_INTERNAL_LINKAGE**
+ Mark the affected entity as having internal linkage (i.e. the `static`
+ keyword in C). This is only a best effort: 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. This is an internal macro
+ used as an implementation detail by other visibility macros. Never mark
+ a function or a class with this macro directly.
+
+**_LIBCPP_ALWAYS_INLINE**
+ Forces inlining of the function it is applied to. For visibility purposes,
+ this macro is used to make sure that an externally visible symbol is never
+ generated in an object file when the `internal_linkage` attribute is not
+ available. This is an internal macro used by other visibility macros, and
+ it should not be used directly.
+
Links
=
Index: include/__config
===
--- include/__config
+++ include/__config
@@ -491,6 +491,8 @@
#define _LIBCPP_HAS_UNIQUE_OBJECT_REPRESENTATIONS
#endif
+#define _LIBCPP_ALWAYS_INLINE __attribute__ ((__always_inline__))
+
#elif defined(_LIBCPP_COMPILER_GCC)
#define _ALIGNAS(x) __attribute__((__aligned__(x)))
@@ -582,6 +584,8 @@
#define _LIBCPP_HAS_UNIQUE_OBJECT_REPRESENTATIONS
#endif
+#define _LIBCPP_ALWAYS_INLINE __attribute__ ((__always_inline__))
+
#elif defined(_LIBCPP_COMPILER_MSVC)
#define _LIBCPP_TOSTRING2(x) #x
@@ -614,6 +618,8 @@
#define _LIBCPP_HAS_NO_ASAN
+#define _LIBCPP_ALWAYS_INLINE __forceinline
+
#elif defined(_LIBCPP_COMPILER_IBM)
#define _ALIGNAS(x) __attribute__((__aligned__(x)))
@@ -644,6 +650,8 @@
#define _LIBCPP_HAS_NO_ASAN
+#define _LIBCPP_ALWAYS_INLINE __attribute__ ((__always_inline__))
+
#endif // _LIBCPP_COMPILER_[CLANG|GCC|MSVC|IBM]
#if _LIBCPP_STD_VER >= 17
@@ -695,10 +703,8 @@
#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
@@ -785,14 +791,19 @@
#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(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__))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
