Make CreateFromPinnedArray visible and move it to MemoryMarshal

[ahsonkhan]

Marking the method has EditorBrowsableState.Never hurts usability for users (like ASP.NET) who want to use the API.

Since the developer needs to be careful with the use of this API (it can be considered unsafe, for instance, if someone calls it on an array that isn't already pre-pinned), we should consider moving it to MemoryMarshal instead, but not hide it.

namespace System
{
    public readonly partial struct Memory<T>
    {
-        [EditorBrowsable(EditorBrowsableState.Never)]
-        public static Memory<T> CreateFromPinnedArray(T[] array, int start, int length) { throw null; }
    }
}

namespace System.Runtime.InteropServices
{
    public static partial class MemoryMarshal
    {
+        public static Memory<T> CreateFromPinnedArray(T[] array, int start, int length) { throw null; }
    }
}

cc @davidfowl, @GrabYourPitchforks, @stephentoub, @KrzysztofCwalina, @pakrym, @benaadams

[stephentoub]

There's another reason to move it. I don't see any way to write Memory.Pin in a way that's safe against torn reads/writes with Memory created from this API. A Memory could be created using CreateFromPinnedArray, stored in a field, and then concurrently with some thread calling Pin on that memory in the field, another thread writes to it, and the original thread ends up reading part of one Memory and part of the other. We currently have an issue in Pin where we don't validate that the read _index is within the read _array, but even if/when we fix that, in the above situation we could end up thinking that the array is already pinned and thus not creating a GCHandle for it; then the returned MemoryHandle might be giving out an address that becomes invalid when the GC moves it.

Moving this to MemoryMarshal to signal there-be-dragons seems prudent.

[GrabYourPitchforks]

I agree that it should move to MemoryMarshal, but for different reasons than what @stephentoub stated. From my perspective it's not safe to use Memory<T>.Pin at all if the underlying struct could be mutated unexpectedly, regardless of how the Memory<T> was constructed.

Here's the scenario for why I'm not worried too much about this for Pin.

1. The structure is fully self-consistent.
2. The user calls Pin and gets back a pointer. (Pin has now returned to the caller.)
3. The structure is now modified.
4. The user calls Length to get the length of the buffer, then invokes memcpy or something similar to overwrite the buffer.
5. memcpy merrily stomps on heap memory.

At no point in this scenario does the Pin method observe a torn struct or other inconsistency. This scenario also doesn't rely on pre-pinned buffers. The race condition would exist regardless of whether the Memory<T> instance is backed by a MemoryManager<T>, a string, a T[], or a void* (if we add this capability in the future).

That aside, the reason I'm a proponent of moving this API to MemoryMarshal is as @ahsonkhan said: the contract of CreateFromPinnedArray mandates that the creator performed some manual memory management before constructing the object. Moving the API to MemoryMarshal better signals this requirement.

[KrzysztofCwalina]

We should make sure that all MemoryMarshal APIs have good doc comments explaining the dangers and requirements for using the APIs safely/correctly.

[ahsonkhan]

Fixed in dotnet/corefx#29077