Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

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

Authored by philnik on Jul 9 2023, 11:21 AM.

Details

Reviewers
ldionne
Mordante
Group Reviewers
Restricted Project
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.

Diff Detail

Unit TestsFailed

TimeTest
1,870 mslibcxx-ci - MinGW (DLL, i686) > llvm-libc++-mingw-cfg-in.std/input_output/string_streams/ostringstream/ostringstream_assign::move.pass.cpp
Script: -- : 'COMPILED WITH'; C:/llvm-mingw/bin/i686-w64-mingw32-clang++.exe C:\ws\src\libcxx\test\std\input.output\string.streams\ostringstream\ostringstream.assign\move.pass.cpp --target=i686-w64-windows-gnu -nostdinc++ -I C:/ws/src/build/mingw-dll-i686/include/c++/v1 -I C:/ws/src/build/mingw-dll-i686/include/c++/v1 -I C:/ws/src/libcxx/test/support -std=c++2b -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wunused-template -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-noexcept-type -Wno-atomic-alignment -Wdeprecated-copy -Wdeprecated-copy-dtor -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -Wno-local-type-template-args -Wno-c++11-extensions -Wno-unknown-pragmas -Wno-pass-failed -Wno-mismatched-new-delete -Wno-redundant-move -Wno-self-move -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -Werror=thread-safety -Wuser-defined-warnings -DTEST_WINDOWS_DLL -lc++experimental -nostdlib++ -L C:/ws/src/build/mingw-dll-i686/lib -lc++ -o C:\ws\src\build\mingw-dll-i686\test\std\input.output\string.streams\ostringstream\ostringstream.assign\Output\move.pass.cpp.dir\t.tmp.exe
1,840 mslibcxx-ci - MinGW (DLL, i686) > llvm-libc++-mingw-cfg-in.std/input_output/string_streams/stringstream/stringstream_assign::move.pass.cpp
Script: -- : 'COMPILED WITH'; C:/llvm-mingw/bin/i686-w64-mingw32-clang++.exe C:\ws\src\libcxx\test\std\input.output\string.streams\stringstream\stringstream.assign\move.pass.cpp --target=i686-w64-windows-gnu -nostdinc++ -I C:/ws/src/build/mingw-dll-i686/include/c++/v1 -I C:/ws/src/build/mingw-dll-i686/include/c++/v1 -I C:/ws/src/libcxx/test/support -std=c++2b -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wunused-template -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-noexcept-type -Wno-atomic-alignment -Wdeprecated-copy -Wdeprecated-copy-dtor -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -Wno-local-type-template-args -Wno-c++11-extensions -Wno-unknown-pragmas -Wno-pass-failed -Wno-mismatched-new-delete -Wno-redundant-move -Wno-self-move -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -Werror=thread-safety -Wuser-defined-warnings -DTEST_WINDOWS_DLL -lc++experimental -nostdlib++ -L C:/ws/src/build/mingw-dll-i686/lib -lc++ -o C:\ws\src\build\mingw-dll-i686\test\std\input.output\string.streams\stringstream\stringstream.assign\Output\move.pass.cpp.dir\t.tmp.exe
1,830 mslibcxx-ci - MinGW (DLL, x86_64) > llvm-libc++-mingw-cfg-in.std/input_output/string_streams/ostringstream/ostringstream_assign::move.pass.cpp
Script: -- : 'COMPILED WITH'; C:/llvm-mingw/bin/x86_64-w64-mingw32-clang++.exe C:\ws\src\libcxx\test\std\input.output\string.streams\ostringstream\ostringstream.assign\move.pass.cpp --target=x86_64-w64-windows-gnu -nostdinc++ -I C:/ws/src/build/mingw-dll/include/c++/v1 -I C:/ws/src/build/mingw-dll/include/c++/v1 -I C:/ws/src/libcxx/test/support -std=c++2b -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wunused-template -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-noexcept-type -Wno-atomic-alignment -Wdeprecated-copy -Wdeprecated-copy-dtor -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -Wno-local-type-template-args -Wno-c++11-extensions -Wno-unknown-pragmas -Wno-pass-failed -Wno-mismatched-new-delete -Wno-redundant-move -Wno-self-move -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -Werror=thread-safety -Wuser-defined-warnings -DTEST_WINDOWS_DLL -lc++experimental -nostdlib++ -L C:/ws/src/build/mingw-dll/lib -lc++ -o C:\ws\src\build\mingw-dll\test\std\input.output\string.streams\ostringstream\ostringstream.assign\Output\move.pass.cpp.dir\t.tmp.exe
1,850 mslibcxx-ci - MinGW (DLL, x86_64) > llvm-libc++-mingw-cfg-in.std/input_output/string_streams/stringstream/stringstream_assign::move.pass.cpp
Script: -- : 'COMPILED WITH'; C:/llvm-mingw/bin/x86_64-w64-mingw32-clang++.exe C:\ws\src\libcxx\test\std\input.output\string.streams\stringstream\stringstream.assign\move.pass.cpp --target=x86_64-w64-windows-gnu -nostdinc++ -I C:/ws/src/build/mingw-dll/include/c++/v1 -I C:/ws/src/build/mingw-dll/include/c++/v1 -I C:/ws/src/libcxx/test/support -std=c++2b -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wunused-template -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-noexcept-type -Wno-atomic-alignment -Wdeprecated-copy -Wdeprecated-copy-dtor -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -Wno-local-type-template-args -Wno-c++11-extensions -Wno-unknown-pragmas -Wno-pass-failed -Wno-mismatched-new-delete -Wno-redundant-move -Wno-self-move -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -Werror=thread-safety -Wuser-defined-warnings -DTEST_WINDOWS_DLL -lc++experimental -nostdlib++ -L C:/ws/src/build/mingw-dll/lib -lc++ -o C:\ws\src\build\mingw-dll\test\std\input.output\string.streams\stringstream\stringstream.assign\Output\move.pass.cpp.dir\t.tmp.exe

Event Timeline

philnik created this revision.Jul 9 2023, 11:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 9 2023, 11:21 AM
philnik requested review of this revision.Jul 9 2023, 11:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 9 2023, 11:21 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante retitled this revision from [libc++] Recategorize additional instnatiations in the dylib as availability macros to [libc++] Recategorize additional instantiations in the dylib as availability macros.Jul 10 2023, 8:48 AM

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

ldionne requested changes to this revision.Jul 10 2023, 11:20 AM
ldionne added a subscriber: 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.

This revision now requires changes to proceed.Jul 10 2023, 11:20 AM
philnik updated this revision to Diff 539248.Jul 11 2023, 12:34 PM
philnik marked 2 inline comments as done.

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 updated this revision to Diff 540493.Jul 14 2023, 10:42 AM

Try to fix CI

Mordante accepted this revision as: Mordante.Jul 17 2023, 9:18 AM

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

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 updated this revision to Diff 551280.Aug 17 2023, 2:45 PM
philnik marked 4 inline comments as done.

Address comments

philnik edited the summary of this revision. (Show Details)Aug 18 2023, 8:59 AM
ldionne requested changes to this revision.Aug 18 2023, 9:37 AM
ldionne added inline comments.
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).

This revision now requires changes to proceed.Aug 18 2023, 9:37 AM
philnik updated this revision to Diff 555640.Sun, Sep 3, 4:09 PM
philnik marked 2 inline comments as done.

Address comments

ldionne accepted this revision.Thu, Sep 14, 9:21 AM

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.

This revision is now accepted and ready to land.Thu, Sep 14, 9:21 AM
philnik edited the summary of this revision. (Show Details)Thu, Sep 14, 12:17 PM
philnik updated this revision to Diff 556926.Sun, Sep 17, 11:26 PM
philnik edited the summary of this revision. (Show Details)

Try to fix CI

philnik updated this revision to Diff 556955.Mon, Sep 18, 8:48 AM

Try to fix CI

philnik updated this revision to Diff 556994.Mon, Sep 18, 8:52 PM

Try to fix CI

philnik updated this revision to Diff 557001.Mon, Sep 18, 11:01 PM

Try to fix CI

ldionne requested changes to this revision.Thu, Sep 21, 2:35 PM

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

This revision now requires changes to proceed.Thu, Sep 21, 2:35 PM