[libc++] Recategorize additional instantiations in the dylib as availability macros

[philnik]

Summary

Adding additional instantiations to the dylib isn't actually an ABI break as long as programs targeting an older dylib don't start to depend on them. Making additional instantiations a matter of availability allows us to add them without an ABI break.

[Mordante]

No objection to this approach, but I would like to see the CI pass.

[ldionne]

Nice! I don't know why we didn't do those using availability in the first place, it seems strictly better.

libcxx/include/__availability

180

I would turn these conditions around instead and use HAS_NO_XXX like all the other availability macros.

181

This macro needs a separate comment.

327

Let's add an entry here for _LIBCPP_AVAILABILITY_ENABLE_BAD_FUNCTION_CALL_KEY_FUNCTION. Those are the macros you should check to determine that the dylib *did not yet* contain the key function:

defined(__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__) && __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ < 120000
defined(__ENVIRONMENT_IPHONE_OS_VERSION_MIN_REQUIRED__) && __ENVIRONMENT_IPHONE_OS_VERSION_MIN_REQUIRED__ < 150000
defined(__ENVIRONMENT_TV_OS_VERSION_MIN_REQUIRED__) && __ENVIRONMENT_TV_OS_VERSION_MIN_REQUIRED__ < 150000
defined(__ENVIRONMENT_WATCH_OS_VERSION_MIN_REQUIRED__) && __ENVIRONMENT_WATCH_OS_VERSION_MIN_REQUIRED__ < 80000

libcxx/include/__config

176–181

This needs to be translated to the availability, or it'll fail on FreeBSD. FreeBSD doesn't currently enable availability markup, but that is probably needed as a pre-requisite. We could start by unconditionally pretending that all the _LIBCPP_AVAILABILITY_XXXXXXXX are enabled on FreeBSD, which would be equivalent to what they have today.

[philnik]

Address comments

libcxx/include/__availability

180

I think it makes sense to do it this way, since these are "we added them later" instead of "this is the minimum OS version for this feature".

libcxx/include/__config

176–181

I'm a bit confused. This has nothing to do with FreeBSD.

[philnik]

Try to fix CI

[Mordante]

LGTM, but I like @ldionne to give the final approval.

[ldionne]

I would like to split the good-what-message part of this change into its own patch, since it's significantly different from the other changes. But apart from that, this change makes sense to me.

libcxx/include/__availability

183

This comment should probably be copied from the original one?

360

First, this doesn't match the macro above (which is _LIBCPP_AVAILABILITY_ENABLE_BAD_FUNCTION_CALL_GOOD_WHAT_MESSAGE_AND_KEY_FUNCTION). Second, I would like us to split the good-what-message part of this change into its own patch since it is more tricky to get right, and in fact it *is* an ABI break (it might just be one that we can do in case it only affects arm64e).

libcxx/include/__config

176–181

I think I meant AIX here. The comment makes sense if you re-read and say AIX in your head.

[philnik]

Address comments

[ldionne]

libcxx/include/__availability

1

The commit message should be updated to be specific to the additional iostream instantiations.

356–361

#  if (defined(__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__) && __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ < 120000)   || \
      (defined(__ENVIRONMENT_IPHONE_OS_VERSION_MIN_REQUIRED__) && __ENVIRONMENT_IPHONE_OS_VERSION_MIN_REQUIRED__ < 150000) || \
      (defined(__ENVIRONMENT_TV_OS_VERSION_MIN_REQUIRED__) && __ENVIRONMENT_TV_OS_VERSION_MIN_REQUIRED__ < 150000)         || \
      (defined(__ENVIRONMENT_WATCH_OS_VERSION_MIN_REQUIRED__) && __ENVIRONMENT_WATCH_OS_VERSION_MIN_REQUIRED__ < 80000)
#    define _LIBCPP_AVAILABILITY_HAS_NO_ADDITIONAL_IOSTREAM_EXPLICIT_INSTANTIATIONS_1
#  endif

This would be more consistent with the other availability macros. If we want to invert the logic and instead use _LIBCPP_AVAILABILITY_HAS_FOO, let's do it for all the macros in a separate patch (I think it would be a reasonable change since it would avoid breaking vendors when adding new availability macros).

[philnik]

Address comments

[ldionne]

Can you rebase to poke the CI? Also, please remove the reference to the radar from the commit message, since the radar was about bad_function_call (which we will handle separately).

Other than that, LGTM.

[philnik]

Rebased

[philnik]

Try to fix CI

[philnik]

Try to fix CI

[philnik]

Try to fix CI

[philnik]

Try to fix CI

[ldionne]

Requesting changes since I'd like to discuss the latest changes to the patch during our next 1:1

[ldionne]

libcxx/include/__availability

180

Here, let's do this instead:

// Enable additional explicit instantiations of iostreams components. This
// reduces the number of weak definitions generated in programs that use
// iostreams by providing a single strong definition in the shared library.
//
// TODO: Enable additional explicit instantiations on GCC once it supports 
//           exclude_from_explicit_instantiation, or once libc++ doesn't use the attribute anymore.
// TODO: Enable them on Windows once https://llvm.org/PR41018 has been fixed.
#if defined(_LIBCPP_COMPILER_GCC) || defined(_WIN32)
#  define _LIBCPP_AVAILABILITY_HAS_NO_ADDITIONAL_IOSTREAM_EXPLICIT_INSTANTIATIONS_1
#else
// # define _LIBCPP_AVAILABILITY_HAS_NO_ADDITIONAL_IOSTREAM_EXPLICIT_INSTANTIATIONS_1
#endif

Then you can remove the workarounds in libcxx/include/sstream.

[philnik]

Address comments

[thakis]

Does settings _LIBCPP_ABI_VERSION to 2 still opt in to this after this change?

[philnik]

In D154796#4653350, @thakis wrote:

Does settings _LIBCPP_ABI_VERSION to 2 still opt in to this after this change?

I guess it doesn't anymore for Windows or GCC. That definitely wasn't intended, but I'm not sure that's such a bad thing, since we ran into multiple bugs when trying to enable it in ABIv1 there.

[hnrklssn]

This patch is currently our main suspect for this stage2 linker error. @philnik Do you have any ideas off the top of your head for what might cause this?

[philnik]

In D154796#4654012, @hnrklssn wrote:

This patch is currently our main suspect for this stage2 linker error. @philnik Do you have any ideas off the top of your head for what might cause this?

Thanks for the heads-up! Could you check whether e9c101a fixes it?

[hnrklssn]

In D154796#4654047, @philnik wrote:

In D154796#4654012, @hnrklssn wrote:

This patch is currently our main suspect for this stage2 linker error. @philnik Do you have any ideas off the top of your head for what might cause this?

Thanks for the heads-up! Could you check whether e9c101a fixes it?

I’ve had some issues building stage2 builds on my machine, so I’ve struggled reproducing it locally. If you’re certain the new include should be there regardless, I’d say the fastest way to learn whether it fixes this particular problem would be to commit it and wait for CI to build, tbh. Otherwise I’ll give it another go in the morning.

[philnik]

In D154796#4654067, @hnrklssn wrote:

In D154796#4654047, @philnik wrote:

In D154796#4654012, @hnrklssn wrote:

This patch is currently our main suspect for this stage2 linker error. @philnik Do you have any ideas off the top of your head for what might cause this?

Thanks for the heads-up! Could you check whether e9c101a fixes it?

I’ve had some issues building stage2 builds on my machine, so I’ve struggled reproducing it locally. If you’re certain the new include should be there regardless, I’d say the fastest way to learn whether it fixes this particular problem would be to commit it and wait for CI to build, tbh. Otherwise I’ll give it another go in the morning.

I've committed it already, since it's definitely missing. I wasn't sure whether the bot was building from trunk or not.

[hnrklssn]

In D154796#4654068, @philnik wrote:

In D154796#4654067, @hnrklssn wrote:

In D154796#4654047, @philnik wrote:

In D154796#4654012, @hnrklssn wrote:

This patch is currently our main suspect for this stage2 linker error. @philnik Do you have any ideas off the top of your head for what might cause this?

Thanks for the heads-up! Could you check whether e9c101a fixes it?

I’ve had some issues building stage2 builds on my machine, so I’ve struggled reproducing it locally. If you’re certain the new include should be there regardless, I’d say the fastest way to learn whether it fixes this particular problem would be to commit it and wait for CI to build, tbh. Otherwise I’ll give it another go in the morning.

I've committed it already, since it's definitely missing. I wasn't sure whether the bot was building from trunk or not.

Hmm, unfortunately it seems like the issue still persists. This build should contain your fix for both the stage1 and stage2 builds: https://green.lab.llvm.org/green/job/clang-stage2-Rthinlto/7627/console. I still haven't been able to do a stage2 build locally to replicate.

libcxx/include/__availability

 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 

 // #   define _LIBCPP_AVAILABILITY_HAS_NO_VERBOSE_ABORT
 #   define _LIBCPP_AVAILABILITY_VERBOSE_ABORT
 
     // This controls the availability of the C++17 std::pmr library,
     // which is implemented in large part in the built library.
 // #   define _LIBCPP_AVAILABILITY_HAS_NO_PMR
 #   define _LIBCPP_AVAILABILITY_PMR
 
+// Enable additional explicit instantiations of iostreams components. This
+// reduces the number of weak definitions generated in programs that use
+// iostreams by providing a single strong definition in the shared library.
+#   define _LIBCPP_AVAILABILITY_ENABLE_ADDITIONAL_IOSTREAM_EXPLICIT_INSTANTIATIONS_1
+
 #elif defined(__APPLE__)
 
     // shared_mutex and shared_timed_mutex
 #   if (defined(__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__) && __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ < 101200) ||    \
         (defined(__ENVIRONMENT_IPHONE_OS_VERSION_MIN_REQUIRED__) && __ENVIRONMENT_IPHONE_OS_VERSION_MIN_REQUIRED__ < 100000) || \
         (defined(__ENVIRONMENT_TV_OS_VERSION_MIN_REQUIRED__) && __ENVIRONMENT_TV_OS_VERSION_MIN_REQUIRED__ < 100000) ||         \
         (defined(__ENVIRONMENT_WATCH_OS_VERSION_MIN_REQUIRED__) && __ENVIRONMENT_WATCH_OS_VERSION_MIN_REQUIRED__ < 30000)
 #       define _LIBCPP_AVAILABILITY_HAS_NO_SHARED_MUTEX
 #   endif
 #   define _LIBCPP_AVAILABILITY_SHARED_MUTEX                                    \
#   if (defined(__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__) && __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ < 110000) ||    \
         (defined(__ENVIRONMENT_WATCH_OS_VERSION_MIN_REQUIRED__) && __ENVIRONMENT_WATCH_OS_VERSION_MIN_REQUIRED__ < 70000)
 #       define _LIBCPP_AVAILABILITY_HAS_NO_SYNC
 #   endif
 #   define _LIBCPP_AVAILABILITY_SYNC                                            \
         __attribute__((availability(macos,strict,introduced=11.0)))             \
         __attribute__((availability(ios,strict,introduced=14.0)))               \
         __attribute__((availability(tvos,strict,introduced=14.0)))              \
         __attribute__((availability(watchos,strict,introduced=7.0)))
 
     // __libcpp_verbose_abort
 #   if 1 // TODO: Update once this is released
 #       define _LIBCPP_AVAILABILITY_HAS_NO_VERBOSE_ABORT
 #   endif
 #   define _LIBCPP_AVAILABILITY_VERBOSE_ABORT                                   \
         __attribute__((unavailable))
 
     // std::pmr
#    define _LIBCPP_AVAILABILITY_PMR                                                                                   \
       __attribute__((availability(macos, strict, introduced = 14.0)))                                                  \
       __attribute__((availability(ios, strict, introduced = 17.0)))                                                    \
       __attribute__((availability(tvos, strict, introduced = 17.0)))                                                   \
       __attribute__((availability(watchos, strict, introduced = 10.0)))
 #  else
 #    define _LIBCPP_AVAILABILITY_PMR
 #  endif
 
+#  if (defined(__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__) && __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ >= 120000)   || \
+      (defined(__ENVIRONMENT_IPHONE_OS_VERSION_MIN_REQUIRED__) && __ENVIRONMENT_IPHONE_OS_VERSION_MIN_REQUIRED__ >= 150000) || \
+      (defined(__ENVIRONMENT_TV_OS_VERSION_MIN_REQUIRED__) && __ENVIRONMENT_TV_OS_VERSION_MIN_REQUIRED__ >= 150000)         || \
+      (defined(__ENVIRONMENT_WATCH_OS_VERSION_MIN_REQUIRED__) && __ENVIRONMENT_WATCH_OS_VERSION_MIN_REQUIRED__ >= 80000)
+#    define _LIBCPP_AVAILABILITY_ENABLE_ADDITIONAL_IOSTREAM_EXPLICIT_INSTANTIATIONS_1
+#  endif
+
 #else
 
 // ...New vendors can add availability markup here...
 
 #   error "It looks like you're trying to enable vendor availability markup, but you haven't defined the corresponding macros yet!"
 
 #endif
 

libcxx/include/__config

 // so disable it.
 #      define _LIBCPP_DEPRECATED_ABI_LEGACY_LIBRARY_DEFINITIONS_FOR_INLINE_FUNCTIONS
 #    endif
 // Feature macros for disabling pre ABI v1 features. All of these options
 // are deprecated.
 #    if defined(__FreeBSD__)
 #      define _LIBCPP_DEPRECATED_ABI_DISABLE_PAIR_TRIVIAL_COPY_CTOR
 #    endif
 // For XCOFF linkers, we have problems if we see a weak hidden version of a symbol
 // in user code (like you get with -fvisibility-inlines-hidden) and then a strong def
 // in the library, so we need to always rely on the library version.
 #    if defined(_AIX)
 #      define _LIBCPP_ABI_BAD_FUNCTION_CALL_KEY_FUNCTION
 #    endif
 #  endif
 
 #  if defined(_LIBCPP_BUILDING_LIBRARY) || _LIBCPP_ABI_VERSION >= 2
-// Enable additional explicit instantiations of iostreams components. This
-// reduces the number of weak definitions generated in programs that use
-// iostreams by providing a single strong definition in the shared library.
-#    define _LIBCPP_ABI_ENABLE_ADDITIONAL_IOSTREAM_EXPLICIT_INSTANTIATIONS_1
-
 // Define a key function for `bad_function_call` in the library, to centralize
 // its vtable and typeinfo to libc++ rather than having all other libraries
 // using that class define their own copies.
 #    define _LIBCPP_ABI_BAD_FUNCTION_CALL_KEY_FUNCTION
 #  endif
 
 // Changes the iterator type of select containers (see below) to a bounded iterator that keeps track of whether it's
 // within the bounds of the original container and asserts it on every dereference.

libcxx/include/fstream

 inline
 void
 basic_fstream<_CharT, _Traits>::close()
 {
     if (__sb_.close() == nullptr)
         this->setstate(ios_base::failbit);
 }
 
-#if defined(_LIBCPP_ABI_ENABLE_ADDITIONAL_IOSTREAM_EXPLICIT_INSTANTIATIONS_1)
+#if defined(_LIBCPP_AVAILABILITY_ENABLE_ADDITIONAL_IOSTREAM_EXPLICIT_INSTANTIATIONS_1)
 extern template class _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS basic_ifstream<char>;
 extern template class _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS basic_ofstream<char>;
 extern template class _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS basic_filebuf<char>;
 #endif
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP_HAS_NO_FILESYSTEM

libcxx/include/sstream

 inline _LIBCPP_INLINE_VISIBILITY
 void
 swap(basic_stringstream<_CharT, _Traits, _Allocator>& __x,
      basic_stringstream<_CharT, _Traits, _Allocator>& __y)
 {
     __x.swap(__y);
 }
 
-#if defined(_LIBCPP_ABI_ENABLE_ADDITIONAL_IOSTREAM_EXPLICIT_INSTANTIATIONS_1)
+#if defined(_LIBCPP_AVAILABILITY_ENABLE_ADDITIONAL_IOSTREAM_EXPLICIT_INSTANTIATIONS_1)
 extern template class _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS basic_stringbuf<char>;
 extern template class _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS basic_stringstream<char>;
 extern template class _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS basic_ostringstream<char>;
 extern template class _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS basic_istringstream<char>;
 #endif
 
 _LIBCPP_END_NAMESPACE_STD
 
 _LIBCPP_POP_MACROS
 
 #if _LIBCPP_STD_VER <= 20 && !defined(_LIPCPP_REMOVE_TRANSITIVE_INCLUDES)
 #  include <type_traits>
 #endif
 
 #endif // _LIBCPP_SSTREAM

libcxx/src/ios.instantiations.cpp

 #ifndef _LIBCPP_HAS_NO_WIDE_CHARACTERS
 template class _LIBCPP_CLASS_TEMPLATE_INSTANTIATION_VIS basic_ios<wchar_t>;
 template class _LIBCPP_CLASS_TEMPLATE_INSTANTIATION_VIS basic_streambuf<wchar_t>;
 template class _LIBCPP_CLASS_TEMPLATE_INSTANTIATION_VIS basic_istream<wchar_t>;
 template class _LIBCPP_CLASS_TEMPLATE_INSTANTIATION_VIS basic_ostream<wchar_t>;
 #endif
 
 // Additional instantiations added later. Whether programs rely on these being
-// available is protected by _LIBCPP_ABI_ENABLE_ADDITIONAL_IOSTREAM_EXPLICIT_INSTANTIATIONS_1.
+// available is protected by _LIBCPP_AVAILABILITY_ENABLE_ADDITIONAL_IOSTREAM_EXPLICIT_INSTANTIATIONS_1.
 template class _LIBCPP_CLASS_TEMPLATE_INSTANTIATION_VIS basic_stringbuf<char>;
 template class _LIBCPP_CLASS_TEMPLATE_INSTANTIATION_VIS basic_stringstream<char>;
 template class _LIBCPP_CLASS_TEMPLATE_INSTANTIATION_VIS basic_ostringstream<char>;
 template class _LIBCPP_CLASS_TEMPLATE_INSTANTIATION_VIS basic_istringstream<char>;
 
 #ifndef _LIBCPP_HAS_NO_FILESYSTEM
 template class _LIBCPP_CLASS_TEMPLATE_INSTANTIATION_VIS basic_ifstream<char>;
 template class _LIBCPP_CLASS_TEMPLATE_INSTANTIATION_VIS basic_ofstream<char>;
 template class _LIBCPP_CLASS_TEMPLATE_INSTANTIATION_VIS basic_filebuf<char>;
 #endif
 
 // Add more here if needed...
 
 _LIBCPP_END_NAMESPACE_STD