This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Fix std::move algorithm with trivial move-only types
ClosedPublic

Authored by ldionne on Jul 6 2023, 7:48 AM.

Details

Summary

As reported in https://reviews.llvm.org/D151953#4472195, the std::move
algorithm (and various other functions that relied on it) stopped working
after starting to use __constexpr_memmove in its implementation. This
patch fixes that and adds tests for various related algorithms and
functions that were not exercising trivial move-only types.

Diff Detail

Event Timeline

ldionne created this revision.Jul 6 2023, 7:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 6 2023, 7:48 AM
ldionne requested review of this revision.Jul 6 2023, 7:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 6 2023, 7:48 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne added a subscriber: philnik.Jul 6 2023, 7:54 AM

(This doesn't fix anything yet, this only adds the regression tests. I'm meeting with @philnik in a few minutes and we can discuss what we think is the best way to address this)

ldionne updated this revision to Diff 537862.Jul 6 2023, 1:31 PM

Add the solution we discussed with Nikolas.

philnik added inline comments.Jul 6 2023, 2:00 PM
libcxx/include/__string/constexpr_c_functions.h
183–186

Would it make sense to first try construction? Then we could use __constexpr_memmove for uninitialized_{copy, move} too.

206

Why do you bit_cast here? Also, could we add a __bit_cast instead of using builtins randomly in the code?

libcxx/test/std/containers/sequences/vector/vector.modifiers/erase_iter.pass.cpp
54–55

Instead of moving all the code an indentation deeper, maybe move the braces an indentation higher? That would basically change this test to use two-space indentation instead of four spaces, like we do in new code. (or just clang-format the whole thing)

ldionne updated this revision to Diff 537890.Jul 6 2023, 2:47 PM
ldionne marked an inline comment as done.

Address clang-format comment.

alexfh added a subscriber: alexfh.Jul 7 2023, 1:58 AM

It looks like this fix will take some time to converge. Can we first return to green, please? The premerge checks succeeded on https://reviews.llvm.org/D154595

It looks like this fix will take some time to converge. Can we first return to green, please? The premerge checks succeeded on https://reviews.llvm.org/D154595

See https://reviews.llvm.org/D154595#4480304

libcxx/include/__string/constexpr_c_functions.h
183–186

I actually ran into issues doing this (I think it triggers some issues in older standard modes), plus the semantics of this function (as currently written) are to perform an assignment, so I think trying the assignment first makes sense. I'm definitely willing to investigate, but I'd like to do that in its own patch. Are you OK with that?

206

I bit_cast in order to avoid having to deal with _Tp and _Up in the assignment -- I want to deal with a single type over there.

Also, could we add a __bit_cast instead of using builtins randomly in the code?

Yes, I can do that.

Edit: I just tried introducing __bit_cast and using it, and it actually triggers some test failures in C++11 and C++14 mode:

In file included from libcxx/strings/c.strings/constexpr_memmove.pass.cpp:25:
In file included from include/c++/v1/__string/constexpr_c_functions.h:12:
include/c++/v1/__bit/bit_cast.h:36:10: error: call to deleted constructor of 'CopyConstructible'
  return __builtin_bit_cast(_ToType, __from);
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/c++/v1/__string/constexpr_c_functions.h:212:71: note: in instantiation of function template specialization 'std::__bit_cast<CopyConstructible, CopyConstructible>' requested here
        std::__assign_trivially_copyable(__dest + (__count - 1), std::__bit_cast<_Tp>(__src[__count - 1]));
                                                                      ^
libcxx/strings/c.strings/constexpr_memmove.pass.cpp:91:23: note: in instantiation of function template specialization 'std::__constexpr_memmove<CopyConstructible, CopyConstructible, 0>' requested here
  Dest* result = std::__constexpr_memmove(dst, src, std::__element_count(3));
                      ^
libcxx/strings/c.strings/constexpr_memmove.pass.cpp:111:3: note: in instantiation of function template specialization 'test_user_defined_types<CopyConstructible, CopyConstructible>' requested here
  test_user_defined_types<CopyConstructible, CopyConstructible>();
  ^
libcxx/strings/c.strings/constexpr_memmove.pass.cpp:40:3: note: 'CopyConstructible' has been explicitly marked deleted here
  CopyConstructible(CopyConstructible&&)                 = delete;
  ^

I think this is because RVO isn't mandated before C++17 (IIRC?), and so we actually require a move constructor to return from __bit_cast for some of the funky types I use in the test. So it looks like I might need to keep using __builtin_bit_cast directly here (even though I agree with you it would make more sense to use some std::__bit_cast).

libcxx/test/std/containers/sequences/vector/vector.modifiers/erase_iter.pass.cpp
54–55

I'll clang-format it, you're right the diff is actually smaller.

ldionne updated this revision to Diff 538082.Jul 7 2023, 4:39 AM

Address comments

philnik added inline comments.Jul 7 2023, 8:43 AM
libcxx/include/__string/constexpr_c_functions.h
183–186

That's fine with me.

206

Ugh. Ok, I'd be fine with using __builtin_bit_cast for now and adding a TODO to investigate further. There has to be some way to fix this.

libcxx/test/libcxx/strings/c.strings/constexpr_memmove.pass.cpp
10

Why is this unsupported in C++03?

ldionne marked an inline comment as done.Jul 7 2023, 9:47 AM
ldionne added inline comments.
libcxx/test/libcxx/strings/c.strings/constexpr_memmove.pass.cpp
10

Actually, for the same reason that __bit_cast won't work:

In file included from /c.strings/constexpr_memmove.pass.cpp:25:
include/c++/v1/__string/constexpr_c_functions.h:211:66: error: copying parameter of type 'CopyConstructible' invokes deleted constructor
        std::__assign_trivially_copyable(__dest + (__count - 1), __builtin_bit_cast(_Tp, __src[__count - 1]));
                                                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/c.strings/constexpr_memmove.pass.cpp:92:23: note: in instantiation of function template specialization 'std::__constexpr_memmove<CopyConstructible, CopyConstructible, 0>' requested here
  Dest* result = std::__constexpr_memmove(dst, src, std::__element_count(3));
                      ^
/c.strings/constexpr_memmove.pass.cpp:112:3: note: in instantiation of function template specialization 'test_user_defined_types<CopyConstructible, CopyConstructible>' requested here
  test_user_defined_types<CopyConstructible, CopyConstructible>();
  ^
/c.strings/constexpr_memmove.pass.cpp:41:3: note: 'CopyConstructible' has been explicitly marked deleted here
  CopyConstructible(CopyConstructible&&)                 = delete;
  ^
ldionne marked 5 inline comments as done.Jul 7 2023, 2:00 PM
ldionne added inline comments.
libcxx/include/__string/constexpr_c_functions.h
206

In the next revision, I am actually removing __builtin_bit_cast entirely so this is moot.

libcxx/test/libcxx/strings/c.strings/constexpr_memmove.pass.cpp
10

In the next update, I'm dropping __builtin_bit_cast and this test now works in C++03.

ldionne updated this revision to Diff 538259.Jul 7 2023, 2:01 PM
ldionne marked an inline comment as done.

Drop bit_cast, add tests. This should fix CI.

philnik accepted this revision.Jul 7 2023, 3:10 PM

LGTM with a comment.

libcxx/include/__string/constexpr_c_functions.h
171

Could you add a comment here explaining that we implicitly end the lifetime of the old object and then start the lifetime of the new object? I think this is a quite obscure part of the standard.

179

Having if constexpr would make this so much nicer :(

This revision is now accepted and ready to land.Jul 7 2023, 3:10 PM
ldionne updated this revision to Diff 538622.Jul 10 2023, 6:48 AM

Fix clang-format.

ldionne marked an inline comment as done.Jul 10 2023, 7:07 AM
ldionne added inline comments.
libcxx/include/__string/constexpr_c_functions.h
171

A program is not required to call the destructor of an object to end its lifetime if the object is trivially-destructible (be careful that the correct behavior of the program may depend on the destructor).

From https://en.cppreference.com/w/cpp/language/lifetime (under storage reuse). Comment added x2.

ldionne updated this revision to Diff 538635.Jul 10 2023, 7:18 AM
ldionne marked an inline comment as done.

Address comments, fix CI, rebase.