[libcxx] r282345 - Use __attribute__((internal_linkage)) when available.

[Mehdi Amini via cfe-commits]

Hi Eric,

This is not clear to me that this patch is correct as-is. 

See the last message in the thread: http://lists.llvm.org/pipermail/cfe-dev/2016-July/049985.html <http://lists.llvm.org/pipermail/cfe-dev/2016-July/049985.html>

I think we should reach a consensus on the right course of actions about this first.

— 
Mehdi




> On Sep 24, 2016, at 8:14 PM, Eric Fiselier via cfe-commits <cfe-commits at lists.llvm.org> wrote:
> 
> Author: ericwf
> Date: Sat Sep 24 22:14:13 2016
> New Revision: 282345
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=282345&view=rev
> Log:
> Use __attribute__((internal_linkage)) when available.
> 
> Summary:
> This patch has been a long time coming (Thanks @eugenis). It changes `_LIBCPP_INLINE_VISIBILITY` to use `__attribute__((internal_linkage))` instead of `__attribute__((visibility("hidden"), always_inline))`.
> 
> The point of `_LIBCPP_INLINE_VISIBILITY` is to prevent inline functions from being exported from both the libc++ library and from user libraries. This helps libc++ better manage it's ABI.
> Previously this was done by forcing inlining and modifying the symbols visibility. However inlining isn't guaranteed and symbol visibility only affects shared libraries making this an imperfect solution.  `internal_linkage` improves this situation by making all symbols local to the TU they are emitted in, regardless of inlining or visibility. IIRC the effect of applying `__attribute__((internal_linkage))` to an inline function is the same as applying `static`.
> 
> For more information about the attribute see: http://lists.llvm.org/pipermail/cfe-dev/2015-October/045580.html
> 
> Most of the work for this patch was done by @eugenis.
> 
> 
> Reviewers: mclow.lists, eugenis
> 
> Subscribers: eugenis, cfe-commits
> 
> Differential Revision: https://reviews.llvm.org/D24642
> 
> Modified:
>    libcxx/trunk/include/__config
>    libcxx/trunk/src/string.cpp
> 
> Modified: libcxx/trunk/include/__config
> URL: http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/__config?rev=282345&r1=282344&r2=282345&view=diff
> ==============================================================================
> --- libcxx/trunk/include/__config (original)
> +++ libcxx/trunk/include/__config Sat Sep 24 22:14:13 2016
> @@ -34,6 +34,7 @@
> #endif
> 
> #if defined(_LIBCPP_ABI_UNSTABLE) || _LIBCPP_ABI_VERSION >= 2
> +#define _LIBCPP_ABI_EXTERN_TEMPLATE_SYMBOLS_VERSION 2
> // Change short string representation so that string data starts at offset 0,
> // improving its alignment in some cases.
> #define _LIBCPP_ABI_ALTERNATE_STRING_LAYOUT
> @@ -49,6 +50,7 @@
> #define _LIBCPP_ABI_FIX_UNORDERED_CONTAINER_SIZE_TYPE
> #define _LIBCPP_ABI_VARIADIC_LOCK_GUARD
> #elif _LIBCPP_ABI_VERSION == 1
> +#define _LIBCPP_ABI_EXTERN_TEMPLATE_SYMBOLS_VERSION 1
> // Feature macros for disabling pre ABI v1 features. All of these options
> // are deprecated.
> #if defined(__FreeBSD__)
> @@ -629,11 +631,19 @@ namespace std {
> #endif
> 
> #ifndef _LIBCPP_INLINE_VISIBILITY
> -#define _LIBCPP_INLINE_VISIBILITY __attribute__ ((__visibility__("hidden"), __always_inline__))
> +# if __has_attribute(__internal_linkage__)
> +#   define _LIBCPP_INLINE_VISIBILITY __attribute__((__internal_linkage__, __always_inline__))
> +# else
> +#   define _LIBCPP_INLINE_VISIBILITY __attribute__ ((__visibility__("hidden"), __always_inline__))
> +# endif
> #endif
> 
> #ifndef _LIBCPP_ALWAYS_INLINE
> -#define _LIBCPP_ALWAYS_INLINE  __attribute__ ((__visibility__("hidden"), __always_inline__))
> +# if __has_attribute(__internal_linkage__)
> +#   define _LIBCPP_ALWAYS_INLINE __attribute__((__internal_linkage__, __always_inline__))
> +# else
> +#   define _LIBCPP_ALWAYS_INLINE  __attribute__ ((__visibility__("hidden"), __always_inline__))
> +# endif
> #endif
> 
> #ifndef _LIBCPP_EXTERN_TEMPLATE_INLINE_VISIBILITY
> 
> Modified: libcxx/trunk/src/string.cpp
> URL: http://llvm.org/viewvc/llvm-project/libcxx/trunk/src/string.cpp?rev=282345&r1=282344&r2=282345&view=diff
> ==============================================================================
> --- libcxx/trunk/src/string.cpp (original)
> +++ libcxx/trunk/src/string.cpp Sat Sep 24 22:14:13 2016
> @@ -29,6 +29,29 @@ template
>     string
>     operator+<char, char_traits<char>, allocator<char> >(char const*, string const&);
> 
> +// These external instantiations are required to maintain dylib compatibility
> +// for ABI v1 when using __attribute__((internal_linkage)) as opposed to
> +// __attribute__((visibility("hidden"), always_inline)).
> +#if _LIBCPP_ABI_EXTERN_TEMPLATE_SYMBOLS_VERSION == 1
> +template basic_string<char>::iterator
> +basic_string<char>::insert(basic_string<char>::const_iterator,
> +                           char const *, char const *);
> +
> +template basic_string<wchar_t>::iterator
> +basic_string<wchar_t>::insert(basic_string<wchar_t>::const_iterator,
> +                              wchar_t const *, wchar_t const *);
> +
> +template basic_string<char> &
> +basic_string<char>::replace(basic_string<char>::const_iterator,
> +                               basic_string<char>::const_iterator,
> +                               char const *, char const *);
> +
> +template basic_string<wchar_t> &
> +basic_string<wchar_t>::replace(basic_string<wchar_t>::const_iterator,
> +                               basic_string<wchar_t>::const_iterator,
> +                               wchar_t const *, wchar_t const *);
> +#endif // _LIBCPP_ABI_EXTERN_TEMPLATE_SYMBOLS_VERSION
> +
> namespace
> {
> 
> 
> 
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20161012/6f7211ea/attachment-0001.html>