Re: [PATCH] D24642: Use __attribute__((internal_linkage)) when available.

2016-09-15 Thread Eric Fiselier via cfe-commits 
EricWF added a comment.

I'll commit this change tomorrow barring any objections.


https://reviews.llvm.org/D24642



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24642: Use __attribute__((internal_linkage)) when available.

2016-09-15 Thread Eric Fiselier via cfe-commits 
EricWF updated this revision to Diff 71590.
EricWF added a comment.

- Add `always_inline` to _LIBCPP_INLINE_VISIBILITY. In future I think only 
_LIBCPP_ALWAYS_INLINE should actually apply `always_inline`, but I'll make that 
change separately.
- Change `_LIBCPP_ALWAYS_INLINE` to use `internal_linkage` as well.


https://reviews.llvm.org/D24642

Files:
  include/__config
  src/string.cpp

Index: src/string.cpp
===
--- src/string.cpp
+++ src/string.cpp
@@ -29,6 +29,29 @@
 string
 operator+(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::iterator
+basic_string::insert(basic_string::const_iterator,
+   char const *, char const *);
+
+template basic_string::iterator
+basic_string::insert(basic_string::const_iterator,
+  wchar_t const *, wchar_t const *);
+
+template basic_string &
+basic_string::replace(basic_string::const_iterator,
+   basic_string::const_iterator,
+   char const *, char const *);
+
+template basic_string &
+basic_string::replace(basic_string::const_iterator,
+   basic_string::const_iterator,
+   wchar_t const *, wchar_t const *);
+#endif // _LIBCPP_ABI_EXTERN_TEMPLATE_SYMBOLS_VERSION
+
 namespace
 {
 
Index: include/__config
===
--- include/__config
+++ include/__config
@@ -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__)
@@ -610,11 +612,19 @@
 #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


Index: src/string.cpp
===
--- src/string.cpp
+++ src/string.cpp
@@ -29,6 +29,29 @@
 string
 operator+(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::iterator
+basic_string::insert(basic_string::const_iterator,
+   char const *, char const *);
+
+template basic_string::iterator
+basic_string::insert(basic_string::const_iterator,
+  wchar_t const *, wchar_t const *);
+
+template basic_string &
+basic_string::replace(basic_string::const_iterator,
+   basic_string::const_iterator,
+   char const *, char const *);
+
+template basic_string &
+basic_string::replace(basic_string::const_iterator,
+   basic_string::const_iterator,
+   wchar_t const *, wchar_t const *);
+#endif // _LIBCPP_ABI_EXTERN_TEMPLATE_SYMBOLS_VERSION
+
 namespace
 {
 
Index: include/__config
===
--- include/__config
+++ include/__config
@@ -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.

Re: [PATCH] D24642: Use __attribute__((internal_linkage)) when available.

2016-09-15 Thread Evgeniy Stepanov via cfe-commits 
eugenis accepted this revision.
eugenis added a comment.
This revision is now accepted and ready to land.

Looks great.
Thank you for seeing it through!


https://reviews.llvm.org/D24642



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24642: Use __attribute__((internal_linkage)) when available.

2016-09-15 Thread Eric Fiselier via cfe-commits 
EricWF updated this revision to Diff 71579.
EricWF added a comment.

Revert accidental change.


https://reviews.llvm.org/D24642

Files:
  include/__config
  src/string.cpp

Index: src/string.cpp
===
--- src/string.cpp
+++ src/string.cpp
@@ -29,6 +29,29 @@
 string
 operator+(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::iterator
+basic_string::insert(basic_string::const_iterator,
+   char const *, char const *);
+
+template basic_string::iterator
+basic_string::insert(basic_string::const_iterator,
+  wchar_t const *, wchar_t const *);
+
+template basic_string &
+basic_string::replace(basic_string::const_iterator,
+   basic_string::const_iterator,
+   char const *, char const *);
+
+template basic_string &
+basic_string::replace(basic_string::const_iterator,
+   basic_string::const_iterator,
+   wchar_t const *, wchar_t const *);
+#endif // _LIBCPP_ABI_EXTERN_TEMPLATE_SYMBOLS_VERSION
+
 namespace
 {
 
Index: include/__config
===
--- include/__config
+++ include/__config
@@ -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__)
@@ -610,7 +612,11 @@
 #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__))
+# else
+#   define _LIBCPP_INLINE_VISIBILITY __attribute__ ((__visibility__("hidden"), 
__always_inline__))
+# endif
 #endif
 
 #ifndef _LIBCPP_ALWAYS_INLINE


Index: src/string.cpp
===
--- src/string.cpp
+++ src/string.cpp
@@ -29,6 +29,29 @@
 string
 operator+(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::iterator
+basic_string::insert(basic_string::const_iterator,
+   char const *, char const *);
+
+template basic_string::iterator
+basic_string::insert(basic_string::const_iterator,
+  wchar_t const *, wchar_t const *);
+
+template basic_string &
+basic_string::replace(basic_string::const_iterator,
+   basic_string::const_iterator,
+   char const *, char const *);
+
+template basic_string &
+basic_string::replace(basic_string::const_iterator,
+   basic_string::const_iterator,
+   wchar_t const *, wchar_t const *);
+#endif // _LIBCPP_ABI_EXTERN_TEMPLATE_SYMBOLS_VERSION
+
 namespace
 {
 
Index: include/__config
===
--- include/__config
+++ include/__config
@@ -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__)
@@ -610,7 +612,11 @@
 #endif
 
 #ifndef _LIBCPP_INLINE_VISIBILITY
-#define _LIBCPP_INLINE_VISIBILITY __attribute__ ((__visibility__("hidden"), __always_inline__))
+# if __has_attribute(__internal_linkage__)
+#   define _LIBCPP_INLINE_VISIBILITY

[PATCH] D24642: Use __attribute__((internal_linkage)) when available.

2016-09-15 Thread Eric Fiselier via cfe-commits 
EricWF created this revision.
EricWF added reviewers: mclow.lists, eugenis.
EricWF added subscribers: cfe-commits, eugenis.

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`.

Most of the work for this patch was done by @eugenis.


https://reviews.llvm.org/D24642

Files:
  include/__config
  src/string.cpp

Index: src/string.cpp
===
--- src/string.cpp
+++ src/string.cpp
@@ -29,6 +29,29 @@
 string
 operator+(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::iterator
+basic_string::insert(basic_string::const_iterator,
+   char const *, char const *);
+
+template basic_string::iterator
+basic_string::insert(basic_string::const_iterator,
+  wchar_t const *, wchar_t const *);
+
+template basic_string &
+basic_string::replace(basic_string::const_iterator,
+   basic_string::const_iterator,
+   char const *, char const *);
+
+template basic_string &
+basic_string::replace(basic_string::const_iterator,
+   basic_string::const_iterator,
+   wchar_t const *, wchar_t const *);
+#endif // _LIBCPP_ABI_EXTERN_TEMPLATE_SYMBOLS_VERSION
+
 namespace
 {
 
Index: include/__config
===
--- include/__config
+++ include/__config
@@ -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__)
@@ -610,15 +612,19 @@
 #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__))
+# 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__))
 #endif
 
 #ifndef _LIBCPP_EXTERN_TEMPLATE_INLINE_VISIBILITY
-# ifdef _LIBCPP_BUILDING_LIBRARY
+# if defined(_LIBCPP_BUILDING_LIBRARY)
 #   define _LIBCPP_EXTERN_TEMPLATE_INLINE_VISIBILITY 
__attribute__((__visibility__("default"), __always_inline__))
 # else
 #   define _LIBCPP_EXTERN_TEMPLATE_INLINE_VISIBILITY _LIBCPP_INLINE_VISIBILITY


Index: src/string.cpp
===
--- src/string.cpp
+++ src/string.cpp
@@ -29,6 +29,29 @@
 string
 operator+(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::iterator
+basic_string::insert(basic_string::const_iterator,
+   char const *, char const *);
+
+template basic_string::iterator
+basic_string::insert(basic_string::const_iterator,
+  wchar_t const *, wchar_t const *);
+
+template basic_string &
+basic_string::replace(basic_string::const_iterator,
+