From f82f21bfcb640a434ddef9ab1efa7dde17628a22 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 18 Apr 2022 14:28:30 +0200 Subject: [PATCH 1/8] Fix JIT using too wide indirections when returning small structs Fix #58874 Fix #64802 Fix #68157 --- src/coreclr/jit/lower.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index faa9e22ec22655..37b3ed13cfaea4 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -3645,14 +3645,14 @@ void Lowering::LowerRetStruct(GenTreeUnOp* ret) assert(varTypeIsStruct(ret)); GenTree* retVal = ret->gtGetOp1(); + var_types nativeReturnType = comp->info.compRetNativeType; // Note: small types are returned as INT. - var_types nativeReturnType = genActualType(comp->info.compRetNativeType); - ret->ChangeType(nativeReturnType); + ret->ChangeType(genActualType(nativeReturnType)); switch (retVal->OperGet()) { case GT_CALL: - assert(retVal->TypeIs(nativeReturnType)); // Type should be changed during call processing. + assert(retVal->TypeIs(genActualType(nativeReturnType))); // Type should be changed during call processing. break; case GT_CNS_INT: From fe6444c4ad56ad2629778fd204e19d6a22203380 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 18 Apr 2022 14:29:31 +0200 Subject: [PATCH 2/8] Add test --- .../JitBlue/Runtime_58874/Runtime_58874.cs | 56 +++++++++++++++++++ .../Runtime_58874/Runtime_58874.csproj | 9 +++ 2 files changed, 65 insertions(+) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_58874/Runtime_58874.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_58874/Runtime_58874.csproj diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_58874/Runtime_58874.cs b/src/tests/JIT/Regression/JitBlue/Runtime_58874/Runtime_58874.cs new file mode 100644 index 00000000000000..2c9a0abd4fca76 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_58874/Runtime_58874.cs @@ -0,0 +1,56 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; + +unsafe class Runtime_58874 +{ + private static int Main(string[] args) + { + void* mem; + if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) + { + const int MEM_COMMIT = 0x1000; + const int MEM_RESERVE = 0x2000; + const int PAGE_READWRITE = 0x04; + + // Reserve 2 pages + void* pages = VirtualAlloc(null, 0x2000, MEM_RESERVE, PAGE_READWRITE); + if (pages == null) + { + return 100; + } + // Commit first page + mem = VirtualAlloc(pages, 0x1000, MEM_COMMIT, PAGE_READWRITE); + if (mem != pages) + { + return 100; + } + } + else + { + mem = NativeMemory.Alloc(0x1000); + } + + ref Test validRef = ref *(Test*)((byte*)mem + 0x1000 - sizeof(Test)); + validRef = default; + Foo(ref validRef); + return 100; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static Test Foo(ref Test t) + { + // This read was too wide. + return t; + } + + [DllImport("kernel32")] + private static extern void* VirtualAlloc(void* lpAddress, nuint dwSize, uint flAllocationType, uint flProtect); +} + +struct Test +{ + public byte A, B; +} \ No newline at end of file diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_58874/Runtime_58874.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_58874/Runtime_58874.csproj new file mode 100644 index 00000000000000..6946bed81bfd5b --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_58874/Runtime_58874.csproj @@ -0,0 +1,9 @@ + + + Exe + True + + + + + From a019a3c7b2327d2a94896f81114aae4fb8db2725 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 18 Apr 2022 14:31:28 +0200 Subject: [PATCH 3/8] Run jit-format --- src/coreclr/jit/lower.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 37b3ed13cfaea4..3682fb9e53cf8b 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -3644,7 +3644,7 @@ void Lowering::LowerRetStruct(GenTreeUnOp* ret) assert(ret->OperIs(GT_RETURN)); assert(varTypeIsStruct(ret)); - GenTree* retVal = ret->gtGetOp1(); + GenTree* retVal = ret->gtGetOp1(); var_types nativeReturnType = comp->info.compRetNativeType; // Note: small types are returned as INT. ret->ChangeType(genActualType(nativeReturnType)); From d8b518403b931bdb248d366ab60bfd0f473da41f Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 18 Apr 2022 15:25:25 +0200 Subject: [PATCH 4/8] Do not leak in case of merged tests --- .../JitBlue/Runtime_58874/Runtime_58874.cs | 91 +++++++++++++------ 1 file changed, 63 insertions(+), 28 deletions(-) diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_58874/Runtime_58874.cs b/src/tests/JIT/Regression/JitBlue/Runtime_58874/Runtime_58874.cs index 2c9a0abd4fca76..9df97b2d18b888 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_58874/Runtime_58874.cs +++ b/src/tests/JIT/Regression/JitBlue/Runtime_58874/Runtime_58874.cs @@ -8,46 +8,81 @@ unsafe class Runtime_58874 { private static int Main(string[] args) { - void* mem; - if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) + using EndOfPage endOfPage = EndOfPage.Create(); + if (endOfPage == null) { - const int MEM_COMMIT = 0x1000; - const int MEM_RESERVE = 0x2000; - const int PAGE_READWRITE = 0x04; + return 100; + } + + Foo(endOfPage.Pointer); + return 100; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static Test Foo(Test* t) + { + // This read was too wide. + return *t; + } - // Reserve 2 pages - void* pages = VirtualAlloc(null, 0x2000, MEM_RESERVE, PAGE_READWRITE); - if (pages == null) + private class EndOfPage : IDisposable + { + private void* _addr; + private EndOfPage() + { + } + + public Test* Pointer => (Test*)((byte*)_addr + 0x1000 - sizeof(Test)); + public void Dispose() + { + if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) { - return 100; + const int MEM_RELEASE = 0x8000; + VirtualFree(_addr, 0, MEM_RELEASE); } - // Commit first page - mem = VirtualAlloc(pages, 0x1000, MEM_COMMIT, PAGE_READWRITE); - if (mem != pages) + else { - return 100; + NativeMemory.Free(_addr); } } - else + + public static EndOfPage Create() { - mem = NativeMemory.Alloc(0x1000); + void* mem; + if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) + { + const int MEM_COMMIT = 0x1000; + const int MEM_RESERVE = 0x2000; + const int PAGE_READWRITE = 0x04; + + // Reserve 2 pages + void* pages = VirtualAlloc(null, 0x2000, MEM_RESERVE, PAGE_READWRITE); + if (pages == null) + { + return null; + } + // Commit first page + mem = VirtualAlloc(pages, 0x1000, MEM_COMMIT, PAGE_READWRITE); + if (mem != pages) + { + return null; + } + } + else + { + mem = NativeMemory.Alloc(0x1000); + } + + return new EndOfPage { _addr = mem }; } - ref Test validRef = ref *(Test*)((byte*)mem + 0x1000 - sizeof(Test)); - validRef = default; - Foo(ref validRef); - return 100; - } + [DllImport("kernel32")] + private static extern void* VirtualAlloc(void* lpAddress, nuint dwSize, uint flAllocationType, uint flProtect); - [MethodImpl(MethodImplOptions.NoInlining)] - private static Test Foo(ref Test t) - { - // This read was too wide. - return t; + [DllImport("kernel32")] + [return: MarshalAs(UnmanagedType.Bool)] + private static extern bool VirtualFree(void* lpAddress, nuint dwSize, uint dwFreeType); } - - [DllImport("kernel32")] - private static extern void* VirtualAlloc(void* lpAddress, nuint dwSize, uint flAllocationType, uint flProtect); } struct Test From a6aa282625e6a89fdf9b34a8c1beec5a2182c801 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 18 Apr 2022 15:25:48 +0200 Subject: [PATCH 5/8] Small test nit --- .../JIT/Regression/JitBlue/Runtime_58874/Runtime_58874.cs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_58874/Runtime_58874.cs b/src/tests/JIT/Regression/JitBlue/Runtime_58874/Runtime_58874.cs index 9df97b2d18b888..17cc8e1af8bef9 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_58874/Runtime_58874.cs +++ b/src/tests/JIT/Regression/JitBlue/Runtime_58874/Runtime_58874.cs @@ -9,12 +9,10 @@ unsafe class Runtime_58874 private static int Main(string[] args) { using EndOfPage endOfPage = EndOfPage.Create(); - if (endOfPage == null) + if (endOfPage != null) { - return 100; + Foo(endOfPage.Pointer); } - - Foo(endOfPage.Pointer); return 100; } From b7160e2bf8a73d0b234440c5a70b299e8262485b Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 18 Apr 2022 15:28:37 +0200 Subject: [PATCH 6/8] Another test nit --- .../JitBlue/Runtime_58874/Runtime_58874.cs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_58874/Runtime_58874.cs b/src/tests/JIT/Regression/JitBlue/Runtime_58874/Runtime_58874.cs index 17cc8e1af8bef9..a3bd3b8fcadfe3 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_58874/Runtime_58874.cs +++ b/src/tests/JIT/Regression/JitBlue/Runtime_58874/Runtime_58874.cs @@ -54,16 +54,11 @@ public static EndOfPage Create() const int PAGE_READWRITE = 0x04; // Reserve 2 pages - void* pages = VirtualAlloc(null, 0x2000, MEM_RESERVE, PAGE_READWRITE); - if (pages == null) + mem = VirtualAlloc(null, 0x2000, MEM_RESERVE, PAGE_READWRITE); + if (mem != null) { - return null; - } - // Commit first page - mem = VirtualAlloc(pages, 0x1000, MEM_COMMIT, PAGE_READWRITE); - if (mem != pages) - { - return null; + // Commit first page + mem = VirtualAlloc(mem, 0x1000, MEM_COMMIT, PAGE_READWRITE); } } else @@ -71,6 +66,11 @@ public static EndOfPage Create() mem = NativeMemory.Alloc(0x1000); } + if (mem == null) + { + return null; + } + return new EndOfPage { _addr = mem }; } From 5ca923b80ac36fbf25f39c05831ff705b9d41421 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 18 Apr 2022 15:36:18 +0200 Subject: [PATCH 7/8] Final test nit --- .../JitBlue/Runtime_58874/Runtime_58874.cs | 27 ++++++++++++------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_58874/Runtime_58874.cs b/src/tests/JIT/Regression/JitBlue/Runtime_58874/Runtime_58874.cs index a3bd3b8fcadfe3..aa186c0baf2efc 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_58874/Runtime_58874.cs +++ b/src/tests/JIT/Regression/JitBlue/Runtime_58874/Runtime_58874.cs @@ -54,21 +54,28 @@ public static EndOfPage Create() const int PAGE_READWRITE = 0x04; // Reserve 2 pages - mem = VirtualAlloc(null, 0x2000, MEM_RESERVE, PAGE_READWRITE); - if (mem != null) + void* pages = VirtualAlloc(null, 0x2000, MEM_RESERVE, PAGE_READWRITE); + if (pages == null) { - // Commit first page - mem = VirtualAlloc(mem, 0x1000, MEM_COMMIT, PAGE_READWRITE); + return null; + } + // Commit first page + mem = VirtualAlloc(pages, 0x1000, MEM_COMMIT, PAGE_READWRITE); + if (mem != pages) + { + return null; } } else { - mem = NativeMemory.Alloc(0x1000); - } - - if (mem == null) - { - return null; + try + { + mem = NativeMemory.Alloc(0x1000); + } + catch (OutOfMemoryException) + { + return null; + } } return new EndOfPage { _addr = mem }; From af1a9071d636433c3c69212b17f2f00e67136456 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 18 Apr 2022 21:17:54 +0200 Subject: [PATCH 8/8] Make test compile --- .../JIT/Regression/JitBlue/Runtime_58874/Runtime_58874.cs | 5 +++-- .../Regression/JitBlue/Runtime_58874/Runtime_58874.csproj | 1 + 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_58874/Runtime_58874.cs b/src/tests/JIT/Regression/JitBlue/Runtime_58874/Runtime_58874.cs index aa186c0baf2efc..f1f7207b3c6c64 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_58874/Runtime_58874.cs +++ b/src/tests/JIT/Regression/JitBlue/Runtime_58874/Runtime_58874.cs @@ -1,10 +1,11 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System; using System.Runtime.CompilerServices; using System.Runtime.InteropServices; -unsafe class Runtime_58874 +public unsafe class Runtime_58874 { private static int Main(string[] args) { @@ -93,4 +94,4 @@ public static EndOfPage Create() struct Test { public byte A, B; -} \ No newline at end of file +} diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_58874/Runtime_58874.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_58874/Runtime_58874.csproj index 6946bed81bfd5b..8c7132fd350c9d 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_58874/Runtime_58874.csproj +++ b/src/tests/JIT/Regression/JitBlue/Runtime_58874/Runtime_58874.csproj @@ -2,6 +2,7 @@ Exe True + True