Convert uses of the Dangerous APIs to use MemoryMarshal.GetReference #15532

ahsonkhan · 2017-12-15T01:44:20Z

Part of:
https://github.com/dotnet/corefx/issues/25412
https://github.com/dotnet/corefx/issues/25615

Depends on: #15548

Related to: #15417

Similar changes will be made to the other repositories, specifically corefx - see dotnet/corefx#25936.

Following the staging plan from here: https://github.com/dotnet/corefx/issues/23881#issuecomment-343767740

Add MemoryExtensions.GetReference/TryGetArray
Convert all uses of DangerousGetPinnableReference/DangerousTryGetArray in coreclr, corefx, corert, corefxlab, aspnet, ... to MemoryExtensions.GetReference
Change DangerousGetPinnableReference to whatever we like to make it fit the pinning pattern and remove DangerousTryGetArray.

Doing it this way will avoid the need for complex staging or things being on the floor for extensive periods of time.

cc @jkotas, @stephentoub, @KrzysztofCwalina, @davidfowl, @pakrym

jkotas · 2017-12-15T01:56:27Z

src/mscorlib/shared/System/BitConverter.cs

@@ -267,7 +269,7 @@ public static int ToInt32(ReadOnlySpan<byte> value)
        {
            if (value.Length < sizeof(int))
                ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.value);
-            return Unsafe.ReadUnaligned<int>(ref value.DangerousGetPinnableReference());
+            return Unsafe.ReadUnaligned<int>(ref MemoryMarshal.GetReference(value));


Does this compile? I would expect that you will get readonly vs. non-readonly mismatch.

No it won't compile. I need to add the following Unsafe API:

ref T AsRef<T>(in T source)

Doing that now.

jkotas · 2017-12-15T02:35:18Z

I am wondering whether it is a good idea for Marshal.GetReference to return readonly ref. It just adds clutter.

All this is unsafe stuff. There are no readonly unsafe pointers in C# . @VSadov I assume that you are not planning to ever add unsafe readonly pointers to C# (e.g. readonly byte * that I can read from but not write from). Correct?

I think the ref-pointer unsafe operations should have parity with the unmanaged-pointer unsafe operation. Since there are no readonly unsafe unmanaged-pointers, we should not have readonly unsafe ref-pointers either.

jkotas

I would like to wait for @VSadov to comment

ahsonkhan · 2017-12-15T02:47:12Z

I would like to wait for @VSadov to comment

OK.

As an aside, I am getting the following EE error. It can't find AsRef.

I am assuming the issue is with the metasig. How do I specify ref readonly or in? Currently it is just ref

DEFINE_METHOD(UNSAFE,               REF_READONLY_AS_REF,    AsRef, GM_RefT_RetT)

Assert failure(PID 12440 [0x00003098], Thread: 25908 [0x6534]): Compiler optimization assumption invalid: EE expects method to exist: Internal.Runtime.CompilerServices.Unsafe:AsRef  Sig pointer: 00007FF73E32F850
FAILED: pMD != 0

CROSSGEN! CHECK::Trigger + 0x254 (0x00007ff7`3df99e74)
CROSSGEN! MscorlibBinder::LookupMethodLocal + 0x1EF (0x00007ff7`3dcfa1ef)
CROSSGEN! MscorlibBinder::GetMethodLocal + 0x4A (0x00007ff7`3dcf79ea)
CROSSGEN! MscorlibBinder::BindAll + 0x89 (0x00007ff7`3dcf6e49)
CROSSGEN! Module::ExpandAll + 0x105A (0x00007ff7`3dc3d15a)
CROSSGEN! CEEPreloader::Preload + 0x8A (0x00007ff7`3db8ad3a)
CROSSGEN! CEECompileInfo::PreloadModule + 0xA1 (0x00007ff7`3db8ae21)
CROSSGEN! ZapImage::Preload + 0x9C (0x00007ff7`3df39e9c)
CROSSGEN! Zapper::CompileModule + 0x190 (0x00007ff7`3df16a40)
CROSSGEN! Zapper::CompileAssembly + 0x3BD (0x00007ff7`3df15fad)
    File: d:\github\fork\coreclr\src\vm\binder.cpp Line: 128
    Image: D:\GitHub\Fork\coreclr\bin\Product\Windows_NT.x64.Debug\crossgen.exe

jkotas · 2017-12-15T02:54:32Z

How do I specificy ref readonly or in?

You cannot do it today. There would be a pile of plumbing required to create readonly flavors of all the unsafe stuff to make this work ok. I would love to not do it.

Also, readonly allows compiler to create clones as needed that can break the unsafe ref-pointer arithmetic in unexpected ways if you write anything more complex.

VSadov · 2017-12-15T23:59:45Z

no, no plans for readonly unmanaged pointers :)

Marshal.GetReference is supposed to be “unsafe/dangerous” API, like other “Marshal/Unsafe” stuff.
I think returning writeable reference is ok for “Marshal”. If user is already into this sort of things, he can trivially create a private helper which does Unsafe.AsRef, so why bother about readonly.

VSadov · 2017-12-16T00:01:48Z

CC: @jaredpar - in case there are any comments on respecting readonly at the level of “Marshal” APIs

ahsonkhan · 2017-12-16T00:06:55Z

Marshal.GetReference is supposed to be “unsafe/dangerous” API, like other “Marshal/Unsafe” stuff.
I think returning writeable reference is ok for “Marshal”. If user is already into this sort of things, he can trivially create a private helper which does Unsafe.AsRef, so why bother about readonly.

Does this mean we should close this issue: https://github.com/dotnet/corefx/issues/23881?

VSadov · 2017-12-16T01:56:02Z

@ahsonkhan I have preferences for “safe” methods that live on ReadOnlySpan - they should not expose the data to writes since that is for normal “if it compiles, I am using it correctly” situations.

What lives in Marshal/Unsafe is known to be specialized/interop stuff that is risky to use so I am less concerned.

jkotas · 2017-12-16T01:58:51Z

@dotnet-bot test this please

ahsonkhan · 2017-12-16T02:00:05Z

I have preferences for “safe” methods that live on ReadOnlySpan
What lives in Marshal/Unsafe is known to be specialized/interop stuff that is risky to use so I am less concerned.

That makes sense. I was thinking that given that the issue is about DangerousGetPinnableReference, which is being replaced with MemoryMarshal.GetReference, there won't be a method on ReadOnlySpan that would need to be marked ref readonly. However, I misread the issue, since we likely still need something like DangerousGetPinnableReference for supporting pinning in the language.

This reverts commit 4dbe38c.

ahsonkhan · 2017-12-16T03:03:14Z

https://ci.dot.net/job/dotnet_coreclr/job/master/job/x64_windows_nt_formatting_prtest/13679/consoleFull#781895581e0cf1bf6-b9e4-4679-b910-ada9d1ab2bcb

clang-format: there are formatting errors in D:\j\workspace\x64_windows_n---616f30f2\src\jit\compiler.h
18:23:53 At Line 7705 Before:
18:23:53     // Note - cannot be used for System.Runtime.Intrinsic 
18:23:53     unsigned getSIMDVectorRegisterByteLength()
18:23:53 After:
18:23:53     // Note - cannot be used for System.Runtime.Intrinsic
18:23:53     unsigned getSIMDVectorRegisterByteLength()
18:23:53 
18:23:53 Clang-format found formatting errors.
18:23:53 jit-format found formatting errors. Run:
18:23:53 	jit-format --fix --untidy
18:23:53 Formatting jit directory.

Note the extra space.

The clang-format errors came from #15528
cc @fiigii, @CarolEidt

This reverts commit 44d7948.

…eUseOfDangerous

jkotas · 2017-12-16T03:16:19Z

I have included the fix for the format error into #15550

fiigii · 2017-12-16T05:01:18Z

The clang-format errors came from #15528

Oops, my bad. @jkotas thank you for fixing it.

…otnet#15532) * Convert uses of the Dangerous APIs to use MemoryMarshal.GetReference * Adding Unsafe.AsRef(in...) and using that for ReadOnlySpan GetReference * Fix typo - add missing bracket. * Change AsRef(ref...) to AsRef(in...) * Remove unnecessary whitespace * Remove Unsafe.AsRef(in...) and its uses. * Revert "Remove unnecessary whitespace" This reverts commit 4dbe38c. * Revert "Revert "Remove unnecessary whitespace"" This reverts commit 44d7948. * Remove extra space to fix formatting.

Convert uses of the Dangerous APIs to use MemoryMarshal.GetReference

f6d951c

jkotas approved these changes Dec 15, 2017

View reviewed changes

jkotas reviewed Dec 15, 2017

View reviewed changes

ahsonkhan added 3 commits December 14, 2017 18:15

Adding Unsafe.AsRef(in...) and using that for ReadOnlySpan GetReference

8cce5c9

Fix typo - add missing bracket.

9556900

Change AsRef(ref...) to AsRef(in...)

f5fdb73

jkotas reviewed Dec 15, 2017

View reviewed changes

ahsonkhan mentioned this pull request Dec 15, 2017

Convert uses of the Dangerous APIs to use MemoryMarshal.GetReference dotnet/corert#5118

Merged

3 tasks

ahsonkhan self-assigned this Dec 15, 2017

ahsonkhan mentioned this pull request Dec 15, 2017

Convert uses of the Dangerous APIs to use MemoryMarshal.GetReference dotnet/corefx#25936

Merged

3 tasks

Remove unnecessary whitespace

4dbe38c

ahsonkhan mentioned this pull request Dec 16, 2017

ReadOnlySpan GetReference returns ref instead of ref readonly #15548

Merged

Remove Unsafe.AsRef(in...) and its uses.

d0648a0

Revert "Remove unnecessary whitespace"

44d7948

This reverts commit 4dbe38c.

ahsonkhan added 3 commits December 15, 2017 19:04

Revert "Revert "Remove unnecessary whitespace""

cdb30c2

This reverts commit 44d7948.

Merge branch 'master' of https://github.com/dotnet/coreclr into Remov…

a5b32c5

…eUseOfDangerous

Remove extra space to fix formatting.

7765fd8

ahsonkhan merged commit 8ec9bdf into dotnet:master Dec 16, 2017

ahsonkhan deleted the RemoveUseOfDangerous branch December 16, 2017 06:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert uses of the Dangerous APIs to use MemoryMarshal.GetReference #15532

Convert uses of the Dangerous APIs to use MemoryMarshal.GetReference #15532

ahsonkhan commented Dec 15, 2017 •

edited

Loading

jkotas Dec 15, 2017

ahsonkhan Dec 15, 2017 •

edited

Loading

jkotas commented Dec 15, 2017

jkotas left a comment

ahsonkhan commented Dec 15, 2017 •

edited

Loading

jkotas commented Dec 15, 2017 •

edited

Loading

VSadov commented Dec 15, 2017 •

edited

Loading

VSadov commented Dec 16, 2017

ahsonkhan commented Dec 16, 2017

VSadov commented Dec 16, 2017

jkotas commented Dec 16, 2017

ahsonkhan commented Dec 16, 2017

ahsonkhan commented Dec 16, 2017 •

edited

Loading

jkotas commented Dec 16, 2017 •

edited

Loading

fiigii commented Dec 16, 2017

Convert uses of the Dangerous APIs to use MemoryMarshal.GetReference #15532

Convert uses of the Dangerous APIs to use MemoryMarshal.GetReference #15532

Conversation

ahsonkhan commented Dec 15, 2017 • edited Loading

jkotas Dec 15, 2017

Choose a reason for hiding this comment

ahsonkhan Dec 15, 2017 • edited Loading

Choose a reason for hiding this comment

jkotas commented Dec 15, 2017

jkotas left a comment

Choose a reason for hiding this comment

ahsonkhan commented Dec 15, 2017 • edited Loading

jkotas commented Dec 15, 2017 • edited Loading

VSadov commented Dec 15, 2017 • edited Loading

VSadov commented Dec 16, 2017

ahsonkhan commented Dec 16, 2017

VSadov commented Dec 16, 2017

jkotas commented Dec 16, 2017

ahsonkhan commented Dec 16, 2017

ahsonkhan commented Dec 16, 2017 • edited Loading

jkotas commented Dec 16, 2017 • edited Loading

fiigii commented Dec 16, 2017

ahsonkhan commented Dec 15, 2017 •

edited

Loading

ahsonkhan Dec 15, 2017 •

edited

Loading

ahsonkhan commented Dec 15, 2017 •

edited

Loading

jkotas commented Dec 15, 2017 •

edited

Loading

VSadov commented Dec 15, 2017 •

edited

Loading

ahsonkhan commented Dec 16, 2017 •

edited

Loading

jkotas commented Dec 16, 2017 •

edited

Loading