From a1684da76918844405035b567cec16c7520eaaf6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= <35800402+MichalPetryka@users.noreply.github.com> Date: Tue, 27 Feb 2024 23:16:25 +0100 Subject: [PATCH 01/13] Fix an assert with `Unsafe.Subtract` Found in #99011. --- src/coreclr/jit/morph.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 6ba44215a5f5da..420f980ce66ccc 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -9241,7 +9241,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac, bool* optA noway_assert(op1); if (op1->IsCnsIntOrI()) { - noway_assert(varTypeIsIntOrI(tree)); + noway_assert(varTypeIsIntegralOrI(tree)); // The type of the new GT_NEG node cannot just be op2->TypeGet(). // Otherwise we may sign-extend incorrectly in cases where the GT_NEG From 49042feb39aa38614456a555356688e0a8bfda1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= <35800402+MichalPetryka@users.noreply.github.com> Date: Tue, 27 Feb 2024 23:19:48 +0100 Subject: [PATCH 02/13] Add test --- .../UnsafeTests.cs | 3 +++ 1 file changed, 3 insertions(+) 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 13488161781df5..ebfd339248c23d 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 @@ -609,6 +609,9 @@ public static void RefSubtract() ref string r3 = ref Unsafe.Subtract(ref r2, 3); Assert.Equal("abc", r3); + + static ref byte NullTest(nuint offset) => ref Unsafe.Subtract(ref Unsafe.NullRef(), offset); + Assert.Equal(0, (nuint)Unsafe.AsPointer(ref NullTest(0))); } [Fact] From 96079d0425fec3b95da190a5a9d4c05092ec8cef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= <35800402+MichalPetryka@users.noreply.github.com> Date: Tue, 27 Feb 2024 23:33:03 +0100 Subject: [PATCH 03/13] Update UnsafeTests.cs --- .../System.Runtime.CompilerServices.Unsafe.Tests/UnsafeTests.cs | 1 + 1 file changed, 1 insertion(+) 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 ebfd339248c23d..912c49fe59a1a1 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 @@ -610,6 +610,7 @@ public static void RefSubtract() ref string r3 = ref Unsafe.Subtract(ref r2, 3); Assert.Equal("abc", r3); + [MethodImpl(MethodImplOptions.NoInlining)] static ref byte NullTest(nuint offset) => ref Unsafe.Subtract(ref Unsafe.NullRef(), offset); Assert.Equal(0, (nuint)Unsafe.AsPointer(ref NullTest(0))); } From b6b9f6c3d006bcd69118face7555a54a30932463 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= <35800402+MichalPetryka@users.noreply.github.com> Date: Tue, 27 Feb 2024 23:43:46 +0100 Subject: [PATCH 04/13] Update morph.cpp --- src/coreclr/jit/morph.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 420f980ce66ccc..c005f1f95cb8f1 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -9239,7 +9239,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac, bool* optA /* Check for "cns1 - op2" , we change it to "(cns1 + (-op2))" */ noway_assert(op1); - if (op1->IsCnsIntOrI()) + if (op1->IsCnsIntOrI() && (op2->TypeGet() != TYP_BYREF)) { noway_assert(varTypeIsIntegralOrI(tree)); From d10f779c70231442919ef4382e3722eedff96cca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= <35800402+MichalPetryka@users.noreply.github.com> Date: Tue, 27 Feb 2024 23:47:06 +0100 Subject: [PATCH 05/13] Update UnsafeTests.cs --- .../UnsafeTests.cs | 8 ++++++++ 1 file changed, 8 insertions(+) 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 912c49fe59a1a1..ac46f62f847679 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 @@ -462,6 +462,14 @@ public static void ByteOffsetStackByte4() Assert.Equal(new IntPtr(-3), Unsafe.ByteOffset(ref byte4.B3, ref byte4.B0)); } + [Fact] + public static void ByteOffsetNull() + { + [MethodImpl(MethodImplOptions.NoInlining)] + static nint NullTest(ref byte origin) => Unsafe.ByteOffset(ref origin, ref Unsafe.NullRef()); + Assert.Equal(0, NullTest(ref Unsafe.NullRef())); + } + [Fact] public static unsafe void AsRef() { From 116e941e94de85ee02d8ee07c5fc357c87eb47f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= <35800402+MichalPetryka@users.noreply.github.com> Date: Tue, 27 Feb 2024 23:48:06 +0100 Subject: [PATCH 06/13] Update morph.cpp --- src/coreclr/jit/morph.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index c005f1f95cb8f1..357e0d4489b83a 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -9239,7 +9239,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac, bool* optA /* Check for "cns1 - op2" , we change it to "(cns1 + (-op2))" */ noway_assert(op1); - if (op1->IsCnsIntOrI() && (op2->TypeGet() != TYP_BYREF)) + if (op1->IsCnsIntOrI() && !op2->TypeIs(TYP_BYREF)) { noway_assert(varTypeIsIntegralOrI(tree)); From e885f2f037a4103c912909e50cbc63f184b5a236 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= <35800402+MichalPetryka@users.noreply.github.com> Date: Wed, 28 Feb 2024 00:28:26 +0100 Subject: [PATCH 07/13] Update UnsafeTests.cs --- .../System.Runtime.CompilerServices.Unsafe.Tests/UnsafeTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 ac46f62f847679..cab4c1318be78c 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 @@ -605,7 +605,7 @@ public static void RefAddNuintByteOffset() } [Fact] - public static void RefSubtract() + public static unsafe void RefSubtract() { string[] a = new string[] { "abc", "def", "ghi", "jkl" }; From e5582580aedcf9d6eae5358dd9dce5d69e75432e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= <35800402+MichalPetryka@users.noreply.github.com> Date: Wed, 28 Feb 2024 01:02:42 +0100 Subject: [PATCH 08/13] Update UnsafeTests.cs --- .../System.Runtime.CompilerServices.Unsafe.Tests/UnsafeTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 cab4c1318be78c..658da42387d16f 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 @@ -620,7 +620,7 @@ public static unsafe void RefSubtract() [MethodImpl(MethodImplOptions.NoInlining)] static ref byte NullTest(nuint offset) => ref Unsafe.Subtract(ref Unsafe.NullRef(), offset); - Assert.Equal(0, (nuint)Unsafe.AsPointer(ref NullTest(0))); + Assert.True(Unsafe.IsNullRef(ref NullTest(0))); } [Fact] From 69836352cd2ab4aab7b5680e728b494d6c1fd569 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= <35800402+MichalPetryka@users.noreply.github.com> Date: Wed, 28 Feb 2024 13:20:47 +0100 Subject: [PATCH 09/13] Move the check --- src/coreclr/jit/morph.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 357e0d4489b83a..86b06861886257 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -9219,11 +9219,11 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac, bool* optA // TODO #4104: there are a lot of other places where // this condition is not checked before transformations. - if (fgGlobalMorph) + noway_assert(op2); + if (fgGlobalMorph && !op2->TypeIs(TYP_BYREF)) { /* Check for "op1 - cns2" , we change it to "op1 + (-cns2)" */ - noway_assert(op2); if (op2->IsCnsIntOrI() && !op2->IsIconHandle()) { // Negate the constant and change the node to be "+", @@ -9239,7 +9239,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac, bool* optA /* Check for "cns1 - op2" , we change it to "(cns1 + (-op2))" */ noway_assert(op1); - if (op1->IsCnsIntOrI() && !op2->TypeIs(TYP_BYREF)) + if (op1->IsCnsIntOrI()) { noway_assert(varTypeIsIntegralOrI(tree)); From 2f0058d6823332f65d46f0383d97f42325ac9b25 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= <35800402+MichalPetryka@users.noreply.github.com> Date: Wed, 28 Feb 2024 13:56:32 +0100 Subject: [PATCH 10/13] Add test for 3rd bug --- .../UnsafeTests.cs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) 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 658da42387d16f..71f266ddeb29b5 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 @@ -462,12 +462,23 @@ public static void ByteOffsetStackByte4() Assert.Equal(new IntPtr(-3), Unsafe.ByteOffset(ref byte4.B3, ref byte4.B0)); } + private static class StaticReadonlyHolder + { + public static readonly void* Pointer = (void*)RuntimeHelpers.AllocateTypeAssociatedMemory(typeof(BitcastDisasm), 1); + } + [Fact] - public static void ByteOffsetNull() + public static void ByteOffsetConstantRef() { [MethodImpl(MethodImplOptions.NoInlining)] static nint NullTest(ref byte origin) => Unsafe.ByteOffset(ref origin, ref Unsafe.NullRef()); Assert.Equal(0, NullTest(ref Unsafe.NullRef())); + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + static ref byte GetStatic(ref byte x) => ref x; + [MethodImpl(MethodImplOptions.NoInlining)] + static nint StaticReadonlyTest(ref byte x) => Unsafe.ByteOffset(ref GetStatic(ref Unsafe.AsRef(StaticReadonlyHolder.Pointer)), ref x); + Assert.Equal(0, StaticReadonlyTest(ref Unsafe.AsRef(StaticReadonlyHolder.Pointer))); } [Fact] From d9684924d52a576bf7bc7d51ee054dcd6c0f3959 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= <35800402+MichalPetryka@users.noreply.github.com> Date: Wed, 28 Feb 2024 14:21:08 +0100 Subject: [PATCH 11/13] Update UnsafeTests.cs --- .../UnsafeTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 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 71f266ddeb29b5..531dcdeee8ee2f 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 @@ -462,13 +462,13 @@ public static void ByteOffsetStackByte4() Assert.Equal(new IntPtr(-3), Unsafe.ByteOffset(ref byte4.B3, ref byte4.B0)); } - private static class StaticReadonlyHolder + private static unsafe class StaticReadonlyHolder { public static readonly void* Pointer = (void*)RuntimeHelpers.AllocateTypeAssociatedMemory(typeof(BitcastDisasm), 1); } [Fact] - public static void ByteOffsetConstantRef() + public static unsafe void ByteOffsetConstantRef() { [MethodImpl(MethodImplOptions.NoInlining)] static nint NullTest(ref byte origin) => Unsafe.ByteOffset(ref origin, ref Unsafe.NullRef()); From 6e5f3b8e3c36a5aec9844ec5687df630b3a6db6f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= <35800402+MichalPetryka@users.noreply.github.com> Date: Wed, 28 Feb 2024 14:53:26 +0100 Subject: [PATCH 12/13] Update UnsafeTests.cs --- .../System.Runtime.CompilerServices.Unsafe.Tests/UnsafeTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 531dcdeee8ee2f..723fc3a653684e 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 @@ -464,7 +464,7 @@ public static void ByteOffsetStackByte4() private static unsafe class StaticReadonlyHolder { - public static readonly void* Pointer = (void*)RuntimeHelpers.AllocateTypeAssociatedMemory(typeof(BitcastDisasm), 1); + public static readonly void* Pointer = (void*)RuntimeHelpers.AllocateTypeAssociatedMemory(typeof(StaticReadonlyHolder), 1); } [Fact] From a248717ffd514dc3fc414721a3cb6110c8934ce4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= <35800402+MichalPetryka@users.noreply.github.com> Date: Thu, 29 Feb 2024 13:20:11 +0100 Subject: [PATCH 13/13] Add links to the PR in comments --- .../System.Runtime.CompilerServices.Unsafe.Tests/UnsafeTests.cs | 2 ++ 1 file changed, 2 insertions(+) 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 723fc3a653684e..6eb527f923255b 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 @@ -470,6 +470,7 @@ private static unsafe class StaticReadonlyHolder [Fact] public static unsafe void ByteOffsetConstantRef() { + // https://github.com/dotnet/runtime/pull/99019 [MethodImpl(MethodImplOptions.NoInlining)] static nint NullTest(ref byte origin) => Unsafe.ByteOffset(ref origin, ref Unsafe.NullRef()); Assert.Equal(0, NullTest(ref Unsafe.NullRef())); @@ -629,6 +630,7 @@ public static unsafe void RefSubtract() ref string r3 = ref Unsafe.Subtract(ref r2, 3); Assert.Equal("abc", r3); + // https://github.com/dotnet/runtime/pull/99019 [MethodImpl(MethodImplOptions.NoInlining)] static ref byte NullTest(nuint offset) => ref Unsafe.Subtract(ref Unsafe.NullRef(), offset); Assert.True(Unsafe.IsNullRef(ref NullTest(0)));