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

[Eric Fiselier via cfe-commits]

Sure, I've reverted the change in r284101.

I'll try and respond in detail tomorrow.

/Eric

On Wed, Oct 12, 2016 at 9:57 PM, Mehdi Amini <mehdi.amini at apple.com> wrote:

> 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
>
> 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/9db5e3c6/attachment.html>