[libc++] Make _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS export members

[smeenai]

Summary

When building libc++ with hidden visibility, we want explicit template
instantiations to export members. This is consistent with existing
Windows behavior, and is necessary for clients to be able to link
against a hidden visibility built libc++ without running into lots of
missing symbols.

An unfortunate side effect, however, is that any template methods of a
class with an explicit instantiation will get default visibility when
instantiated, unless the methods are explicitly marked inline or hidden
visibility. This is not desirable for clients of libc++ headers who wish
to control their visibility, and led to PR30642.

Annotate all problematic methods with an explicit visibility specifier
to avoid this. The problematic methods were found by running
https://github.com/smeenai/bad-visibility-finder against the libc++
headers after making the _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS change. The
methods were marked with the new _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS
macro, which was created for this purpose.

It should be noted that _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS was originally
intended to expand to default visibility, and was changed to expanding
to default type visibility to fix PR30642. The visibility macro
documentation was not updated accordingly, however, so this change makes
the macro consistent with its documentation again, while explicitly
fixing the methods which resulted in that PR.

[smeenai]

Ping.

[smeenai]

Rebase and ping

[EricWF]

include/string

1100 ↗

(On Diff #87912)

Why inline _LIBCPP_INLINE_VISIBILITY here but _LIBCPP_HIDDEN everywhere else?

[EricWF]

I applied inline to most of the functions changed in basic_string, and updated the ABI lists accordingly.

Please update this patch so it merges with trunk. Note you can add _LIBCPP_HIDDEN to the functions in basic_string as well if you want, however the inlineis enough to make them hidden in the dylib.

[EricWF]

include/locale

626 ↗

(On Diff #87912)

After applying the changes to __config but not this header check-cxx-abilist didn't report any changes to these symbols. Why is this needed again?

[smeenai]

Will do the rebase.

include/locale

626 ↗

(On Diff #87912)

It's to prevent https://llvm.org/bugs/show_bug.cgi?id=30642 from occurring again. If an extern template class is marked default visibility, and you have template methods of that class which get instantiated in other libraries, those template method instantiations also get marked default visibility, so now other libraries are exporting libc++ symbols. This change marks all such template methods as hidden to prevent the leakage.

That might not be the most clear explanation, so https://ghostbin.com/paste/29y9d demonstrates what I mean. (It's also the root cause of PR30642.)

https://reviews.llvm.org/D25208 addresses the same issue for non-template classes.

include/string

1100 ↗

(On Diff #87912)

This function is really small, so I figured marking it for inlining was more appropriate.

[smeenai]

Rebase

[EricWF]

LGTM, but with a macro other than _LIBCPP_HIDDEN.

Also could you please add doc explaining the rational for this new macro in depth. I think the summary for this revision would be sufficient.

[EricWF]

include/string

1100 ↗

(On Diff #87912)

This should probably have _LIBCPP_INLINE_VISIBILITY on it then.

[smeenai]

include/string

1100 ↗

(On Diff #87912)

I was assuming any client that cares about what they're exporting would build with -fvisibility-inlines-hidden, in which case just inline should be enough.

[smeenai]

Addressing comments

[smeenai]

Changing more instances to new macro, per IRC discussion with Eric

[smeenai]

Including inline in macro to work around PR32114

libcxx/trunk/docs/DesignDocs/VisibilityMacros.rst

**_LIBCPP_CLASS_TEMPLATE_INSTANTIATION_VIS**
   Mark the member functions, typeinfo, and vtable of an explicit instantiation
   of a class template as being exported by the libc++ library. This attribute
   must be specified on all template class explicit instantiations.
 
   It is only necessary to mark the explicit instantiation itself (as opposed to
   the extern template declaration) as exported on Windows, as discussed above.
   On all other platforms, this macro has an empty definition.
 
+**_LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS**
+  Mark a symbol as hidden so it will not be exported from shared libraries. This
+  is intended specifically for method templates of either classes marked with
+  `_LIBCPP_TYPE_VIS` or classes with an extern template instantiation
+  declaration marked with `_LIBCPP_EXTERN_TEMPLATE_TYPE_VIS`.
+
+  When building libc++ with hidden visibility, we want explicit template
+  instantiations to export members, which is consistent with existing Windows
+  behavior. We also want classes annotated with `_LIBCPP_TYPE_VIS` to export
+  their members, which is again consistent with existing Windows behavior.
+  Both these changes are necessary for clients to be able to link against a
+  libc++ DSO built with hidden visibility without encountering missing symbols.
+
+  An unfortunate side effect, however, is that method templates of classes
+  either marked `_LIBCPP_TYPE_VIS` or with extern template instantiation
+  declarations marked with `_LIBCPP_EXTERN_TEMPLATE_TYPE_VIS` also get default
+  visibility when instantiated. These methods are often implicitly instantiated
+  inside other libraries which use the libc++ headers, and will therefore end up
+  being exported from those libraries, since those implicit instantiations will
+  receive default visibility. This is not acceptable for libraries that wish to
+  control their visibility, and led to PR30642.
+
+  Consequently, all such problematic method templates are explicitly marked
+  either hidden (via this macro) or inline, so that they don't leak into client
+  libraries. The problematic methods were found by running
+  `bad-visibility-finder <https://github.com/smeenai/bad-visibility-finder>`_
+  against the libc++ headers after making `_LIBCPP_TYPE_VIS` and
+  `_LIBCPP_EXTERN_TEMPLATE_TYPE_VIS` expand to default visibility.
+
 **_LIBCPP_EXTERN_TEMPLATE_INLINE_VISIBILITY**
   Mark a member function of a class template as visible and always inline. This
   macro should only be applied to member functions of class templates that are
   externally instantiated. It is important that these symbols are not marked
   as hidden as that will prevent the dylib definition from being found.
 
   This macro is used to maintain ABI compatibility for symbols that have been
   historically exported by the libc++ library but are now marked inline.

libcxx/trunk/include/__config

 # define _LIBCPP_OVERRIDABLE_FUNC_VIS
 #endif
 
 #define _LIBCPP_TYPE_VIS            _LIBCPP_DLL_VIS
 #define _LIBCPP_FUNC_VIS            _LIBCPP_DLL_VIS
 #define _LIBCPP_EXTERN_VIS          _LIBCPP_DLL_VIS
 #define _LIBCPP_EXCEPTION_ABI       _LIBCPP_DLL_VIS
 #define _LIBCPP_HIDDEN
+#define _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS
 #define _LIBCPP_TEMPLATE_VIS
 #define _LIBCPP_FUNC_VIS_ONLY
 #define _LIBCPP_ENUM_VIS
 
 #if defined(_LIBCPP_COMPILER_MSVC)
 # define _LIBCPP_INLINE_VISIBILITY __forceinline
 # define _LIBCPP_ALWAYS_INLINE     __forceinline
 # define _LIBCPP_EXTERN_TEMPLATE_INLINE_VISIBILITY __forceinline
 #else
 # define _LIBCPP_INLINE_VISIBILITY __attribute__ ((__always_inline__))
 # define _LIBCPP_ALWAYS_INLINE     __attribute__ ((__always_inline__))
 # define _LIBCPP_EXTERN_TEMPLATE_INLINE_VISIBILITY __attribute__ ((__always_inline__))
 #endif
 #endif // defined(_LIBCPP_OBJECT_FORMAT_COFF)
 
 #ifndef _LIBCPP_HIDDEN
 #if !defined(_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS)
 #define _LIBCPP_HIDDEN __attribute__ ((__visibility__("hidden")))
 #else
 #define _LIBCPP_HIDDEN
 #endif
 #endif
 
+// The inline should be removed once PR32114 is resolved
+#define _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS inline _LIBCPP_HIDDEN
+
 #ifndef _LIBCPP_FUNC_VIS
 #if !defined(_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS)
 #define _LIBCPP_FUNC_VIS __attribute__ ((__visibility__("default")))
 #else
 #define _LIBCPP_FUNC_VIS
 #endif
 #endif
 

 #    define _LIBCPP_ENUM_VIS __attribute__ ((__type_visibility__("default")))
 #  else
 #    define _LIBCPP_ENUM_VIS
 #  endif
 #endif
 
 #ifndef _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS
 #  if !defined(_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS) && __has_attribute(__type_visibility__)
-#    define _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS __attribute__ ((__type_visibility__("default")))
+#    define _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS __attribute__ ((__visibility__("default")))
 #  else
 #    define _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS
 #  endif
 #endif
 
 #ifndef _LIBCPP_CLASS_TEMPLATE_INSTANTIATION_VIS
 #  define _LIBCPP_CLASS_TEMPLATE_INSTANTIATION_VIS
 #endif

libcxx/trunk/include/locale

public:
 
     static locale::id id;
 
 protected:
     _LIBCPP_ALWAYS_INLINE
     ~num_get() {}
 
     template <class _Fp>
-    inline iter_type __do_get_floating_point
+    _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS
+    iter_type __do_get_floating_point
                             (iter_type __b, iter_type __e, ios_base& __iob,
                              ios_base::iostate& __err, _Fp& __v) const;
 
     template <class _Signed>
-    inline iter_type __do_get_signed
+    _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS
+    iter_type __do_get_signed
                             (iter_type __b, iter_type __e, ios_base& __iob,
                              ios_base::iostate& __err, _Signed& __v) const;
 
     template <class _Unsigned>
-    inline iter_type __do_get_unsigned
+    _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS
+    iter_type __do_get_unsigned
                             (iter_type __b, iter_type __e, ios_base& __iob,
                              ios_base::iostate& __err, _Unsigned& __v) const;
 
 
     virtual iter_type do_get(iter_type __b, iter_type __e, ios_base& __iob,
                              ios_base::iostate& __err, bool& __v) const;
 
     virtual iter_type do_get(iter_type __b, iter_type __e, ios_base& __iob,

libcxx/trunk/include/string

#endif  // _LIBCPP_HAS_NO_RVALUE_REFERENCES
     _LIBCPP_INLINE_VISIBILITY
     basic_string(size_type __n, _CharT __c, const _Allocator& __a);
     basic_string(const basic_string& __str, size_type __pos, size_type __n,
                  const _Allocator& __a = _Allocator());
     _LIBCPP_INLINE_VISIBILITY
     basic_string(const basic_string& __str, size_type __pos,
                  const _Allocator& __a = _Allocator());
     template<class _Tp>
+        _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS
         basic_string(const _Tp& __t, size_type __pos, size_type __n,
                      const allocator_type& __a = allocator_type(),
                      typename enable_if<__can_be_converted_to_string_view<_CharT, _Traits, _Tp>::value, void>::type* = 0);
     _LIBCPP_INLINE_VISIBILITY explicit
     basic_string(__self_view __sv);
     _LIBCPP_INLINE_VISIBILITY
     basic_string(__self_view __sv, const _Allocator& __a);
     template<class _InputIterator>

 #endif  // _LIBCPP_HAS_NO_GENERALIZED_INITIALIZERS
 
     _LIBCPP_INLINE_VISIBILITY
     basic_string& append(const basic_string& __str);
     _LIBCPP_INLINE_VISIBILITY
     basic_string& append(__self_view __sv) { return append(__sv.data(), __sv.size()); }
     basic_string& append(const basic_string& __str, size_type __pos, size_type __n=npos);
     template <class _Tp>
-    inline typename enable_if
+    _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS
+    typename enable_if
         <
             __can_be_converted_to_string_view<_CharT, _Traits, _Tp>::value,
             basic_string&
         >::type
                   append(const _Tp& __t, size_type __pos, size_type __n=npos);
     basic_string& append(const value_type* __s, size_type __n);
     basic_string& append(const value_type* __s);
     basic_string& append(size_type __n, value_type __c);
     template <class _ForwardIterator>
-    inline basic_string& __append_forward_unsafe(_ForwardIterator, _ForwardIterator);
+    _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS
+    basic_string& __append_forward_unsafe(_ForwardIterator, _ForwardIterator);
     template<class _InputIterator>
-    inline typename enable_if
+    _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS
+    typename enable_if
         <
             __is_exactly_input_iterator<_InputIterator>::value
                 || !__libcpp_string_gets_noexcept_iterator<_InputIterator>::value,
             basic_string&
         >::type
     _LIBCPP_INLINE_VISIBILITY
     append(_InputIterator __first, _InputIterator __last) {
       const basic_string __temp (__first, __last, __alloc());
       append(__temp.data(), __temp.size());
       return *this;
     }
     template<class _ForwardIterator>
-    inline typename enable_if
+    _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS
+    typename enable_if
         <
             __is_forward_iterator<_ForwardIterator>::value
                 && __libcpp_string_gets_noexcept_iterator<_ForwardIterator>::value,
             basic_string&
         >::type
     _LIBCPP_INLINE_VISIBILITY
     append(_ForwardIterator __first, _ForwardIterator __last) {
       return __append_forward_unsafe(__first, __last);

 #ifndef _LIBCPP_HAS_NO_RVALUE_REFERENCES
     _LIBCPP_INLINE_VISIBILITY
     basic_string& assign(basic_string&& str)
         _NOEXCEPT_((__noexcept_move_assign_container<_Allocator, __alloc_traits>::value))
         {*this = _VSTD::move(str); return *this;}
 #endif
     basic_string& assign(const basic_string& __str, size_type __pos, size_type __n=npos);
     template <class _Tp>
-    inline typename enable_if
+    _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS
+    typename enable_if
         <
             __can_be_converted_to_string_view<_CharT, _Traits, _Tp>::value,
             basic_string&
         >::type
                   assign(const _Tp & __t, size_type pos, size_type n=npos);
     basic_string& assign(const value_type* __s, size_type __n);
     basic_string& assign(const value_type* __s);
     basic_string& assign(size_type __n, value_type __c);
     template<class _InputIterator>
-    inline typename enable_if
+    _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS
+    typename enable_if
         <
            __is_exactly_input_iterator<_InputIterator>::value
                 || !__libcpp_string_gets_noexcept_iterator<_InputIterator>::value,
             basic_string&
         >::type
         assign(_InputIterator __first, _InputIterator __last);
     template<class _ForwardIterator>
-    inline typename enable_if
+    _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS
+    typename enable_if
         <
             __is_forward_iterator<_ForwardIterator>::value
                  && __libcpp_string_gets_noexcept_iterator<_ForwardIterator>::value,
             basic_string&
         >::type
         assign(_ForwardIterator __first, _ForwardIterator __last);
 #ifndef _LIBCPP_HAS_NO_GENERALIZED_INITIALIZERS
     _LIBCPP_INLINE_VISIBILITY
     basic_string& assign(initializer_list<value_type> __il) {return assign(__il.begin(), __il.size());}
 #endif  // _LIBCPP_HAS_NO_GENERALIZED_INITIALIZERS
 
     _LIBCPP_INLINE_VISIBILITY
     basic_string& insert(size_type __pos1, const basic_string& __str);
     _LIBCPP_INLINE_VISIBILITY
     basic_string& insert(size_type __pos1, __self_view __sv) { return insert(__pos1, __sv.data(), __sv.size()); }
     template <class _Tp>
-    inline typename enable_if
+    _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS
+    typename enable_if
         <
             __can_be_converted_to_string_view<_CharT, _Traits, _Tp>::value,
             basic_string&
         >::type
                   insert(size_type __pos1, const _Tp& __t, size_type __pos2, size_type __n=npos);
     basic_string& insert(size_type __pos1, const basic_string& __str, size_type __pos2, size_type __n=npos);
     basic_string& insert(size_type __pos, const value_type* __s, size_type __n);
     basic_string& insert(size_type __pos, const value_type* __s);
     basic_string& insert(size_type __pos, size_type __n, value_type __c);
     iterator      insert(const_iterator __pos, value_type __c);
     _LIBCPP_INLINE_VISIBILITY
     iterator      insert(const_iterator __pos, size_type __n, value_type __c);
     template<class _InputIterator>
-    inline typename enable_if
+    _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS
+    typename enable_if
         <
            __is_exactly_input_iterator<_InputIterator>::value
                 || !__libcpp_string_gets_noexcept_iterator<_InputIterator>::value,
             iterator
         >::type
         insert(const_iterator __pos, _InputIterator __first, _InputIterator __last);
     template<class _ForwardIterator>
-    inline typename enable_if
+    _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS
+    typename enable_if
         <
             __is_forward_iterator<_ForwardIterator>::value
                  && __libcpp_string_gets_noexcept_iterator<_ForwardIterator>::value,
             iterator
         >::type
         insert(const_iterator __pos, _ForwardIterator __first, _ForwardIterator __last);
 #ifndef _LIBCPP_HAS_NO_GENERALIZED_INITIALIZERS
     _LIBCPP_INLINE_VISIBILITY
     iterator insert(const_iterator __pos, initializer_list<value_type> __il)
                     {return insert(__pos, __il.begin(), __il.end());}
 #endif  // _LIBCPP_HAS_NO_GENERALIZED_INITIALIZERS
 
     basic_string& erase(size_type __pos = 0, size_type __n = npos);
     _LIBCPP_INLINE_VISIBILITY
     iterator      erase(const_iterator __pos);
     _LIBCPP_INLINE_VISIBILITY
     iterator      erase(const_iterator __first, const_iterator __last);
 
     _LIBCPP_INLINE_VISIBILITY
     basic_string& replace(size_type __pos1, size_type __n1, const basic_string& __str);
     _LIBCPP_INLINE_VISIBILITY
     basic_string& replace(size_type __pos1, size_type __n1, __self_view __sv) { return replace(__pos1, __n1, __sv.data(), __sv.size()); }
     basic_string& replace(size_type __pos1, size_type __n1, const basic_string& __str, size_type __pos2, size_type __n2=npos);
     template <class _Tp>
-    inline typename enable_if
+    _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS
+    typename enable_if
         <
             __can_be_converted_to_string_view<_CharT, _Traits, _Tp>::value,
             basic_string&
         >::type
                   replace(size_type __pos1, size_type __n1, const _Tp& __t, size_type __pos2, size_type __n2=npos);
     basic_string& replace(size_type __pos, size_type __n1, const value_type* __s, size_type __n2);
     basic_string& replace(size_type __pos, size_type __n1, const value_type* __s);
     basic_string& replace(size_type __pos, size_type __n1, size_type __n2, value_type __c);
     _LIBCPP_INLINE_VISIBILITY
     basic_string& replace(const_iterator __i1, const_iterator __i2, const basic_string& __str);
     _LIBCPP_INLINE_VISIBILITY
     basic_string& replace(const_iterator __i1, const_iterator __i2, __self_view __sv) { return replace(__i1 - begin(), __i2 - __i1, __sv); }
     _LIBCPP_INLINE_VISIBILITY
     basic_string& replace(const_iterator __i1, const_iterator __i2, const value_type* __s, size_type __n);
     _LIBCPP_INLINE_VISIBILITY
     basic_string& replace(const_iterator __i1, const_iterator __i2, const value_type* __s);
     _LIBCPP_INLINE_VISIBILITY
     basic_string& replace(const_iterator __i1, const_iterator __i2, size_type __n, value_type __c);
     template<class _InputIterator>
-    inline typename enable_if
+    _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS
+    typename enable_if
         <
             __is_input_iterator<_InputIterator>::value,
             basic_string&
         >::type
         replace(const_iterator __i1, const_iterator __i2, _InputIterator __j1, _InputIterator __j2);
 #ifndef _LIBCPP_HAS_NO_GENERALIZED_INITIALIZERS
     _LIBCPP_INLINE_VISIBILITY
     basic_string& replace(const_iterator __i1, const_iterator __i2, initializer_list<value_type> __il)

libcxx/trunk/lib/abi/CHANGELOG.TXT

 Afterwards the ABI list should be updated to include the new changes.
 
 New entries should be added directly below the "Version" header.
 
 -----------
 Version 5.0
 -----------
 
-* rTBD - Remove std::num_get template methods which should be inline
+* r296729 - Remove std::num_get template methods which should be inline
 
   These functions should never have had visible definitions in the dylib but
   since they were previously not specified with 'inline' they accidentally
   got emitted. This change is non-ABI breaking because every "linkage unit"
   must contain its own definition.
 
   x86_64-linux-gnu
   ----------------