From e2f06fcb1f40349422122f872aed468b243e0f02 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Tue, 5 Mar 2024 15:02:38 +1100 Subject: [PATCH 1/2] Remove or document Unsafe.AsPointer uses - In tests and miscellaneous code --- .../UnsafeTests.cs | 20 ++++++++++--------- .../BrowserDebugProxy/MonoSDBHelper.cs | 8 ++++++-- .../sample/wasm/simple-raytracer/Program.cs | 2 +- .../Directed/debugging/poisoning/poison.cs | 6 ++++-- .../JitBlue/Runtime_61510/Runtime_61510.cs | 1 + .../JitBlue/Runtime_92349/Runtime_92349.cs | 2 +- .../classloader/generics/Pointers/Pointers.cs | 14 +++++++------ .../DynamicGenerics/universal_generics.cs | 5 ++++- 8 files changed, 36 insertions(+), 22 deletions(-) diff --git a/src/libraries/System.Runtime/tests/System.Runtime.CompilerServices.Unsafe.Tests/UnsafeTests.cs b/src/libraries/System.Runtime/tests/System.Runtime.CompilerServices.Unsafe.Tests/UnsafeTests.cs index 6eb527f923255..76c7781075483 100644 --- a/src/libraries/System.Runtime/tests/System.Runtime.CompilerServices.Unsafe.Tests/UnsafeTests.cs +++ b/src/libraries/System.Runtime/tests/System.Runtime.CompilerServices.Unsafe.Tests/UnsafeTests.cs @@ -14,7 +14,7 @@ public class UnsafeTests public static unsafe void ReadInt32() { int expected = 10; - void* address = Unsafe.AsPointer(ref expected); + void* address = &expected; int ret = Unsafe.Read(address); Assert.Equal(expected, ret); } @@ -23,7 +23,7 @@ public static unsafe void ReadInt32() public static unsafe void WriteInt32() { int value = 10; - int* address = (int*)Unsafe.AsPointer(ref value); + int* address = &value; int expected = 20; Unsafe.Write(address, expected); @@ -36,7 +36,7 @@ public static unsafe void WriteInt32() public static unsafe void WriteBytesIntoInt32() { int value = 20; - int* intAddress = (int*)Unsafe.AsPointer(ref value); + int* intAddress = &value; byte* byteAddress = (byte*)intAddress; for (int i = 0; i < 4; i++) { @@ -70,7 +70,7 @@ public static unsafe void WriteBytesIntoInt32() public static unsafe void LongIntoCompoundStruct() { long value = 1234567891011121314L; - long* longAddress = (long*)Unsafe.AsPointer(ref value); + long* longAddress = &value; Byte4Short2 b4s2 = Unsafe.Read(longAddress); if (BitConverter.IsLittleEndian) { @@ -117,7 +117,7 @@ public static unsafe void ReadWriteDoublePointer() { int value1 = 10; int value2 = 20; - int* valueAddress = (int*)Unsafe.AsPointer(ref value1); + int* valueAddress = &value1; int** valueAddressPtr = &valueAddress; Unsafe.Write(valueAddressPtr, new IntPtr(&value2)); @@ -132,7 +132,7 @@ public static unsafe void CopyToRef() { int value = 10; int destination = -1; - Unsafe.Copy(ref destination, Unsafe.AsPointer(ref value)); + Unsafe.Copy(ref destination, &value); Assert.Equal(10, destination); Assert.Equal(10, value); @@ -147,7 +147,7 @@ public static unsafe void CopyToVoidPtr() { int value = 10; int destination = -1; - Unsafe.Copy(Unsafe.AsPointer(ref destination), ref value); + Unsafe.Copy(&destination, ref value); Assert.Equal(10, destination); Assert.Equal(10, value); @@ -157,13 +157,14 @@ public static unsafe void CopyToVoidPtr() Assert.Equal(10, value); } +#pragma warning disable CS8500 // takes address of managed type [Fact] public static unsafe void CopyToRefGenericStruct() { Int32Generic destination = default; Int32Generic value = new() { Int32 = 5, Value = "a" }; - Unsafe.Copy(ref destination, Unsafe.AsPointer(ref value)); + Unsafe.Copy(ref destination, &value); Assert.Equal(5, destination.Int32); Assert.Equal("a", destination.Value); @@ -175,11 +176,12 @@ public static unsafe void CopyToVoidPtrGenericStruct() Int32Generic destination = default; Int32Generic value = new() { Int32 = 5, Value = "a" }; - Unsafe.Copy(Unsafe.AsPointer(ref destination), ref value); + Unsafe.Copy(&destination, ref value); Assert.Equal(5, destination.Int32); Assert.Equal("a", destination.Value); } +#pragma warning restore CS8500 [Fact] public static unsafe void SizeOf() diff --git a/src/mono/browser/debugger/BrowserDebugProxy/MonoSDBHelper.cs b/src/mono/browser/debugger/BrowserDebugProxy/MonoSDBHelper.cs index 69e1cd780c056..81871b356e16d 100644 --- a/src/mono/browser/debugger/BrowserDebugProxy/MonoSDBHelper.cs +++ b/src/mono/browser/debugger/BrowserDebugProxy/MonoSDBHelper.cs @@ -525,7 +525,9 @@ private unsafe T ReadBigEndian() where T : struct { data.Reverse(); } - data.CopyTo(new Span(Unsafe.AsPointer(ref ret), data.Length)); +#pragma warning disable CS8500 // takes address of managed type + data.CopyTo(new Span(&ret, data.Length)); +#pragma warning restore CS8500 return ret; } } @@ -546,7 +548,9 @@ public override void Write(string val) private unsafe void WriteBigEndian(T val) where T : struct { Span data = stackalloc byte[Unsafe.SizeOf()]; - new Span(Unsafe.AsPointer(ref val), data.Length).CopyTo(data); +#pragma warning disable CS8500 // takes address of managed type + new Span(&val, data.Length).CopyTo(data); +#pragma warning restore CS8500 if (BitConverter.IsLittleEndian) { data.Reverse(); diff --git a/src/mono/sample/wasm/simple-raytracer/Program.cs b/src/mono/sample/wasm/simple-raytracer/Program.cs index f356394f0de1d..5ee605e8ef683 100644 --- a/src/mono/sample/wasm/simple-raytracer/Program.cs +++ b/src/mono/sample/wasm/simple-raytracer/Program.cs @@ -195,7 +195,7 @@ private static void renderPixel (int i, int j, ref Vec3f light, Intersector inte if (didHitZ && (hitZ > sphere.Center.z)) continue; - if (intersector.Intersect(ref pos, ref dir, Unsafe.AsPointer(ref sphere), ref intersection_normal)) { + if (intersector.Intersect(ref pos, ref dir, &sphere, ref intersection_normal)) { sampleEnv(ref intersection_normal, ref color); const float ambientScale = 0.2f; diff --git a/src/tests/JIT/Directed/debugging/poisoning/poison.cs b/src/tests/JIT/Directed/debugging/poisoning/poison.cs index c5bcccded9270..07543fcc53446 100644 --- a/src/tests/JIT/Directed/debugging/poisoning/poison.cs +++ b/src/tests/JIT/Directed/debugging/poisoning/poison.cs @@ -8,6 +8,7 @@ public class Program [Fact] public static unsafe int TestEntryPoint() { +#pragma warning disable CS8500 // takes address of managed type bool result = true; int poisoned; @@ -16,7 +17,7 @@ public static unsafe int TestEntryPoint() GCRef zeroed; Unsafe.SkipInit(out zeroed); - result &= VerifyZero(Unsafe.AsPointer(ref zeroed), Unsafe.SizeOf()); + result &= VerifyZero(&zeroed, sizeof(GCRef)); WithoutGCRef poisoned2; Unsafe.SkipInit(out poisoned2); @@ -36,9 +37,10 @@ public static unsafe int TestEntryPoint() GCRef zeroed2; Unsafe.SkipInit(out zeroed2); - result &= VerifyZero(Unsafe.AsPointer(ref zeroed2), Unsafe.SizeOf()); + result &= VerifyZero(&zeroed2, sizeof(GCRef)); return result ? 100 : 101; +#pragma warning restore CS8500 } [MethodImpl(MethodImplOptions.NoInlining)] diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_61510/Runtime_61510.cs b/src/tests/JIT/Regression/JitBlue/Runtime_61510/Runtime_61510.cs index fe76c604ef1b0..122e8d0c604f2 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_61510/Runtime_61510.cs +++ b/src/tests/JIT/Regression/JitBlue/Runtime_61510/Runtime_61510.cs @@ -12,6 +12,7 @@ public unsafe class Runtime_61510 [Fact] public static int TestEntryPoint() { + // Unsafe.AsPointer is safe since static field is marked with [FixedAddressValueType] ref byte result = ref AddZeroByrefToNativeInt((nint)Unsafe.AsPointer(ref s_field)); return Unsafe.AreSame(ref s_field, ref result) ? 100 : 101; diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_92349/Runtime_92349.cs b/src/tests/JIT/Regression/JitBlue/Runtime_92349/Runtime_92349.cs index 5de0a28895b26..5ddf458793104 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_92349/Runtime_92349.cs +++ b/src/tests/JIT/Regression/JitBlue/Runtime_92349/Runtime_92349.cs @@ -22,7 +22,7 @@ public unsafe static void EntryPoint() if (Sse2.IsSupported) { ulong value = 0; - Test((byte*)Unsafe.AsPointer(ref value)); + Test((byte*)&value); Assert.True(value == 246); } } diff --git a/src/tests/Loader/classloader/generics/Pointers/Pointers.cs b/src/tests/Loader/classloader/generics/Pointers/Pointers.cs index f8c1e6116dbff..8017887d710ca 100644 --- a/src/tests/Loader/classloader/generics/Pointers/Pointers.cs +++ b/src/tests/Loader/classloader/generics/Pointers/Pointers.cs @@ -106,18 +106,20 @@ public static void PointerArray() [MethodImpl(MethodImplOptions.NoInlining)] private static void PointerArrayImpl() { - int*[] intPtrArray = new int*[5]; - Span intSpan = stackalloc int[intPtrArray.Length]; - int* intArray = (int*)Unsafe.AsPointer(ref intSpan.GetPinnableReference()); + int intAllocationSize = 5; + int*[] intPtrArray = new int*[intAllocationSize]; + int* intArray = stackalloc int[intAllocationSize]; + Span intSpan = new Span(intArray, intAllocationSize); for (int i = 0; i < intPtrArray.Length; i++) { intArray[i] = i; intPtrArray[i] = &intArray[i]; } - Struct*[] structPtrArray = new Struct*[5]; - Span structSpan = stackalloc Struct[structPtrArray.Length]; - Struct* structArray = (Struct*)Unsafe.AsPointer(ref structSpan.GetPinnableReference()); + int structAllocationSize = 5; + Struct*[] structPtrArray = new Struct*[structAllocationSize]; + Struct* structArray = stackalloc Struct[structAllocationSize]; + Span structSpan = new Span(structArray, structAllocationSize); for (int i = 0; i < structPtrArray.Length; i++) { structArray[i] = new Struct() { Num = i }; diff --git a/src/tests/nativeaot/SmokeTests/DynamicGenerics/universal_generics.cs b/src/tests/nativeaot/SmokeTests/DynamicGenerics/universal_generics.cs index 12415d8ec581a..85fc88fb9058d 100644 --- a/src/tests/nativeaot/SmokeTests/DynamicGenerics/universal_generics.cs +++ b/src/tests/nativeaot/SmokeTests/DynamicGenerics/universal_generics.cs @@ -461,7 +461,9 @@ public class UnmanagedByRef : Base where T : struct, IGetValue [MethodImpl(MethodImplOptions.NoInlining)] public unsafe void TestAsPointer(T x) { - IntPtr unsafeValuePtr = (IntPtr)Unsafe.AsPointer(ref x); +#pragma warning disable CS8500 // takes address of managed type + IntPtr unsafeValuePtr = (IntPtr)&x; +#pragma warning restore CS8500 GC.Collect(); GC.Collect(); GC.Collect(); @@ -487,6 +489,7 @@ public unsafe void TestGeneralFunction(T x) [MethodImpl(MethodImplOptions.NoInlining)] unsafe IntPtr someFuncWithByRefArgs(ref T x) { + // Unsafe.AsPointer is safe since the reference is expected to be pinned return (IntPtr)Unsafe.AsPointer(ref x); } From 609c818f4ce57513dd19781a0a1d4fea22dc6e45 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Tue, 5 Mar 2024 15:26:18 +1100 Subject: [PATCH 2/2] Address feedback --- .../UnsafeTests.cs | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/libraries/System.Runtime/tests/System.Runtime.CompilerServices.Unsafe.Tests/UnsafeTests.cs b/src/libraries/System.Runtime/tests/System.Runtime.CompilerServices.Unsafe.Tests/UnsafeTests.cs index 76c7781075483..955bc189b4331 100644 --- a/src/libraries/System.Runtime/tests/System.Runtime.CompilerServices.Unsafe.Tests/UnsafeTests.cs +++ b/src/libraries/System.Runtime/tests/System.Runtime.CompilerServices.Unsafe.Tests/UnsafeTests.cs @@ -14,7 +14,7 @@ public class UnsafeTests public static unsafe void ReadInt32() { int expected = 10; - void* address = &expected; + void* address = Unsafe.AsPointer(ref expected); // Unsafe.AsPointer is safe since expected is on stack int ret = Unsafe.Read(address); Assert.Equal(expected, ret); } @@ -23,7 +23,7 @@ public static unsafe void ReadInt32() public static unsafe void WriteInt32() { int value = 10; - int* address = &value; + int* address = (int*)Unsafe.AsPointer(ref value); // Unsafe.AsPointer is safe since value is on stack int expected = 20; Unsafe.Write(address, expected); @@ -36,7 +36,7 @@ public static unsafe void WriteInt32() public static unsafe void WriteBytesIntoInt32() { int value = 20; - int* intAddress = &value; + int* intAddress = (int*)Unsafe.AsPointer(ref value); // Unsafe.AsPointer is safe since value is on stack byte* byteAddress = (byte*)intAddress; for (int i = 0; i < 4; i++) { @@ -70,7 +70,7 @@ public static unsafe void WriteBytesIntoInt32() public static unsafe void LongIntoCompoundStruct() { long value = 1234567891011121314L; - long* longAddress = &value; + long* longAddress = (long*)Unsafe.AsPointer(ref value); // Unsafe.AsPointer is safe since value is on stack Byte4Short2 b4s2 = Unsafe.Read(longAddress); if (BitConverter.IsLittleEndian) { @@ -117,7 +117,7 @@ public static unsafe void ReadWriteDoublePointer() { int value1 = 10; int value2 = 20; - int* valueAddress = &value1; + int* valueAddress = (int*)Unsafe.AsPointer(ref value1); // Unsafe.AsPointer is safe since value1 is on stack int** valueAddressPtr = &valueAddress; Unsafe.Write(valueAddressPtr, new IntPtr(&value2)); @@ -132,7 +132,7 @@ public static unsafe void CopyToRef() { int value = 10; int destination = -1; - Unsafe.Copy(ref destination, &value); + Unsafe.Copy(ref destination, Unsafe.AsPointer(ref value)); // Unsafe.AsPointer is safe since value is on stack Assert.Equal(10, destination); Assert.Equal(10, value); @@ -147,7 +147,7 @@ public static unsafe void CopyToVoidPtr() { int value = 10; int destination = -1; - Unsafe.Copy(&destination, ref value); + Unsafe.Copy(Unsafe.AsPointer(ref destination), ref value); // Unsafe.AsPointer is safe since destination is on stack Assert.Equal(10, destination); Assert.Equal(10, value); @@ -157,14 +157,13 @@ public static unsafe void CopyToVoidPtr() Assert.Equal(10, value); } -#pragma warning disable CS8500 // takes address of managed type [Fact] public static unsafe void CopyToRefGenericStruct() { Int32Generic destination = default; Int32Generic value = new() { Int32 = 5, Value = "a" }; - Unsafe.Copy(ref destination, &value); + Unsafe.Copy(ref destination, Unsafe.AsPointer(ref value)); // Unsafe.AsPointer is safe since value is on stack Assert.Equal(5, destination.Int32); Assert.Equal("a", destination.Value); @@ -176,12 +175,11 @@ public static unsafe void CopyToVoidPtrGenericStruct() Int32Generic destination = default; Int32Generic value = new() { Int32 = 5, Value = "a" }; - Unsafe.Copy(&destination, ref value); + Unsafe.Copy(Unsafe.AsPointer(ref destination), ref value); // Unsafe.AsPointer is safe since destination is on stack Assert.Equal(5, destination.Int32); Assert.Equal("a", destination.Value); } -#pragma warning restore CS8500 [Fact] public static unsafe void SizeOf()