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
- Repository
- rG LLVM Github Monorepo
Time | Test | |
---|---|---|
1,870 ms | libcxx-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 ms | libcxx-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 ms | libcxx-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 ms | libcxx-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
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. |
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. |
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). |
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.
Requesting changes since I'd like to discuss the latest changes to the patch during our next 1:1