This is an archive of the discontinued LLVM Phabricator instance.

Constrain tuple/unique_ptr move constructors (2899)
Needs RevisionPublic

Authored by zoecarver on Aug 14 2019, 5:08 PM.

Details

Reviewers
ldionne
mclow.lists
EricWF
Group Reviewers
Restricted Project
Summary

This patch disables unique_ptr& operator=(unique_ptr&&), unique_ptr(unique_ptr&&), and tuple(tuple&&) unless is_move_constructible_v is true. This patch resolves LWG issue 2899.

Diff Detail

Event Timeline

zoecarver created this revision.Aug 14 2019, 5:08 PM
zoecarver marked 3 inline comments as done.Aug 14 2019, 5:13 PM

It did not appear that <tuple> needed to be updated. Correct me if I am wrong.

libcxx/include/memory
2268

The indentation is wrong in this section of the file.

2300

The issue said to use is_move_assignable_v<D> but, that will cause some deleters to break. Thoughts on a better way to resolve this? Or should we make people change how they pass their deleter types?

2329

This is unconditionally added because otherwise this overload may be picked when the other overloads are disabled.

ldionne added inline comments.Aug 15 2019, 8:47 AM
libcxx/include/memory
2278

Any reason for preferring this style of enable_if (as a default argument) as opposed to using a default _template_ argument?

2300

Deleters are allowed to be lvalue references to function objects, so I don't think you want to use remove_reference here (nor remove_const). I think it is expected that a unique_ptr<T, Deleter const&> should be expected _not_ to be move assignable.

zoecarver marked 2 inline comments as done.Aug 15 2019, 9:05 AM
zoecarver added inline comments.
libcxx/include/memory
2278

For some reason the first time I tried this it didn't work, so I looked at shared_ptr (where I had remembered something similar), and this is how it was implemented. Just tried it again and it did seem to work, so I will use your suggestion.

2300

Alrighty. That does seem more correct. But, it will force us to change our tests, and when we have to change our tests, people will probably have to change their code. Given how often unique_ptr is used, it may not be desirable to break people's code without guarding it to another version (they might expect it when switching from C++17 to C++2a, but it might be bad for them to re-build libcxx and have their program stop working).

That being said, we should implement the standard, so I will make the change.

ldionne added inline comments.Aug 15 2019, 10:42 AM
libcxx/include/memory
2300

I think what would be useful is to have @mclow.lists 's opinion on this matter. Marshall, do you know whether it was understood that the LWG issue resolution would break some code? Was it deemed OK because it only breaks unique_ptr using something like a Deleter const& (which should be rare)? Are we misunderstanding something?

zoecarver marked an inline comment as done.Sep 1 2019, 3:15 PM
zoecarver added inline comments.
libcxx/include/memory
2300

Agreed. @mclow.lists friendly ping. What are your thoughts on this?

mclow.lists added inline comments.Sep 4 2019, 11:28 AM
libcxx/include/memory
2300

What's the breakage that you're seeing here?

If you're worried about op= on a unique_ptr<T, const D&> - I don't see how that would have ever worked.

zoecarver marked an inline comment as done.Sep 4 2019, 5:31 PM
zoecarver added inline comments.
libcxx/include/memory
2300

That was what I was worried about. Not sure why, though. I think I may have misdiagnosed the cause of an error while creating this patch. I will update this template to be class _Dummy = _Dp.

ldionne added inline comments.Sep 5 2019, 8:26 AM
libcxx/include/memory
2300

So, my initial point is that our test suite currently tests for:

using P = std::unique_ptr<const T, Deleter const&>;
static_assert(std::is_nothrow_assignable<P, P&&>::value, "");

See here. When I take Zoe's patch and use class _Dummy = _Dp instead of class _Dummy = typename remove_const<typename remove_reference<_Dp>::type>::type, this test breaks (and others too):

<snip>/libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.asgn/move_convert.pass.cpp:224:5: error: static_assert failed due to requirement 'std::is_nothrow_assignable<std::__1::unique_ptr<const A, const GenericConvertingDeleter<0> &>, std::__1::unique_ptr<A, const GenericConvertingDeleter<0> &> &&>::value' ""
    static_assert(std::is_nothrow_assignable<U1C, U1&&>::value, "");
    ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<snip>/libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.asgn/move_convert.pass.cpp:410:5: note: in instantiation of function template specialization 'test_sfinae<false>' requested here
    test_sfinae</*IsArray*/false>();
    ^
<snip>/libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.asgn/move_convert.pass.cpp:301:5: error: static_assert failed due to requirement 'std::is_nothrow_assignable<std::__1::unique_ptr<const A, NCDeleter<const A> &>, std::__1::unique_ptr<A, NCDeleter<const A> &> >::value' ""
    static_assert(std::is_nothrow_assignable<APtr, BPtr>::value, "");
    ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Given that, my question was:

  1. Why do we test for that? Does the Standard say it should be valid? I thought this was the case, but it isn't (see below).
  2. If the Standard says it's valid, then was it discussed in LWG that LWG2899's resolution would break that use case?

Why I don't think std::declval<unique_ptr<T, Deleter const&>&>() = std::declval<unique_ptr<T, Deleter const&>&&>() is valid according to the Standard

  1. Deleters are allowed to be lvalue references to function object types per unique_ptr.single#1. And a Deleter const is a valid function object type if Deleter's operator() is const-qualified, per function.objects#1. So Deleter const& is a valid template argument for unique_ptr's deleter.
  2. However, per unique.ptr.single/2: "Otherwise, D is a reference type; remove_­reference_­t<D> shall meet the Cpp17CopyAssignable requirements and assignment of the deleter from an lvalue of type D shall not throw an exception." Since remove_reference_t<D> is Deleter const, it's not Cpp17CopyAssignable and the requirements are not met.

So our tests are wrong, they shouldn't expect move-assignment of a unique_ptr with a Deleter const& (or a Deleter const, for that matter) to be valid.

ldionne added inline comments.Sep 5 2019, 8:29 AM
libcxx/include/memory
2300

In other words, my concern was that LWG2899's resolution would break valid code. But, if my analysis above is correct, that code (move-assigning unique_ptr with Deleter const&) was already invalid. So LWG2899's resolution doesn't break valid code: it causes already invalid code to break in a different way than previously, and that happens to break our tests (because they are incorrect).

zoecarver marked an inline comment as done.Sep 5 2019, 8:58 AM
zoecarver added inline comments.
libcxx/include/memory
2300

Thank you for the message, @ldionne. I talked with @mclow.lists a bit about this yesterday and came to the same conclusion, std::declval<unique_ptr<T, Deleter const&>&>() = std::declval<unique_ptr<T, Deleter const&>&&>() has never been valid. Even before this patch, the above line would fail to compile, however,

using P = std::unique_ptr<const T, Deleter const&>;
static_assert(std::is_nothrow_assignable<P, P&&>::value, "");

Would not. This patch makes sure that is_assignable<...>corrisponds correctly to whether that type may be assigned. Our tests are incorrect; people's code may also be incorrect, but, it is more important that the type trait is correct than the possibility we break someone's code.

With that said, I plan to update both our tests and the enable_if statement (though, for a different reason than I stated in my last comment).

mclow.lists added inline comments.Sep 5 2019, 10:13 AM
libcxx/include/memory
2300

So our tests are wrong, they shouldn't expect move-assignment of a unique_ptr with a Deleter const& (or a Deleter const, for that matter) to be valid.

I concur.

zoecarver updated this revision to Diff 218950.Sep 5 2019, 10:34 AM
  • use _EnableIf when possible
  • update tests and remove std::remove_ref
Herald added a project: Restricted Project. · View Herald TranscriptSep 5 2019, 10:34 AM
mclow.lists added inline comments.Sep 12 2019, 8:36 AM
libcxx/include/memory
2332

Ok, this is just weird. Everywhere else in libc++, we avoid using = delete in C++03 mode, because it's not a thing.
We even have a macro for it in __config:

#ifdef _LIBCPP_CXX03_LANG
#  define _LIBCPP_EQUAL_DELETE
#else
#  define _LIBCPP_EQUAL_DELETE = delete
#endif

And yet, here (twice!) we only do this for C++03.
@EricWF added this in https://llvm.org/r364161; so I think he should explain.

libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.asgn/move.pass.cpp
113

We have a type trait for move_assignable.

zoecarver marked an inline comment as done.Sep 12 2019, 9:02 AM
zoecarver added inline comments.
libcxx/include/memory
2332

Looks like it was just after we dropped support for GCC 4. He was probably just removing some of the C++03 clutter & cleaning up our implementation. But, I'll wait for him to respond.

ldionne added inline comments.Oct 16 2019, 12:05 PM
libcxx/include/memory
2332

Ping @EricWF

ldionne added a reviewer: Restricted Project.Nov 2 2020, 2:34 PM

I think this looks good to me, but can you please rebase onto master and update the review? It'll run the CI and we can see if it fails under some configuration. Then I'll re-read the discussion we had to confirm we're good to go, and we can ship it.

ldionne requested changes to this revision.Nov 2 2020, 2:35 PM

Requesting changes for now so it shows up correctly in the review queue.

This revision now requires changes to proceed.Nov 2 2020, 2:35 PM
zoecarver added inline comments.Nov 3 2020, 10:42 AM
libcxx/include/memory
2332

Feel free to correct me Eric, but to elaborate on my original comment, I suspect this was just to "clean up our implementation." Looking at the commit, you can see that previously these methods were "deleted" in C++03 mode by declaring them privately. After we dropped GCC C++03 support we were able to use = delete instead which is much more explicit. However, as far as I can tell, those methods weren't deleted in any way in the "normal" implementation, so that's probably why they were only applied to C++03 here.

zoecarver updated this revision to Diff 302645.Nov 3 2020, 11:45 AM
zoecarver marked an inline comment as done.
  • Rebase off master.
zoecarver updated this revision to Diff 302647.Nov 3 2020, 11:48 AM
  • Use is_move_constructible and is_move_assignable whenever possible.
zoecarver updated this revision to Diff 302678.Nov 3 2020, 1:40 PM
  • Mark LWG 2899 as complete in status.
zoecarver updated this revision to Diff 304014.Nov 9 2020, 4:49 PM
  • Mark the move constructor as deleted to fix the C++03 tests.
zoecarver updated this revision to Diff 304656.Nov 11 2020, 2:22 PM
  • Rebase to trigger CI.

While attempting to fix the remaining CI build failures I arrived at a problem and I'm not sure what is the best way to proceed. This is the problem:

  1. The "trivial_abi" attribute requires a move constructor or a copy constructor.
  2. The standard requires that unique_ptr's "move constructor" doesn't participate in overload resolution unless a requirement is met. This requires a templated "move constructor" so that the function may be disabled.
  3. A move constructor is not permitted to be templated. From the standard's point of view, I think it's OK for unique_ptr not to have a move constructor so long as it is move constructible (which would be the case if it has a templated "move constructor").

I am not super familiar with the trivial_abi attribute, so maybe there's some way around this. Any suggestions would be appreciated :)

ldionne requested changes to this revision.Nov 24 2020, 7:37 AM

While attempting to fix the remaining CI build failures I arrived at a problem and I'm not sure what is the best way to proceed. This is the problem:

  1. The "trivial_abi" attribute requires a move constructor or a copy constructor.
  2. The standard requires that unique_ptr's "move constructor" doesn't participate in overload resolution unless a requirement is met. This requires a templated "move constructor" so that the function may be disabled.
  3. A move constructor is not permitted to be templated. From the standard's point of view, I think it's OK for unique_ptr not to have a move constructor so long as it is move constructible (which would be the case if it has a templated "move constructor").

I am not super familiar with the trivial_abi attribute, so maybe there's some way around this. Any suggestions would be appreciated :)

Oh, so that's the reason for the failures. Basically we're bypassing the trivial_abi attribute altogether.

The only way I can *maybe* foresee fixing this in the library would be to specialize unique_ptr based on whether the deleter is move constructible (i.e. specialize based on whether the move constructor should be enabled or not). Now, we can't do that without breaking ABI (because it would require changing template parameters to add one we can use enable_if on), so we'd have to instead:

  1. Inherit from a class like __unique_ptr_base which itself does some SFINAE in its template parameters, and has two specializations (one with a move ctor and one without).
  2. Use using __unique_ptr_base::__unique_ptr_base; inside std::unique_ptr.

I'm not 100% sure that works cause I haven't tried it out, but it might be worth trying out. Otherwise, I believe we'd have to discuss with the compiler folks to see whether it makes sense to change the attribute, or we'd need to drop that extension (but that's pretty bad).

Can you try out my suggestion with the base class? I'm thinking about something like:

template <class _Tp, class _Deleter, bool = is_move_constructible<_Deleter>::value>
class __unique_ptr_base;

// Move constructible version
template <class _Tp, class _Deleter, bool = is_move_constructible<_Deleter>::value>
class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI __unique_ptr_base<_Tp, _Deleter, true> {
    __compressed_pair<pointer, deleter_type> __ptr_;

public:
    _LIBCPP_INLINE_VISIBILITY
    __unique_ptr_base(__unique_ptr_base&& __u) _NOEXCEPT { ... }

    // other constructors...
};

// Non move-constructible version
template <class _Tp, class _Deleter, bool = is_move_constructible<_Deleter>::value>
class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI __unique_ptr_base<_Tp, _Deleter, false> {
    __compressed_pair<pointer, deleter_type> __ptr_;

public:
    _LIBCPP_INLINE_VISIBILITY
    __unique_ptr_base(__unique_ptr_base&&) = delete;

    // other constructors...
};

template <class _Tp, class _Dp = default_delete<_Tp> >
class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS unique_ptr : public __unique_ptr_base<_Tp, _Dp> {
    ...
    using __unique_ptr_base::__unique_ptr_base;
};
This revision now requires changes to proceed.Nov 24 2020, 7:37 AM

I'm going to have to revisit this patch in depth, likely after consulting with philnik.

Herald added a project: Restricted Project. · View Herald TranscriptSep 14 2023, 1:44 PM