[libcxx-commits] [PATCH] D151953: [libc++] Fix std::copy and std::move for ranges with potentially overlapping tail padding

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__string/constexpr_c_functions.h:127
 
+// TODO: find a better name
+template <class _Tp, class _Up, __enable_if_t<__is_always_bitcastable<_Up, _Tp>::value, int> = 0>
----------------
I think we are fine with `__constexpr_memmove`, but the confusion here comes from the fact that we don't have a great way of conveying what the `__count` is: is it a number of elements or a number of bytes? This is also a problem we have in our other functions like `__constexpr_memcmp_equal` and others.

One suggestion would be to introduce types like this:

```
enum class _NumberOfElements : size_t {};
enum class _NumberOfBytes : size_t {};
```

And then we could take the appropriate parameter in `__constexpr_memmove`. The benefit is that callers would now explicitly pass the value so there's no ambiguity:

```
__constexpr_memmove(dest, src, _NumberOfElements(n)) // works
__constexpr_memmove(dest, src, _NumberOfBytes(n)) // doesn't work
__constexpr_memmove(dest, src, n) // doesn't work
```

We could apply the same thing to the other functions. If we do this, perhaps we should start with a refactoring of the existing functions?



================
Comment at: libcxx/include/__string/constexpr_c_functions.h:130
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 _Tp*
+__constexpr_memmove_n(_Tp* __dest, _Up* __src, size_t __count) {
+  if (__libcpp_is_constant_evaluated()) {
----------------
Can we replace `__char_traits_move` by this?


================
Comment at: libcxx/include/__string/constexpr_c_functions.h:138
+#endif
+    if (std::__is_pointer_in_range(__dest, __dest + __count, __src)) {
+      for (; __count > 0; --__count)
----------------
This is the case where `__is_ptr_in_range(dst, dstend, src)` is true. This must be implemented using a normal forward for-loop:

```
|-------------------------------------------------|
                         ^ src                    ^ srcend
        ^ dst                         ^ dstend
```

This is the case where `__is_ptr_in_range(src, srcend, dst)` is true. This must be implemented by looping backwards from srcend to src and writing from dstend to dst.

```
|-------------------------------------------------|
        ^ src                    ^ srcend
                    ^ dst                         ^ dstend
```

Right now I think you have the logic the wrong way around. This also needs a test.

The logic should be

```
std::__is_pointer_in_range(__src, __src + __count, __dest)
```


================
Comment at: libcxx/include/__type_traits/datasizeof.h:22-23
+
+template <class _Tp>
+struct __libcpp_datasizeof {
+#if __has_cpp_attribute (__no_unique_address__)
----------------



================
Comment at: libcxx/include/__type_traits/datasizeof.h:31-32
+#else
+  template <bool = __libcpp_is_final<_Tp>::value>
+  struct _FirstPaddingByte : _Tp {
+    char __first_padding_byte_;
----------------
This fails for builtin types. Needs a test!


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/copy.pass.cpp:1
 //===----------------------------------------------------------------------===//
 //
----------------
What about `copy_n`, `copy_backward` and `move_backward`?


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/copy.pass.cpp:53-59
+    // Make sure that padding bits aren't copied
+    Derived src(1, 2, 3);
+    Derived dst(4, 5, 6);
+    std::copy(static_cast<PaddedBase*>(&src), static_cast<PaddedBase*>(&src) + 1, static_cast<PaddedBase*>(&dst));
+    assert(dst.a_ == 1);
+    assert(dst.b_ == 2);
+    assert(dst.c_ == 6);
----------------
Let's enclose this in `{}` to make it clear we're testing a separate property. (everywhere)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151953/new/

https://reviews.llvm.org/D151953