<div dir="ltr">Sure, I've reverted the change in r284101.<div><br></div><div>I'll try and respond in detail tomorrow.</div><div><br></div><div>/Eric</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Oct 12, 2016 at 9:57 PM, Mehdi Amini <span dir="ltr"><<a href="mailto:mehdi.amini@apple.com" target="_blank">mehdi.amini@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">Hi Eric,<div><br></div><div>This is not clear to me that this patch is correct as-is. </div><div><br></div><div>See the last message in the thread: <a href="http://lists.llvm.org/pipermail/cfe-dev/2016-July/049985.html" target="_blank">http://lists.llvm.org/<wbr>pipermail/cfe-dev/2016-July/<wbr>049985.html</a></div><div><br></div><div>I think we should reach a consensus on the right course of actions about this first.</div><div><br></div><div>— </div><span class="HOEnZb"><font color="#888888"><div>Mehdi</div></font></span><div><div class="h5"><div><br></div><div><br></div><div><br></div><div><br></div><div><div><blockquote type="cite"><div>On Sep 24, 2016, at 8:14 PM, Eric Fiselier via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:</div><br class="m_-959051960974549967Apple-interchange-newline"><div><div>Author: ericwf<br>Date: Sat Sep 24 22:14:13 2016<br>New Revision: 282345<br><br>URL: <a href="http://llvm.org/viewvc/llvm-project?rev=282345&view=rev" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project?rev=282345&view=rev</a><br>Log:<br>Use __attribute__((internal_<wbr>linkage)) when available.<br><br>Summary:<br>This patch has been a long time coming (Thanks @eugenis). It changes `_LIBCPP_INLINE_VISIBILITY` to use `__attribute__((internal_<wbr>linkage))` instead of `__attribute__((visibility("<wbr>hidden"), always_inline))`.<br><br>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.<br>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_<wbr>linkage))` to an inline function is the same as applying `static`.<br><br>For more information about the attribute see: <a href="http://lists.llvm.org/pipermail/cfe-dev/2015-October/045580.html" target="_blank">http://lists.llvm.org/<wbr>pipermail/cfe-dev/2015-<wbr>October/045580.html</a><br><br>Most of the work for this patch was done by @eugenis.<br><br><br>Reviewers: mclow.lists, eugenis<br><br>Subscribers: eugenis, cfe-commits<br><br>Differential Revision: <a href="https://reviews.llvm.org/D24642" target="_blank">https://reviews.llvm.org/<wbr>D24642</a><br><br>Modified:<br>    libcxx/trunk/include/__<wbr>config<br>    libcxx/trunk/src/string.cpp<br><br>Modified: libcxx/trunk/include/__config<br>URL: <a href="http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/__config?rev=282345&r1=282344&r2=282345&view=diff" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/libcxx/trunk/include/_<wbr>_config?rev=282345&r1=282344&<wbr>r2=282345&view=diff</a><br>==============================<wbr>==============================<wbr>==================<br>--- libcxx/trunk/include/__config (original)<br>+++ libcxx/trunk/include/__config Sat Sep 24 22:14:13 2016<br>@@ -34,6 +34,7 @@<br> #endif<br><br> #if defined(_LIBCPP_ABI_UNSTABLE) || _LIBCPP_ABI_VERSION >= 2<br>+#define _LIBCPP_ABI_EXTERN_TEMPLATE_<wbr>SYMBOLS_VERSION 2<br> // Change short string representation so that string data starts at offset 0,<br> // improving its alignment in some cases.<br> #define _LIBCPP_ABI_ALTERNATE_STRING_<wbr>LAYOUT<br>@@ -49,6 +50,7 @@<br> #define _LIBCPP_ABI_FIX_UNORDERED_<wbr>CONTAINER_SIZE_TYPE<br> #define _LIBCPP_ABI_VARIADIC_LOCK_<wbr>GUARD<br> #elif _LIBCPP_ABI_VERSION == 1<br>+#define _LIBCPP_ABI_EXTERN_TEMPLATE_<wbr>SYMBOLS_VERSION 1<br> // Feature macros for disabling pre ABI v1 features. All of these options<br> // are deprecated.<br> #if defined(__FreeBSD__)<br>@@ -629,11 +631,19 @@ namespace std {<br> #endif<br><br> #ifndef _LIBCPP_INLINE_VISIBILITY<br>-#define _LIBCPP_INLINE_VISIBILITY __attribute__ ((__visibility__("hidden"), __always_inline__))<br>+# if __has_attribute(__internal_<wbr>linkage__)<br>+#   define _LIBCPP_INLINE_VISIBILITY __attribute__((__internal_<wbr>linkage__, __always_inline__))<br>+# else<br>+#   define _LIBCPP_INLINE_VISIBILITY __attribute__ ((__visibility__("hidden"), __always_inline__))<br>+# endif<br> #endif<br><br> #ifndef _LIBCPP_ALWAYS_INLINE<br>-#define _LIBCPP_ALWAYS_INLINE  __attribute__ ((__visibility__("hidden"), __always_inline__))<br>+# if __has_attribute(__internal_<wbr>linkage__)<br>+#   define _LIBCPP_ALWAYS_INLINE __attribute__((__internal_<wbr>linkage__, __always_inline__))<br>+# else<br>+#   define _LIBCPP_ALWAYS_INLINE  __attribute__ ((__visibility__("hidden"), __always_inline__))<br>+# endif<br> #endif<br><br> #ifndef _LIBCPP_EXTERN_TEMPLATE_<wbr>INLINE_VISIBILITY<br><br>Modified: libcxx/trunk/src/string.cpp<br>URL: <a href="http://llvm.org/viewvc/llvm-project/libcxx/trunk/src/string.cpp?rev=282345&r1=282344&r2=282345&view=diff" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/libcxx/trunk/src/<wbr>string.cpp?rev=282345&r1=<wbr>282344&r2=282345&view=diff</a><br>==============================<wbr>==============================<wbr>==================<br>--- libcxx/trunk/src/string.cpp (original)<br>+++ libcxx/trunk/src/string.cpp Sat Sep 24 22:14:13 2016<br>@@ -29,6 +29,29 @@ template<br>     string<br>     operator+<char, char_traits<char>, allocator<char> >(char const*, string const&);<br><br>+// These external instantiations are required to maintain dylib compatibility<br>+// for ABI v1 when using __attribute__((internal_<wbr>linkage)) as opposed to<br>+// __attribute__((visibility("<wbr>hidden"), always_inline)).<br>+#if _LIBCPP_ABI_EXTERN_TEMPLATE_<wbr>SYMBOLS_VERSION == 1<br>+template basic_string<char>::iterator<br>+basic_string<char>::insert(<wbr>basic_string<char>::const_<wbr>iterator,<br>+                           char const *, char const *);<br>+<br>+template basic_string<wchar_t>::<wbr>iterator<br>+basic_string<wchar_t>::<wbr>insert(basic_string<wchar_t>::<wbr>const_iterator,<br>+                              <wbr>wchar_t const *, wchar_t const *);<br>+<br>+template basic_string<char> &<br>+basic_string<char>::replace(<wbr>basic_string<char>::const_<wbr>iterator,<br>+                               <wbr>basic_string<char>::const_<wbr>iterator,<br>+                               <wbr>char const *, char const *);<br>+<br>+template basic_string<wchar_t> &<br>+basic_string<wchar_t>::<wbr>replace(basic_string<wchar_t>:<wbr>:const_iterator,<br>+                               <wbr>basic_string<wchar_t>::const_<wbr>iterator,<br>+                               <wbr>wchar_t const *, wchar_t const *);<br>+#endif // _LIBCPP_ABI_EXTERN_TEMPLATE_<wbr>SYMBOLS_VERSION<br>+<br> namespace<br> {<br><br><br><br>______________________________<wbr>_________________<br>cfe-commits mailing list<br><a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br><a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-commits</a><br></div></div></blockquote></div><br></div></div></div></div></blockquote></div><br></div>