From 1fb49a3e8432a5af7fb8102ca0dae31af9abb56e Mon Sep 17 00:00:00 2001 From: Milos Kotlar Date: Tue, 15 Apr 2025 12:14:14 +0200 Subject: [PATCH 01/26] Implement localloc in the interpreter --- src/coreclr/interpreter/compiler.cpp | 19 +++++++++++++++++ src/coreclr/interpreter/intops.def | 1 + src/coreclr/vm/interpexec.cpp | 32 ++++++++++++++++++++++++++++ 3 files changed, 52 insertions(+) diff --git a/src/coreclr/interpreter/compiler.cpp b/src/coreclr/interpreter/compiler.cpp index b1b3b81cf17d73..e135e1b441e105 100644 --- a/src/coreclr/interpreter/compiler.cpp +++ b/src/coreclr/interpreter/compiler.cpp @@ -3119,6 +3119,25 @@ int InterpCompiler::GenerateCode(CORINFO_METHOD_INFO* methodInfo) m_ip += 5; break; } + case CEE_LOCALLOC: + CHECK_STACK(1); +#if SIZEOF_VOID_P == 8 + if (m_pStackPointer[-1].type == StackTypeI8) + EmitConv(m_pStackPointer - 1, NULL, StackTypeI4, INTOP_MOV_8); +#endif + AddIns(INTOP_LOCALLOC); + m_pStackPointer--; + if (m_pStackPointer != m_pStackBase) + { + m_hasInvalidCode = true; + goto exit_bad_code; + } + + m_pLastNewIns->SetSVar(m_pStackPointer[0].var); + PushStackType(StackTypeByRef, NULL); + m_pLastNewIns->SetDVar(m_pStackPointer[-1].var); + m_ip++; + break; default: assert(0); break; diff --git a/src/coreclr/interpreter/intops.def b/src/coreclr/interpreter/intops.def index c23ed142308333..627ec8cad18c03 100644 --- a/src/coreclr/interpreter/intops.def +++ b/src/coreclr/interpreter/intops.def @@ -252,6 +252,7 @@ OPDEF(INTOP_NEWOBJ_VT, "newobj.vt", 5, 1, 1, InterpOpMethodToken) OPDEF(INTOP_CALL_HELPER_PP, "call.helper.pp", 5, 1, 0, InterpOpThreeInts) OPDEF(INTOP_ZEROBLK_IMM, "zeroblk.imm", 3, 0, 1, InterpOpInt) +OPDEF(INTOP_LOCALLOC, "localloc", 3, 1, 1, InterpOpNoArgs) OPDEF(INTOP_BREAKPOINT, "breakpoint", 1, 0, 0, InterpOpNoArgs) OPDEF(INTOP_FAILFAST, "failfast", 1, 0, 0, InterpOpNoArgs) diff --git a/src/coreclr/vm/interpexec.cpp b/src/coreclr/vm/interpexec.cpp index 8aa38305c64246..c73e07c80d021a 100644 --- a/src/coreclr/vm/interpexec.cpp +++ b/src/coreclr/vm/interpexec.cpp @@ -43,6 +43,20 @@ static void InterpBreakpoint() // TODO once we have basic EH support #define NULL_CHECK(o) +void* frame_data_allocator_alloc(InterpThreadContext *pThreadContext, int8_t** stack, int size) { + void* res = NULL; + if (*stack + size <= (int8_t*)pThreadContext->pStackEnd) { + res = *stack; + *stack += size; + } else { + // Handle stack overflow or allocate additional memory + // Add fragment to the stack or throw an error + assert(0 && "Stack overflow in frame_data_allocator_alloc"); + } + + return res; +} + void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFrame *pFrame, InterpThreadContext *pThreadContext) { const int32_t *ip; @@ -1074,6 +1088,24 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr memset(LOCAL_VAR(ip[1], void*), 0, ip[2]); ip += 3; break; + case INTOP_LOCALLOC: + { + int32_t len = LOCAL_VAR(ip[2], int32_t); + void* mem; + + if (len > 0) { + mem = frame_data_allocator_alloc(pThreadContext, &stack, len); + + // if (pMethod->initLocals) { + // memset(mem, 0, len); + // } + } else { + mem = NULL; + } + LOCAL_VAR(ip[1], void*) = mem; + ip += 3; + break; + } case INTOP_FAILFAST: assert(0); break; From e5fb653e0768dc08240493acac151556b1a501c1 Mon Sep 17 00:00:00 2001 From: Milos Kotlar Date: Tue, 15 Apr 2025 12:14:24 +0200 Subject: [PATCH 02/26] Add comment --- src/coreclr/vm/interpexec.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/coreclr/vm/interpexec.cpp b/src/coreclr/vm/interpexec.cpp index c73e07c80d021a..7fa365c97d0875 100644 --- a/src/coreclr/vm/interpexec.cpp +++ b/src/coreclr/vm/interpexec.cpp @@ -49,8 +49,7 @@ void* frame_data_allocator_alloc(InterpThreadContext *pThreadContext, int8_t** s res = *stack; *stack += size; } else { - // Handle stack overflow or allocate additional memory - // Add fragment to the stack or throw an error + // Add stack fragments assert(0 && "Stack overflow in frame_data_allocator_alloc"); } From 6b81d9b152c02e9a0cc19dc758e31280fee53866 Mon Sep 17 00:00:00 2001 From: Milos Kotlar Date: Mon, 21 Apr 2025 13:09:51 +0200 Subject: [PATCH 03/26] Implement FrameDataAllocator for dynamic stack allocations --- src/coreclr/interpreter/compiler.cpp | 8 + src/coreclr/interpreter/intops.def | 6 + src/coreclr/vm/interpexec.cpp | 179 ++++++++++++++++++++--- src/coreclr/vm/interpexec.h | 31 ++++ src/tests/JIT/interpreter/Interpreter.cs | 82 +++++++++++ 5 files changed, 286 insertions(+), 20 deletions(-) diff --git a/src/coreclr/interpreter/compiler.cpp b/src/coreclr/interpreter/compiler.cpp index e135e1b441e105..3513f1435f1e0e 100644 --- a/src/coreclr/interpreter/compiler.cpp +++ b/src/coreclr/interpreter/compiler.cpp @@ -2582,6 +2582,14 @@ int InterpCompiler::GenerateCode(CORINFO_METHOD_INFO* methodInfo) EmitBinaryArithmeticOp(INTOP_MUL_I4); m_ip++; break; + case CEE_MUL_OVF: + EmitBinaryArithmeticOp(INTOP_MUL_OVF_I4); + m_ip++; + break; + case CEE_MUL_OVF_UN: + EmitBinaryArithmeticOp(INTOP_MUL_OVF_UN_I4); + m_ip++; + break; case CEE_DIV: EmitBinaryArithmeticOp(INTOP_DIV_I4); m_ip++; diff --git a/src/coreclr/interpreter/intops.def b/src/coreclr/interpreter/intops.def index 627ec8cad18c03..4c387ced0b8010 100644 --- a/src/coreclr/interpreter/intops.def +++ b/src/coreclr/interpreter/intops.def @@ -160,6 +160,12 @@ OPDEF(INTOP_MUL_I8, "mul.i8", 4, 1, 2, InterpOpNoArgs) OPDEF(INTOP_MUL_R4, "mul.r4", 4, 1, 2, InterpOpNoArgs) OPDEF(INTOP_MUL_R8, "mul.r8", 4, 1, 2, InterpOpNoArgs) +OPDEF(INTOP_MUL_OVF_I4, "mul.ovf.i8", 4, 1, 2, InterpOpNoArgs) +OPDEF(INTOP_MUL_OVF_I8, "mul.ovf.i8", 4, 1, 2, InterpOpNoArgs) + +OPDEF(INTOP_MUL_OVF_UN_I4, "mul.ovf.un.i8", 4, 1, 2, InterpOpNoArgs) +OPDEF(INTOP_MUL_OVF_UN_I8, "mul.ovf.un.i8", 4, 1, 2, InterpOpNoArgs) + OPDEF(INTOP_DIV_I4, "div.i4", 4, 1, 2, InterpOpNoArgs) OPDEF(INTOP_DIV_I8, "div.i8", 4, 1, 2, InterpOpNoArgs) OPDEF(INTOP_DIV_R4, "div.r4", 4, 1, 2, InterpOpNoArgs) diff --git a/src/coreclr/vm/interpexec.cpp b/src/coreclr/vm/interpexec.cpp index 7fa365c97d0875..a6cea982427dd0 100644 --- a/src/coreclr/vm/interpexec.cpp +++ b/src/coreclr/vm/interpexec.cpp @@ -11,6 +11,110 @@ typedef void* (*HELPER_FTN_PP)(void*); thread_local InterpThreadContext *t_pThreadContext = NULL; +FrameDataFragment *frame_data_new_fragment(size_t size) +{ + if (size < INTERP_STACK_FRAGMENT_SIZE) + size = INTERP_STACK_FRAGMENT_SIZE; + + FrameDataFragment *frag = (FrameDataFragment*)malloc(sizeof(FrameDataFragment) + size); + if (!frag) return NULL; + + frag->start = (uint8_t*)(frag + 1); + frag->end = frag->start + size; + frag->pos = frag->start; + frag->next = NULL; + return frag; +} + +void frame_data_allocator_init(FrameDataAllocator *allocator, size_t size) +{ + allocator->first = frame_data_new_fragment(size); + assert(allocator->first && "Failed to allocate initial fragment"); + allocator->current = allocator->first; + allocator->infos = NULL; + allocator->infos_len = 0; + allocator->infos_capacity = 0; +} + +void frame_data_fragment_free(FrameDataFragment *frag) +{ + while (frag) { + FrameDataFragment *next = frag->next; + free(frag); + frag = next; + } +} + +void frame_data_allocator_destroy(FrameDataAllocator *allocator) +{ + assert (allocator->current == allocator->first && allocator->current->pos == allocator->current->start); + frame_data_fragment_free(allocator->first); + + free(allocator->infos); + allocator->first = allocator->current = NULL; + allocator->infos = NULL; + allocator->infos_len = allocator->infos_capacity = 0; +} + +void frame_data_push_info(FrameDataAllocator *allocator, InterpreterFrame *frame) +{ + if (allocator->infos_len == allocator->infos_capacity) { + int new_capacity = allocator->infos_capacity == 0 ? 8 : allocator->infos_capacity * 2; + allocator->infos = (FrameDataInfo*)realloc(allocator->infos, new_capacity * sizeof(FrameDataInfo)); + assert(allocator->infos && "Failed to reallocate frame info"); + allocator->infos_capacity = new_capacity; + } + + FrameDataInfo *info = &allocator->infos[allocator->infos_len++]; + info->frame = frame; + info->frag = allocator->current; + info->pos = allocator->current->pos; +} + +void *frame_data_alloc(FrameDataAllocator *allocator, InterpreterFrame *frame, size_t size) +{ + + if (!allocator->infos_len || (allocator->infos_len > 0 && allocator->infos[allocator->infos_len - 1].frame != frame)) + { + frame_data_push_info(allocator, frame); + } + + uint8_t *pos = allocator->current->pos; + + if (pos + size > allocator->current->end) { + if (allocator->current->next && ((allocator->current->next->start + size) <= allocator->current->next->end)) + { + allocator->current = allocator->current->next; + pos = allocator->current->pos = allocator->current->start; + } + else + { + frame_data_fragment_free(allocator->current->next); + FrameDataFragment *new_frag = frame_data_new_fragment(size); + assert(new_frag && "Failed to allocate new fragment"); + allocator->current->next = new_frag; + allocator->current = new_frag; + + pos = new_frag->pos; + } + } + + void *result = (void*)pos; + allocator->current->pos = (uint8_t *)(pos + size); + return result; +} + +void frame_data_pop_info(FrameDataAllocator *allocator, InterpreterFrame *pFrame) +{ + int top = allocator->infos_len - 1; + if (top >= 0 && allocator->infos[top].frame == pFrame) + { + FrameDataInfo *info = &allocator->infos[--allocator->infos_len]; + allocator->current = info->frag; + allocator->current->pos = info->pos; + } +} + InterpThreadContext* InterpGetThreadContext() { InterpThreadContext *threadContext = t_pThreadContext; @@ -21,6 +125,7 @@ InterpThreadContext* InterpGetThreadContext() // FIXME VirtualAlloc/mmap with INTERP_STACK_ALIGNMENT alignment threadContext->pStackStart = threadContext->pStackPointer = (int8_t*)malloc(INTERP_STACK_SIZE); threadContext->pStackEnd = threadContext->pStackStart + INTERP_STACK_SIZE; + frame_data_allocator_init(&threadContext->frameDataAllocator, INTERP_STACK_FRAGMENT_SIZE); t_pThreadContext = threadContext; return threadContext; @@ -43,19 +148,6 @@ static void InterpBreakpoint() // TODO once we have basic EH support #define NULL_CHECK(o) -void* frame_data_allocator_alloc(InterpThreadContext *pThreadContext, int8_t** stack, int size) { - void* res = NULL; - if (*stack + size <= (int8_t*)pThreadContext->pStackEnd) { - res = *stack; - *stack += size; - } else { - // Add stack fragments - assert(0 && "Stack overflow in frame_data_allocator_alloc"); - } - - return res; -} - void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFrame *pFrame, InterpThreadContext *pThreadContext) { const int32_t *ip; @@ -596,7 +688,53 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr LOCAL_VAR(ip[1], double) = LOCAL_VAR(ip[2], double) * LOCAL_VAR(ip[3], double); ip += 4; break; + case INTOP_MUL_OVF_I4: + { + int32_t i1 = LOCAL_VAR(ip[2], int32_t); + int32_t i2 = LOCAL_VAR(ip[3], int32_t); + int32_t i3; + if (__builtin_mul_overflow(i1, i2, &i3)) + assert(0); // Interpreter-TODO: OverflowException + LOCAL_VAR(ip[1], int32_t) = i3; + ip += 4; + break; + } + case INTOP_MUL_OVF_I8: + { + int64_t i1 = LOCAL_VAR(ip[2], int64_t); + int64_t i2 = LOCAL_VAR(ip[3], int64_t); + int64_t i3; + if (__builtin_mul_overflow(i1, i2, &i3)) + assert(0); // Interpreter-TODO: OverflowException + LOCAL_VAR(ip[1], int64_t) = i3; + ip += 4; + break; + } + + case INTOP_MUL_OVF_UN_I4: + { + uint32_t i1 = LOCAL_VAR(ip[2], uint32_t); + uint32_t i2 = LOCAL_VAR(ip[3], uint32_t); + uint32_t i3; + if (__builtin_mul_overflow(i1, i2, &i3)) + assert(0); // Interpreter-TODO: OverflowException + LOCAL_VAR(ip[1], uint32_t) = i3; + ip += 4; + break; + } + + case INTOP_MUL_OVF_UN_I8: + { + uint64_t i1 = LOCAL_VAR(ip[2], uint64_t); + uint64_t i2 = LOCAL_VAR(ip[3], uint64_t); + uint64_t i3; + if (__builtin_mul_overflow(i1, i2, &i3)) + assert(0); // Interpreter-TODO: OverflowException + LOCAL_VAR(ip[1], uint64_t) = i3; + ip += 4; + break; + } case INTOP_DIV_I4: { int32_t i1 = LOCAL_VAR(ip[2], int32_t); @@ -1092,13 +1230,11 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr int32_t len = LOCAL_VAR(ip[2], int32_t); void* mem; - if (len > 0) { - mem = frame_data_allocator_alloc(pThreadContext, &stack, len); - - // if (pMethod->initLocals) { - // memset(mem, 0, len); - // } - } else { + if (len > 0) + { + mem = frame_data_alloc(&pThreadContext->frameDataAllocator, (InterpreterFrame*)pFrame, ALIGN_UP(len, INTERP_STACK_ALIGNMENT)); + } else + { mem = NULL; } LOCAL_VAR(ip[1], void*) = mem; @@ -1115,6 +1251,8 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr } EXIT_FRAME: + + frame_data_pop_info(&pThreadContext->frameDataAllocator, (InterpreterFrame*)pFrame); if (pFrame->pParent && pFrame->pParent->ip) { // Return to the main loop after a non-recursive interpreter call @@ -1129,6 +1267,7 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr goto MAIN_LOOP; } + frame_data_allocator_destroy(&pThreadContext->frameDataAllocator); pThreadContext->pStackPointer = pFrame->pStack; } diff --git a/src/coreclr/vm/interpexec.h b/src/coreclr/vm/interpexec.h index f153006df658c5..e984f02929a61a 100644 --- a/src/coreclr/vm/interpexec.h +++ b/src/coreclr/vm/interpexec.h @@ -7,6 +7,7 @@ #include "../interpreter/interpretershared.h" #define INTERP_STACK_SIZE 1024*1024 +#define INTERP_STACK_FRAGMENT_SIZE 4096 struct StackVal { @@ -42,6 +43,33 @@ struct InterpMethodContextFrame #endif // DACCESS_COMPILE }; +struct FrameDataFragment { + // Memory region for this fragment + uint8_t *start, *end; + // Current allocation pointer within this fragment + uint8_t *pos; + // Pointer to the next fragment + FrameDataFragment *next; +}; + +struct FrameDataInfo { + // Pointer to the frame that this info is associated with + InterpreterFrame *frame; + // Pointers for restoring the localloc memory: + // frag - the current allocation fragment at frame entry + // pos - the fragment pointer at frame entry + // When the frame returns, we use these to roll back any local allocations + FrameDataFragment *frag; + uint8_t *pos; +}; + +struct FrameDataAllocator { + FrameDataFragment *first, *current; + FrameDataInfo *infos; + int infos_len; + int infos_capacity; +}; + struct InterpThreadContext { int8_t *pStackStart; @@ -52,6 +80,9 @@ struct InterpThreadContext // stack pointer. It is needed when re-entering interp, to know from which address we can start using // stack, and also needed for the GC to be able to scan the stack. int8_t *pStackPointer; + + // This is an allocator for the dynamic stack memory + FrameDataAllocator frameDataAllocator; }; InterpThreadContext* InterpGetThreadContext(); diff --git a/src/tests/JIT/interpreter/Interpreter.cs b/src/tests/JIT/interpreter/Interpreter.cs index 1a634ee9fada7a..c2b0bb21fbaf79 100644 --- a/src/tests/JIT/interpreter/Interpreter.cs +++ b/src/tests/JIT/interpreter/Interpreter.cs @@ -71,6 +71,9 @@ public static void RunInterpreterTests() // Environment.FailFast(null); if (!TestFloat()) Environment.FailFast(null); + + if (!TestLocalloc()) + Environment.FailFast(null); } public static int Mul4(int a, int b, int c, int d) @@ -209,4 +212,83 @@ public static bool TestFloat() return true; } + + public static bool TestLocalloc() + { + // Default fragment size is 4096 bytes + + // Small tests + if (0 != LocallocIntTests(0)) return false; + if (0 != LocallocIntTests(1)) return false; + if (2 != LocallocIntTests(2)) return false; + + // Smoke tests + if (32 != LocallocByteTests(32)) return false; + if (32 != LocallocIntTests(32)) return false; + if (32 != LocallocLongTests(32)) return false; + + // Single frame tests + if (1024 != LocallocIntTests(1024)) return false; + if (512 != LocallocLongTests(512)) return false; + + // New fragment tests + if (1025 != LocallocIntTests(1025)) return false; + if (513 != LocallocLongTests(513)) return false; + + // Multi-fragment tests + if (10240 != LocallocIntTests(10240)) return false; + if (5120 != LocallocLongTests(5120)) return false; + + // Consecutive allocations tests + if ((256 + 512) != LocallocConsecutiveTests(256, 512)) return false; + + // Nested frames tests + if (1024 != LocallocNestedTests(256, 256, 256, 256)) return false; + if (2560 != LocallocNestedTests(1024, 256, 256, 1024)) return false; + + // Reuse fragment tests + if (3072 != LocallocNestedTests(1024, 512, 512, 1024)) return false; + + return true; + } + + public static unsafe int LocallocIntTests(int n) + { + int* a = stackalloc int[n]; + for (int i = 0; i < n; i++) a[i] = i; + return n < 2 ? 0 : a[0] + a[1] + a[n - 1]; + } + + public static unsafe long LocallocLongTests(int n) + { + long* a = stackalloc long[n]; + for (int i = 0; i < n; i++) a[i] = i; + return n < 2 ? 0 : a[0] + a[1] + a[n - 1]; + } + + public static unsafe int LocallocByteTests(int n) + { + byte* a = stackalloc byte[n]; + for (int i = 0; i < n; i++) a[i] = (byte)(i); + return n < 2 ? 0 : a[0] + a[1] + a[n - 1]; + } + + public static unsafe int LocallocConsecutiveTests(int n, int m) + { + int* a = stackalloc int[n]; + int* b = stackalloc int[m]; + for (int i = 0; i < n; i++) a[i] = i; + for (int i = 0; i < m; i++) b[i] = i; + return a[0] + a[1] + a[n - 1] + b[0] + b[1] + b[m - 1]; + } + + public static unsafe int LocallocNestedTests(int n, int m, int p, int k) + { + int* a1 = stackalloc int[n]; + for (int i = 0; i < n; i++) a1[i] = i; + int inner = LocallocConsecutiveTests(m, p); + int* a2 = stackalloc int[k]; + for (int i = 0; i < k; i++) a2[i] = i; + return a1[0] + a1[1] + a1[n - 1] + inner + a2[0] + a2[1] + a2[k - 1]; + } } From f6603655b37d67aefb26e2f16b58f9e5f6a3983f Mon Sep 17 00:00:00 2001 From: Milos Kotlar Date: Mon, 21 Apr 2025 13:16:16 +0200 Subject: [PATCH 04/26] Fix merge conflicts --- src/tests/JIT/interpreter/Interpreter.cs | 106 ++++++++++++++++++++++- 1 file changed, 104 insertions(+), 2 deletions(-) diff --git a/src/tests/JIT/interpreter/Interpreter.cs b/src/tests/JIT/interpreter/Interpreter.cs index 8c54f9704a6d2f..3f681ba7c4a871 100644 --- a/src/tests/JIT/interpreter/Interpreter.cs +++ b/src/tests/JIT/interpreter/Interpreter.cs @@ -98,11 +98,12 @@ public static void RunInterpreterTests() // Environment.FailFast(null); if (!TestFloat()) Environment.FailFast(null); -// if (!TestVirtual()) -// Environment.FailFast(null); if (!TestLocalloc()) Environment.FailFast(null); + +// if (!TestVirtual()) +// Environment.FailFast(null); } public static int Mul4(int a, int b, int c, int d) @@ -241,4 +242,105 @@ public static bool TestFloat() return true; } + + public static bool TestLocalloc() + { + // Default fragment size is 4096 bytes + + // Small tests + if (0 != LocallocIntTests(0)) return false; + if (0 != LocallocIntTests(1)) return false; + if (2 != LocallocIntTests(2)) return false; + + // Smoke tests + if (32 != LocallocByteTests(32)) return false; + if (32 != LocallocIntTests(32)) return false; + if (32 != LocallocLongTests(32)) return false; + + // Single frame tests + if (1024 != LocallocIntTests(1024)) return false; + if (512 != LocallocLongTests(512)) return false; + + // New fragment tests + if (1025 != LocallocIntTests(1025)) return false; + if (513 != LocallocLongTests(513)) return false; + + // Multi-fragment tests + if (10240 != LocallocIntTests(10240)) return false; + if (5120 != LocallocLongTests(5120)) return false; + + // Consecutive allocations tests + if ((256 + 512) != LocallocConsecutiveTests(256, 512)) return false; + + // Nested frames tests + if (1024 != LocallocNestedTests(256, 256, 256, 256)) return false; + if (2560 != LocallocNestedTests(1024, 256, 256, 1024)) return false; + + // Reuse fragment tests + if (3072 != LocallocNestedTests(1024, 512, 512, 1024)) return false; + + return true; + } + + public static unsafe int LocallocIntTests(int n) + { + int* a = stackalloc int[n]; + for (int i = 0; i < n; i++) a[i] = i; + return n < 2 ? 0 : a[0] + a[1] + a[n - 1]; + } + + public static unsafe long LocallocLongTests(int n) + { + long* a = stackalloc long[n]; + for (int i = 0; i < n; i++) a[i] = i; + return n < 2 ? 0 : a[0] + a[1] + a[n - 1]; + } + + public static unsafe int LocallocByteTests(int n) + { + byte* a = stackalloc byte[n]; + for (int i = 0; i < n; i++) a[i] = (byte)(i); + return n < 2 ? 0 : a[0] + a[1] + a[n - 1]; + } + + public static unsafe int LocallocConsecutiveTests(int n, int m) + { + int* a = stackalloc int[n]; + int* b = stackalloc int[m]; + for (int i = 0; i < n; i++) a[i] = i; + for (int i = 0; i < m; i++) b[i] = i; + return a[0] + a[1] + a[n - 1] + b[0] + b[1] + b[m - 1]; + } + + public static unsafe int LocallocNestedTests(int n, int m, int p, int k) + { + int* a1 = stackalloc int[n]; + for (int i = 0; i < n; i++) a1[i] = i; + int inner = LocallocConsecutiveTests(m, p); + int* a2 = stackalloc int[k]; + for (int i = 0; i < k; i++) a2[i] = i; + return a1[0] + a1[1] + a1[n - 1] + inner + a2[0] + a2[1] + a2[k - 1]; + } + + public static bool TestVirtual() + { + BaseClass bc = new DerivedClass(); + ITest itest = bc; + + if (bc.NonVirtualMethod() != 0xbaba) + return false; + if (bc.VirtualMethod() != 0xdede) + return false; + if (itest.VirtualMethod() != 0xdede) + return false; + bc = new BaseClass(); + itest = bc; + if (bc.NonVirtualMethod() != 0xbaba) + return false; + if (bc.VirtualMethod() != 0xbebe) + return false; + if (itest.VirtualMethod() != 0xbebe) + return false; + return true; + } } From e59cace3c8830226a7d9610e2201035928b5a386 Mon Sep 17 00:00:00 2001 From: Milos Kotlar Date: Tue, 22 Apr 2025 15:25:18 +0200 Subject: [PATCH 05/26] Initialize local variables to zero if the flag is set --- src/coreclr/vm/interpexec.cpp | 26 +++++++++++++++++++----- src/tests/JIT/interpreter/Interpreter.cs | 11 ++++++++++ 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/src/coreclr/vm/interpexec.cpp b/src/coreclr/vm/interpexec.cpp index abe960b4bb0b8c..47e83b807b087b 100644 --- a/src/coreclr/vm/interpexec.cpp +++ b/src/coreclr/vm/interpexec.cpp @@ -694,7 +694,7 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr int32_t i1 = LOCAL_VAR(ip[2], int32_t); int32_t i2 = LOCAL_VAR(ip[3], int32_t); int32_t i3; - if (__builtin_mul_overflow(i1, i2, &i3)) + if (!ClrSafeInt::multiply(i1, i2, i3)) assert(0); // Interpreter-TODO: OverflowException LOCAL_VAR(ip[1], int32_t) = i3; ip += 4; @@ -706,7 +706,7 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr int64_t i1 = LOCAL_VAR(ip[2], int64_t); int64_t i2 = LOCAL_VAR(ip[3], int64_t); int64_t i3; - if (__builtin_mul_overflow(i1, i2, &i3)) + if (!ClrSafeInt::multiply(i1, i2, i3)) assert(0); // Interpreter-TODO: OverflowException LOCAL_VAR(ip[1], int64_t) = i3; ip += 4; @@ -718,7 +718,7 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr uint32_t i1 = LOCAL_VAR(ip[2], uint32_t); uint32_t i2 = LOCAL_VAR(ip[3], uint32_t); uint32_t i3; - if (__builtin_mul_overflow(i1, i2, &i3)) + if (!ClrSafeInt::multiply(i1, i2, i3)) assert(0); // Interpreter-TODO: OverflowException LOCAL_VAR(ip[1], uint32_t) = i3; ip += 4; @@ -730,7 +730,7 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr uint64_t i1 = LOCAL_VAR(ip[2], uint64_t); uint64_t i2 = LOCAL_VAR(ip[3], uint64_t); uint64_t i3; - if (__builtin_mul_overflow(i1, i2, &i3)) + if (!ClrSafeInt::multiply(i1, i2, i3)) assert(0); // Interpreter-TODO: OverflowException LOCAL_VAR(ip[1], uint64_t) = i3; ip += 4; @@ -1263,7 +1263,23 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr if (len > 0) { - mem = frame_data_alloc(&pThreadContext->frameDataAllocator, (InterpreterFrame*)pFrame, ALIGN_UP(len, INTERP_STACK_ALIGNMENT)); + len = ALIGN_UP(len, INTERP_STACK_ALIGNMENT); + mem = frame_data_alloc(&pThreadContext->frameDataAllocator, (InterpreterFrame*)pFrame, len); + if (mem != NULL) + { + MethodDesc* md = reinterpret_cast(pMethod->methodHnd); + COR_ILMETHOD* pIL = md->GetILHeader(); + + if (pIL != NULL) + { + COR_ILMETHOD_DECODER header(pIL); + // Initialize local variables to zero if the flag is set + if (header.GetFlags() & CorILMethod_InitLocals) + { + memset(mem, 0, len); + } + } + } } else { mem = NULL; diff --git a/src/tests/JIT/interpreter/Interpreter.cs b/src/tests/JIT/interpreter/Interpreter.cs index 3f681ba7c4a871..a9f05cf9066fae 100644 --- a/src/tests/JIT/interpreter/Interpreter.cs +++ b/src/tests/JIT/interpreter/Interpreter.cs @@ -279,6 +279,9 @@ public static bool TestLocalloc() // Reuse fragment tests if (3072 != LocallocNestedTests(1024, 512, 512, 1024)) return false; + // SkipLocalsInit tests + if (!LocallocSkipLocalsInit(32)) return false; + return true; } @@ -322,6 +325,14 @@ public static unsafe int LocallocNestedTests(int n, int m, int p, int k) return a1[0] + a1[1] + a1[n - 1] + inner + a2[0] + a2[1] + a2[k - 1]; } + [SkipLocalsInit] + public static unsafe bool LocallocSkipLocalsInit(int n) + { + int* a = stackalloc int[n]; + for (int i = 0; i < n; i++) if (a[i] != 0) return true; + return false; + } + public static bool TestVirtual() { BaseClass bc = new DerivedClass(); From 7c649a839bf427b6cc67bfc85fbf474412406ff1 Mon Sep 17 00:00:00 2001 From: Milos Kotlar Date: Tue, 22 Apr 2025 17:41:20 +0200 Subject: [PATCH 06/26] Initialize local variables based on the CORINFO_OPT_INIT_LOCALS. Move FrameDataAllocator code to interpframeallocator.cpp/h. Add TODO comments. --- src/coreclr/interpreter/compiler.cpp | 4 +- src/coreclr/interpreter/interpretershared.h | 4 +- src/coreclr/vm/CMakeLists.txt | 2 + src/coreclr/vm/interpexec.cpp | 129 ++------------------ src/coreclr/vm/interpexec.h | 30 +---- src/coreclr/vm/interpframeallocator.cpp | 108 ++++++++++++++++ src/coreclr/vm/interpframeallocator.h | 52 ++++++++ src/tests/JIT/interpreter/Interpreter.cs | 3 +- 8 files changed, 180 insertions(+), 152 deletions(-) create mode 100644 src/coreclr/vm/interpframeallocator.cpp create mode 100644 src/coreclr/vm/interpframeallocator.h diff --git a/src/coreclr/interpreter/compiler.cpp b/src/coreclr/interpreter/compiler.cpp index da54d52082062e..97a95ac8714817 100644 --- a/src/coreclr/interpreter/compiler.cpp +++ b/src/coreclr/interpreter/compiler.cpp @@ -815,7 +815,9 @@ InterpMethod* InterpCompiler::CreateInterpMethod() for (int i = 0; i < numDataItems; i++) pDataItems[i] = m_dataItems.Get(i); - InterpMethod *pMethod = new InterpMethod(m_methodHnd, m_totalVarsStackSize, pDataItems); + bool initLocals = (m_methodInfo->options & CORINFO_OPT_INIT_LOCALS) != 0; + + InterpMethod *pMethod = new InterpMethod(m_methodHnd, m_totalVarsStackSize, pDataItems, initLocals); return pMethod; } diff --git a/src/coreclr/interpreter/interpretershared.h b/src/coreclr/interpreter/interpretershared.h index 5e8928b840bafd..53af35196a597a 100644 --- a/src/coreclr/interpreter/interpretershared.h +++ b/src/coreclr/interpreter/interpretershared.h @@ -18,12 +18,14 @@ struct InterpMethod CORINFO_METHOD_HANDLE methodHnd; int32_t allocaSize; void** pDataItems; + uint32_t initLocals : 1; - InterpMethod(CORINFO_METHOD_HANDLE methodHnd, int32_t allocaSize, void** pDataItems) + InterpMethod(CORINFO_METHOD_HANDLE methodHnd, int32_t allocaSize, void** pDataItems, bool initLocals) { this->methodHnd = methodHnd; this->allocaSize = allocaSize; this->pDataItems = pDataItems; + this->initLocals = initLocals; } }; diff --git a/src/coreclr/vm/CMakeLists.txt b/src/coreclr/vm/CMakeLists.txt index 7fbeffa86df59d..1b904463de7cfc 100644 --- a/src/coreclr/vm/CMakeLists.txt +++ b/src/coreclr/vm/CMakeLists.txt @@ -344,6 +344,7 @@ set(VM_SOURCES_WKS interopconverter.cpp interoputil.cpp interpexec.cpp + interpframeallocator.cpp invokeutil.cpp jithelpers.cpp managedmdimport.cpp @@ -444,6 +445,7 @@ set(VM_HEADERS_WKS interoputil.h interoputil.inl interpexec.h + interpframallocator.h invokeutil.h managedmdimport.hpp marshalnative.h diff --git a/src/coreclr/vm/interpexec.cpp b/src/coreclr/vm/interpexec.cpp index 47e83b807b087b..e6d234717c01c5 100644 --- a/src/coreclr/vm/interpexec.cpp +++ b/src/coreclr/vm/interpexec.cpp @@ -11,110 +11,6 @@ typedef void* (*HELPER_FTN_PP)(void*); thread_local InterpThreadContext *t_pThreadContext = NULL; -FrameDataFragment *frame_data_new_fragment(size_t size) -{ - if (size < INTERP_STACK_FRAGMENT_SIZE) - size = INTERP_STACK_FRAGMENT_SIZE; - - FrameDataFragment *frag = (FrameDataFragment*)malloc(sizeof(FrameDataFragment) + size); - if (!frag) return NULL; - - frag->start = (uint8_t*)(frag + 1); - frag->end = frag->start + size; - frag->pos = frag->start; - frag->next = NULL; - return frag; -} - -void frame_data_allocator_init(FrameDataAllocator *allocator, size_t size) -{ - allocator->first = frame_data_new_fragment(size); - assert(allocator->first && "Failed to allocate initial fragment"); - allocator->current = allocator->first; - allocator->infos = NULL; - allocator->infos_len = 0; - allocator->infos_capacity = 0; -} - -void frame_data_fragment_free(FrameDataFragment *frag) -{ - while (frag) { - FrameDataFragment *next = frag->next; - free(frag); - frag = next; - } -} - -void frame_data_allocator_destroy(FrameDataAllocator *allocator) -{ - assert (allocator->current == allocator->first && allocator->current->pos == allocator->current->start); - frame_data_fragment_free(allocator->first); - - free(allocator->infos); - allocator->first = allocator->current = NULL; - allocator->infos = NULL; - allocator->infos_len = allocator->infos_capacity = 0; -} - -void frame_data_push_info(FrameDataAllocator *allocator, InterpreterFrame *frame) -{ - if (allocator->infos_len == allocator->infos_capacity) { - int new_capacity = allocator->infos_capacity == 0 ? 8 : allocator->infos_capacity * 2; - allocator->infos = (FrameDataInfo*)realloc(allocator->infos, new_capacity * sizeof(FrameDataInfo)); - assert(allocator->infos && "Failed to reallocate frame info"); - allocator->infos_capacity = new_capacity; - } - - FrameDataInfo *info = &allocator->infos[allocator->infos_len++]; - info->frame = frame; - info->frag = allocator->current; - info->pos = allocator->current->pos; -} - -void *frame_data_alloc(FrameDataAllocator *allocator, InterpreterFrame *frame, size_t size) -{ - - if (!allocator->infos_len || (allocator->infos_len > 0 && allocator->infos[allocator->infos_len - 1].frame != frame)) - { - frame_data_push_info(allocator, frame); - } - - uint8_t *pos = allocator->current->pos; - - if (pos + size > allocator->current->end) { - if (allocator->current->next && ((allocator->current->next->start + size) <= allocator->current->next->end)) - { - allocator->current = allocator->current->next; - pos = allocator->current->pos = allocator->current->start; - } - else - { - frame_data_fragment_free(allocator->current->next); - FrameDataFragment *new_frag = frame_data_new_fragment(size); - assert(new_frag && "Failed to allocate new fragment"); - allocator->current->next = new_frag; - allocator->current = new_frag; - - pos = new_frag->pos; - } - } - - void *result = (void*)pos; - allocator->current->pos = (uint8_t *)(pos + size); - return result; -} - -void frame_data_pop_info(FrameDataAllocator *allocator, InterpreterFrame *pFrame) -{ - int top = allocator->infos_len - 1; - if (top >= 0 && allocator->infos[top].frame == pFrame) - { - FrameDataInfo *info = &allocator->infos[--allocator->infos_len]; - allocator->current = info->frag; - allocator->current->pos = info->pos; - } -} - InterpThreadContext* InterpGetThreadContext() { InterpThreadContext *threadContext = t_pThreadContext; @@ -125,7 +21,7 @@ InterpThreadContext* InterpGetThreadContext() // FIXME VirtualAlloc/mmap with INTERP_STACK_ALIGNMENT alignment threadContext->pStackStart = threadContext->pStackPointer = (int8_t*)malloc(INTERP_STACK_SIZE); threadContext->pStackEnd = threadContext->pStackStart + INTERP_STACK_SIZE; - frame_data_allocator_init(&threadContext->frameDataAllocator, INTERP_STACK_FRAGMENT_SIZE); + threadContext->pFrameDataAllocator = new FrameDataAllocator(INTERP_STACK_FRAGMENT_SIZE); t_pThreadContext = threadContext; return threadContext; @@ -1264,21 +1160,10 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr if (len > 0) { len = ALIGN_UP(len, INTERP_STACK_ALIGNMENT); - mem = frame_data_alloc(&pThreadContext->frameDataAllocator, (InterpreterFrame*)pFrame, len); - if (mem != NULL) + mem = pThreadContext->pFrameDataAllocator->Alloc((InterpreterFrame*)pFrame, len); + if (mem != NULL && pMethod->initLocals) { - MethodDesc* md = reinterpret_cast(pMethod->methodHnd); - COR_ILMETHOD* pIL = md->GetILHeader(); - - if (pIL != NULL) - { - COR_ILMETHOD_DECODER header(pIL); - // Initialize local variables to zero if the flag is set - if (header.GetFlags() & CorILMethod_InitLocals) - { - memset(mem, 0, len); - } - } + memset(mem, 0, len); } } else { @@ -1299,7 +1184,8 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr EXIT_FRAME: - frame_data_pop_info(&pThreadContext->frameDataAllocator, (InterpreterFrame*)pFrame); + // Interpreter-TODO: Don't run PopInfo on the main return path, Add RET_LOCALLOC instead + pThreadContext->pFrameDataAllocator->PopInfo((InterpreterFrame*)pFrame); if (pFrame->pParent && pFrame->pParent->ip) { // Return to the main loop after a non-recursive interpreter call @@ -1314,7 +1200,8 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr goto MAIN_LOOP; } - frame_data_allocator_destroy(&pThreadContext->frameDataAllocator); + // Interpreter-TODO: FrameDataAllocator is owned by the current thread, free it when the thread exits instead of here + delete pThreadContext->pFrameDataAllocator; pThreadContext->pStackPointer = pFrame->pStack; } diff --git a/src/coreclr/vm/interpexec.h b/src/coreclr/vm/interpexec.h index e984f02929a61a..654116d448c3c3 100644 --- a/src/coreclr/vm/interpexec.h +++ b/src/coreclr/vm/interpexec.h @@ -5,6 +5,7 @@ #define _INTERPEXEC_H_ #include "../interpreter/interpretershared.h" +#include "interpframeallocator.h" #define INTERP_STACK_SIZE 1024*1024 #define INTERP_STACK_FRAGMENT_SIZE 4096 @@ -43,33 +44,6 @@ struct InterpMethodContextFrame #endif // DACCESS_COMPILE }; -struct FrameDataFragment { - // Memory region for this fragment - uint8_t *start, *end; - // Current allocation pointer within this fragment - uint8_t *pos; - // Pointer to the next fragment - FrameDataFragment *next; -}; - -struct FrameDataInfo { - // Pointer to the frame that this info is associated with - InterpreterFrame *frame; - // Pointers for restoring the localloc memory: - // frag - the current allocation fragment at frame entry - // pos - the fragment pointer at frame entry - // When the frame returns, we use these to roll back any local allocations - FrameDataFragment *frag; - uint8_t *pos; -}; - -struct FrameDataAllocator { - FrameDataFragment *first, *current; - FrameDataInfo *infos; - int infos_len; - int infos_capacity; -}; - struct InterpThreadContext { int8_t *pStackStart; @@ -82,7 +56,7 @@ struct InterpThreadContext int8_t *pStackPointer; // This is an allocator for the dynamic stack memory - FrameDataAllocator frameDataAllocator; + FrameDataAllocator *pFrameDataAllocator; }; InterpThreadContext* InterpGetThreadContext(); diff --git a/src/coreclr/vm/interpframeallocator.cpp b/src/coreclr/vm/interpframeallocator.cpp new file mode 100644 index 00000000000000..bf554bb01271d6 --- /dev/null +++ b/src/coreclr/vm/interpframeallocator.cpp @@ -0,0 +1,108 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +#include "interpexec.h" +#include "interpframeallocator.h" + +FrameDataFragment::FrameDataFragment(size_t size) +{ + if (size < INTERP_STACK_FRAGMENT_SIZE) + { + size = INTERP_STACK_FRAGMENT_SIZE; + } + + void *pMemory = malloc(sizeof(FrameDataFragment) + size); + assert(pMemory && "Failed to allocate FrameDataFragment"); + + start = (uint8_t*)pMemory + sizeof(FrameDataFragment); + end = start + size; + pos = start; + pNext = nullptr; +} + +FrameDataAllocator::FrameDataAllocator(size_t size) +{ + pFirst = new FrameDataFragment(size); + assert(pFirst && "Failed to allocate initial fragment"); + pCurrent = pFirst; + pInfos = nullptr; + infosLen = 0; + infosCapacity = 0; +} + +FrameDataAllocator::~FrameDataAllocator() +{ + assert(pCurrent == pFirst && pCurrent->pos == pCurrent->start); + FreeFragments(pFirst); + free(pInfos); +} + +void FrameDataAllocator::FreeFragments(FrameDataFragment *pFrag) +{ + while (pFrag) + { + FrameDataFragment *pNext = pFrag->pNext; + delete pFrag; + pFrag = pNext; + } +} + +void FrameDataAllocator::PushInfo(InterpreterFrame *pFrame) +{ + if (infosLen == infosCapacity) + { + size_t newCapacity = infosCapacity == 0 ? 8 : infosCapacity * 2; + pInfos = (FrameDataInfo*)realloc(pInfos, newCapacity * sizeof(FrameDataInfo)); + assert(pInfos && "Failed to reallocate frame info"); + infosCapacity = newCapacity; + } + + FrameDataInfo *pInfo = &pInfos[infosLen++]; + pInfo->frame = pFrame; + pInfo->pFrag = pCurrent; + pInfo->pos = pCurrent->pos; +} + +void *FrameDataAllocator::Alloc(InterpreterFrame *pFrame, size_t size) +{ + if (!infosLen || (infosLen > 0 && pInfos[infosLen - 1].frame != pFrame)) + { + PushInfo(pFrame); + } + + uint8_t *pos = pCurrent->pos; + + if (pos + size > pCurrent->end) + { + if (pCurrent->pNext && ((pCurrent->pNext->start + size) <= pCurrent->pNext->end)) + { + pCurrent = pCurrent->pNext; + pos = pCurrent->pos = pCurrent->start; + } + else + { + FreeFragments(pCurrent->pNext); + FrameDataFragment *pNewFrag = new FrameDataFragment(size); + assert(pNewFrag && "Failed to allocate new fragment"); + pCurrent->pNext = pNewFrag; + pCurrent = pNewFrag; + + pos = pNewFrag->pos; + } + } + + void *result = (void*)pos; + pCurrent->pos = (uint8_t*)(pos + size); + return result; +} + +void FrameDataAllocator::PopInfo(InterpreterFrame *pFrame) +{ + int top = infosLen - 1; + if (top >= 0 && pInfos[top].frame == pFrame) + { + FrameDataInfo *pInfo = &pInfos[--infosLen]; + pCurrent = pInfo->pFrag; + pCurrent->pos = pInfo->pos; + } +} diff --git a/src/coreclr/vm/interpframeallocator.h b/src/coreclr/vm/interpframeallocator.h new file mode 100644 index 00000000000000..0f2945f3545ec3 --- /dev/null +++ b/src/coreclr/vm/interpframeallocator.h @@ -0,0 +1,52 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +#ifndef _INTERPFRAMEALLOCATOR_H_ +#define _INTERPFRAMEALLOCATOR_H_ + +struct FrameDataFragment +{ + // The start of the fragment + uint8_t *start; + // The end of the fragment + uint8_t *end; + // The current position in the fragment + uint8_t *pos; + // The next fragment in the list + FrameDataFragment *pNext; + + FrameDataFragment(size_t size); +}; + +struct FrameDataInfo +{ + // The frame that this data belongs to + InterpreterFrame *frame; + // Pointers for restoring the localloc memory: + // frag - the current allocation fragment at frame entry + // pos - the fragment pointer at frame entry + // When the frame returns, we use these to roll back any local allocations + FrameDataFragment *pFrag; + uint8_t *pos; + + FrameDataInfo(InterpreterFrame *pFrame, FrameDataFragment *pFrag, uint8_t *pos); +}; + +struct FrameDataAllocator +{ + FrameDataFragment *pFirst; + FrameDataFragment *pCurrent; + FrameDataInfo *pInfos; + size_t infosLen; + size_t infosCapacity; + + FrameDataAllocator(size_t size); + ~FrameDataAllocator(); + + void *Alloc(InterpreterFrame *pFrame, size_t size); + void FreeFragments(FrameDataFragment *pFrag); + void PushInfo(InterpreterFrame *pFrame); + void PopInfo(InterpreterFrame *pFrame); +}; + +#endif diff --git a/src/tests/JIT/interpreter/Interpreter.cs b/src/tests/JIT/interpreter/Interpreter.cs index a9f05cf9066fae..4d6cba0b59c2df 100644 --- a/src/tests/JIT/interpreter/Interpreter.cs +++ b/src/tests/JIT/interpreter/Interpreter.cs @@ -280,7 +280,8 @@ public static bool TestLocalloc() if (3072 != LocallocNestedTests(1024, 512, 512, 1024)) return false; // SkipLocalsInit tests - if (!LocallocSkipLocalsInit(32)) return false; + // Test could be flaky if the allocated memory is already zeroed + // if (!LocallocSkipLocalsInit(32)) return false; return true; } From 1d1e226663660709efb5def5f0a24971cf39ca72 Mon Sep 17 00:00:00 2001 From: Milos Kotlar Date: Tue, 22 Apr 2025 18:10:28 +0200 Subject: [PATCH 07/26] Implement FrameDataFragment destructor --- src/coreclr/vm/interpframeallocator.cpp | 17 ++++++++++------- src/coreclr/vm/interpframeallocator.h | 5 +++-- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/src/coreclr/vm/interpframeallocator.cpp b/src/coreclr/vm/interpframeallocator.cpp index bf554bb01271d6..5fd3d459a9f84a 100644 --- a/src/coreclr/vm/interpframeallocator.cpp +++ b/src/coreclr/vm/interpframeallocator.cpp @@ -11,15 +11,18 @@ FrameDataFragment::FrameDataFragment(size_t size) size = INTERP_STACK_FRAGMENT_SIZE; } - void *pMemory = malloc(sizeof(FrameDataFragment) + size); - assert(pMemory && "Failed to allocate FrameDataFragment"); - - start = (uint8_t*)pMemory + sizeof(FrameDataFragment); + start = (uint8_t*)malloc(size); + assert(start && "Failed to allocate FrameDataFragment"); end = start + size; pos = start; pNext = nullptr; } +FrameDataFragment::~FrameDataFragment() +{ + free(start); +} + FrameDataAllocator::FrameDataAllocator(size_t size) { pFirst = new FrameDataFragment(size); @@ -58,14 +61,14 @@ void FrameDataAllocator::PushInfo(InterpreterFrame *pFrame) } FrameDataInfo *pInfo = &pInfos[infosLen++]; - pInfo->frame = pFrame; + pInfo->pFrame = pFrame; pInfo->pFrag = pCurrent; pInfo->pos = pCurrent->pos; } void *FrameDataAllocator::Alloc(InterpreterFrame *pFrame, size_t size) { - if (!infosLen || (infosLen > 0 && pInfos[infosLen - 1].frame != pFrame)) + if (!infosLen || (infosLen > 0 && pInfos[infosLen - 1].pFrame != pFrame)) { PushInfo(pFrame); } @@ -99,7 +102,7 @@ void *FrameDataAllocator::Alloc(InterpreterFrame *pFrame, size_t size) void FrameDataAllocator::PopInfo(InterpreterFrame *pFrame) { int top = infosLen - 1; - if (top >= 0 && pInfos[top].frame == pFrame) + if (top >= 0 && pInfos[top].pFrame == pFrame) { FrameDataInfo *pInfo = &pInfos[--infosLen]; pCurrent = pInfo->pFrag; diff --git a/src/coreclr/vm/interpframeallocator.h b/src/coreclr/vm/interpframeallocator.h index 0f2945f3545ec3..99d0fc28f8a216 100644 --- a/src/coreclr/vm/interpframeallocator.h +++ b/src/coreclr/vm/interpframeallocator.h @@ -16,14 +16,15 @@ struct FrameDataFragment FrameDataFragment *pNext; FrameDataFragment(size_t size); + ~FrameDataFragment(); }; struct FrameDataInfo { // The frame that this data belongs to - InterpreterFrame *frame; + InterpreterFrame *pFrame; // Pointers for restoring the localloc memory: - // frag - the current allocation fragment at frame entry + // pFrag - the current allocation fragment at frame entry // pos - the fragment pointer at frame entry // When the frame returns, we use these to roll back any local allocations FrameDataFragment *pFrag; From cb0b21223d2964bf19731377cb82eeff28c1da85 Mon Sep 17 00:00:00 2001 From: Milos Kotlar Date: Tue, 22 Apr 2025 18:56:44 +0200 Subject: [PATCH 08/26] Handle memory allocation failure in frame data allocation --- src/coreclr/vm/interpexec.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/coreclr/vm/interpexec.cpp b/src/coreclr/vm/interpexec.cpp index e6d234717c01c5..fd1bce97591c5c 100644 --- a/src/coreclr/vm/interpexec.cpp +++ b/src/coreclr/vm/interpexec.cpp @@ -1161,7 +1161,13 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr { len = ALIGN_UP(len, INTERP_STACK_ALIGNMENT); mem = pThreadContext->pFrameDataAllocator->Alloc((InterpreterFrame*)pFrame, len); - if (mem != NULL && pMethod->initLocals) + if (!mem) + { + // Interpreter-TODO: OutOfMemoryException + assert(0); + } + + if (pMethod->initLocals) { memset(mem, 0, len); } From 581e8562556f9ec06b8eb81fca154db7caa0673b Mon Sep 17 00:00:00 2001 From: Milos Kotlar Date: Tue, 22 Apr 2025 19:22:02 +0200 Subject: [PATCH 09/26] Fix typo --- src/coreclr/vm/CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/CMakeLists.txt b/src/coreclr/vm/CMakeLists.txt index 1b904463de7cfc..d16c32d384be44 100644 --- a/src/coreclr/vm/CMakeLists.txt +++ b/src/coreclr/vm/CMakeLists.txt @@ -445,7 +445,7 @@ set(VM_HEADERS_WKS interoputil.h interoputil.inl interpexec.h - interpframallocator.h + interpframeallocator.h invokeutil.h managedmdimport.hpp marshalnative.h From e72e0e3c6ea42d83e54d95c7a926a267a29636bb Mon Sep 17 00:00:00 2001 From: Milos Kotlar Date: Tue, 22 Apr 2025 19:54:27 +0200 Subject: [PATCH 10/26] Add preprocessor directive FEATURE_INTERPRETER --- src/coreclr/vm/interpframeallocator.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/coreclr/vm/interpframeallocator.cpp b/src/coreclr/vm/interpframeallocator.cpp index 5fd3d459a9f84a..07f846a74e33da 100644 --- a/src/coreclr/vm/interpframeallocator.cpp +++ b/src/coreclr/vm/interpframeallocator.cpp @@ -1,6 +1,8 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +#ifdef FEATURE_INTERPRETER + #include "interpexec.h" #include "interpframeallocator.h" @@ -109,3 +111,5 @@ void FrameDataAllocator::PopInfo(InterpreterFrame *pFrame) pCurrent->pos = pInfo->pos; } } + +#endif // FEATURE_INTERPRETER From 50e94f64b6504a70f736f155de602d05e5e26437 Mon Sep 17 00:00:00 2001 From: Milos Kotlar Date: Tue, 22 Apr 2025 20:37:39 +0200 Subject: [PATCH 11/26] Fix windows build --- src/coreclr/vm/interpframeallocator.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/interpframeallocator.cpp b/src/coreclr/vm/interpframeallocator.cpp index 07f846a74e33da..9e32d674e20060 100644 --- a/src/coreclr/vm/interpframeallocator.cpp +++ b/src/coreclr/vm/interpframeallocator.cpp @@ -103,7 +103,7 @@ void *FrameDataAllocator::Alloc(InterpreterFrame *pFrame, size_t size) void FrameDataAllocator::PopInfo(InterpreterFrame *pFrame) { - int top = infosLen - 1; + size_t top = infosLen - 1; if (top >= 0 && pInfos[top].pFrame == pFrame) { FrameDataInfo *pInfo = &pInfos[--infosLen]; From dfb170b87b601c8545737e83818d1acc29ce9471 Mon Sep 17 00:00:00 2001 From: Milos Kotlar Date: Tue, 22 Apr 2025 21:43:16 +0200 Subject: [PATCH 12/26] If size is 0 allocates a zero-length item and returns a valid pointer to that item --- src/coreclr/vm/interpexec.cpp | 22 +++++----------------- src/coreclr/vm/interpframeallocator.cpp | 2 -- src/tests/JIT/interpreter/Interpreter.cs | 12 ------------ 3 files changed, 5 insertions(+), 31 deletions(-) diff --git a/src/coreclr/vm/interpexec.cpp b/src/coreclr/vm/interpexec.cpp index fd1bce97591c5c..fb27ac92d55075 100644 --- a/src/coreclr/vm/interpexec.cpp +++ b/src/coreclr/vm/interpexec.cpp @@ -1155,26 +1155,14 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr case INTOP_LOCALLOC: { int32_t len = LOCAL_VAR(ip[2], int32_t); - void* mem; + len = ALIGN_UP(len, INTERP_STACK_ALIGNMENT); - if (len > 0) + void* mem = pThreadContext->pFrameDataAllocator->Alloc((InterpreterFrame*)pFrame, len); + if (pMethod->initLocals) { - len = ALIGN_UP(len, INTERP_STACK_ALIGNMENT); - mem = pThreadContext->pFrameDataAllocator->Alloc((InterpreterFrame*)pFrame, len); - if (!mem) - { - // Interpreter-TODO: OutOfMemoryException - assert(0); - } - - if (pMethod->initLocals) - { - memset(mem, 0, len); - } - } else - { - mem = NULL; + memset(mem, 0, len); } + LOCAL_VAR(ip[1], void*) = mem; ip += 3; break; diff --git a/src/coreclr/vm/interpframeallocator.cpp b/src/coreclr/vm/interpframeallocator.cpp index 9e32d674e20060..cea22c4429469e 100644 --- a/src/coreclr/vm/interpframeallocator.cpp +++ b/src/coreclr/vm/interpframeallocator.cpp @@ -28,7 +28,6 @@ FrameDataFragment::~FrameDataFragment() FrameDataAllocator::FrameDataAllocator(size_t size) { pFirst = new FrameDataFragment(size); - assert(pFirst && "Failed to allocate initial fragment"); pCurrent = pFirst; pInfos = nullptr; infosLen = 0; @@ -88,7 +87,6 @@ void *FrameDataAllocator::Alloc(InterpreterFrame *pFrame, size_t size) { FreeFragments(pCurrent->pNext); FrameDataFragment *pNewFrag = new FrameDataFragment(size); - assert(pNewFrag && "Failed to allocate new fragment"); pCurrent->pNext = pNewFrag; pCurrent = pNewFrag; diff --git a/src/tests/JIT/interpreter/Interpreter.cs b/src/tests/JIT/interpreter/Interpreter.cs index 4d6cba0b59c2df..3f681ba7c4a871 100644 --- a/src/tests/JIT/interpreter/Interpreter.cs +++ b/src/tests/JIT/interpreter/Interpreter.cs @@ -279,10 +279,6 @@ public static bool TestLocalloc() // Reuse fragment tests if (3072 != LocallocNestedTests(1024, 512, 512, 1024)) return false; - // SkipLocalsInit tests - // Test could be flaky if the allocated memory is already zeroed - // if (!LocallocSkipLocalsInit(32)) return false; - return true; } @@ -326,14 +322,6 @@ public static unsafe int LocallocNestedTests(int n, int m, int p, int k) return a1[0] + a1[1] + a1[n - 1] + inner + a2[0] + a2[1] + a2[k - 1]; } - [SkipLocalsInit] - public static unsafe bool LocallocSkipLocalsInit(int n) - { - int* a = stackalloc int[n]; - for (int i = 0; i < n; i++) if (a[i] != 0) return true; - return false; - } - public static bool TestVirtual() { BaseClass bc = new DerivedClass(); From c3bc007eb0618866ee8e597c28d14d251355d84f Mon Sep 17 00:00:00 2001 From: Milos Kotlar Date: Wed, 23 Apr 2025 10:20:25 +0200 Subject: [PATCH 13/26] Update frame allocator to use InterpMethodContextFrame --- src/coreclr/interpreter/interpretershared.h | 2 +- src/coreclr/interpreter/intops.def | 4 ++-- src/coreclr/vm/interpexec.cpp | 4 ++-- src/coreclr/vm/interpframeallocator.cpp | 6 +++--- src/coreclr/vm/interpframeallocator.h | 10 +++++----- 5 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/coreclr/interpreter/interpretershared.h b/src/coreclr/interpreter/interpretershared.h index 53af35196a597a..e778d541b0b5d6 100644 --- a/src/coreclr/interpreter/interpretershared.h +++ b/src/coreclr/interpreter/interpretershared.h @@ -18,7 +18,7 @@ struct InterpMethod CORINFO_METHOD_HANDLE methodHnd; int32_t allocaSize; void** pDataItems; - uint32_t initLocals : 1; + bool initLocals; InterpMethod(CORINFO_METHOD_HANDLE methodHnd, int32_t allocaSize, void** pDataItems, bool initLocals) { diff --git a/src/coreclr/interpreter/intops.def b/src/coreclr/interpreter/intops.def index 666f35dcbfc47c..23c09d68117970 100644 --- a/src/coreclr/interpreter/intops.def +++ b/src/coreclr/interpreter/intops.def @@ -160,10 +160,10 @@ OPDEF(INTOP_MUL_I8, "mul.i8", 4, 1, 2, InterpOpNoArgs) OPDEF(INTOP_MUL_R4, "mul.r4", 4, 1, 2, InterpOpNoArgs) OPDEF(INTOP_MUL_R8, "mul.r8", 4, 1, 2, InterpOpNoArgs) -OPDEF(INTOP_MUL_OVF_I4, "mul.ovf.i8", 4, 1, 2, InterpOpNoArgs) +OPDEF(INTOP_MUL_OVF_I4, "mul.ovf.i4", 4, 1, 2, InterpOpNoArgs) OPDEF(INTOP_MUL_OVF_I8, "mul.ovf.i8", 4, 1, 2, InterpOpNoArgs) -OPDEF(INTOP_MUL_OVF_UN_I4, "mul.ovf.un.i8", 4, 1, 2, InterpOpNoArgs) +OPDEF(INTOP_MUL_OVF_UN_I4, "mul.ovf.un.i4", 4, 1, 2, InterpOpNoArgs) OPDEF(INTOP_MUL_OVF_UN_I8, "mul.ovf.un.i8", 4, 1, 2, InterpOpNoArgs) OPDEF(INTOP_DIV_I4, "div.i4", 4, 1, 2, InterpOpNoArgs) diff --git a/src/coreclr/vm/interpexec.cpp b/src/coreclr/vm/interpexec.cpp index fb27ac92d55075..2af2e8cae3a8a1 100644 --- a/src/coreclr/vm/interpexec.cpp +++ b/src/coreclr/vm/interpexec.cpp @@ -1157,7 +1157,7 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr int32_t len = LOCAL_VAR(ip[2], int32_t); len = ALIGN_UP(len, INTERP_STACK_ALIGNMENT); - void* mem = pThreadContext->pFrameDataAllocator->Alloc((InterpreterFrame*)pFrame, len); + void* mem = pThreadContext->pFrameDataAllocator->Alloc(pFrame, len); if (pMethod->initLocals) { memset(mem, 0, len); @@ -1179,7 +1179,7 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr EXIT_FRAME: // Interpreter-TODO: Don't run PopInfo on the main return path, Add RET_LOCALLOC instead - pThreadContext->pFrameDataAllocator->PopInfo((InterpreterFrame*)pFrame); + pThreadContext->pFrameDataAllocator->PopInfo(pFrame); if (pFrame->pParent && pFrame->pParent->ip) { // Return to the main loop after a non-recursive interpreter call diff --git a/src/coreclr/vm/interpframeallocator.cpp b/src/coreclr/vm/interpframeallocator.cpp index cea22c4429469e..e0be45afa3fc5a 100644 --- a/src/coreclr/vm/interpframeallocator.cpp +++ b/src/coreclr/vm/interpframeallocator.cpp @@ -51,7 +51,7 @@ void FrameDataAllocator::FreeFragments(FrameDataFragment *pFrag) } } -void FrameDataAllocator::PushInfo(InterpreterFrame *pFrame) +void FrameDataAllocator::PushInfo(InterpMethodContextFrame *pFrame) { if (infosLen == infosCapacity) { @@ -67,7 +67,7 @@ void FrameDataAllocator::PushInfo(InterpreterFrame *pFrame) pInfo->pos = pCurrent->pos; } -void *FrameDataAllocator::Alloc(InterpreterFrame *pFrame, size_t size) +void *FrameDataAllocator::Alloc(InterpMethodContextFrame *pFrame, size_t size) { if (!infosLen || (infosLen > 0 && pInfos[infosLen - 1].pFrame != pFrame)) { @@ -99,7 +99,7 @@ void *FrameDataAllocator::Alloc(InterpreterFrame *pFrame, size_t size) return result; } -void FrameDataAllocator::PopInfo(InterpreterFrame *pFrame) +void FrameDataAllocator::PopInfo(InterpMethodContextFrame *pFrame) { size_t top = infosLen - 1; if (top >= 0 && pInfos[top].pFrame == pFrame) diff --git a/src/coreclr/vm/interpframeallocator.h b/src/coreclr/vm/interpframeallocator.h index 99d0fc28f8a216..075e8f1bf741ab 100644 --- a/src/coreclr/vm/interpframeallocator.h +++ b/src/coreclr/vm/interpframeallocator.h @@ -22,7 +22,7 @@ struct FrameDataFragment struct FrameDataInfo { // The frame that this data belongs to - InterpreterFrame *pFrame; + InterpMethodContextFrame *pFrame; // Pointers for restoring the localloc memory: // pFrag - the current allocation fragment at frame entry // pos - the fragment pointer at frame entry @@ -30,7 +30,7 @@ struct FrameDataInfo FrameDataFragment *pFrag; uint8_t *pos; - FrameDataInfo(InterpreterFrame *pFrame, FrameDataFragment *pFrag, uint8_t *pos); + FrameDataInfo(InterpMethodContextFrame *pFrame, FrameDataFragment *pFrag, uint8_t *pos); }; struct FrameDataAllocator @@ -44,10 +44,10 @@ struct FrameDataAllocator FrameDataAllocator(size_t size); ~FrameDataAllocator(); - void *Alloc(InterpreterFrame *pFrame, size_t size); + void *Alloc(InterpMethodContextFrame *pFrame, size_t size); void FreeFragments(FrameDataFragment *pFrag); - void PushInfo(InterpreterFrame *pFrame); - void PopInfo(InterpreterFrame *pFrame); + void PushInfo(InterpMethodContextFrame *pFrame); + void PopInfo(InterpMethodContextFrame *pFrame); }; #endif From c91b99d0d15b636f5b11346c4adad3c5c69828d1 Mon Sep 17 00:00:00 2001 From: Milos Kotlar Date: Wed, 23 Apr 2025 12:32:52 +0200 Subject: [PATCH 14/26] Throw OutOfMemory exception if alloc fails --- src/coreclr/vm/interpframeallocator.cpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/coreclr/vm/interpframeallocator.cpp b/src/coreclr/vm/interpframeallocator.cpp index e0be45afa3fc5a..47480e93befd23 100644 --- a/src/coreclr/vm/interpframeallocator.cpp +++ b/src/coreclr/vm/interpframeallocator.cpp @@ -14,7 +14,10 @@ FrameDataFragment::FrameDataFragment(size_t size) } start = (uint8_t*)malloc(size); - assert(start && "Failed to allocate FrameDataFragment"); + if (start == nullptr) + { + ThrowOutOfMemory(); + } end = start + size; pos = start; pNext = nullptr; @@ -57,7 +60,10 @@ void FrameDataAllocator::PushInfo(InterpMethodContextFrame *pFrame) { size_t newCapacity = infosCapacity == 0 ? 8 : infosCapacity * 2; pInfos = (FrameDataInfo*)realloc(pInfos, newCapacity * sizeof(FrameDataInfo)); - assert(pInfos && "Failed to reallocate frame info"); + if (pInfos == nullptr) + { + ThrowOutOfMemory(); + } infosCapacity = newCapacity; } From 2bfe64fc0ac7a84922f56d79ffe4b0fafcf1a6dd Mon Sep 17 00:00:00 2001 From: Milos Kotlar Date: Wed, 23 Apr 2025 15:42:39 +0200 Subject: [PATCH 15/26] Revert assert changes --- src/coreclr/vm/interpframeallocator.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/coreclr/vm/interpframeallocator.cpp b/src/coreclr/vm/interpframeallocator.cpp index 47480e93befd23..f9fd0b4abdbc62 100644 --- a/src/coreclr/vm/interpframeallocator.cpp +++ b/src/coreclr/vm/interpframeallocator.cpp @@ -16,7 +16,8 @@ FrameDataFragment::FrameDataFragment(size_t size) start = (uint8_t*)malloc(size); if (start == nullptr) { - ThrowOutOfMemory(); + // Interpreter-TODO: Throw OutOfMemory exception + assert(start && "Failed to allocate FrameDataFragment"); } end = start + size; pos = start; @@ -62,7 +63,8 @@ void FrameDataAllocator::PushInfo(InterpMethodContextFrame *pFrame) pInfos = (FrameDataInfo*)realloc(pInfos, newCapacity * sizeof(FrameDataInfo)); if (pInfos == nullptr) { - ThrowOutOfMemory(); + // Interpreter-TODO: Throw OutOfMemory exception + assert(pInfos && "Failed to allocate FrameDataInfo"); } infosCapacity = newCapacity; } From c08ab74c57e04fee468ebca3a0319db7f0a29bcb Mon Sep 17 00:00:00 2001 From: Milos Kotlar Date: Thu, 24 Apr 2025 11:04:31 +0200 Subject: [PATCH 16/26] Update FrameDataAllocator to return nullptr if alloc fails --- src/coreclr/vm/interpexec.cpp | 10 ++++++++ src/coreclr/vm/interpframeallocator.cpp | 34 +++++++++++++++---------- src/coreclr/vm/interpframeallocator.h | 2 +- 3 files changed, 32 insertions(+), 14 deletions(-) diff --git a/src/coreclr/vm/interpexec.cpp b/src/coreclr/vm/interpexec.cpp index 2af2e8cae3a8a1..8ef06925da1193 100644 --- a/src/coreclr/vm/interpexec.cpp +++ b/src/coreclr/vm/interpexec.cpp @@ -22,6 +22,11 @@ InterpThreadContext* InterpGetThreadContext() threadContext->pStackStart = threadContext->pStackPointer = (int8_t*)malloc(INTERP_STACK_SIZE); threadContext->pStackEnd = threadContext->pStackStart + INTERP_STACK_SIZE; threadContext->pFrameDataAllocator = new FrameDataAllocator(INTERP_STACK_FRAGMENT_SIZE); + if (threadContext->pFrameDataAllocator->pFirst->start == NULL) + { + // Interpreter-TODO: OutOfMemoryException + assert(0); + } t_pThreadContext = threadContext; return threadContext; @@ -1158,6 +1163,11 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr len = ALIGN_UP(len, INTERP_STACK_ALIGNMENT); void* mem = pThreadContext->pFrameDataAllocator->Alloc(pFrame, len); + if (mem == NULL) + { + // Interpreter-TODO: OutOfMemoryException + assert(0); + } if (pMethod->initLocals) { memset(mem, 0, len); diff --git a/src/coreclr/vm/interpframeallocator.cpp b/src/coreclr/vm/interpframeallocator.cpp index f9fd0b4abdbc62..a097a57414b7e9 100644 --- a/src/coreclr/vm/interpframeallocator.cpp +++ b/src/coreclr/vm/interpframeallocator.cpp @@ -14,13 +14,11 @@ FrameDataFragment::FrameDataFragment(size_t size) } start = (uint8_t*)malloc(size); - if (start == nullptr) + if (start != nullptr) { - // Interpreter-TODO: Throw OutOfMemory exception - assert(start && "Failed to allocate FrameDataFragment"); + end = start + size; + pos = start; } - end = start + size; - pos = start; pNext = nullptr; } @@ -55,17 +53,17 @@ void FrameDataAllocator::FreeFragments(FrameDataFragment *pFrag) } } -void FrameDataAllocator::PushInfo(InterpMethodContextFrame *pFrame) +bool FrameDataAllocator::PushInfo(InterpMethodContextFrame *pFrame) { if (infosLen == infosCapacity) { size_t newCapacity = infosCapacity == 0 ? 8 : infosCapacity * 2; - pInfos = (FrameDataInfo*)realloc(pInfos, newCapacity * sizeof(FrameDataInfo)); - if (pInfos == nullptr) + FrameDataInfo* newInfos = (FrameDataInfo*)realloc(pInfos, newCapacity * sizeof(FrameDataInfo)); + if (newInfos == nullptr) { - // Interpreter-TODO: Throw OutOfMemory exception - assert(pInfos && "Failed to allocate FrameDataInfo"); + return false; } + pInfos = newInfos; infosCapacity = newCapacity; } @@ -73,13 +71,17 @@ void FrameDataAllocator::PushInfo(InterpMethodContextFrame *pFrame) pInfo->pFrame = pFrame; pInfo->pFrag = pCurrent; pInfo->pos = pCurrent->pos; + return true; } void *FrameDataAllocator::Alloc(InterpMethodContextFrame *pFrame, size_t size) { - if (!infosLen || (infosLen > 0 && pInfos[infosLen - 1].pFrame != pFrame)) + if (!infosLen || pInfos[infosLen - 1].pFrame != pFrame) { - PushInfo(pFrame); + if (!PushInfo(pFrame)) + { + return nullptr; + } } uint8_t *pos = pCurrent->pos; @@ -94,10 +96,16 @@ void *FrameDataAllocator::Alloc(InterpMethodContextFrame *pFrame, size_t size) else { FreeFragments(pCurrent->pNext); + pCurrent->pNext = nullptr; + FrameDataFragment *pNewFrag = new FrameDataFragment(size); + if (pNewFrag->start == nullptr) + { + return nullptr; + } + pCurrent->pNext = pNewFrag; pCurrent = pNewFrag; - pos = pNewFrag->pos; } } diff --git a/src/coreclr/vm/interpframeallocator.h b/src/coreclr/vm/interpframeallocator.h index 075e8f1bf741ab..204ca7ad0822a4 100644 --- a/src/coreclr/vm/interpframeallocator.h +++ b/src/coreclr/vm/interpframeallocator.h @@ -46,7 +46,7 @@ struct FrameDataAllocator void *Alloc(InterpMethodContextFrame *pFrame, size_t size); void FreeFragments(FrameDataFragment *pFrag); - void PushInfo(InterpMethodContextFrame *pFrame); + bool PushInfo(InterpMethodContextFrame *pFrame); void PopInfo(InterpMethodContextFrame *pFrame); }; From 7f4217616899d1837bb164e130518f65eaf50939 Mon Sep 17 00:00:00 2001 From: Milos Kotlar Date: Thu, 24 Apr 2025 11:28:15 +0200 Subject: [PATCH 17/26] Check if infosLen is >0 --- src/coreclr/vm/interpframeallocator.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/coreclr/vm/interpframeallocator.cpp b/src/coreclr/vm/interpframeallocator.cpp index a097a57414b7e9..a257de9e95859b 100644 --- a/src/coreclr/vm/interpframeallocator.cpp +++ b/src/coreclr/vm/interpframeallocator.cpp @@ -117,8 +117,7 @@ void *FrameDataAllocator::Alloc(InterpMethodContextFrame *pFrame, size_t size) void FrameDataAllocator::PopInfo(InterpMethodContextFrame *pFrame) { - size_t top = infosLen - 1; - if (top >= 0 && pInfos[top].pFrame == pFrame) + if (infosLen > 0 && pInfos[infosLen - 1].pFrame == pFrame) { FrameDataInfo *pInfo = &pInfos[--infosLen]; pCurrent = pInfo->pFrag; From df20036ccca6d3cd05cf0ace30fcb3544a4d3b7f Mon Sep 17 00:00:00 2001 From: Milos Kotlar Date: Thu, 24 Apr 2025 11:34:06 +0200 Subject: [PATCH 18/26] Use consistent naming for frame pointers --- src/coreclr/vm/interpexec.cpp | 2 +- src/coreclr/vm/interpframeallocator.cpp | 34 ++++++++++++------------- src/coreclr/vm/interpframeallocator.h | 10 ++++---- 3 files changed, 23 insertions(+), 23 deletions(-) diff --git a/src/coreclr/vm/interpexec.cpp b/src/coreclr/vm/interpexec.cpp index 8ef06925da1193..1dca07bbe129be 100644 --- a/src/coreclr/vm/interpexec.cpp +++ b/src/coreclr/vm/interpexec.cpp @@ -22,7 +22,7 @@ InterpThreadContext* InterpGetThreadContext() threadContext->pStackStart = threadContext->pStackPointer = (int8_t*)malloc(INTERP_STACK_SIZE); threadContext->pStackEnd = threadContext->pStackStart + INTERP_STACK_SIZE; threadContext->pFrameDataAllocator = new FrameDataAllocator(INTERP_STACK_FRAGMENT_SIZE); - if (threadContext->pFrameDataAllocator->pFirst->start == NULL) + if (threadContext->pFrameDataAllocator->pFirst->pFrameStart == NULL) { // Interpreter-TODO: OutOfMemoryException assert(0); diff --git a/src/coreclr/vm/interpframeallocator.cpp b/src/coreclr/vm/interpframeallocator.cpp index a257de9e95859b..cd77aeb547564c 100644 --- a/src/coreclr/vm/interpframeallocator.cpp +++ b/src/coreclr/vm/interpframeallocator.cpp @@ -13,18 +13,18 @@ FrameDataFragment::FrameDataFragment(size_t size) size = INTERP_STACK_FRAGMENT_SIZE; } - start = (uint8_t*)malloc(size); - if (start != nullptr) + pFrameStart = (uint8_t*)malloc(size); + if (pFrameStart != nullptr) { - end = start + size; - pos = start; + pFrameEnd = pFrameStart + size; + pFramePos = pFrameStart; } pNext = nullptr; } FrameDataFragment::~FrameDataFragment() { - free(start); + free(pFrameStart); } FrameDataAllocator::FrameDataAllocator(size_t size) @@ -38,7 +38,7 @@ FrameDataAllocator::FrameDataAllocator(size_t size) FrameDataAllocator::~FrameDataAllocator() { - assert(pCurrent == pFirst && pCurrent->pos == pCurrent->start); + assert(pCurrent == pFirst && pCurrent->pFramePos == pCurrent->pFrameStart); FreeFragments(pFirst); free(pInfos); } @@ -70,7 +70,7 @@ bool FrameDataAllocator::PushInfo(InterpMethodContextFrame *pFrame) FrameDataInfo *pInfo = &pInfos[infosLen++]; pInfo->pFrame = pFrame; pInfo->pFrag = pCurrent; - pInfo->pos = pCurrent->pos; + pInfo->pFramePos = pCurrent->pFramePos; return true; } @@ -84,14 +84,14 @@ void *FrameDataAllocator::Alloc(InterpMethodContextFrame *pFrame, size_t size) } } - uint8_t *pos = pCurrent->pos; + uint8_t *pFramePos = pCurrent->pFramePos; - if (pos + size > pCurrent->end) + if (pFramePos + size > pCurrent->pFrameEnd) { - if (pCurrent->pNext && ((pCurrent->pNext->start + size) <= pCurrent->pNext->end)) + if (pCurrent->pNext && ((pCurrent->pNext->pFrameStart + size) <= pCurrent->pNext->pFrameEnd)) { pCurrent = pCurrent->pNext; - pos = pCurrent->pos = pCurrent->start; + pFramePos = pCurrent->pFramePos = pCurrent->pFrameStart; } else { @@ -99,20 +99,20 @@ void *FrameDataAllocator::Alloc(InterpMethodContextFrame *pFrame, size_t size) pCurrent->pNext = nullptr; FrameDataFragment *pNewFrag = new FrameDataFragment(size); - if (pNewFrag->start == nullptr) + if (pNewFrag->pFrameStart == nullptr) { return nullptr; } pCurrent->pNext = pNewFrag; pCurrent = pNewFrag; - pos = pNewFrag->pos; + pFramePos = pNewFrag->pFramePos; } } - void *result = (void*)pos; - pCurrent->pos = (uint8_t*)(pos + size); - return result; + void *pMemory = (void*)pFramePos; + pCurrent->pFramePos = (uint8_t*)(pFramePos + size); + return pMemory; } void FrameDataAllocator::PopInfo(InterpMethodContextFrame *pFrame) @@ -121,7 +121,7 @@ void FrameDataAllocator::PopInfo(InterpMethodContextFrame *pFrame) { FrameDataInfo *pInfo = &pInfos[--infosLen]; pCurrent = pInfo->pFrag; - pCurrent->pos = pInfo->pos; + pCurrent->pFramePos = pInfo->pFramePos; } } diff --git a/src/coreclr/vm/interpframeallocator.h b/src/coreclr/vm/interpframeallocator.h index 204ca7ad0822a4..cdc3f773a576ee 100644 --- a/src/coreclr/vm/interpframeallocator.h +++ b/src/coreclr/vm/interpframeallocator.h @@ -7,11 +7,11 @@ struct FrameDataFragment { // The start of the fragment - uint8_t *start; + uint8_t *pFrameStart; // The end of the fragment - uint8_t *end; + uint8_t *pFrameEnd; // The current position in the fragment - uint8_t *pos; + uint8_t *pFramePos; // The next fragment in the list FrameDataFragment *pNext; @@ -28,9 +28,9 @@ struct FrameDataInfo // pos - the fragment pointer at frame entry // When the frame returns, we use these to roll back any local allocations FrameDataFragment *pFrag; - uint8_t *pos; + uint8_t *pFramePos; - FrameDataInfo(InterpMethodContextFrame *pFrame, FrameDataFragment *pFrag, uint8_t *pos); + FrameDataInfo(InterpMethodContextFrame *pFrame, FrameDataFragment *pFrag, uint8_t *pFramePos); }; struct FrameDataAllocator From 8dc322322868ca612fd775da032817a9b1ede1ce Mon Sep 17 00:00:00 2001 From: Milos Kotlar Date: Thu, 24 Apr 2025 15:15:14 +0200 Subject: [PATCH 19/26] Test __SIZEOF_POINTER__ for pointer size checks --- src/coreclr/interpreter/compiler.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/interpreter/compiler.cpp b/src/coreclr/interpreter/compiler.cpp index 97a95ac8714817..4e5ac127b99778 100644 --- a/src/coreclr/interpreter/compiler.cpp +++ b/src/coreclr/interpreter/compiler.cpp @@ -1364,7 +1364,7 @@ void InterpCompiler::EmitBinaryArithmeticOp(int32_t opBase) } else { -#if SIZEOF_VOID_P == 8 +#if __SIZEOF_POINTER__ == 8 if (type1 == StackTypeI8 && type2 == StackTypeI4) { EmitConv(m_pStackPointer - 1, NULL, StackTypeI8, INTOP_CONV_I8_I4); @@ -3146,7 +3146,7 @@ int InterpCompiler::GenerateCode(CORINFO_METHOD_INFO* methodInfo) } case CEE_LOCALLOC: CHECK_STACK(1); -#if SIZEOF_VOID_P == 8 +#if __SIZEOF_POINTER__ == 8 if (m_pStackPointer[-1].type == StackTypeI8) EmitConv(m_pStackPointer - 1, NULL, StackTypeI4, INTOP_MOV_8); #endif From cd3d5b7a578a0ec34bf67fe3e1ba63708abd527b Mon Sep 17 00:00:00 2001 From: Milos Kotlar Date: Thu, 24 Apr 2025 16:27:57 +0200 Subject: [PATCH 20/26] Replace __SIZEOF_POINTER__ with TARGET_64BIT --- src/coreclr/interpreter/compiler.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/interpreter/compiler.cpp b/src/coreclr/interpreter/compiler.cpp index 4e5ac127b99778..15c3793e46d150 100644 --- a/src/coreclr/interpreter/compiler.cpp +++ b/src/coreclr/interpreter/compiler.cpp @@ -1364,7 +1364,7 @@ void InterpCompiler::EmitBinaryArithmeticOp(int32_t opBase) } else { -#if __SIZEOF_POINTER__ == 8 +#if TARGET_64BIT if (type1 == StackTypeI8 && type2 == StackTypeI4) { EmitConv(m_pStackPointer - 1, NULL, StackTypeI8, INTOP_CONV_I8_I4); @@ -3146,7 +3146,7 @@ int InterpCompiler::GenerateCode(CORINFO_METHOD_INFO* methodInfo) } case CEE_LOCALLOC: CHECK_STACK(1); -#if __SIZEOF_POINTER__ == 8 +#if TARGET_64BIT if (m_pStackPointer[-1].type == StackTypeI8) EmitConv(m_pStackPointer - 1, NULL, StackTypeI4, INTOP_MOV_8); #endif From bbb43458adbc8476f67fab6cef7bd65d9b12159f Mon Sep 17 00:00:00 2001 From: Milos Kotlar Date: Thu, 24 Apr 2025 21:26:06 +0200 Subject: [PATCH 21/26] Encapsulate FrameDataAllocator structs into class and make them private --- src/coreclr/vm/interpexec.cpp | 2 +- src/coreclr/vm/interpframeallocator.cpp | 10 +++- src/coreclr/vm/interpframeallocator.h | 65 +++++++++++++------------ 3 files changed, 43 insertions(+), 34 deletions(-) diff --git a/src/coreclr/vm/interpexec.cpp b/src/coreclr/vm/interpexec.cpp index 1dca07bbe129be..d164d13ec8a476 100644 --- a/src/coreclr/vm/interpexec.cpp +++ b/src/coreclr/vm/interpexec.cpp @@ -22,7 +22,7 @@ InterpThreadContext* InterpGetThreadContext() threadContext->pStackStart = threadContext->pStackPointer = (int8_t*)malloc(INTERP_STACK_SIZE); threadContext->pStackEnd = threadContext->pStackStart + INTERP_STACK_SIZE; threadContext->pFrameDataAllocator = new FrameDataAllocator(INTERP_STACK_FRAGMENT_SIZE); - if (threadContext->pFrameDataAllocator->pFirst->pFrameStart == NULL) + if (!threadContext->pFrameDataAllocator->IsAllocated()) { // Interpreter-TODO: OutOfMemoryException assert(0); diff --git a/src/coreclr/vm/interpframeallocator.cpp b/src/coreclr/vm/interpframeallocator.cpp index cd77aeb547564c..a28e83f747f5d1 100644 --- a/src/coreclr/vm/interpframeallocator.cpp +++ b/src/coreclr/vm/interpframeallocator.cpp @@ -6,7 +6,7 @@ #include "interpexec.h" #include "interpframeallocator.h" -FrameDataFragment::FrameDataFragment(size_t size) +FrameDataAllocator::FrameDataFragment::FrameDataFragment(size_t size) { if (size < INTERP_STACK_FRAGMENT_SIZE) { @@ -22,7 +22,7 @@ FrameDataFragment::FrameDataFragment(size_t size) pNext = nullptr; } -FrameDataFragment::~FrameDataFragment() +FrameDataAllocator::FrameDataFragment::~FrameDataFragment() { free(pFrameStart); } @@ -88,6 +88,7 @@ void *FrameDataAllocator::Alloc(InterpMethodContextFrame *pFrame, size_t size) if (pFramePos + size > pCurrent->pFrameEnd) { + // Move to the next fragment or create a new one if necessary if (pCurrent->pNext && ((pCurrent->pNext->pFrameStart + size) <= pCurrent->pNext->pFrameEnd)) { pCurrent = pCurrent->pNext; @@ -125,4 +126,9 @@ void FrameDataAllocator::PopInfo(InterpMethodContextFrame *pFrame) } } +bool FrameDataAllocator::IsAllocated() +{ + return pFirst != nullptr && pFirst->pFrameStart != nullptr; +} + #endif // FEATURE_INTERPRETER diff --git a/src/coreclr/vm/interpframeallocator.h b/src/coreclr/vm/interpframeallocator.h index cdc3f773a576ee..e7ebc39fe87a7c 100644 --- a/src/coreclr/vm/interpframeallocator.h +++ b/src/coreclr/vm/interpframeallocator.h @@ -4,50 +4,53 @@ #ifndef _INTERPFRAMEALLOCATOR_H_ #define _INTERPFRAMEALLOCATOR_H_ -struct FrameDataFragment +class FrameDataAllocator { - // The start of the fragment - uint8_t *pFrameStart; - // The end of the fragment - uint8_t *pFrameEnd; - // The current position in the fragment - uint8_t *pFramePos; - // The next fragment in the list - FrameDataFragment *pNext; - - FrameDataFragment(size_t size); - ~FrameDataFragment(); -}; +private: + struct FrameDataFragment + { + // The start of the fragment + uint8_t *pFrameStart; + // The end of the fragment + uint8_t *pFrameEnd; + // The current position in the fragment + uint8_t *pFramePos; + // The next fragment in the list + FrameDataFragment *pNext; + + FrameDataFragment(size_t size); + ~FrameDataFragment(); + }; + + struct FrameDataInfo + { + // The frame that this data belongs to + InterpMethodContextFrame *pFrame; + // Pointers for restoring the localloc memory: + // pFrag - the current allocation fragment at frame entry + // pos - the fragment pointer at frame entry + // When the frame returns, we use these to roll back any local allocations + FrameDataFragment *pFrag; + uint8_t *pFramePos; + + FrameDataInfo(InterpMethodContextFrame *pFrame, FrameDataFragment *pFrag, uint8_t *pFramePos); + }; -struct FrameDataInfo -{ - // The frame that this data belongs to - InterpMethodContextFrame *pFrame; - // Pointers for restoring the localloc memory: - // pFrag - the current allocation fragment at frame entry - // pos - the fragment pointer at frame entry - // When the frame returns, we use these to roll back any local allocations - FrameDataFragment *pFrag; - uint8_t *pFramePos; - - FrameDataInfo(InterpMethodContextFrame *pFrame, FrameDataFragment *pFrag, uint8_t *pFramePos); -}; - -struct FrameDataAllocator -{ FrameDataFragment *pFirst; FrameDataFragment *pCurrent; FrameDataInfo *pInfos; size_t infosLen; size_t infosCapacity; + bool PushInfo(InterpMethodContextFrame *pFrame); + void FreeFragments(FrameDataFragment *pFrag); +public: FrameDataAllocator(size_t size); ~FrameDataAllocator(); void *Alloc(InterpMethodContextFrame *pFrame, size_t size); - void FreeFragments(FrameDataFragment *pFrag); - bool PushInfo(InterpMethodContextFrame *pFrame); void PopInfo(InterpMethodContextFrame *pFrame); + bool IsAllocated(); }; #endif From 4b8c03c5d29d4d9230072946b59f3fa5b5d18742 Mon Sep 17 00:00:00 2001 From: Milos Kotlar Date: Fri, 25 Apr 2025 10:55:08 +0200 Subject: [PATCH 22/26] Implement destructor for InterpThreadContext to free FrameDataAllocator --- src/coreclr/vm/interpexec.cpp | 2 -- src/coreclr/vm/interpexec.h | 8 ++++++++ src/coreclr/vm/interpframeallocator.cpp | 1 + 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/coreclr/vm/interpexec.cpp b/src/coreclr/vm/interpexec.cpp index d164d13ec8a476..a6c634f5b7ca76 100644 --- a/src/coreclr/vm/interpexec.cpp +++ b/src/coreclr/vm/interpexec.cpp @@ -1204,8 +1204,6 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr goto MAIN_LOOP; } - // Interpreter-TODO: FrameDataAllocator is owned by the current thread, free it when the thread exits instead of here - delete pThreadContext->pFrameDataAllocator; pThreadContext->pStackPointer = pFrame->pStack; } diff --git a/src/coreclr/vm/interpexec.h b/src/coreclr/vm/interpexec.h index 654116d448c3c3..f23bd74d504866 100644 --- a/src/coreclr/vm/interpexec.h +++ b/src/coreclr/vm/interpexec.h @@ -57,6 +57,14 @@ struct InterpThreadContext // This is an allocator for the dynamic stack memory FrameDataAllocator *pFrameDataAllocator; + + ~InterpThreadContext() { + if (pFrameDataAllocator) + { + delete pFrameDataAllocator; + pFrameDataAllocator = NULL; + } + } }; InterpThreadContext* InterpGetThreadContext(); diff --git a/src/coreclr/vm/interpframeallocator.cpp b/src/coreclr/vm/interpframeallocator.cpp index a28e83f747f5d1..696999dd4c9e30 100644 --- a/src/coreclr/vm/interpframeallocator.cpp +++ b/src/coreclr/vm/interpframeallocator.cpp @@ -8,6 +8,7 @@ FrameDataAllocator::FrameDataFragment::FrameDataFragment(size_t size) { + // Amortize allocation cost by allocating a larger chunk of memory if (size < INTERP_STACK_FRAGMENT_SIZE) { size = INTERP_STACK_FRAGMENT_SIZE; From db6355f674e2ef8b8316e3f8c82b423756e0ee42 Mon Sep 17 00:00:00 2001 From: Milos Kotlar Date: Fri, 25 Apr 2025 18:34:18 +0200 Subject: [PATCH 23/26] Move InterpThreadContext instance to the CoreCLR Thread and implement destructor. Inline FrameDataAllocator into InterpThreadContext and ensure allocated memory is released --- src/coreclr/vm/interpexec.cpp | 58 ++++++++++--------------- src/coreclr/vm/interpexec.h | 13 ++---- src/coreclr/vm/interpframeallocator.cpp | 33 ++++++++------ src/coreclr/vm/interpframeallocator.h | 3 +- src/coreclr/vm/prestub.cpp | 7 ++- src/coreclr/vm/threads.cpp | 29 +++++++++++++ src/coreclr/vm/threads.h | 9 ++++ 7 files changed, 91 insertions(+), 61 deletions(-) diff --git a/src/coreclr/vm/interpexec.cpp b/src/coreclr/vm/interpexec.cpp index a6c634f5b7ca76..6dbf461fbe1765 100644 --- a/src/coreclr/vm/interpexec.cpp +++ b/src/coreclr/vm/interpexec.cpp @@ -9,32 +9,16 @@ typedef void* (*HELPER_FTN_PP)(void*); -thread_local InterpThreadContext *t_pThreadContext = NULL; - -InterpThreadContext* InterpGetThreadContext() +InterpThreadContext::InterpThreadContext() { - InterpThreadContext *threadContext = t_pThreadContext; - - if (!threadContext) - { - threadContext = new InterpThreadContext; - // FIXME VirtualAlloc/mmap with INTERP_STACK_ALIGNMENT alignment - threadContext->pStackStart = threadContext->pStackPointer = (int8_t*)malloc(INTERP_STACK_SIZE); - threadContext->pStackEnd = threadContext->pStackStart + INTERP_STACK_SIZE; - threadContext->pFrameDataAllocator = new FrameDataAllocator(INTERP_STACK_FRAGMENT_SIZE); - if (!threadContext->pFrameDataAllocator->IsAllocated()) - { - // Interpreter-TODO: OutOfMemoryException - assert(0); - } + // FIXME VirtualAlloc/mmap with INTERP_STACK_ALIGNMENT alignment + pStackStart = pStackPointer = (int8_t*)malloc(INTERP_STACK_SIZE); + pStackEnd = pStackStart + INTERP_STACK_SIZE; +} - t_pThreadContext = threadContext; - return threadContext; - } - else - { - return threadContext; - } +InterpThreadContext::~InterpThreadContext() +{ + free(pStackStart); } #ifdef DEBUG @@ -1160,20 +1144,24 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr case INTOP_LOCALLOC: { int32_t len = LOCAL_VAR(ip[2], int32_t); - len = ALIGN_UP(len, INTERP_STACK_ALIGNMENT); + len = ALIGN_UP(len, sizeof(void*)); + void* pMemory = NULL; - void* mem = pThreadContext->pFrameDataAllocator->Alloc(pFrame, len); - if (mem == NULL) - { - // Interpreter-TODO: OutOfMemoryException - assert(0); - } - if (pMethod->initLocals) + if (len > 0) { - memset(mem, 0, len); + pMemory = pThreadContext->frameDataAllocator.Alloc(pFrame, len); + if (pMemory == NULL) + { + // Interpreter-TODO: OutOfMemoryException + assert(0); + } + if (pMethod->initLocals) + { + memset(pMemory, 0, len); + } } - LOCAL_VAR(ip[1], void*) = mem; + LOCAL_VAR(ip[1], void*) = pMemory; ip += 3; break; } @@ -1189,7 +1177,7 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr EXIT_FRAME: // Interpreter-TODO: Don't run PopInfo on the main return path, Add RET_LOCALLOC instead - pThreadContext->pFrameDataAllocator->PopInfo(pFrame); + pThreadContext->frameDataAllocator.PopInfo(pFrame); if (pFrame->pParent && pFrame->pParent->ip) { // Return to the main loop after a non-recursive interpreter call diff --git a/src/coreclr/vm/interpexec.h b/src/coreclr/vm/interpexec.h index f23bd74d504866..7306f9796c137c 100644 --- a/src/coreclr/vm/interpexec.h +++ b/src/coreclr/vm/interpexec.h @@ -55,19 +55,12 @@ struct InterpThreadContext // stack, and also needed for the GC to be able to scan the stack. int8_t *pStackPointer; - // This is an allocator for the dynamic stack memory - FrameDataAllocator *pFrameDataAllocator; + FrameDataAllocator frameDataAllocator; - ~InterpThreadContext() { - if (pFrameDataAllocator) - { - delete pFrameDataAllocator; - pFrameDataAllocator = NULL; - } - } + InterpThreadContext(); + ~InterpThreadContext(); }; -InterpThreadContext* InterpGetThreadContext(); void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFrame *pFrame, InterpThreadContext *pThreadContext); #endif diff --git a/src/coreclr/vm/interpframeallocator.cpp b/src/coreclr/vm/interpframeallocator.cpp index 696999dd4c9e30..7523240bdf2b04 100644 --- a/src/coreclr/vm/interpframeallocator.cpp +++ b/src/coreclr/vm/interpframeallocator.cpp @@ -28,10 +28,9 @@ FrameDataAllocator::FrameDataFragment::~FrameDataFragment() free(pFrameStart); } -FrameDataAllocator::FrameDataAllocator(size_t size) +FrameDataAllocator::FrameDataAllocator() { - pFirst = new FrameDataFragment(size); - pCurrent = pFirst; + pFirst = pCurrent = nullptr; pInfos = nullptr; infosLen = 0; infosCapacity = 0; @@ -39,9 +38,12 @@ FrameDataAllocator::FrameDataAllocator(size_t size) FrameDataAllocator::~FrameDataAllocator() { - assert(pCurrent == pFirst && pCurrent->pFramePos == pCurrent->pFrameStart); - FreeFragments(pFirst); - free(pInfos); + if (pFirst != nullptr) + { + assert(pCurrent == pFirst && pCurrent->pFramePos == pCurrent->pFrameStart); + FreeFragments(pFirst); + free(pInfos); + } } void FrameDataAllocator::FreeFragments(FrameDataFragment *pFrag) @@ -77,6 +79,16 @@ bool FrameDataAllocator::PushInfo(InterpMethodContextFrame *pFrame) void *FrameDataAllocator::Alloc(InterpMethodContextFrame *pFrame, size_t size) { + if (pFirst == nullptr) + { + pFirst = new (nothrow) FrameDataFragment(size); + if (pFirst == nullptr || pFirst->pFrameStart == nullptr) + { + return nullptr; + } + pCurrent = pFirst; + } + if (!infosLen || pInfos[infosLen - 1].pFrame != pFrame) { if (!PushInfo(pFrame)) @@ -100,8 +112,8 @@ void *FrameDataAllocator::Alloc(InterpMethodContextFrame *pFrame, size_t size) FreeFragments(pCurrent->pNext); pCurrent->pNext = nullptr; - FrameDataFragment *pNewFrag = new FrameDataFragment(size); - if (pNewFrag->pFrameStart == nullptr) + FrameDataFragment *pNewFrag = new (nothrow) FrameDataFragment(size); + if (pNewFrag == nullptr || pNewFrag->pFrameStart == nullptr) { return nullptr; } @@ -127,9 +139,4 @@ void FrameDataAllocator::PopInfo(InterpMethodContextFrame *pFrame) } } -bool FrameDataAllocator::IsAllocated() -{ - return pFirst != nullptr && pFirst->pFrameStart != nullptr; -} - #endif // FEATURE_INTERPRETER diff --git a/src/coreclr/vm/interpframeallocator.h b/src/coreclr/vm/interpframeallocator.h index e7ebc39fe87a7c..3069dd6c36a5fa 100644 --- a/src/coreclr/vm/interpframeallocator.h +++ b/src/coreclr/vm/interpframeallocator.h @@ -45,12 +45,11 @@ class FrameDataAllocator bool PushInfo(InterpMethodContextFrame *pFrame); void FreeFragments(FrameDataFragment *pFrag); public: - FrameDataAllocator(size_t size); + FrameDataAllocator(); ~FrameDataAllocator(); void *Alloc(InterpMethodContextFrame *pFrame, size_t size); void PopInfo(InterpMethodContextFrame *pFrame); - bool IsAllocated(); }; #endif diff --git a/src/coreclr/vm/prestub.cpp b/src/coreclr/vm/prestub.cpp index 50a8ce012b5f60..a0e9883ebb7b04 100644 --- a/src/coreclr/vm/prestub.cpp +++ b/src/coreclr/vm/prestub.cpp @@ -1943,7 +1943,12 @@ extern "C" void STDCALL ExecuteInterpretedMethod(TransitionBlock* pTransitionBlo { // Argument registers are in the TransitionBlock // The stack arguments are right after the pTransitionBlock - InterpThreadContext *threadContext = InterpGetThreadContext(); + Thread *pThread = GetThread(); + InterpThreadContext *threadContext = pThread->GetInterpThreadContext(); + if (threadContext == nullptr || threadContext->pStackStart == nullptr) + { + COMPlusThrow(kOutOfMemoryException); + } int8_t *sp = threadContext->pStackPointer; // This construct ensures that the InterpreterFrame is always stored at a higher address than the diff --git a/src/coreclr/vm/threads.cpp b/src/coreclr/vm/threads.cpp index daa9c4932e80eb..9d5ee093dd540a 100644 --- a/src/coreclr/vm/threads.cpp +++ b/src/coreclr/vm/threads.cpp @@ -56,6 +56,10 @@ #include "exinfo.h" #endif +#ifdef FEATURE_INTERPRETER +#include "interpexec.h" +#endif // FEATURE_INTERPRETER + static const PortableTailCallFrame g_sentinelTailCallFrame = { NULL, NULL }; TailCallTls::TailCallTls() @@ -1549,6 +1553,10 @@ Thread::Thread() #ifdef _DEBUG memset(dangerousObjRefs, 0, sizeof(dangerousObjRefs)); #endif // _DEBUG + +#ifdef FEATURE_INTERPRETER + m_pInterpThreadContext = nullptr; +#endif // FEATURE_INTERPRETER } //-------------------------------------------------------------------- @@ -2517,6 +2525,13 @@ Thread::~Thread() // Wait for another thread to leave its loop in DeadlockAwareLock::TryBeginEnterLock CrstHolder lock(&g_DeadlockAwareCrst); + +#ifdef FEATURE_INTERPRETER + if (m_pInterpThreadContext != nullptr) + { + delete m_pInterpThreadContext; + } +#endif // FEATURE_INTERPRETER } #ifdef FEATURE_COMINTEROP_APARTMENT_SUPPORT @@ -7784,6 +7799,20 @@ void ClrRestoreNonvolatileContext(PCONTEXT ContextRecord, size_t targetSSP) #endif } +#ifdef FEATURE_INTERPRETER +InterpThreadContext* Thread::GetInterpThreadContext() +{ + WRAPPER_NO_CONTRACT; + + if (m_pInterpThreadContext == nullptr) + { + m_pInterpThreadContext = new (nothrow) InterpThreadContext(); + } + + return m_pInterpThreadContext; +} +#endif // FEATURE_INTERPRETER + #endif // #ifndef DACCESS_COMPILE #ifdef DACCESS_COMPILE diff --git a/src/coreclr/vm/threads.h b/src/coreclr/vm/threads.h index a1ca4943a6be8c..93b77963aa0cd4 100644 --- a/src/coreclr/vm/threads.h +++ b/src/coreclr/vm/threads.h @@ -142,6 +142,7 @@ class FaultingExceptionFrame; enum BinderMethodID : int; class PrepareCodeConfig; class NativeCodeVersion; +struct InterpThreadContext; typedef void(*ADCallBackFcnType)(LPVOID); @@ -3979,6 +3980,14 @@ friend class DebuggerController; bool m_hasPendingActivation; friend struct ::cdac_data; + +#ifdef FEATURE_INTERPRETER +private: + InterpThreadContext *m_pInterpThreadContext; + +public: + InterpThreadContext* GetInterpThreadContext(); +#endif // FEATURE_INTERPRETER }; template<> From 0e97a9c13b9c461b6c08b44637f074ead43870cf Mon Sep 17 00:00:00 2001 From: Milos Kotlar Date: Sat, 26 Apr 2025 15:01:11 +0200 Subject: [PATCH 24/26] Move InterpThreadContext destructor call to OnThreadTerminate --- src/coreclr/interpreter/compiler.cpp | 8 ++++++-- src/coreclr/vm/interpexec.cpp | 5 ++--- src/coreclr/vm/interpframeallocator.cpp | 24 ++++++++++++++++++------ src/coreclr/vm/interpframeallocator.h | 2 +- src/coreclr/vm/threads.cpp | 14 +++++++------- 5 files changed, 34 insertions(+), 19 deletions(-) diff --git a/src/coreclr/interpreter/compiler.cpp b/src/coreclr/interpreter/compiler.cpp index 15c3793e46d150..b2afc56321633d 100644 --- a/src/coreclr/interpreter/compiler.cpp +++ b/src/coreclr/interpreter/compiler.cpp @@ -3147,8 +3147,12 @@ int InterpCompiler::GenerateCode(CORINFO_METHOD_INFO* methodInfo) case CEE_LOCALLOC: CHECK_STACK(1); #if TARGET_64BIT - if (m_pStackPointer[-1].type == StackTypeI8) - EmitConv(m_pStackPointer - 1, NULL, StackTypeI4, INTOP_MOV_8); + // Length is natural unsigned int + if (m_pStackPointer[-1].type == StackTypeI4) + { + EmitConv(m_pStackPointer - 1, NULL, StackTypeI8, INTOP_MOV_8); + m_pStackPointer[-1].type = StackTypeI8; + } #endif AddIns(INTOP_LOCALLOC); m_pStackPointer--; diff --git a/src/coreclr/vm/interpexec.cpp b/src/coreclr/vm/interpexec.cpp index 6dbf461fbe1765..bd3ddca4ca8e89 100644 --- a/src/coreclr/vm/interpexec.cpp +++ b/src/coreclr/vm/interpexec.cpp @@ -1143,13 +1143,12 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr break; case INTOP_LOCALLOC: { - int32_t len = LOCAL_VAR(ip[2], int32_t); - len = ALIGN_UP(len, sizeof(void*)); + size_t len = LOCAL_VAR(ip[2], size_t); void* pMemory = NULL; if (len > 0) { - pMemory = pThreadContext->frameDataAllocator.Alloc(pFrame, len); + pMemory = pThreadContext->frameDataAllocator.Alloc(pFrame, &len); if (pMemory == NULL) { // Interpreter-TODO: OutOfMemoryException diff --git a/src/coreclr/vm/interpframeallocator.cpp b/src/coreclr/vm/interpframeallocator.cpp index 7523240bdf2b04..48d748b9a7c576 100644 --- a/src/coreclr/vm/interpframeallocator.cpp +++ b/src/coreclr/vm/interpframeallocator.cpp @@ -61,6 +61,10 @@ bool FrameDataAllocator::PushInfo(InterpMethodContextFrame *pFrame) if (infosLen == infosCapacity) { size_t newCapacity = infosCapacity == 0 ? 8 : infosCapacity * 2; + if (newCapacity > SIZE_MAX / sizeof(FrameDataInfo)) + { + return false; + } FrameDataInfo* newInfos = (FrameDataInfo*)realloc(pInfos, newCapacity * sizeof(FrameDataInfo)); if (newInfos == nullptr) { @@ -77,11 +81,19 @@ bool FrameDataAllocator::PushInfo(InterpMethodContextFrame *pFrame) return true; } -void *FrameDataAllocator::Alloc(InterpMethodContextFrame *pFrame, size_t size) +void *FrameDataAllocator::Alloc(InterpMethodContextFrame *pFrame, size_t *size) { + if (size == nullptr || *size == 0) + { + return nullptr; + } + + *size = ALIGN_UP(*size, sizeof(void*)); + size_t alignedSize = *size; + if (pFirst == nullptr) { - pFirst = new (nothrow) FrameDataFragment(size); + pFirst = new (nothrow) FrameDataFragment(alignedSize); if (pFirst == nullptr || pFirst->pFrameStart == nullptr) { return nullptr; @@ -99,10 +111,10 @@ void *FrameDataAllocator::Alloc(InterpMethodContextFrame *pFrame, size_t size) uint8_t *pFramePos = pCurrent->pFramePos; - if (pFramePos + size > pCurrent->pFrameEnd) + if (pFramePos + alignedSize > pCurrent->pFrameEnd) { // Move to the next fragment or create a new one if necessary - if (pCurrent->pNext && ((pCurrent->pNext->pFrameStart + size) <= pCurrent->pNext->pFrameEnd)) + if (pCurrent->pNext && ((pCurrent->pNext->pFrameStart + alignedSize) <= pCurrent->pNext->pFrameEnd)) { pCurrent = pCurrent->pNext; pFramePos = pCurrent->pFramePos = pCurrent->pFrameStart; @@ -112,7 +124,7 @@ void *FrameDataAllocator::Alloc(InterpMethodContextFrame *pFrame, size_t size) FreeFragments(pCurrent->pNext); pCurrent->pNext = nullptr; - FrameDataFragment *pNewFrag = new (nothrow) FrameDataFragment(size); + FrameDataFragment *pNewFrag = new (nothrow) FrameDataFragment(alignedSize); if (pNewFrag == nullptr || pNewFrag->pFrameStart == nullptr) { return nullptr; @@ -125,7 +137,7 @@ void *FrameDataAllocator::Alloc(InterpMethodContextFrame *pFrame, size_t size) } void *pMemory = (void*)pFramePos; - pCurrent->pFramePos = (uint8_t*)(pFramePos + size); + pCurrent->pFramePos = (uint8_t*)(pCurrent->pFramePos + alignedSize); return pMemory; } diff --git a/src/coreclr/vm/interpframeallocator.h b/src/coreclr/vm/interpframeallocator.h index 3069dd6c36a5fa..bd31db11d0e920 100644 --- a/src/coreclr/vm/interpframeallocator.h +++ b/src/coreclr/vm/interpframeallocator.h @@ -48,7 +48,7 @@ class FrameDataAllocator FrameDataAllocator(); ~FrameDataAllocator(); - void *Alloc(InterpMethodContextFrame *pFrame, size_t size); + void *Alloc(InterpMethodContextFrame *pFrame, size_t *size); void PopInfo(InterpMethodContextFrame *pFrame); }; diff --git a/src/coreclr/vm/threads.cpp b/src/coreclr/vm/threads.cpp index 9d5ee093dd540a..c745619a52d94f 100644 --- a/src/coreclr/vm/threads.cpp +++ b/src/coreclr/vm/threads.cpp @@ -2525,13 +2525,6 @@ Thread::~Thread() // Wait for another thread to leave its loop in DeadlockAwareLock::TryBeginEnterLock CrstHolder lock(&g_DeadlockAwareCrst); - -#ifdef FEATURE_INTERPRETER - if (m_pInterpThreadContext != nullptr) - { - delete m_pInterpThreadContext; - } -#endif // FEATURE_INTERPRETER } #ifdef FEATURE_COMINTEROP_APARTMENT_SUPPORT @@ -2817,6 +2810,13 @@ void Thread::OnThreadTerminate(BOOL holdingLock) DWORD CurrentThreadID = pCurrentThread?pCurrentThread->GetThreadId():0; DWORD ThisThreadID = GetThreadId(); +#ifdef FEATURE_INTERPRETER + if (m_pInterpThreadContext != nullptr) + { + delete m_pInterpThreadContext; + } +#endif // FEATURE_INTERPRETER + #ifdef FEATURE_COMINTEROP_APARTMENT_SUPPORT // If the currently running thread is the thread that died and it is an STA thread, then we // need to release all the RCW's in the current context. However, we cannot do this if we From 980eb787583ef3d0893504202c28a94055af418f Mon Sep 17 00:00:00 2001 From: Milos Kotlar Date: Sat, 26 Apr 2025 23:31:44 +0200 Subject: [PATCH 25/26] Refactor Alloc to accept size by value instead of pointer --- src/coreclr/vm/interpexec.cpp | 2 +- src/coreclr/vm/interpframeallocator.cpp | 20 +++++++------------- src/coreclr/vm/interpframeallocator.h | 2 +- 3 files changed, 9 insertions(+), 15 deletions(-) diff --git a/src/coreclr/vm/interpexec.cpp b/src/coreclr/vm/interpexec.cpp index bd3ddca4ca8e89..0d275e5bb26a0f 100644 --- a/src/coreclr/vm/interpexec.cpp +++ b/src/coreclr/vm/interpexec.cpp @@ -1148,7 +1148,7 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr if (len > 0) { - pMemory = pThreadContext->frameDataAllocator.Alloc(pFrame, &len); + pMemory = pThreadContext->frameDataAllocator.Alloc(pFrame, len); if (pMemory == NULL) { // Interpreter-TODO: OutOfMemoryException diff --git a/src/coreclr/vm/interpframeallocator.cpp b/src/coreclr/vm/interpframeallocator.cpp index 48d748b9a7c576..cc9513f9b09b78 100644 --- a/src/coreclr/vm/interpframeallocator.cpp +++ b/src/coreclr/vm/interpframeallocator.cpp @@ -81,19 +81,13 @@ bool FrameDataAllocator::PushInfo(InterpMethodContextFrame *pFrame) return true; } -void *FrameDataAllocator::Alloc(InterpMethodContextFrame *pFrame, size_t *size) +void *FrameDataAllocator::Alloc(InterpMethodContextFrame *pFrame, size_t size) { - if (size == nullptr || *size == 0) - { - return nullptr; - } - - *size = ALIGN_UP(*size, sizeof(void*)); - size_t alignedSize = *size; + size = ALIGN_UP(size, sizeof(void*)); if (pFirst == nullptr) { - pFirst = new (nothrow) FrameDataFragment(alignedSize); + pFirst = new (nothrow) FrameDataFragment(size); if (pFirst == nullptr || pFirst->pFrameStart == nullptr) { return nullptr; @@ -111,10 +105,10 @@ void *FrameDataAllocator::Alloc(InterpMethodContextFrame *pFrame, size_t *size) uint8_t *pFramePos = pCurrent->pFramePos; - if (pFramePos + alignedSize > pCurrent->pFrameEnd) + if (pFramePos + size > pCurrent->pFrameEnd) { // Move to the next fragment or create a new one if necessary - if (pCurrent->pNext && ((pCurrent->pNext->pFrameStart + alignedSize) <= pCurrent->pNext->pFrameEnd)) + if (pCurrent->pNext && ((pCurrent->pNext->pFrameStart + size) <= pCurrent->pNext->pFrameEnd)) { pCurrent = pCurrent->pNext; pFramePos = pCurrent->pFramePos = pCurrent->pFrameStart; @@ -124,7 +118,7 @@ void *FrameDataAllocator::Alloc(InterpMethodContextFrame *pFrame, size_t *size) FreeFragments(pCurrent->pNext); pCurrent->pNext = nullptr; - FrameDataFragment *pNewFrag = new (nothrow) FrameDataFragment(alignedSize); + FrameDataFragment *pNewFrag = new (nothrow) FrameDataFragment(size); if (pNewFrag == nullptr || pNewFrag->pFrameStart == nullptr) { return nullptr; @@ -137,7 +131,7 @@ void *FrameDataAllocator::Alloc(InterpMethodContextFrame *pFrame, size_t *size) } void *pMemory = (void*)pFramePos; - pCurrent->pFramePos = (uint8_t*)(pCurrent->pFramePos + alignedSize); + pCurrent->pFramePos = (uint8_t*)(pCurrent->pFramePos + size); return pMemory; } diff --git a/src/coreclr/vm/interpframeallocator.h b/src/coreclr/vm/interpframeallocator.h index bd31db11d0e920..3069dd6c36a5fa 100644 --- a/src/coreclr/vm/interpframeallocator.h +++ b/src/coreclr/vm/interpframeallocator.h @@ -48,7 +48,7 @@ class FrameDataAllocator FrameDataAllocator(); ~FrameDataAllocator(); - void *Alloc(InterpMethodContextFrame *pFrame, size_t *size); + void *Alloc(InterpMethodContextFrame *pFrame, size_t size); void PopInfo(InterpMethodContextFrame *pFrame); }; From 902e4d074c022175d086fd2d75ea302a6b09bfbb Mon Sep 17 00:00:00 2001 From: Milos Kotlar Date: Mon, 28 Apr 2025 12:38:06 +0200 Subject: [PATCH 26/26] Fix typo --- src/coreclr/vm/interpexec.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/coreclr/vm/interpexec.cpp b/src/coreclr/vm/interpexec.cpp index e550efd2770779..78e0e1fe667612 100644 --- a/src/coreclr/vm/interpexec.cpp +++ b/src/coreclr/vm/interpexec.cpp @@ -1162,6 +1162,8 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr LOCAL_VAR(ip[1], void*) = pMemory; ip += 3; + break; + } case INTOP_GC_COLLECT: { // HACK: blocking gc of all generations to enable early stackwalk testing // Interpreter-TODO: Remove this