Undefined behavior when deep copying from and to an empty unmanaged view

[dalg24]

This can occur for unmanaged views (non-zero rank) because std::memcpy() gets called with a null pointer.

It is a bit tricky because the behavior depends on whether deprecated code is disabled or not.

View<int*, MemoryTraits<Unmanaged>> v1( nullptr, 0 );
View<int*, MemoryTraits<Unmanaged>> v2( reinterpret_cast<int*>(-1), 0 );
View<int*> v3("v3", 0);
assert( !v1.data() );
assert( v2.data() );
#ifdef KOKKOS_ENABLE_DEPRECATED_CODE
assert( !v3.data() );
deep_copy(v1, v2); // bad
deep_copy(v2, v1); // bad
deep_copy(v1, v3); // ok
deep_copy(v3, v1); // ok
deep_copy(v2, v3); // bad
deep_copy(v3, v2); // bad
#else
assert( v3.data() );
deep_copy(v1, v2); // bad
deep_copy(v2, v1); // bad
deep_copy(v1, v3); // bad
deep_copy(v3, v1); // bad
deep_copy(v2, v3); // ok
deep_copy(v3, v2); // ok
#endif

I am working on a patch. Will follow up on this.

[crtrott]

I understand what the new behavior is and I like it more than the old one. That said: we probably should just allow deep copies between zero length views.

[dalg24]

@crtrott Did you look at #1968 ?

[crtrott]

Yeah I had a short look. We will look at it in more detail in our team meeting in 10 mins.

[crtrott]

Decided this is correct, it mirrors std::copy behavior.

[mhoemmen]

@crtrott wrote:

Decided this is correct, it mirrors std::copy behavior.

The closest analog is ranges copy. If you give copy an empty range, it correctly copies nothing:

http://eel.is/c++draft/alg.copy

[dalg24]

Decided this is correct, it mirrors std::copy behavior.

I disagree. I just looked in libc++ and sure std::copy() calls std::memmove() if the value type is trivially copy assignable but it is protected by a check that the size (last - first) is greater than zero. STL uses iterators to represent the range [first, last) whereas Kokkos takes a pointer to the data and a size.
IMO {nullptr, 0} as well as {some_ptr_not_dereferencable, 0} are valid representations of an empty range so I expect the copy to ensure that the view is not empty before calling memcpy() or memmove().

[crtrott]

@dalg24 you misunderstood me. "this" referred to this pull request. I.e. we decided this pull request implements the correct behaviour. We had some discussion about whether or not a copy between a default constructed view and a non-default constructed view should be allowed but said yeah its ok because in the end its going to mirror std::copy.

[crtrott]

So we are just waiting for some spot-check to be done by us and then merge it in.

[mhoemmen]

Just for practice, I'll forestall potential objections by language lawyering :D

[iterator.requirements.general] (link)

8. .... A range is an iterator and a sentinel that designate the beginning and end of the computation....
9. An iterator and a sentinel denoting a range are comparable. A range [i, s) is empty if i == s; otherwise, [i, s) refers to the elements in the data structure starting with the element pointed to by i and up to but not including the element, if any, pointed to by the first iterator j such that j == s.
10. A sentinel s is called reachable from an iterator i if and only if there is a finite sequence of applications of the expression ++i that makes i == s. If s is reachable from i, [i, s) denotes a valid range.
11. ....
12. The result of the application of library functions to invalid ranges is undefined.

The range [i, i) is valid, because the sentinel i is reachable from the beginning i via a finite number (zero, in this case) of applications of prefix ++.