Improve Buffer.MemoryCopy documentation (or fix a bug)

[GSPP]

(1)

The documentation should be clarified:

This method copies sourceBytesToCopy bytes from the address specified by source to the address specified by destination. If the buffers overlap and the difference between destination minus source is less than sourceBytesToCopy, the source block is copied to the destination block in reverse order.

It is not clear what reverse order means (I understood that part only because I'm familiar with the overlapping buffers problem with memcpy). Also, "buffers overlap" is unclear because nowhere is it defined what a buffer is. We are talking about ranges of addresses here. Interpreting them as a buffer is out of scope for this API. The wording could be "memory regions specified by source and sourceBytesToCopy as well as destination and destinationSizeInBytes". Or it could be what the native version says.

I would have suggest a text but I wasn't sure if I was causing any licensing issues with that. I'd be willing to contribute a documentation fix if this is helping.

(2) Please clarify what a negative length or a negative destination size cause. Is that valid? What does it do?

(3) I'm actually unclear on what the function is supposed to do. It behaves differently depending on whether src and dest are ascending or descending in memory (where the ranges overlap in both cases). The behavior deviates from native memmove. Is this supposed to happen? If no, it's a bug. If yes, the documentation should spell that out.

    static unsafe void MemoryCopyTest()
    {
        var bytesToCopy = 3;

        var array1 = new byte[] { 1, 2, 3, 4, 5, 6, 7, 8 };
        fixed (byte* arrayPtr = array1)
        {
            MemoryCopy(arrayPtr + 2, arrayPtr + 1, bytesToCopy, bytesToCopy);
        }

        Console.WriteLine(BitConverter.ToString(array1));

        var array2 = new byte[] { 1, 2, 3, 4, 5, 6, 7, 8 };
        fixed (byte* arrayPtr = array2)
        {
            memmove(arrayPtr + 2, arrayPtr + 1, bytesToCopy);
        }

        Console.WriteLine(BitConverter.ToString(array2));
    }

    [DllImport("msvcrt.dll", SetLastError = false)]
    static unsafe extern IntPtr memmove(void* dest, void* src, int count);

01-03-04-05-05-06-07-08
01-02-02-03-04-06-07-08

The output is different because the check if ((ulong)dest - (ulong)src < len) goto PInvoke; is sometimes false, sometimes true. And if it's false the unsafe managed version behaves questionably.

Ideally, the function would mirror memmove.

Maybe it's a good solution to call the bytewise managed loop for small overlapping buffers. For big overlapping buffers call the native code.

[jkotas]

If the buffers overlap and the difference between destination minus source is less than sourceBytesToCopy, the source block is copied to the destination block in reverse order.

I agree that this sentence should be deleted. Instead, it should say the same thing as C++ memmove documentation: "If some regions of the source area and the destination overlap, both functions ensure that the original source bytes in the overlapping region are copied before being overwritten."

@mairaw What is the best way to get API documentation bugs like this one fixed?

The output is different

The output is different because of MemoryCopy takes source as first argument, and memmove takes destination as the first argument. Once the test is fixed to pass the arguments in the right order, the output will be the same. Also, PInvoke declaration for msvcrt memmove should have CallingConvention=CallingConvention.Cdecl to make it work on x86.

[GSPP]

I missed the different argument order and indeed that makes the difference in this particular case go away. But I think there are actual differences:

    static unsafe void MemoryCopyTestExhaustive()
    {
        long totalCount = 0;
        long failCount = 0;

        var maxOffset = 10;
        var maxCount = maxOffset * 2;
        var arrayLength = maxOffset + maxCount + 10;

        var baseArray = Enumerable.Range(0, arrayLength).Select(i => (byte)(i + 1)).ToArray();

        foreach (var srcOffset in Enumerable.Range(0, maxOffset))
            foreach (var destOffset in Enumerable.Range(0, maxOffset))
                foreach (var count in Enumerable.Range(0, maxCount))
                {
                    //if (AreRangesOverlapping(srcOffset, destOffset, count)) continue;

                    var array1 = baseArray.ToArray();
                    fixed (byte* arrayPtr = array1)
                        MemoryCopy(arrayPtr + srcOffset, arrayPtr + destOffset, count, count);

                    var array2 = baseArray.ToArray();
                    fixed (byte* arrayPtr = array2)
                        memmove(arrayPtr + destOffset, arrayPtr + srcOffset, count);

                    if (!array1.SequenceEqual(array2))
                    {
                        Console.WriteLine($"srcOffset: {srcOffset}, destOffset: {destOffset}, count: {count}");
                        Console.WriteLine(BitConverter.ToString(array1));
                        Console.WriteLine(BitConverter.ToString(array2));
                        failCount++;
                    }

                    totalCount++;
                }

        Console.WriteLine($"totalCount: {totalCount}, failCount: {failCount}");
    }

    static bool AreRangesOverlapping(int srcOffset, int destOffset, int count)
    {
        return (destOffset >= srcOffset && destOffset < srcOffset + count) || (srcOffset >= destOffset && srcOffset < destOffset + count);
    }

Prints:

totalCount: 2000, failCount: 690

I hope I didn't mess it up again. Here's a concrete case which looks like a true bug to me:

srcOffset: 0, destOffset: 1, count: 2
01-01-01-04-05-06-07-08-09-0A-0B-0C-0D-0E-0F-10-11-12-13-14-15-16-17-18-19-1A-1B
-1C-1D-1E
01-01-02-04-05-06-07-08-09-0A-0B-0C-0D-0E-0F-10-11-12-13-14-15-16-17-18-19-1A-1B
-1C-1D-1E

And I think it's due to copying in chunks bigger than one byte in the overlapping case.

If I exclude overlapping ranges by uncommenting the respective check I get no failures.

I think this exhaustive test case should be added to the test suite. I'm a big fan of exhaustive tests in cases where they are applicable. Writing an exhaustive test can be faster than adding big amounts of manual test cases. It can give a lot of confidence in correctness.

[GSPP]

One question remains: What about negative lengths? It should be documented what is supposed to happen. Is it just undefined behavior?

Another question: What does the function do if length == 0 but the pointers are invalid (e.g. null). Is this defined? This should be clarified as well.

[mikedn]

totalCount: 2000, failCount: 690

I don't get that result when I run your code, failCount is 0 for me.

And I think it's due to copying in chunks bigger than one byte in the overlapping case.

That would mean that VC++'s memmove is broken.

What about negative lengths? It should be documented what is supposed to happen. Is it just undefined behavior?

Yeah, it should be documented. You get an overflow exception instead of an argument exception. It's unusual but at least bad stuff doesn't happen if you accidentally pass it a negative length.

[GSPP]

I made a mistake when copying out the MemoryCopy code to my project. Indeed, zero failures when correcting that...

I guess that's good news: No bug! Now provably so thanks to exhaustive testing. :)

[joperezr]

From what I read above seems like this issue is resolved now so I'll go ahead and close it for now. Feel free to reopen if that is not the case and there is still something we can fix in the documentation side.