This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Complete the implementation of N4190
ClosedPublic

Authored by philnik on Apr 24 2022, 8:13 AM.

Details

Summary

Fixes #37402

Diff Detail

Event Timeline

philnik created this revision.Apr 24 2022, 8:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 24 2022, 8:13 AM
Herald added a subscriber: arphaman. · View Herald Transcript
philnik requested review of this revision.Apr 24 2022, 8:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 24 2022, 8:13 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne requested changes to this revision.Apr 25 2022, 9:21 AM
ldionne added inline comments.
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
1100

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
102

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
24

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.

This revision now requires changes to proceed.Apr 25 2022, 9:21 AM
philnik marked 5 inline comments as done.Apr 30 2022, 7:23 AM
philnik added inline comments.
libcxx/include/__config
1100

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
102

How is the condition always valid?

libcxx/include/ext/__hash
24

I'm going with removing them, since they are removed in C++20 anyways.

philnik updated this revision to Diff 426230.Apr 30 2022, 7:23 AM
philnik marked 3 inline comments as done.
  • Address comments
avogelsgesang added inline comments.
libcxx/docs/ReleaseNotes.rst
131

The -> The functions

134

deprectaiton -> deprecation

philnik updated this revision to Diff 427598.May 6 2022, 4:31 AM
philnik marked 2 inline comments as done.
  • Fix CI
philnik updated this revision to Diff 428012.May 9 2022, 2:26 AM
  • Fix GCC C++11
philnik edited the summary of this revision. (Show Details)May 9 2022, 4:26 AM
ldionne requested changes to this revision.May 18 2022, 2:08 PM
ldionne added inline comments.
libcxx/include/__functional/binder2nd.h
24

Unrelated, but we should rename this to _Operation (elsewhere too). Can you make a NFC commit?

ldionne added inline comments.May 18 2022, 2:08 PM
libcxx/include/__config
1098–1099

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?

1100

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 (🤞🏻).

libcxx/include/ext/__hash
24

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.

This revision now requires changes to proceed.May 18 2022, 2:08 PM
philnik marked 4 inline comments as done.May 19 2022, 8:10 AM
philnik added inline comments.
libcxx/include/__config
1098–1099

There are already tests for all of them. They haven't been guarded with _LIBCPP_ABI_NO_BINDER_BASES.

libcxx/include/__functional/binder2nd.h
24

I've added a TODO on my list.

philnik updated this revision to Diff 430689.May 19 2022, 8:10 AM
philnik marked 2 inline comments as done.
  • Address comments
philnik updated this revision to Diff 431238.May 22 2022, 6:51 AM
  • Next try
philnik updated this revision to Diff 431265.May 22 2022, 2:25 PM
  • Try to fix CI
ldionne added inline comments.Jun 2 2022, 8:57 AM
libcxx/docs/ReleaseNotes.rst
134

Still a typo!

libcxx/include/__functional/binary_function.h
41
libcxx/include/__functional/hash.h
380–381

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
42–44

Why is this needed?

libcxx/include/__functional/unary_function.h
31
libcxx/include/__functional/unary_negate.h
37–38

I don't think that should be there.

libcxx/include/__functional/weak_result_type.h
224–225

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
4539–4540

I don't think you need to push/pop here?

libcxx/include/string_view
915–916

Same.

libcxx/include/system_error
439–440

Same.

libcxx/include/thread
202–203

Same.

libcxx/include/typeindex
96–97

Same.

libcxx/include/vector
3140–3141

Same.

libcxx/test/std/depr/depr.function.objects/depr.base/depr.verify.cpp
9–18 ↗(On Diff #431265)

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
2

Thanks for adding this coverage.

philnik updated this revision to Diff 434462.Jun 6 2022, 7:12 AM
philnik marked 16 inline comments as done.
  • Address comments
libcxx/include/__functional/mem_fn.h
42–44

I can't remember why I added it and everything works without it, so I removed it.

libcxx/include/__functional/weak_result_type.h
224–225

Added a TODO.

philnik updated this revision to Diff 435236.Jun 8 2022, 10:09 AM
  • Rebased
  • Add new feature release note
ldionne accepted this revision.Jun 9 2022, 7:58 AM

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
21

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

20

Same thing here, let's add a comment explaining the tricks we do.

This revision is now accepted and ready to land.Jun 9 2022, 7:58 AM
philnik updated this revision to Diff 435626.Jun 9 2022, 12:00 PM
philnik marked 2 inline comments as done.
  • Rebased
  • Address comments
philnik marked an inline comment as done.Jun 9 2022, 12:00 PM
EricWF requested changes to this revision.Jun 9 2022, 2:32 PM
EricWF added a subscriber: EricWF.

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
58–60

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

Or just turn off the warning?

This revision now requires changes to proceed.Jun 9 2022, 2:32 PM
ldionne added inline comments.Jun 10 2022, 7:47 AM
libcxx/include/__functional/unary_function.h
51

Also this generates different ABI's between C++14 and C++17, which is a not OK.

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):

  • C++ <= 14: we inherit from unary_function<Arg, Result>
  • C++ > 14: we inherit from __unary_function_keep_layout_base<Arg, Result>

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.

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?

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.

EricWF added inline comments.Jun 11 2022, 8:02 AM
libcxx/include/__functional/unary_function.h
51

Consider when _LIBCPP_ABI_NO_BINDER_BASES is defined.
It's observable if you're inheriting from __unary_function_ebo_base base or not.

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?

EricWF added inline comments.Jun 11 2022, 8:05 AM
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.

philnik updated this revision to Diff 436332.Jun 13 2022, 3:30 AM
  • Remove _LIBCPP_ABI_NO_BINDER_BASES
philnik updated this revision to Diff 438485.Jun 20 2022, 3:03 PM
  • Rebased
  • Try to fix CI
philnik updated this revision to Diff 438570.Jun 20 2022, 11:52 PM
  • Try to fix CI
philnik updated this revision to Diff 438590.Jun 21 2022, 1:18 AM
  • Next try
This revision was not accepted when it landed; it landed in state Needs Review.Jun 22 2022, 1:13 AM
This revision was automatically updated to reflect the committed changes.
cmtice added a subscriber: cmtice.Jun 22 2022, 7:34 AM

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.

@philnik Ah, thanks for pointing that out. Ignore my previous comment. :-)