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> 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@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@lists.llvm.org > 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
