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

332

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

170–175

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

170–175

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?

365

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

170–175

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.

361–366

#  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