Fixes #37402
Details
- Reviewers
var-const Mordante ldionne EricWF - Group Reviewers
Restricted Project - Commits
- rG681cde7dd8b5: [libc++] Complete the implementation of N4190
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
libcxx/docs/Status/Cxx17Papers.csv | ||
---|---|---|
1 | We need to add a release note, since this will cause some code to start failing to compile (because of deprecation warnings and the removed classes). | |
libcxx/include/__config | ||
1235 | I don't understand why we're making an ABI change in the stable ABI? | |
libcxx/include/__functional/binary_function.h | ||
24 | Can you please add a test to check that this is deprecated in C++11? (same for others). | |
libcxx/include/__functional/function.h | ||
100 | This comment is inconsistent with the #if's condition. However, regardless, I think we should remove the #if entirely, since it is always valid. | |
libcxx/include/ext/__hash | ||
25 | Technically, I think we still need to be providing the argument_type and result_type typedefs in C++17, but they should be deprecated. In practice, I don't know whether we care, it may be acceptable to just remove them. But whatever we do, we should be consistent between different specializations of std::hash (the current patch isn't, since e.g. std::hash<std::bitset<...>> provides those typedefs. Furthermore, if we do provide them in C++17, they should be marked as deprecated. In C++20, they should never be provided. |
libcxx/include/__config | ||
---|---|---|
1235 | AFAICT this can only be ABI affecting if you have a class that is derived from both unary_function and another class that derives from it. Since there is no more unary_function to derive from, there is no way to have an ABI break. Same for binary_function. | |
libcxx/include/__functional/function.h | ||
100 | How is the condition always valid? | |
libcxx/include/ext/__hash | ||
25 | I'm going with removing them, since they are removed in C++20 anyways. |
libcxx/include/__functional/binder2nd.h | ||
---|---|---|
24 | Unrelated, but we should rename this to _Operation (elsewhere too). Can you make a NFC commit? |
libcxx/include/__config | ||
---|---|---|
1235 | I don't think that is correct. Indeed, let's consider: template <class _Pred> class binary_negate : public binary_function<_Pred::first_argument_type, _Pred::second_argument_type, bool> { ... }; template <class _Arg1, class _Arg2, class _Result> class pointer_to_binary_function : public binary_function<_Arg1, _Arg2, _Result> { ... }; The exact classes don't matter, I just picked two classes that derived from binary_function. Now, let's take a look at the layout of the following type: struct Foo : binary_negate<bool(int,int)>, pointer_to_binary_function<int, int, bool> { }; Without the bases, sizeof(Foo) is 1, and it is 2 with the bases. See https://godbolt.org/z/5b3PnjK54. This sucks because it makes this PR more complicated. One simple thing we could do is inherit from a new type called __binary_function_or_empty <binary_negate<_Pred>, _Pred::first_argument_type, _Pred::second_argument_type, bool> (and similarly for all function objects): template <class> struct __empty { }; template <class, class, class> struct __binary_function_layout { }; #if _LIBCPP_STD_VER <= 14 || defined(_LIBCPP_ENABLE_CXX17_REMOVED_UNARY_BINARY_FUNCTION) template <class _Token, class, class, class> using __binary_function_or_empty = binary_function<_Arg1, _Arg2, _Result>; #elif defined(_LIBCPP_ABI_NO_BINDER_BASES) template <class _Token, class, class, class> using __binary_function_or_empty = __empty<_Token>; #else template <class _Token, class _Arg1, class _Arg2, class _Result> using __binary_function_or_empty = __binary_function_layout<_Arg1, _Arg2, _Result>; #endif That way, we can define function objects unconditionally like template <class _Pred> class binary_negate : public __binary_function_or_empty<binary_negate<_Pred>, _Pred::first_argument_type, _Pred::second_argument_type, bool> { ... }; Since the _Token is guaranteed to be unique, we'll always get the EBO (🤞🏻). | |
1245 | For the types that are specified as inheriting from unary_function/binary_function, can you please add a test to check that we inherit from them in C++ <= 14? | |
libcxx/include/ext/__hash | ||
25 | I'm sorry for the churn, but I don't know why I said it was reasonable not to provide them. Looking at cppreference, they are clearly documented as being there in C++17, so I think we should provide them, but mark them as deprecated. |
libcxx/docs/ReleaseNotes.rst | ||
---|---|---|
96 | Still a typo! | |
libcxx/include/__functional/binary_function.h | ||
41 | ||
libcxx/include/__functional/hash.h | ||
383 ↗ | (On Diff #431265) | It seems to me like we can probably remove these PUSH/POP macros here now, since unary_function is used behind the __unary_function_or_empty alias, which has the right PUSH/POP macros when it is defined. Can you try removing them (here and elsewhere)? |
libcxx/include/__functional/mem_fn.h | ||
46–48 | Why is this needed? | |
libcxx/include/__functional/unary_function.h | ||
31 | ||
libcxx/include/__functional/unary_negate.h | ||
39–40 | I don't think that should be there. | |
libcxx/include/__functional/weak_result_type.h | ||
256 | Something for a different patch, but we do support variadics in C++03 so we can include this code block in all cases, unless I am mistaken. | |
libcxx/include/string | ||
4381–4382 | I don't think you need to push/pop here? | |
libcxx/include/string_view | ||
911–912 | Same. | |
libcxx/include/system_error | ||
436–437 | Same. | |
libcxx/include/thread | ||
199–200 | Same. | |
libcxx/include/typeindex | ||
104–105 | Same. | |
libcxx/include/vector | ||
3227–3228 | Same. | |
libcxx/test/std/depr/depr.function.objects/depr.base/depr.verify.cpp | ||
9–18 | Can you have one test for binary_function and one test for unary_function? Also, please make sure the comments at the top of the test are accurate. | |
libcxx/test/std/diagnostics/syserr/syserr.hash/error_condition.pass.cpp | ||
13 | I think this can stay? And if it should go (because the Standard doesn't mandate that inheritance), then it should go from the other hash tests too. | |
29 | I think this should be guarded with #if TEST_STD_VER <= 14 instead like you've done elsewhere. | |
libcxx/test/std/utilities/function.objects/refwrap/binder_typedefs.compile.pass.cpp | ||
1 ↗ | (On Diff #431265) | Thanks for adding this coverage. |
What a patch. Thanks a lot for your collaboration on all the iterations here, this is really not a simple one. LGTM once my comments are applied, but I'd like you to ping me once you land this so I can post-commit review the ABI test you added. Also, please make sure to get a green CI run.
libcxx/include/__functional/binary_function.h | ||
---|---|---|
23 | Can you please add a comment in this file explaining what's going on? Point out the fact that we're jumping through these hoops to remain ABI compatible. | |
libcxx/include/__functional/unary_function.h | ||
12 | For both unary_function and binary_function, I would like to add a test that would fail if this patch had the effect of changing the layout. You can base it on something like https://godbolt.org/z/5b3PnjK54, like static_assert(sizeof(Foo) == X). | |
22 | Same thing here, let's add a comment explaining the tricks we do. |
I'm pretty sure this change results in cases where, under ABI v2, C++14 and C++17 produce different ABI's. Am I mistaken?
libcxx/include/__functional/unary_function.h | ||
---|---|---|
38 | As I mention below, I don't think this optimization is worth the cost. And I think adding this extra template parameter will increase debug info sizes enough to matter more than the types sizes. | |
51 | I'm not convinced this optimization is worth the complexity it adds to the library. I suspect nobody is really writing code that would benefit from this, and if they are, I don't think they really care about the extra byte. Also this generates different ABI's between C++14 and C++17, which is a not OK. You should be able to link code compiled in different dialects. | |
libcxx/include/__functional/weak_result_type.h | ||
65–66 | This seems super weird and confusing. Does the thing derived from std::binary_function or not? Now we have weird cases. | |
libcxx/test/std/utilities/function.objects/func.require/binary_function.pass.cpp | ||
13 | Yeah, just disable the warning. It seems like a footgun to configure the library differently than the user would to test conforming behavior. The user shouldn't need to pass that flag, so neither should we. | |
libcxx/test/std/utilities/function.objects/refwrap/weak_result.pass.cpp | ||
17 ↗ | (On Diff #435626) | Or just turn off the warning? |
libcxx/include/__functional/unary_function.h | ||
---|---|---|
51 |
I don't see it -- can you explain why we generate different ABIs? The goal of jumping through these hoops was specifically to generate the same ABI, but maybe we missed something (you know how tricky these changes are, so it's great to have additional opinions). This is how I understand the code right now (let's ignore _LIBCPP_ABI_NO_BINDER_BASES for the moment):
In both cases, the layout of the base classes is the same, and they have the same properties with respect to the empty base optimization applying. Did we miss something? | |
libcxx/test/std/utilities/function.objects/func.require/binary_function.pass.cpp | ||
13 | Just to make sure I understand, are you suggesting that we drop the deprecation warnings from this patch entirely? If so, I'd disagree -- that's the whole point of the patch. I know it means we're requesting that users do some work, and that's not the most fun part of our job, but I think it's really important for the health of the ecosystem as a whole. |
Oh, I missed that you said "Under ABI V2". Yes, I think that's correct, actually. It's tricky because we generally don't think about ABI compatibility at all when we start enabling these ABI macros, however I do agree that if we are to ever consider ABI v2 as stable, we need to. It's doubly tricky because I don't know what's the state of other ABI macros w.r.t. standard versions -- have we done other things that would produce a different ABI between C++14 and C++17 in ABI v2? That's possible, we'd need to check.
Concretely for this patch, I think it only means that we need to drop the _LIBCPP_ABI_NO_BINDER_BASES macro. Furthermore, I think it gives us a new criterion for creating these ABI macros in the first place: An ABI macro must not result in a different ABI for different Standard versions, period. I'll document that.
libcxx/include/__functional/unary_function.h | ||
---|---|---|
51 | Consider when _LIBCPP_ABI_NO_BINDER_BASES is defined. Because we pass the extra arg to make sure the base class is unique, can't that uniqueness make something EBO away in C++17 that wouldn't in C++14? |
libcxx/include/__functional/unary_function.h | ||
---|---|---|
51 | Though I grant the cases which trigger this are likely rare. But they're also the same set of cases this EBO optimization is targeting. Please let me know if I'm misunderstanding something. |
Just a heads up, this looks like it may have broken compiler-rt builds. I'm seeing errors like:
ERROR: third_party/stl/BUILD:57:11: Compiling third_party/stl/stl.cppmap failed: (Exit 1) wrapped_clang failed: error executing command (from target third_party/stl:stl) third_party/crosstool/v18/llvm_unstable/toolchain/bin/wrapped_clang '-frandom-seed=blaze-out/k8-opt/bin/third_party/stl/_objs/stl/stl.pcm' -iquote . -iquote blaze-out/k8-opt/genfiles -iquote ... (remaining 224 arguments skipped). [forge_remote_host=lmdw19]
While building module 'third_party/stl:stl':
In file included from <module-includes>:43:
./third_party/stl/cxx17/ext/functional:17:24: error: no template named 'unary_function' in namespace 'std'; did you mean '__unary_function'?
struct identity : std::unary_function<T, T> {
~~~~~^~~~~~~~~~~~~~ __unary_function
third_party/crosstool/v18/llvm_unstable/toolchain/bin/../include/c++/v1/__functional/unary_function.h:46:1: note: 'unary_function' declared here
using unary_function = unary_function_keep_layout_base<_Arg, _Result>;
^
While building module '//third_party/stl:stl':
In file included from <module-includes>:43:
./third_party/stl/cxx17/ext/functional:22:25: error: no template named 'unary_function' in namespace 'std'; did you mean 'unary_function'?
struct select1st : std::unary_function<Pair, typename Pair::first_type> {
~~~~~^~~~~~~~~~~~~~ __unary_function
third_party/crosstool/v18/llvm_unstable/toolchain/bin/../include/c++/v1/__functional/unary_function.h:46:1: note: 'unary_function' declared here
using unary_function = __unary_function_keep_layout_base<_Arg, _Result>;
(and more)
@cmtice I'm guessing you build with C++17 or higher? In that case it's expected, since unary_function doesn't exist anymore.
The -> The functions