More work on supporting generics in type system and execution engine #3242
Conversation
- Add handler for instruction. Add storage pointers to stack frame to allow dealocation on moethod return.
- Update mscorlib declaration.
nfbot
added
Type: enhancement
Type: dependencies
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds per-frame localloc tracking and implementations for CEE_LOCALLOC/CEE_CPBLK and RVA field loads; introduces DATATYPE_PTR, storage-backed arrays and pointer-aware HeapBlock operations; adds native Span/ReadOnlySpan constructors that bind storage to a void* (or memmove for CopyTo), updates native tables and an mscorlib metadata token. Changes
Sequence Diagram(s)sequenceDiagram
participant IL as IL Interpreter
participant Frame as Stack Frame
participant Heap as Heap / Array
participant GC as GC
IL->>Frame: CEE_LOCALLOC(size)
Frame->>Heap: allocate byte[] (size)
Heap-->>Frame: arrayRef (m_StoragePointer == 0 or storage-backed)
Frame->>GC: protect arrayRef (store in m_localAllocs)
Frame->>Frame: m_localAllocCount++
Frame-->>IL: push arrayRef
Note right of Frame: PushInline/RestoreFromInline copy/restore\nm_localAllocCount & m_localAllocs
Note right of Frame: On Pop: free/clear m_localAllocs and reset count
sequenceDiagram
participant Caller as Managed Call
participant SpanNative as Span native ctor
participant TypeSys as Type System
participant Heap as Heap / Array
participant GC as GC
Caller->>SpanNative: _ctor(void* ptr, int length)
SpanNative->>TypeSys: resolve and validate generic T (no refs)
TypeSys-->>SpanNative: resolved / error
SpanNative->>Heap: CreateInstanceWithStorage(length, storageAddr, elementType)
Heap-->>SpanNative: arrayRef (m_StoragePointer = storageAddr)
SpanNative->>SpanNative: set span length & backing array reference
SpanNative-->>Caller: initialized Span instance (or throw)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/CLR/CorLib/corlib_native_System_Span_1.cpp (1)
9-151: Span(void, int) ctor: logic is sound; consider minor robustness tweaks*Overall the implementation is careful: it reconstructs
Tfrom the caller’s generic context, rejects reference/contains‑reference element types, validateslength, enforces that the backing buffer isbyte[], computes the element size viac_CLR_RT_DataTypeLookup, validates capacity, reshapes the array metadata, and finally sets the Span length. This aligns with the ReadOnlySpan variant and the intendedlocalloc‑backed usage.A few points to double‑check:
sourceArray = stack.Arg1().DereferenceArray();assumes theVOID*argument is always a managed array reference. If any other caller ever passes a non‑array object (or null), this will AV; consider adding a null/array check and returningCLR_E_WRONG_TYPEorCLR_E_NULL_REFERENCEfor robustness.newNumElements = sourceArray->m_numOfElements / elementSizesilently truncates if the byte count is not an exact multiple ofelementSize. If the intent is to require exact divisibility (to avoid silently “losing” trailing bytes), you might want to checkm_numOfElements % elementSize == 0and treat mismatch asCLR_E_INVALID_PARAMETER.ClearElements(0, length)after reshaping is good for safety, but note that it zeroes the region even if the caller had already written into thelocallocbuffer; this is acceptable if the design is that the Span ctor owns initialization, but it’s worth confirming that no caller expects preserved contents.If all current call sites are only the new CoreLib APIs that hand in
localloc-allocatedbyte[], the current behavior is acceptable; the above mainly protects against future misuse.src/CLR/CorLib/corlib_native_System_ReadOnlySpan_1.cpp (1)
9-151: ReadOnlySpan(void, int) mirrors Span correctly*The ReadOnlySpan pointer ctor is essentially identical to the Span version: it reconstructs
Tfrom the generic context, blocks reference/contains‑reference types, validateslength, enforcesbyte[]backing, reshapes the array metadata, and sets span length. That symmetry is good and keeps the two behaviors aligned.Same optional points as for Span:
- Consider a defensive null/array check on
sourceArray = stack.Arg1().DereferenceArray();to avoid hard faults on misuse.- Decide whether
m_numOfElements % elementSize != 0should be rejected explicitly rather than truncating via/.- Confirm that clearing elements via
ClearElements(0, length)matches expected semantics for any caller that might preinitialize the underlying buffer.Given the current intended use (backing from
localloc), the implementation is coherent; the suggestions are mainly to harden against future callers.src/CLR/Core/CLR_RT_StackFrame.cpp (1)
334-359: Inline frame save/restore of localloc state is consistentIn
PushInline, you:
- Save
m_localAllocCountand allm_localAllocs[]into the inline frame backup.- Then reset
m_localAllocCountand clearm_localAllocs[]for the inlined callee.This achieves the desired behavior: the caller’s locallocs are preserved and isolated from any additional locallocs in the inlined method. On
PopInline,RestoreFromInlineStackrestores the caller’sm_localAllocCountand array. This is logically sound and matches how other frame state is managed.src/CLR/Include/nanoCLR_Runtime.h (1)
2532-2534: Localloc tracking fields are consistent; just confirm init/GC integration and fix a small comment nitThe shared
MAX_LOCALALLOC_COUNTand mirroredm_localAllocCount/m_localAllocs[]in bothCLR_RT_InlineFrameandCLR_RT_StackFramelook coherent and match the intended per-frame localloc tracking.Two follow‑ups to double‑check in the implementation files:
- Ensure
m_localAllocCountis always reset andm_localAllocs[]are zeroed when stack frames are created or reused (including inline frames pulled fromCLR_RT_EventCache::m_inlineBufferStart), so no stale GC roots are left behind.- Ensure the GC mark path that walks stack/frame roots also walks each non‑null entry in
m_localAllocs[], otherwise these temporary arrays could be collected while still in use.Minor: the comment at Line 2570 has a typo (“max mumber” → “max number”).
Also applies to: 2545-2547, 2570-2572, 2674-2676
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/CLR/CorLib/corlib_native.cpp(5 hunks)src/CLR/CorLib/corlib_native.h(2 hunks)src/CLR/CorLib/corlib_native_System_ReadOnlySpan_1.cpp(1 hunks)src/CLR/CorLib/corlib_native_System_Span_1.cpp(1 hunks)src/CLR/Core/CLR_RT_StackFrame.cpp(7 hunks)src/CLR/Core/Interpreter.cpp(1 hunks)src/CLR/Include/nanoCLR_Runtime.h(4 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-01-09T13:32:43.711Z
Learnt from: josesimoes
Repo: nanoframework/nf-interpreter PR: 3062
File: src/System.Device.Spi/sys_dev_spi_native_System_Device_Spi_SpiDevice.cpp:106-188
Timestamp: 2025-01-09T13:32:43.711Z
Learning: In nanoFramework, CLR_RT_HeapBlock_Array::Pin() method returns void and cannot fail. It should be called without error handling.
Applied to files:
src/CLR/CorLib/corlib_native_System_ReadOnlySpan_1.cppsrc/CLR/CorLib/corlib_native_System_Span_1.cpp
📚 Learning: 2024-10-12T19:00:39.000Z
Learnt from: josesimoes
Repo: nanoframework/nf-interpreter PR: 3023
File: targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp:191-225
Timestamp: 2024-10-12T19:00:39.000Z
Learning: When working with `nanoCLR_GetNativeAssemblyInformation`, fixed-size assembly names are required, so code that deals with variable-length names cannot be used.
Applied to files:
src/CLR/CorLib/corlib_native.hsrc/CLR/CorLib/corlib_native.cppsrc/CLR/Include/nanoCLR_Runtime.h
📚 Learning: 2025-06-26T09:16:55.184Z
Learnt from: josesimoes
Repo: nanoframework/nf-interpreter PR: 3190
File: src/CLR/Core/TypeSystem.cpp:0-0
Timestamp: 2025-06-26T09:16:55.184Z
Learning: In nanoFramework's CLR attribute parsing (src/CLR/Core/TypeSystem.cpp), the sentinel value 0xFFFF in string tokens represents a null string. When encountered, this should result in a true null reference (using SetObjectReference(nullptr)) rather than an empty string instance, and the boxing operation should be skipped via early return.
Applied to files:
src/CLR/CorLib/corlib_native.cpp
📚 Learning: 2024-09-25T11:28:38.536Z
Learnt from: josesimoes
Repo: nanoframework/nf-interpreter PR: 3023
File: targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp:191-225
Timestamp: 2024-09-25T11:28:38.536Z
Learning: In `nanoCLR_GetNativeAssemblyInformation`, there is no need to return the number of bytes written, as the memory buffer is zeroed, making the string buffer terminated.
Applied to files:
src/CLR/CorLib/corlib_native.cpp
📚 Learning: 2025-01-22T03:38:57.394Z
Learnt from: josesimoes
Repo: nanoframework/nf-interpreter PR: 3074
File: src/CLR/Core/GarbageCollector_Info.cpp:107-167
Timestamp: 2025-01-22T03:38:57.394Z
Learning: In nanoFramework's memory management code, DataSize() validation is comprehensively handled through CLR_RT_HeapCluster::ValidateBlock() and other caller code. Additional size checks in ValidateCluster() are redundant as the validation is already performed at multiple levels.
Applied to files:
src/CLR/Include/nanoCLR_Runtime.h
🧬 Code graph analysis (3)
src/CLR/CorLib/corlib_native_System_ReadOnlySpan_1.cpp (1)
src/CLR/CorLib/corlib_native_System_Span_1.cpp (2)
_ctor___VOID__VOIDptr__I4(11-151)_ctor___VOID__VOIDptr__I4(11-11)
src/CLR/CorLib/corlib_native_System_Span_1.cpp (1)
src/CLR/CorLib/corlib_native_System_ReadOnlySpan_1.cpp (2)
_ctor___VOID__VOIDptr__I4(11-151)_ctor___VOID__VOIDptr__I4(11-11)
src/CLR/CorLib/corlib_native.cpp (2)
src/CLR/CorLib/corlib_native_System_ReadOnlySpan_1.cpp (2)
_ctor___VOID__VOIDptr__I4(11-151)_ctor___VOID__VOIDptr__I4(11-11)src/CLR/CorLib/corlib_native_System_Span_1.cpp (2)
_ctor___VOID__VOIDptr__I4(11-151)_ctor___VOID__VOIDptr__I4(11-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: nf-interpreter (Build_Azure_RTOS_targets SL_STK3701A)
- GitHub Check: nf-interpreter (Build_WIN32_nanoCLR)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_P4_UART)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_ETHERNET_KIT_1.2)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_S3_ALL)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_H2_THREAD)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_C6_THREAD)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_C3)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_BLE_REV0)
- GitHub Check: nf-interpreter (Build_STM32_targets ST_STM32F769I_DISCOVERY)
- GitHub Check: nf-interpreter (Build_STM32_targets ST_STM32F429I_DISCOVERY)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_PSRAM_REV0)
- GitHub Check: nf-interpreter (Build_NXP_targets NXP_MIMXRT1060_EVK)
- GitHub Check: nf-interpreter (Build_TI_SimpleLink_targets TI_CC1352R1_LAUNCHXL_915)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets MXCHIP_AZ3166)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALX)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALTHREE)
- GitHub Check: nf-interpreter (Check_Code_Style)
- GitHub Check: nf-interpreter (Nightly build) (Check_Build_Options)
- GitHub Check: nf-interpreter (Check_Build_Options)
🔇 Additional comments (6)
src/CLR/CorLib/corlib_native.h (1)
781-790: Span/ReadOnlySpan pointer ctor declarations look consistentThe new native declarations for
_ctor___VOID__VOIDptr__I4on bothLibrary_corlib_native_System_ReadOnlySpan_1andLibrary_corlib_native_System_Span_1match the corresponding implementations and are correctly guarded by#if (NANOCLR_REFLECTION == TRUE). No further changes needed here.src/CLR/Core/Interpreter.cpp (1)
1005-1034: Generic .cctor reschedule helper is correct but edge cases should be confirmedThe new
HandleGenericCctorReschedulehelper cleanly centralizes the pattern of:
- Computing a hash for a closed generic type using
ComputeHashForClosedGenericType.- Treating
0xFFFFFFFFas an invalid hash and returningCLR_E_WRONG_TYPE.- Looking up or creating the
CLR_RT_GenericCctorExecutionRecord.- If a .cctor is scheduled but not executed, rewinding
ipby 3 bytes (opcode + compressed field token) and returningCLR_E_RESCHEDULE.Two points to verify:
- The “3 bytes” rewind (
*pIp -= 3) assumes this is only used for opcodes with exactly one‑byte opcode + two‑byte compressed token. You’re using it with field opcodes likeLDSFLD/LDSFLDA/STSFLDwhose encoding here indeed uses a 2‑byte compressed token; please confirm we don’t call this helper from any opcode with a different operand layout in future.FindOrCreateGenericCctorRecord(hash, nullptr)can return non‑null with flags that don’t havec_Scheduledset; you correctly treat that as “no reschedule” and returnS_OK, which is fine. Just be aware this will not attempt to schedule .cctors, only observe already‑scheduled ones.Conceptually this is a good extraction and keeps the LDSFLD/LDSFLDA/STSFLD paths simpler.
src/CLR/Core/CLR_RT_StackFrame.cpp (2)
124-130: Initialize localloc tracking in Push to a known stateZeroing
m_localAllocCountand clearingm_localAllocs[]on frame creation is correct and prevents stale pointers from previous uses of the same event‑heap block. No changes needed here.
425-473: RestoreStack/SaveStack correctly propagate localloc metadata across inline frames
SaveStackandRestoreStacknow copym_localAllocCountand eachm_localAllocs[i]to and from theCLR_RT_InlineFrame. This keeps the localloc tracking coherent across stack saves/restores (e.g., inlining transitions, exception unwinds). The loops mirror each other and usec_Max_Localloc_Count, so they’ll stay in sync with the header definition.src/CLR/CorLib/corlib_native.cpp (2)
699-702: ReadOnlySpan pointer ctor wiring looks correct; please confirm index alignmentThe new
Library_corlib_native_System_ReadOnlySpan_1::_ctor___VOID__VOIDptr__I4entry, with surroundingnullptrpadding and the existingNativeReadOnlySpanConstructorslot, matches the established layout pattern for generic natives in this table.Given how fragile this array/index mapping is, please confirm that:
- The managed
ReadOnlySpan<T>pointer ctor is marked as native and its method index matches this new slot in the reflection-enabled mscorlib.- Any other new
ReadOnlySpan<T>natives introduced in the managed assembly are either accounted for here or intentionally left as managed.Also applies to: 708-709, 718-725
813-825: Span pointer ctor and updated reflection token look consistent; verify against regenerated mscorlibThe added
Library_corlib_native_System_Span_1::_ctor___VOID__VOIDptr__I4entry, along with the nearbyCopyToandNativeSpanConstructorentries and the extranullptrpadding before the threading methods, follows the existing grouping and spacing conventions for Span-related natives.The updated native assembly token
0xCECAB752underNANOCLR_REFLECTION == TRUEis appropriate given the mscorlib metadata change; the non‑reflection token and table remain unchanged.Please verify that:
- The managed
Span<T>pointer ctor’s native index matches this new table position in the reflection-enabled mscorlib build.- The updated token matches the regenerated managed mscorlib (so loader and debugger see this as a single coherent version).
- For
NANOCLR_REFLECTION == FALSE, either the managed mscorlib did not change or its native table/token are regenerated in lockstep in the corresponding build configuration.Also applies to: 843-846, 1616-1616
- Update code accordingly.
- localloc now allocates memory from platform heap. - Stores it as unmaged pointer.
- Now gets pointer to metadata storage.
josesimoes
changed the title
josesimoes
force-pushed
the
implement-localloc
branch
from
9c548a9 to
750b1ef
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Automated fixes for code style.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/CLR/Core/CLR_RT_HeapBlock.cpp (1)
496-657: Fix DATATYPE_PTR handling inStoreToReference; pointer arithmetic looks OKThe new
DATATYPE_PTRsupport inNumericAdd/NumericSubis fine in spirit (treat pointers as byte-wise addresses and only allowI4offsets), but theStoreToReferencebranch forDATATYPE_PTRis unsafe and does not implement cpblk semantics correctly:
dtinStoreToReferenceisref.DataType(), so theDATATYPE_PTRcase runs when the reference is an unmanaged pointer.obj = ref.Dereference();returns the raw address stored in that pointer, which is an arbitrary unmanaged buffer, not aCLR_RT_HeapBlock*.&NumericByRef()is the address of this heap block’s internalNumericunion, not the memory pointed to by the source pointer.memcpy((void *)obj, (void *)&NumericByRef(), size);therefore copies bytes from the heapblock’s internal storage into the unmanaged buffer, not from the source buffer.- After that, the common
obj->Assign(*this);path treats the unmanaged buffer as aCLR_RT_HeapBlockand writes headers/fields into it. That is undefined behavior and will corrupt whatever memory the pointer happens to reference.For
cpblk-style scenarios (destptr,srcptr,size), you likely want:
- Both operands to be unmanaged pointers (e.g.,
src.StoreToReference(dest, size)), and- A raw block copy between the underlying addresses, without interpreting them as heap blocks or assigning heapblock headers.
A safer implementation for the
DATATYPE_PTRbranch would look like:- else if (dt == DATATYPE_PTR) - { - // unmanaged pointer, perform a direct memory copy - obj = ref.Dereference(); - memcpy((void *)obj, (void *)&NumericByRef(), size); - } + else if (dt == DATATYPE_PTR) + { + // Unmanaged pointer: copy raw bytes from this->Dereference() to ref.Dereference(). + void *dst = (void *)ref.Dereference(); + void *src = (void *)this->Dereference(); + + // cpblk allows overlapping regions; memmove is safer than memcpy here. + memmove(dst, src, size); + + // No heapblock assignment for raw pointer copies. + NANOCLR_SET_AND_LEAVE(S_OK); + } @@ - obj->Assign(*this); + obj->Assign(*this);(Where
this->Dereference()andref.Dereference()forDATATYPE_PTRare assumed to return the stored raw addresses.)This avoids treating arbitrary unmanaged buffers as
CLR_RT_HeapBlockinstances and alignsDATATYPE_PTRwith the intended “raw address” semantics.The
NumericAdd/NumericSubDATATYPE_PTRcases themselves look reasonable (byte-wise pointer arithmetic with anI4offset), so onceStoreToReferenceis fixed, the unmanaged pointer story should be consistent.Also applies to: 2005-2095, 2103-2199
♻️ Duplicate comments (1)
src/CLR/Core/Interpreter.cpp (1)
4224-4259: Validate negative sizes and address GC rooting concerns from previous review.This implementation has two issues:
New concern: Negative size check needed. Line 4226 reads the size as
u4(unsigned). If IL passes a negativeint32value, it will be reinterpreted as a large unsigned value, potentially causing a huge allocation attempt. Unlike the array creation path that usesCreateInstance(which checks for negative lengths),platform_mallochas no such guard.Previously flagged: GC rooting and OOM handling still missing. A past review comment (lines 4224-4293) identified that
m_localAllocspointers are not scanned or relocated by the GC, and that OOM handling should include compaction+retry. Those concerns remain unaddressed.Immediate fix for negative size:
OPDEF(CEE_LOCALLOC, "localloc", PopI, PushI, InlineNone, IPrimitive, 2, 0xFE, 0x0F, NEXT) { - CLR_UINT32 size = evalPos[0].NumericByRef().u4; + CLR_INT32 sizeRaw = evalPos[0].NumericByRef().s4; + + if (sizeRaw < 0) + { + NANOCLR_SET_AND_LEAVE(CLR_E_OUT_OF_RANGE); + } + + CLR_UINT32 size = (CLR_UINT32)sizeRaw; evalPos--; CHECKSTACK(stack, evalPos);For the GC rooting and OOM handling issues, please see the detailed previous review comment that recommended:
- Adding
CheckMultipleBlocks(stack->m_localAllocs, stack->m_localAllocCount)inThread_Mark(GC marking)- Calling
Heap_Relocateon each stored pointer in the relocation path- Adding compaction+retry on
CLR_E_OUT_OF_MEMORYsimilar to the array allocation flow (lines ~3410-3420)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/CLR/CorLib/corlib_native_System_ReadOnlySpan_1.cpp(1 hunks)src/CLR/CorLib/corlib_native_System_Span_1.cpp(1 hunks)src/CLR/Core/CLR_RT_HeapBlock.cpp(3 hunks)src/CLR/Core/CLR_RT_StackFrame.cpp(7 hunks)src/CLR/Core/Interpreter.cpp(3 hunks)src/CLR/Include/nanoCLR_Runtime.h(4 hunks)src/CLR/Include/nanoCLR_Runtime__HeapBlock.h(1 hunks)src/CLR/Include/nanoCLR_Types.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/CLR/CorLib/corlib_native_System_ReadOnlySpan_1.cpp
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-01-22T03:38:57.394Z
Learnt from: josesimoes
Repo: nanoframework/nf-interpreter PR: 3074
File: src/CLR/Core/GarbageCollector_Info.cpp:107-167
Timestamp: 2025-01-22T03:38:57.394Z
Learning: In nanoFramework's memory management code, DataSize() validation is comprehensively handled through CLR_RT_HeapCluster::ValidateBlock() and other caller code. Additional size checks in ValidateCluster() are redundant as the validation is already performed at multiple levels.
Applied to files:
src/CLR/Core/CLR_RT_HeapBlock.cppsrc/CLR/Include/nanoCLR_Runtime.h
📚 Learning: 2025-01-09T13:32:43.711Z
Learnt from: josesimoes
Repo: nanoframework/nf-interpreter PR: 3062
File: src/System.Device.Spi/sys_dev_spi_native_System_Device_Spi_SpiDevice.cpp:106-188
Timestamp: 2025-01-09T13:32:43.711Z
Learning: In nanoFramework, CLR_RT_HeapBlock_Array::Pin() method returns void and cannot fail. It should be called without error handling.
Applied to files:
src/CLR/Include/nanoCLR_Runtime__HeapBlock.h
📚 Learning: 2024-10-12T19:00:39.000Z
Learnt from: josesimoes
Repo: nanoframework/nf-interpreter PR: 3023
File: targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp:191-225
Timestamp: 2024-10-12T19:00:39.000Z
Learning: When working with `nanoCLR_GetNativeAssemblyInformation`, fixed-size assembly names are required, so code that deals with variable-length names cannot be used.
Applied to files:
src/CLR/Include/nanoCLR_Runtime.h
📚 Learning: 2025-11-20T14:08:30.044Z
Learnt from: josesimoes
Repo: nanoframework/nf-interpreter PR: 3240
File: src/CLR/Core/TypeSystem.cpp:812-818
Timestamp: 2025-11-20T14:08:30.044Z
Learning: Repo: nanoframework/nf-interpreter
File: src/CLR/Core/TypeSystem.cpp
Context: Generics resolution in CLR_RT_TypeSpec_Instance::ResolveToken
Learning: When resolving a VAR (!T) TypeSpec using the caller’s closed generic, after switching to the caller context (Set(caller->genericType->Assembly(), closedTsRow) and assigning assembly accordingly), always fetch the TypeSpec with target = assembly->GetTypeSpec(closedTsRow) rather than assm->GetTypeSpec(closedTsRow) to avoid cross-assembly mismatches.
Applied to files:
src/CLR/Core/Interpreter.cpp
🧬 Code graph analysis (1)
src/CLR/CorLib/corlib_native_System_Span_1.cpp (1)
src/CLR/CorLib/corlib_native_System_ReadOnlySpan_1.cpp (2)
_ctor___VOID__VOIDptr__I4(11-112)_ctor___VOID__VOIDptr__I4(11-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: nf-interpreter (Check_Code_Style)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets MXCHIP_AZ3166)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALX)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALTHREE)
- GitHub Check: nf-interpreter (Nightly build) (Check_Build_Options)
- GitHub Check: nf-interpreter (Check_Build_Options)
🔇 Additional comments (5)
src/CLR/Include/nanoCLR_Runtime.h (4)
2570-2571: LGTM - Good practice using a class-scoped constant.The constant
c_Max_Localloc_Countcorrectly references the global macro and follows the existing naming convention for constants in this structure.
2545-2546: I'll verify the concerns raised in the original review comment about the new inline frame local allocation tracking fields. Let me start by examining the codebase to understand the implementation details.
<function_calls>
#!/bin/bashFind the full CLR_RT_InlineFrame structure context
rg -n "struct CLR_RT_InlineFrame|class CLR_RT_InlineFrame" -A 50 --type=cpp --type=h | head -100
</function_calls>Now let me search for the PopInline implementation and localloc cleanup logic:
<function_calls>
#!/bin/bashSearch for PopInline and inline frame cleanup
rg -n "PopInline\s*(" -B 3 -A 15 --type=cpp --type=h | head -150
</function_calls>Let me find the MAX_LOCALALLOC_COUNT definition and search for localloc-related cleanup:
<function_calls>
#!/bin/bashFind MAX_LOCALALLOC_COUNT and localloc handling
rg -n "MAX_LOCALALLOC_COUNT|m_localAlloc" --type=cpp --type=h | head -100
</function_calls>Let me also check the frame cleanup mechanism to understand how memory is deallocated:
<function_calls>
#!/bin/bashSearch for frame cleanup and platform_free calls related to allocations
rg -n "platform_free|CleanupAlllocations|m_localAlloc" -B 3 -A 3 --type=cpp | head -150
</function_calls>Let me also look for how regular frames handle localloc allocations to understand the pattern:
<function_calls>
#!/bin/bashCheck if there's a regular frame localloc tracking similar to what's being added for inline frames
rg -n "m_localAllocCount|m_localAllocs" --type=cpp --type=h -B 5 -A 5
</function_calls>
2674-2676: I'm unable to access the repository to verify the specific concerns raised in the review comment. However, based on the review comment itself, I can provide a rewritten version that clarifies the key verification points that need manual confirmation:
Verify cleanup responsibility, document fields, and confirm struct size constraints.
These fields mirror the inline frame additions and share the same requirements for proper maintenance:
Cleanup responsibility: Confirm that
CLR_RT_StackFrame::Pop()properly deallocates pointers tracked inm_localAllocsbefore frame destruction. If cleanup is missing, localloc'd memory could leak.Documentation: Add inline comments explaining that
m_localAllocCountandm_localAllocstrack unmanaged localloc pointers and clarify the cleanup lifecycle.Struct size impact: These additions increase the per-frame overhead by approximately 17–33 bytes (1 byte count + 4–8 bytes × 4 pointers, architecture-dependent). For deeply nested call stacks, verify this trade-off is acceptable and document it.
Struct layout validation: Confirm the placement before
m_extension[1]does not violate alignment expectations and that theCT_ASSERTstatements at lines 2876–2887 still validate field offsets correctly with these new members.
2532-2533: The review comment cannot be verified due to repository access limitations in the sandbox environment.The original comment raises valid concerns about the
MAX_LOCALALLOC_COUNThard limit and requests verification of:
- Overflow handling when the limit of 4 is exceeded
- Documentation explaining why 4 was chosen
- Bounds checking in the CEE_LOCALLOC implementation
However, these verifications require access to:
- The CEE_LOCALLOC instruction handler implementation
- Memory management and frame cleanup code
- Any validation logic related to
m_localAllocCountSince the header file alone cannot confirm whether proper overflow handling exists in the implementation, and sandbox repository access is unavailable, the original review comment's verification request remains unresolved but valid.
The comment should be addressed by the developer reviewing/implementing this change by confirming overflow behavior and adding documentation if not already present.
src/CLR/Include/nanoCLR_Types.h (1)
521-524: I'll help you verify and rewrite this review comment. Let me start by examining the enum structure and the specific concerns raised.
<function_calls>
First, let's read the header file to see the full enum context
cat -n src/CLR/Include/nanoCLR_Types.h | sed -n '500,540p'
</function_calls><function_calls>
Check the full DataType enum to understand the structure
rg -n "enum.*DataType|DATATYPE_" src/CLR/Include/nanoCLR_Types.h | head -50
</function_calls><function_calls>
Let's see more context around DATATYPE_FIRST_INVALID and DATATYPE_PTR
rg -n -B5 -A5 "DATATYPE_FIRST_INVALID|DATATYPE_PTR" src/CLR/Include/nanoCLR_Types.h
</function_calls>
…d0f50-b140-488f-b95f-db856b2f79b6 Code style fixes for nanoframework/nf-interpreter PR#3242
Automated fixes for code style.
…c26bb-406f-4265-bae1-c4f38e4cd866 Code style fixes for nanoframework/nf-interpreter PR#3242
|
@coderabbitai review |
- Noe correctly resolves cross refs from VAR and MVAR.
…pec for context - Rework code to resolve nested VAR and MVAR situations. - General improvement to use recently added helper funcions.
…lution - Update callers and callees accordingly.
- Pre-Execution Validation (before each opcode executes): + Stack Underflow Check: Verifies that the stack has enough items for the opcode to pop + Stack Overflow Check: Ensures the opcode won't exceed maximum stack depth - Post-Execution Validation (after each opcode completes): + Stack Depth Consistency Check: Verifies the stack changed by the expected amount - Enhanced Error Reporting: + All validation errors include: Opcode name and hex value, expected vs. actual values, current stack state Motivation: - Catches underflow before it happens - validates stack has required items - Detects implementation bugs - verifies opcodes modify stack correctly - Pinpoints exact failure - shows which opcode caused the problem - Works with existing infrastructure - uses NANOCLR_OPCODE_STACKCHANGES flag - No performance impact when disabled - all checks are conditional
…eter into implement-localloc
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Automated fixes for code style.
josesimoes
changed the title
…8a279-7b18-46ee-a1c2-e92d9cb20f49 Code style fixes for nanoframework/nf-interpreter PR#3242
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/CLR/Core/CLR_RT_HeapBlock.cpp (1)
644-652: Fix uninitializedobjin DATATYPE_PTR case ofStoreToReferenceIn
StoreToReference, the newdt == DATATYPE_PTRbranch performs amemcpybut then falls through toobj->Assign(*this)withobjnever initialized on that path. If this branch is taken,objis indeterminate and dereferenced, which is undefined behavior.You likely want to treat the unmanaged pointer case as “write raw bytes and return” without touching the heap block target. For example:
- else if (dt == DATATYPE_PTR) - { - // unmanaged pointer, perform a direct memory copy - memcpy((void *)ref.UnmanagedPointer(), (void *)&NumericByRef(), size); - } + else if (dt == DATATYPE_PTR) + { + // unmanaged pointer, perform a direct memory copy + memcpy((void *)ref.UnmanagedPointer(), (void *)&NumericByRef(), size); + + // Nothing to assign back to a HeapBlock in this case + NANOCLR_SET_AND_LEAVE(S_OK); + }(Optionally also guard against non‑positive
sizeif callers might ever pass 0 or −1 here.)src/CLR/Core/TypeSystem.cpp (3)
1170-1401: Guard against nullcaller/ generic context in TypeDef::ResolveToken TypeSpec VAR/MVAR pathIn the
TBL_TypeSpeccase you now resolve VAR/MVAR via thecallermethod context and optionallycontextTypeSpec. Two problems:
In the
DATATYPE_VARbranch you dereferencecallerandcaller->genericTypeand latercaller->arrayElementTypewithout checking thatcallerandcaller->genericTypeare non-null:CLR_RT_TypeSpec_Instance callerTypeSpec; if (!callerTypeSpec.InitializeFromIndex(*caller->genericType)) // potential null deref ... else if (NANOCLR_INDEX_IS_VALID(caller->arrayElementType) && genericPosition == 0) // sameIf
ResolveTokenis ever called with a TypeSpec token that encodes VAR butcalleris null (orcaller->genericTypeis null), this will crash. Other code paths (e.g., the TypeSpec::ResolveToken VAR handler from previous PRs) explicitly checkcaller == nullptr || caller->genericType == nullptrbefore dereferencing. Based on learnings, this should be consistent.In the
DATATYPE_MVARpath you correctly guard oncaller == nullptr || caller->genericType == nullptrand oncontextTypeSpecfor the deferred VAR resolution, but thegoto resolve_generic_argumenttarget is shared with the VAR branch. If the VAR branch is entered with a nullcaller, thegotowill still jump into code that assumescalleris valid.Recommend tightening this block as follows:
- Early-return if
caller == nullptrorcaller->genericType == nullptrbefore any use in theDATATYPE_VARpath.- Guard access to
caller->arrayElementTypewith acaller != nullptrcheck.- Keep the single
resolve_generic_argumentlabel, but ensure every path that reaches it has already validatedcaller/ generic context.Example sketch:
- if (elem.DataType == DATATYPE_VAR) - { + if (elem.DataType == DATATYPE_VAR) + { + if (caller == nullptr || caller->genericType == nullptr) + { + return false; + } CLR_RT_TypeSpec_Instance callerTypeSpec; if (!callerTypeSpec.InitializeFromIndex(*caller->genericType)) return false; ... - else if (NANOCLR_INDEX_IS_VALID(caller->arrayElementType) && genericPosition == 0) + else if (caller != nullptr && + NANOCLR_INDEX_IS_VALID(caller->arrayElementType) && + genericPosition == 0) { ... }This keeps the new behavior but avoids undefined behavior on missing context.
5495-5738: Possible null-dereference ofcontextTypeSpecwhen scheduling generic static constructorsIn
AllocateGenericStaticFieldsOnDemand, after allocating generic static fields and setting up the registry entry, you schedule the generic type’s static constructor:if (SUCCEEDED(CLR_RT_HeapBlock_Delegate::CreateInstance(refDlg, cctorIndex, nullptr))) { CLR_RT_HeapBlock_Delegate *dlg = refDlg.DereferenceDelegate(); ... // Store the TypeSpec index so the .cctor can resolve type generic parameters dlg->m_genericTypeSpec = *contextTypeSpec;
contextTypeSpecis aconst CLR_RT_TypeSpec_Index *parameter and is allowed to be null (and is passed straight through fromGetStaticFieldByFieldDef). Here it’s dereferenced without a null check, which will crash ifAllocateGenericStaticFieldsOnDemandis ever called withcontextTypeSpec == nullptr.Given that this helper is already explicitly passed a
typeSpecIndexfor the generic instance being allocated, andtsInstanceis successfully initialized from it earlier, you can safely store that instead when no external context is needed:- // Store the TypeSpec index so the .cctor can resolve type generic parameters - dlg->m_genericTypeSpec = *contextTypeSpec; + // Prefer the explicit typeSpecIndex; fall back to contextTypeSpec only if provided + if (contextTypeSpec != nullptr && NANOCLR_INDEX_IS_VALID(*contextTypeSpec)) + { + dlg->m_genericTypeSpec = *contextTypeSpec; + } + else + { + dlg->m_genericTypeSpec = typeSpecIndex; + }This keeps the desired behavior and removes a potential null dereference.
7844-7973: BuildMethodName(MethodDef overload): several safety issues aroundgenericTypeand VAR formattingIn the
useGenericpath ofCLR_RT_TypeSystem::BuildMethodName(const CLR_RT_MethodDef_Instance &mdInst, ...)there are a few correctness/safety issues:
Possible null deref of
genericTypewhen formatting method generic argumentsIn the loop over
parser.ParamCountfor MethodSpec generic arguments:if (elem.DataType == DATATYPE_VAR) { CLR_RT_TypeSpec_Instance contextTs; CLR_RT_SignatureParser::Element paramElement; // try to resolve from method context if (!contextTs.InitializeFromIndex(*genericType)) NANOCLR_SET_AND_LEAVE(CLR_E_FAIL); ... }Here
genericTypeis the function parameter, notmdInst.genericType. It may legitimately benullptr(e.g., when the method itself carries its generic context inmdInst.genericType). Dereferencing*genericTypewithout a null check will crash in that case.You should either:
- Prefer
mdInst.genericTypewhen it’s valid, and only fall back togenericTypeif provided, and- Guard all
*genericTypeuses with a null check.Using
contextTs.GetGenericParamfailure but still callingBuildTypeNameon an uninitialized elementIn the same VAR branch:
if (!contextTs.GetGenericParam(elem.GenericParamPosition, paramElement)) { // Couldn't resolve CLR_SafeSprintf(szBuffer, iBuffer, "!%d", elem.GenericParamPosition); } NANOCLR_CHECK_HRESULT(BuildTypeName(paramElement.Class, szBuffer, iBuffer));If
GetGenericParamfails, you print the!Nencoding but then still callBuildTypeName(paramElement.Class, ...)on an uninitializedparamElement. This is undefined behavior and may also produce duplicate output.This should be:
if (!contextTs.GetGenericParam(elem.GenericParamPosition, paramElement)) { CLR_SafeSprintf(szBuffer, iBuffer, "!%d", elem.GenericParamPosition); continue; // don't use paramElement } NANOCLR_CHECK_HRESULT(BuildTypeName(paramElement.Class, szBuffer, iBuffer));DATATYPE_VAR in the earlier MVAR resolution path
Earlier in the file (
BuildTypeName’s MVAR handling and similar code), you already have patterns for deferring VAR resolution back into a type context. It would be good to mirror that here to keep behaviors consistent; right now the VAR handling in this loop is more brittle.Addressing (1) and (2) is important to avoid crashes or malformed method names when generic context is missing or cannot be resolved.
src/CLR/Debugger/Debugger.cpp (1)
3058-3083: Storage-backed arrays inDebugging_Value_GetArrayare only asserting, not handledWhen
array->m_fReference == falseandarray->m_StoragePointer != 0, the new branch justASSERT(FALSE)and leavesblk/referenceunset. In debug builds this will break as soon as a debugger inspects a Span/ReadOnlySpan element backed by storage; in release builds it quietly reports a null/empty value instead of the real element.Either implement the storage-backed element read (e.g., load the value into
tmpfromm_StoragePointer+ index) or at least fall back to a defined behavior (e.g., returning an error reply) instead of asserting and proceeding with an uninitializedblk.
🧹 Nitpick comments (9)
src/CLR/Include/nanoCLR_Types.h (1)
516-524: Tighten DATATYPE_PTR docs and fix comment typoThe new
DATATYPE_PTR/REFLECTION_STORAGE_PTRentries look consistent with the pointer/storage work in this PR. Minor nits:
- Line 522: comment reads
// unamaged pointer; consider correcting to// unmanaged pointer.- Given the nearby “KEEP IN SYNC” notes, double‑check that metadata tooling (
NanoCLRDataTypemirrors in VS extension/MDP, reflection handling forREFLECTION_STORAGE_PTR) has been updated to understand these new enum values.Also applies to: 526-538
src/CLR/Core/CLR_RT_HeapBlock.cpp (1)
1474-1476: EnsureASSERTmacro is defined consistently across targetsThe new SZARRAY equality fast path adds:
// check that array is not stored in stack ASSERT(objLeft->m_StoragePointer == 0);Elsewhere in this file you use
_ASSERTE/NANOCLR_DEBUG_STOP/NANOCLR_STOP. IfASSERTisn’t defined uniformly for all platforms/toolchains, this could become a compile issue. Consider either:
- Reusing the existing
_ASSERTEmacro here, or- Confirming
ASSERTis a project‑wide alias that’s available in all build configurations.src/CLR/Core/Interpreter.cpp (2)
3135-3152: RVA field handling implementation is correct.The implementation properly uses
SetUnmanagedPointer()for RVA-backed fields, which is appropriate for exposing raw metadata storage. ThedummyVarpattern to advance past the element count header works correctly.Minor suggestion: Consider renaming
dummyVartoelementCount(even if unused) for clarity, or use(void)cast pattern to explicitly indicate the intentional discard:- CLR_UINT32 dummyVar; - NANOCLR_READ_UNALIGNED_UINT16(dummyVar, ptrSrc); + CLR_UINT32 elementCount; + NANOCLR_READ_UNALIGNED_UINT16(elementCount, ptrSrc); + (void)elementCount; // Skip element count to get to raw data
4334-4364: CEE_CPBLK implementation follows ECMA-335 specification.The implementation correctly uses
memmove()which handles overlapping memory regions safely. Based on learnings, per ECMA-335 specification,cpblkis an unverifiable instruction where the CIL compiler ensures correctness, so runtime validation is not required.One minor observation: for the 32-bit case, the code uses
.s4(signed) for pointer extraction. While this works in practice (the bit pattern is the same), using.u4would be more semantically correct for pointer values:#else - uintptr_t sourceAddress = evalPos[0].NumericByRef().s4; + uintptr_t sourceAddress = evalPos[0].NumericByRef().u4; #endif evalPos--; // get destination address #ifdef _WIN64 uintptr_t destinationAddress = evalPos[0].NumericByRef().s8; #else - uintptr_t destinationAddress = evalPos[0].NumericByRef().s4; + uintptr_t destinationAddress = evalPos[0].NumericByRef().u4; #endifsrc/CLR/Core/CLR_RT_HeapBlock_Array.cpp (1)
391-392: Consider promotingASSERTto a runtime check for safety.The
ASSERT(arraySrc->m_StoragePointer == 0)only fires in debug builds. In release builds, callingmemmoveon a storage-pointer-backed array could silently corrupt memory or cause undefined behavior. Consider either:
- Adding a runtime check that returns an error (e.g.,
CLR_E_INVALID_OPERATION)- Using the
IsStoragePointer()helper for consistencyif (!arraySrc->m_fReference) { - // check that array is not stored in stack - ASSERT(arraySrc->m_StoragePointer == 0); + // storage-pointer-backed arrays cannot be copied via memmove + if (arraySrc->IsStoragePointer() || arrayDst->IsStoragePointer()) + { + NANOCLR_SET_AND_LEAVE(CLR_E_INVALID_OPERATION); + } memmove(dataDst, dataSrc, length * sizeElem); }src/CLR/Core/TypeSystem.cpp (3)
2501-2556: InitializeFromSignatureParser: VAR resolution viacontextTypeSpecis good, but fallback may mask resolution failuresThe new overload of
CLR_RT_TypeDescriptor::InitializeFromSignatureParserusescontextTypeSpecto resolve aDATATYPE_VARinto a concrete type viacontextTs.GetGenericParam, which aligns with the earlier generic-resolution learnings. Based on learnings, this is desirable.One nuance: if
GetGenericParamfails, you fall back to:// Couldn't resolve, fall back to original behavior NANOCLR_CHECK_HRESULT(InitializeFromType(res.Class));For a VAR,
res.Classis not meaningful, so this will typically fail withCLR_E_WRONG_TYPEanyway; the fallback may obscure that this was specifically a failed generic resolution. Consider, whenres.DataType == DATATYPE_VAR, either:
- Returning a clearer error (e.g.,
CLR_E_WRONG_TYPEwith a comment), or- Leaving a brief comment that this path is expected to fail for VAR and is only here to preserve previous behavior.
No functional bug, but clarity would help future maintainers.
5403-5481: On-demand generic static field allocation: behavior is good, but context assumptions should be documented
CLR_RT_Assembly::GetStaticFieldByFieldDefnow:
- Prefers per-TypeSpec generic statics via
GetGenericStaticField.- For open generics (
!IsClosedGenericType()), callsAllocateGenericStaticFieldsOnDemandwith bothcontextTypeSpecandcontextMethodso that VAR/MVAR resolution during hashing and .cctor scheduling is context-aware.- Falls back to per-assembly static fields when no genericType is provided.
This is a solid design for supporting runtime-bound generic statics.
Given that
AllocateGenericStaticFieldsOnDemandrelies oncontextTypeSpec/contextMethodbeing the same context that was used to bind the generic at IL execution time, it would be worth adding a short comment at the call site clarifying that callers must pass the current closed generic TypeSpec and method context when available; otherwise, hash collisions or missed reuse could occur for open generics that are resolved differently at runtime.No functional bug here, just a suggestion to clarify invariants.
7511-7692: BuildTypeName(TypeSpec): minor robustness issue on VAR resolution failureThe new
BuildTypeName(const CLR_RT_TypeSpec_Index &...)correctly:
- Parses the TypeSpec and prints the generic type definition name.
- Iterates generic arguments and resolves
DATATYPE_VARviacontextTypeSpec, andDATATYPE_MVARviacontextMethodDefand its MethodSpec, falling back to!N/!!Nsyntax when unresolved.One small robustness gap:
if (contextTs.GetGenericParam(element.GenericParamPosition, paramElement)) { // now print the *actual* type name BuildTypeName(paramElement.Class, szBuffer, iBuffer); }If
BuildTypeName(paramElement.Class, ...)fails (e.g., malformed metadata), the HRESULT is ignored. In other places you wrap this withNANOCLR_CHECK_HRESULT. For consistency and easier debugging, it would be better to propagate the error:- if (contextTs.GetGenericParam(element.GenericParamPosition, paramElement)) - { - // now print the *actual* type name - BuildTypeName(paramElement.Class, szBuffer, iBuffer); - } + if (contextTs.GetGenericParam(element.GenericParamPosition, paramElement)) + { + // now print the *actual* type name + NANOCLR_CHECK_HRESULT(BuildTypeName(paramElement.Class, szBuffer, iBuffer)); + }This doesn’t change expected behavior in the normal case but makes failures clearer.
src/CLR/Core/Execution.cpp (1)
2032-2064: Generic parameter resolution (VAR/MVAR) via TypeSpec/MethodSpec is consistent and robustThe new
ResolveGenericTypeParameter()helper plus the updatedDATATYPE_VAR/DATATYPE_MVARhandling inInitializeLocals()correctly:
- Use the enclosing closed
genericTypeTypeSpec to resolve VAR (!T) locals.- Use
CLR_RT_MethodSpec::GetGenericArgument()with aCLR_RT_SignatureParser::Elementto resolve MVAR (!!T) when a MethodSpec is present.- Fall back to
FindGenericParamAtMethodDefand cross-reference lookup for open generic methods.This matches the existing TypeSpec/VAR learnings and centralizes the logic, reducing duplication and future bugs. Based on learnings, this is the intended pattern for consuming VAR/MVAR from signatures.
Also applies to: 2335-2395
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/CLR/CorLib/corlib_native_System_ReadOnlySpan_1.cpp(2 hunks)src/CLR/CorLib/corlib_native_System_Span_1.cpp(3 hunks)src/CLR/Core/CLR_RT_HeapBlock.cpp(4 hunks)src/CLR/Core/CLR_RT_HeapBlock_Array.cpp(6 hunks)src/CLR/Core/Execution.cpp(2 hunks)src/CLR/Core/Interpreter.cpp(5 hunks)src/CLR/Core/TypeSystem.cpp(18 hunks)src/CLR/Debugger/Debugger.cpp(1 hunks)src/CLR/Diagnostics/Info.cpp(3 hunks)src/CLR/Include/nanoCLR_Runtime.h(6 hunks)src/CLR/Include/nanoCLR_Runtime__HeapBlock.h(6 hunks)src/CLR/Include/nanoCLR_Types.h(2 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: josesimoes
Repo: nanoframework/nf-interpreter PR: 3242
File: src/CLR/Core/Interpreter.cpp:4263-4293
Timestamp: 2025-11-27T02:57:09.957Z
Learning: In nanoFramework's nf-interpreter, the cpblk IL instruction implementation in src/CLR/Core/Interpreter.cpp does not require runtime validation of pointer addresses, size, or bounds. Per ECMA-335 specification, cpblk is an unverifiable instruction where the CIL compiler ensures correctness, not the runtime.
📚 Learning: 2025-01-22T03:38:57.394Z
Learnt from: josesimoes
Repo: nanoframework/nf-interpreter PR: 3074
File: src/CLR/Core/GarbageCollector_Info.cpp:107-167
Timestamp: 2025-01-22T03:38:57.394Z
Learning: In nanoFramework's memory management code, DataSize() validation is comprehensively handled through CLR_RT_HeapCluster::ValidateBlock() and other caller code. Additional size checks in ValidateCluster() are redundant as the validation is already performed at multiple levels.
Applied to files:
src/CLR/CorLib/corlib_native_System_Span_1.cppsrc/CLR/Core/Interpreter.cppsrc/CLR/Core/CLR_RT_HeapBlock.cppsrc/CLR/Include/nanoCLR_Runtime__HeapBlock.h
📚 Learning: 2025-01-09T13:32:43.711Z
Learnt from: josesimoes
Repo: nanoframework/nf-interpreter PR: 3062
File: src/System.Device.Spi/sys_dev_spi_native_System_Device_Spi_SpiDevice.cpp:106-188
Timestamp: 2025-01-09T13:32:43.711Z
Learning: In nanoFramework, CLR_RT_HeapBlock_Array::Pin() method returns void and cannot fail. It should be called without error handling.
Applied to files:
src/CLR/CorLib/corlib_native_System_Span_1.cppsrc/CLR/Core/Interpreter.cppsrc/CLR/Core/CLR_RT_HeapBlock.cppsrc/CLR/CorLib/corlib_native_System_ReadOnlySpan_1.cppsrc/CLR/Include/nanoCLR_Runtime__HeapBlock.h
📚 Learning: 2024-09-25T11:28:38.536Z
Learnt from: josesimoes
Repo: nanoframework/nf-interpreter PR: 3023
File: targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp:191-225
Timestamp: 2024-09-25T11:28:38.536Z
Learning: In `nanoCLR_GetNativeAssemblyInformation`, there is no need to return the number of bytes written, as the memory buffer is zeroed, making the string buffer terminated.
Applied to files:
src/CLR/CorLib/corlib_native_System_Span_1.cppsrc/CLR/Include/nanoCLR_Runtime.hsrc/CLR/Include/nanoCLR_Runtime__HeapBlock.h
📚 Learning: 2025-11-20T14:08:30.044Z
Learnt from: josesimoes
Repo: nanoframework/nf-interpreter PR: 3240
File: src/CLR/Core/TypeSystem.cpp:812-818
Timestamp: 2025-11-20T14:08:30.044Z
Learning: Repo: nanoframework/nf-interpreter
File: src/CLR/Core/TypeSystem.cpp
Context: Generics resolution in CLR_RT_TypeSpec_Instance::ResolveToken
Learning: When resolving a VAR (!T) TypeSpec using the caller’s closed generic, after switching to the caller context (Set(caller->genericType->Assembly(), closedTsRow) and assigning assembly accordingly), always fetch the TypeSpec with target = assembly->GetTypeSpec(closedTsRow) rather than assm->GetTypeSpec(closedTsRow) to avoid cross-assembly mismatches.
Applied to files:
src/CLR/Include/nanoCLR_Runtime.hsrc/CLR/Core/Interpreter.cppsrc/CLR/Diagnostics/Info.cppsrc/CLR/Core/CLR_RT_HeapBlock_Array.cppsrc/CLR/Core/Execution.cppsrc/CLR/Include/nanoCLR_Runtime__HeapBlock.hsrc/CLR/Core/TypeSystem.cpp
📚 Learning: 2025-06-26T09:16:55.184Z
Learnt from: josesimoes
Repo: nanoframework/nf-interpreter PR: 3190
File: src/CLR/Core/TypeSystem.cpp:0-0
Timestamp: 2025-06-26T09:16:55.184Z
Learning: In nanoFramework's CLR attribute parsing (src/CLR/Core/TypeSystem.cpp), the sentinel value 0xFFFF in string tokens represents a null string. When encountered, this should result in a true null reference (using SetObjectReference(nullptr)) rather than an empty string instance, and the boxing operation should be skipped via early return.
Applied to files:
src/CLR/Include/nanoCLR_Runtime.hsrc/CLR/Core/CLR_RT_HeapBlock_Array.cppsrc/CLR/Include/nanoCLR_Runtime__HeapBlock.h
📚 Learning: 2024-10-12T19:00:39.000Z
Learnt from: josesimoes
Repo: nanoframework/nf-interpreter PR: 3023
File: targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp:191-225
Timestamp: 2024-10-12T19:00:39.000Z
Learning: When working with `nanoCLR_GetNativeAssemblyInformation`, fixed-size assembly names are required, so code that deals with variable-length names cannot be used.
Applied to files:
src/CLR/Include/nanoCLR_Runtime.hsrc/CLR/Core/Interpreter.cppsrc/CLR/Core/TypeSystem.cpp
📚 Learning: 2025-05-14T16:27:02.573Z
Learnt from: josesimoes
Repo: nanoframework/nf-interpreter PR: 3172
File: src/CLR/Core/CLR_RT_HeapBlock.cpp:899-900
Timestamp: 2025-05-14T16:27:02.573Z
Learning: The CLR_RT_TypeDescriptor type in nanoFramework doesn't have a GetElementType() API for extracting array element types.
Applied to files:
src/CLR/Include/nanoCLR_Runtime.hsrc/CLR/Core/Execution.cppsrc/CLR/Core/TypeSystem.cpp
📚 Learning: 2025-05-14T17:33:51.546Z
Learnt from: josesimoes
Repo: nanoframework/nf-interpreter PR: 3172
File: src/CLR/Core/TypeSystem.cpp:4556-4589
Timestamp: 2025-05-14T17:33:51.546Z
Learning: When parsing TypeSpec signatures in nanoFramework, the first Advance() call consumes the VAR/MVAR token, followed by additional Advance() calls to walk to the specific generic parameter position.
Applied to files:
src/CLR/Include/nanoCLR_Runtime.hsrc/CLR/Core/Execution.cppsrc/CLR/Core/TypeSystem.cpp
📚 Learning: 2025-11-27T02:57:09.957Z
Learnt from: josesimoes
Repo: nanoframework/nf-interpreter PR: 3242
File: src/CLR/Core/Interpreter.cpp:4263-4293
Timestamp: 2025-11-27T02:57:09.957Z
Learning: In nanoFramework's nf-interpreter, the cpblk IL instruction implementation in src/CLR/Core/Interpreter.cpp does not require runtime validation of pointer addresses, size, or bounds. Per ECMA-335 specification, cpblk is an unverifiable instruction where the CIL compiler ensures correctness, not the runtime.
Applied to files:
src/CLR/Include/nanoCLR_Runtime.hsrc/CLR/Core/Interpreter.cppsrc/CLR/Include/nanoCLR_Runtime__HeapBlock.h
📚 Learning: 2025-01-22T03:38:57.394Z
Learnt from: josesimoes
Repo: nanoframework/nf-interpreter PR: 3074
File: src/CLR/Core/GarbageCollector_Info.cpp:107-167
Timestamp: 2025-01-22T03:38:57.394Z
Learning: In CLR_RT_GarbageCollector::ValidateCluster, DataSize() validation is already handled by ValidateBlock() and other caller code, making additional size checks redundant.
Applied to files:
src/CLR/Core/Interpreter.cppsrc/CLR/Include/nanoCLR_Runtime__HeapBlock.h
🧬 Code graph analysis (8)
src/CLR/CorLib/corlib_native_System_Span_1.cpp (2)
src/CLR/CorLib/corlib_native_System_ReadOnlySpan_1.cpp (2)
_ctor___VOID__VOIDptr__I4(11-106)_ctor___VOID__VOIDptr__I4(11-11)src/CLR/Core/CLR_RT_HeapBlock_Array.cpp (2)
CreateInstanceWithStorage(91-119)CreateInstanceWithStorage(91-95)
src/CLR/Include/nanoCLR_Runtime.h (2)
src/CLR/Core/TypeSystem.cpp (12)
ResolveToken(760-846)ResolveToken(760-763)ResolveToken(1167-1427)ResolveToken(1167-1171)ResolveToken(1623-1718)ResolveToken(1623-1626)ResolveToken(1873-2109)ResolveToken(1873-1876)ResolveToken(2206-2230)ResolveToken(2206-2206)GetGenericArgument(2263-2286)GetGenericArgument(2263-2265)src/CLR/Include/nanoCLR_Types.h (2)
const(1112-1184)const(1618-1675)
src/CLR/Core/Interpreter.cpp (1)
src/CLR/Core/CLR_RT_HeapBlock_Array.cpp (4)
CreateInstance(10-69)CreateInstance(10-13)CreateInstance(71-88)CreateInstance(71-74)
src/CLR/Core/CLR_RT_HeapBlock.cpp (1)
src/CLR/Include/nanoCLR_Runtime__HeapBlock.h (1)
SetUnmanagedPointer(1108-1112)
src/CLR/Core/CLR_RT_HeapBlock_Array.cpp (1)
src/CLR/Include/nanoCLR_Runtime__HeapBlock.h (1)
IsStoragePointer(1918-1921)
src/CLR/CorLib/corlib_native_System_ReadOnlySpan_1.cpp (1)
src/CLR/Core/CLR_RT_HeapBlock_Array.cpp (2)
CreateInstanceWithStorage(91-119)CreateInstanceWithStorage(91-95)
src/CLR/Include/nanoCLR_Runtime__HeapBlock.h (1)
src/CLR/Core/CLR_RT_HeapBlock_Array.cpp (2)
CreateInstanceWithStorage(91-119)CreateInstanceWithStorage(91-95)
src/CLR/Core/TypeSystem.cpp (1)
src/CLR/Include/nanoCLR_Types.h (1)
CLR_TkFromStream(650-657)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: nf-interpreter (Check_Code_Style)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets MXCHIP_AZ3166)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALX)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALTHREE)
- GitHub Check: nf-interpreter (Nightly build) (Check_Build_Options)
- GitHub Check: nf-interpreter (Check_Build_Options)
🔇 Additional comments (30)
src/CLR/Core/CLR_RT_HeapBlock.cpp (1)
2084-2099: Pointer add/sub semantics for DATATYPE_PTR look correct and intentionally strictThe new
DATATYPE_PTRbranches inNumericAdd/NumericSub:
- Treat the left operand as an unmanaged byte pointer (
uint8_t*fromUnmanagedPointer()), and- Only allow addition/subtraction of an
I4right operand, updating the stored pointer in place; all other right‑hand types fall through toCLR_E_WRONG_TYPE.This is a clear and predictable model for unmanaged pointer arithmetic, and the explicit comment (“binary numeric add/sub (byte wise)”) documents the byte‑offset behavior well. No issues from a runtime‑semantics perspective; just be aware that adding support for other integral datatypes (e.g.,
U4) later will require extending the type check here.Also applies to: 2185-2200
src/CLR/Include/nanoCLR_Runtime.h (2)
2219-2224: ResolveToken / GetGenericArgument signature changes line up with enhanced generic resolutionThe updated declarations:
CLR_RT_TypeDef_Instance::ResolveToken(...)gaining an optionalconst CLR_RT_TypeSpec_Index* contextTypeSpec, andCLR_RT_MethodSpec_Instance::GetGenericArgument(...)now returning a fullCLR_RT_SignatureParser::Element &are consistent with the generics work described (VAR/MVAR resolution with an explicit TypeSpec context and richer generic-argument inspection). The header signatures match the referenced implementations in
TypeSystem.cppandnanoCLR_Types.h; no issues from an API or ABI standpoint within the CLR.Also applies to: 2356-2357
2536-2551: Guard localloc tracking against overflow of the per-frame arraysThe new localloc tracking implementation—using
MAX_LOCALALLOC_COUNTand them_localAllocCount/m_localAllocsfields inCLR_RT_InlineFrameandCLR_RT_StackFrame—establishes a fixed upper bound of 4 locallocs per frame. This design requires that every writer ofm_localAllocCountenforces the bound before appending tom_localAllocs[...]. If any execution path executes more than 4 localloc instructions without bounds checking, it will write past the array and corrupt the frame.Verify that all interpreter / execution engine code writing locallocs:
- Checks
m_localAllocCount < c_Max_Localloc_Countbefore storing intom_localAllocs[...], and- Fails gracefully (throws OOM or execution engine error) if the bound is exceeded.
src/CLR/Core/Interpreter.cpp (3)
3468-3474: Generic context propagation for array creation looks correct.The additional parameters properly pass the calling context (
&stack->m_call) and generic type storage (&stack->m_genericTypeSpecStorage) to enable correct generic type resolution during array instantiation.
4376-4376: CEE_READONLY prefix correctly added to no-op group.The
readonly.prefix (used beforeldelemato indicate the reference won't be used for modification) is appropriately treated as a no-op in nanoFramework's execution model. Good placement with the other instruction prefixes.
4295-4330: CEE_LOCALLOC implementation uses native heap allocation and requires cleanup verification.The implementation correctly:
- Uses
platform_malloc()for native heap allocation (avoiding GC complexity)- Zero-initializes per ECMA-335 specification
- Bounds-checks against
c_Max_Localloc_Count- Checks for allocation failures
- Uses
SetUnmanagedPointer()to push the resultAllocations stored in
stack->m_localAllocsmust be freed when the stack frame is popped to prevent memory leaks. Verify that deallocation occurs inCLR_RT_StackFrame::Pop()or the frame's destructor.src/CLR/Diagnostics/Info.cpp (1)
581-587: Consistent generic context propagation to BuildTypeName.All three call sites now correctly pass
methodDefInstance.genericTypeand&methodDefInstanceto enable proper resolution of VAR/MVAR parameters in nested type names. The changes are consistent and align with the broader PR objective of enhanced generic context handling across the type system.Also applies to: 731-737, 747-753
src/CLR/Include/nanoCLR_Runtime__HeapBlock.h (4)
763-768: NewUnmanagedPointerstruct forDATATYPE_PTRsupport.The new union member correctly stores unmanaged pointer values for the
locallocinstruction implementation. Theuintptr_ttype ensures proper size across 32/64-bit platforms.
1108-1117: Clean API for unmanaged pointer access.
SetUnmanagedPointercorrectly tags the block asDATATYPE_PTRwith size 1, andUnmanagedPointer()safely returns 0 when the data type doesn't match. This defensive approach prevents misuse.
1886-1893: Storage pointer branch logic in element accessors looks correct.Both
GetFirstElement()andGetFirstElementUInt16()correctly branch onREFLECTION_STORAGE_PTRto return either the external storage address or the inline storage. This enables transparent access regardless of backing store.Also applies to: 1903-1910
1862-1862: Verifym_StoragePointerinitialization in all array creation paths.The new
m_StoragePointerfield requires verification that all array creation paths (particularlyExtractHeapBlocksForArrayin the execution engine) zero-initialize this field, or that theHB_InitializeToZeroflag is always used for array allocations.src/CLR/Core/CLR_RT_HeapBlock_Array.cpp (4)
27-27: Correct handling ofREFLECTION_STORAGE_PTRin CreateInstance.The expanded kind check and conditional
ClearElementsskip are correct—storage pointer arrays have externally-managed memory that shouldn't be zeroed by the runtime.Also applies to: 60-65
91-119:CreateInstanceWithStorageimplementation is sound.The approach of creating a zero-length array first (to avoid allocating unused backing storage), then setting
m_StoragePointerand adjustingm_numOfElements, is efficient and correct. The relevant code snippet from the header confirms the pattern is used consistently.
126-127: Enhanced token resolution with generic context.Passing
contextTypeSpectoResolveTokenenables proper resolution of generic type parameters (VAR/MVAR) when creating arrays in generic contexts. This aligns with the broader PR changes for generic context propagation.Also applies to: 138-138
172-176: Defensive guard inClearElementsfor storage pointers.Returning
CLR_E_WRONG_TYPEwhen attempting to clear elements of a storage-pointer-backed array is the correct behavior—external storage shouldn't be modified by the runtime's array clearing logic.src/CLR/Core/TypeSystem.cpp (9)
543-551: CMOD_REQD/CMOD_OPT handling in SignatureParser::Advance looks correctSkipping CMOD_REQD/CMOD_OPT by consuming the TypeDefOrRef coded index with
CLR_TkFromStream(Signature)and looping to the actual type is consistent with ECMA-335 encoding and reuses the existing token reader correctly. No issues here.
2263-2286: MethodSpec::GetGenericArgument change to use SignatureParser Element looks fineSwitching
CLR_RT_MethodSpec_Instance::GetGenericArgumentto return aCLR_RT_SignatureParser::Element &elementand iterating withparser.Advance(element)up toargumentPositionis consistent with how generics are parsed elsewhere in this file. Bounds checking viaargumentPosition >= parser.ParamCountis correct. No issues here.
2593-2653: InitializeFromSignatureToken: context-aware TypeSpec handling generally soundIn
CLR_RT_TypeDescriptor::InitializeFromSignatureToken, the updatedTBL_TypeSpechandling:
- Correctly special-cases
DATATYPE_VARwithcaller->arrayElementTypefor SZArrayHelper and then falls back to resolving againstcaller->genericTypeviaGetGenericParam.- Handles
DATATYPE_MVARby mapping throughFindGenericParamAtMethodDefand the generic-param cross-reference.- For
DATATYPE_GENERICINST, delegates toInitializeFromSignatureParser(parser, contextTypeSpec)so that VARs inside the instantiation can be resolved using the caller’s closed generic context.Given how
CLR_RT_SignatureParser::Advanceworks for TypeSpec signatures, the extra initialparser.Advance(elem)before delegating is consistent with existing patterns in this file. Overall this looks consistent with the generics behavior described in the retrieved learnings.
5235-5311: ResolveAllocateGenericTypeStaticFields: hash usage with new ComputeHash overload is OK
ResolveAllocateGenericTypeStaticFieldsstill callsComputeHashForClosedGenericType(genericTypeInstance)without a context. With the new signature that accepts optionalcontextTypeSpec/contextMethod, this implicitly uses the default arguments (nullptrs), which is appropriate here because metadata TypeSpecs for preallocated generic statics should already be fully closed without needing runtime context. No changes needed.
6507-6517: Relocating generic static fields in CLR_RT_Assembly::Relocate is correctThe added loop:
for (CLR_UINT32 i = 0; i < g_CLR_RT_TypeSystem.m_genericStaticFieldsCount; i++) { CLR_RT_GarbageCollector::RelocateGenericStaticField(&g_CLR_RT_TypeSystem.m_genericStaticFields[i]); }ensures that all generic static field records are updated during GC relocation, matching how per-assembly
staticFieldsare already handled. This is required for correctness once generic static storage is heap-allocated. Looks good.
6528-6585: TypeSystem_Initialize/Cleanup generic registries are consistent and leak-freeInitializing and tearing down:
m_genericStaticFields,m_genericStaticFieldsCount,m_genericStaticFieldsMaxCountm_genericCctorRegistry,m_genericCctorRegistryCount,m_genericCctorRegistryMaxCountand freeing
m_genericStaticFields[i].m_fieldDefsinTypeSystem_Cleanupis consistent with howplatform_mallocis used elsewhere in the generic static field code. This should avoid leaks when the type system is torn down. No issues.
8388-8498: FindOrCreateGenericStaticFields: good reuse, but double-alloc path is carefully handledThe new
CLR_RT_TypeSystem::FindOrCreateGenericStaticFields:
- Reuses an existing record when the hash matches.
- Grows the registry with
platform_mallocand copies old entries when needed.- Allocates static field storage via
ExtractHeapBlocksForObjectsand field-def mapping viaplatform_malloc.- Correctly initializes each field via
InitializeReference.The out-of-memory path for
pFieldDefsresets the just-allocated heap blocks tonullptrso they can be reclaimed by GC later, which is reasonable given there’s no explicit free forExtractHeapBlocksForObjects. Overall this is careful and consistent with the rest of the generic static field machinery.
8501-8621: ComputeHashForClosedGenericType: context-aware VAR/MVAR resolution is solidThe new
ComputeHashForClosedGenericTypeoverload:
- Starts with the generic type definition token.
- Walks each generic argument via
CLR_RT_SignatureParser.- For
DATATYPE_VAR, resolves viacontextTypeSpecandGetGenericParam.- For
DATATYPE_MVAR, resolves viacontextMethod->methodSpecandMethodSpec::GetGenericArgument, and can defer back to VAR resolution when the method argument is itself a VAR.- Produces 0 on unresolvable VAR/MVAR, which the callers treat as “not supported”.
This ties in correctly with both the metadata preallocated path (which calls with null context) and the on-demand path from
AllocateGenericStaticFieldsOnDemand(which passes both context parameters). The logic matches the VAR/MVAR workflows described in your previous learnings. No issues spotted.
8067-8090: OwnerIndex() on MethodRef refers to the declaring type, not a method definitionPer ECMA-335 II.22, the OwnerIndex (Class column) on a MemberRef/MethodRef is a MemberRefParent coded-index that encodes the declaring type (typically TypeRef, TypeDef, or TypeSpec), not a method definition row. The technical concern in the review is valid: using
mr->OwnerIndex()directly as a MethodDef index would be incorrect and could cause out-of-range access or incorrect name resolution.The suggestion to use
crossReferenceMethodRef[mri.Method()].target.datafor proper MethodRef-to-MethodDef mapping should be verified against existing patterns inResolveMethodRefand confirmed to align with the codebase's metadata resolution strategy.src/CLR/Core/Execution.cpp (1)
1716-1724: Explicitly zeroingm_StoragePointeron normal array allocation looks correctInitializing
pArray->m_StoragePointer = 0ensures non–storage-backed arrays start in a well-defined state, which makes later checks againstm_StoragePointer != 0(e.g., debugger/storage-aware paths) reliable without relying on implicit zeroing.src/CLR/CorLib/corlib_native_System_Span_1.cpp (3)
9-107: Pointer-basedSpan<T>(void* pointer, int length)ctor is consistent with ReadOnlySpan and validates T correctlyThe new ctor mirrors the ReadOnlySpan version: it parses the generic instantiation, rejects reference/contains-references element types via
RuntimeHelpers::CheckReferenceOrContainsReferences, validates length andDATATYPE_PTR, and then usesCreateInstanceWithStorage. This matches the intended unsafe span semantics.
124-143:CopyToreimplementation withmemmoveand GC protection looks correctThe updated
CopyTo:
- Checks destination span is large enough.
- Fast-paths the zero-length case.
- Protects both source and destination arrays from GC movement.
- Uses
memmoveoverlength * elementSizebytes, which correctly handles overlapping spans.Given that both spans are
Span<T>with the sameT, relying onm_sizeOfElementfrom the source array is safe here.
148-183:NativeSpanConstructorstores raw pointers into managed arrays without confirmed retention or GC awarenessThe implementation stores
ptrToStartElement(a rawuintptr_t) viaCreateInstanceWithStorage()but does not demonstrate that:
- The source array is pinned or retained as a managed reference to prevent collection
- The GC/runtime relocates
m_StoragePointerduring heap compaction (nanoCLR uses a compacting GC, and raw addresses are not automatically updated)Without these safeguards, spans constructed from managed arrays risk:
- Immediate collection if the source array has no other references (e.g.,
new Span<int>(new int[10], 2, 5))- Dangling pointers after heap compaction moves the source array
Additionally, range validation on
startandlengthis now the caller's responsibility (no native-side bounds checking visible).Verify that the source array lifetime is managed (pinned or held via a tracked reference in the span's backing structure) and that
m_StoragePointeris relocated by the GC during compaction, or consider representing managed-array spans with explicit array references and offsets instead.src/CLR/CorLib/corlib_native_System_ReadOnlySpan_1.cpp (2)
9-106: Pointer-basedReadOnlySpan<T>(void* pointer, int length)ctor matches Span and enforces non-reference TThis ctor mirrors the Span implementation: it validates the generic instantiation, rejects reference/contains-references element types, checks length and
DATATYPE_PTR, and usesCreateInstanceWithStorage. That’s appropriate for an unsafe ReadOnlySpan over unmanaged memory.
126-143:NativeReadOnlySpanConstructorshares the same GC and compaction risks asNativeSpanConstructorThe span is backed by a storage-pointer array:
uintptr_t ptrToStartElement = (uintptr_t)sourceArray->GetElement(start);CreateInstanceWithStorage(refArray, length, ptrToStartElement, sourceType);Risks identified:
- The backing array stores only
m_StoragePointerinto the source array's data without a managed reference to the sourceT[]- If the original array has no other references, it can be collected while the ReadOnlySpan still exists, leaving
m_StoragePointerdangling- Heap compaction can move the source array, but
m_StoragePointeris not a tracked reference and will not be updatedVerify whether
REFLECTION_STORAGE_PTRarrays keep the source array alive and relocatem_StoragePointerduring compaction. If not, consider representing spans over arrays with real managedT[]references (and offsets) or augmenting GC support for storage-backed arrays over managed memory.
josesimoes
changed the title
- Update declaration and callers. - Rework code to resolve VAR and MVAR of TypeSpec using it.
Automated fixes for code style.
…524bd-c4af-408e-8163-5940f64d18ea Code style fixes for nanoframework/nf-interpreter PR#3242
2 participants
Description
Span<T>andReadOnlySpan<T>.Motivation and Context
stackallocinstruction.Span<T>andReadOnlySpan<T>CoreLibrary#260.How Has This Been Tested?
Span<T>andReadOnlySpan<T>CoreLibrary#260].Screenshots
Types of changes
Checklist
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.