Thanks Eric! > On Oct 12, 2016, at 9:18 PM, Eric Fiselier <e...@efcs.ca> wrote: > > 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.am...@apple.com > <mailto:mehdi.am...@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 > <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@lists.llvm.org <mailto:cfe-commits@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 >> <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 >> <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 >> <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 >> >> <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 >> >> <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@lists.llvm.org <mailto:cfe-commits@lists.llvm.org> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >> <http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits> > >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
