Implement spawning static constructor for closed generic types #3235
Conversation
- Add new spawn helper for static constructors of closed generic types. - Adjust existing SpawnStaticConstructor() to handle these. - CLR_RT_HeapBlock_Delegate now stores a TypeDef index to be able to deal with generic types.
|
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 WalkthroughThe pull request extends static constructor execution to support closed generic types. Changes add a generic type specification field to delegates, introduce a new helper method to spawn generic static constructors, add an assembly flag to track generic constructor completion, and adjust thread context setup and delegate initialization for generic scenarios. Changes
Sequence Diagram(s)sequenceDiagram
actor Runtime
participant Exec as ExecutionEngine
participant Asm as Assembly
participant TypeSpec as TypeSpec Index
participant Thread as cctor Thread
Runtime->>Exec: SpawnStaticConstructor(delegate)
alt delegate has m_genericTypeSpec
Exec->>TypeSpec: Extract generic type info
Exec->>Exec: SpawnGenericTypeStaticConstructorsHelper()
loop For each TypeSpec
Exec->>Asm: HasStaticConstructor(generic type)
alt Has cctor
Exec->>Thread: Create & schedule delegate
Thread->>Thread: Set generic context from m_genericTypeSpec
end
end
Exec->>Asm: Mark StaticGenericConstructorsExecuted
else Regular static constructor
Exec->>Exec: SpawnStaticConstructorHelper()
Exec->>Thread: Schedule on cctor thread
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Automated fixes for code style.
…4b2a5-0a80-441d-a7be-d524cbc7c506 Code style fixes for nanoframework/nf-interpreter PR#3235
|
@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: 3
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/TypeSystem.cpp (1)
5988-5998: Guard against invalid TypeDef index; add bounds assert; consider making method const.If genericTypeDef is invalid, indexing m_assemblies[Assembly()-1] underflows and can crash. Also, early-return when there are no methods, and assert bounds to avoid OOB. This method doesn’t mutate state, so it can be const.
Apply:
-bool CLR_RT_Assembly::HasStaticConstructor(const CLR_RT_TypeDef_Index &genericTypeDef) +bool CLR_RT_Assembly::HasStaticConstructor(const CLR_RT_TypeDef_Index &genericTypeDef) /* could be const */ { NATIVE_PROFILE_CLR_CORE(); - CLR_RT_Assembly *ownerAsm = g_CLR_RT_TypeSystem.m_assemblies[genericTypeDef.Assembly() - 1]; - const CLR_RECORD_TYPEDEF *ownerTd = ownerAsm->GetTypeDef(genericTypeDef.Type()); + // Validate input to prevent underflow/null deref. + if (!NANOCLR_INDEX_IS_VALID(genericTypeDef)) + { + return false; + } + CLR_RT_Assembly *ownerAsm = g_CLR_RT_TypeSystem.m_assemblies[genericTypeDef.Assembly() - 1]; + if (ownerAsm == nullptr) + { + return false; + } + const CLR_RECORD_TYPEDEF *ownerTd = ownerAsm->GetTypeDef(genericTypeDef.Type()); + if (ownerTd == nullptr) + { + return false; + } - // Calculate total method count for this type - int methodCount = ownerTd->virtualMethodCount + ownerTd->instanceMethodCount + ownerTd->staticMethodCount; + // Calculate total method count for this type + const int firstMethod = ownerTd->firstMethod; + const int methodCount = ownerTd->virtualMethodCount + ownerTd->instanceMethodCount + ownerTd->staticMethodCount; + if (firstMethod == CLR_EmptyIndex || methodCount == 0) + { + return false; + } +#if !defined(BUILD_RTM) + _ASSERTE(firstMethod + methodCount <= ownerAsm->tablesSize[TBL_MethodDef]); +#endif // Iterate through all methods of the generic type definition - const CLR_RECORD_METHODDEF *md = ownerAsm->GetMethodDef(ownerTd->firstMethod); + const CLR_RECORD_METHODDEF *md = ownerAsm->GetMethodDef(firstMethod); for (int i = 0; i < methodCount; i++, md++) { // Check if this is a static constructor if (md->flags & CLR_RECORD_METHODDEF::MD_StaticConstructor) { return true; } } return false; }
🧹 Nitpick comments (1)
src/CLR/Core/Execution.cpp (1)
1148-1175: Consider gating generic phase on dependencies' generic phase completionThe code currently gates generic type static constructor execution only on whether dependencies have completed regular static constructors (checking
StaticConstructorsExecuted). To improve robustness and prevent surprises when generic static constructors depend on other generics' initialization, also check that all dependent assemblies have completed their generic phase (StaticGenericConstructorsExecuted). This mirrors the existing pattern for regular static constructor ordering. As noted, this can be relaxed if it causes deadlocks in legitimate dependency cycles.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/CLR/Core/CLR_RT_HeapBlock_Delegate.cpp(1 hunks)src/CLR/Core/Execution.cpp(3 hunks)src/CLR/Core/Thread.cpp(2 hunks)src/CLR/Core/TypeSystem.cpp(1 hunks)src/CLR/Include/nanoCLR_Runtime.h(3 hunks)src/CLR/Include/nanoCLR_Runtime__HeapBlock.h(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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__HeapBlock.hsrc/CLR/Core/CLR_RT_HeapBlock_Delegate.cpp
⏰ 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_WIN32_nanoCLR)
- GitHub Check: nf-interpreter (Build_Azure_RTOS_targets SL_STK3701A)
- 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_ESP32_targets ESP32_PSRAM_REV0)
- GitHub Check: nf-interpreter (Build_TI_SimpleLink_targets TI_CC1352R1_LAUNCHXL_915)
- GitHub Check: nf-interpreter (Build_NXP_targets NXP_MIMXRT1060_EVK)
- GitHub Check: nf-interpreter (Build_STM32_targets ST_STM32F429I_DISCOVERY)
- 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 (4)
src/CLR/Include/nanoCLR_Runtime.h (2)
1342-1342: New assembly flag for generic cctors — LGTMBit 0x00000040 doesn’t collide with existing flags. No issues.
4236-4239: Generic helper only compiled for !NANOCLR_APPDOMAINSClosed generic .cctors won’t run in APPDOMAINS builds with current guards. If AppDomains are supported in your targets, mirror this helper in the APPDOMAINS path or explicitly document the limitation.
src/CLR/Core/Execution.cpp (2)
969-972: Marking regular static constructors executed — LGTMFlagging at the point no more .cctors are found is correct and matches dependency checks later.
1091-1104: Delegate lifecycle and generic context are correctly preserved.The code at lines 1091-1104 is correct. Verification confirms:
m_genericTypeSpec persistence: Set once at Execution.cpp:1056 before the delegate is pushed, and never cleared during delegate invocation. It remains accessible when the callback reads it at line 1091.
Delegate validity: The delegate (m_dlg) is stored in the thread object and remains valid throughout the callback execution. It's only cleared at Thread.cpp:527 after the termination callback returns, ensuring it exists when StaticConstructorTerminationCallback accesses it at line 1091.
Sequence confirmation: The callback chain is: delegate thread proc → termination callback invocation → callback reads m_dlg and m_genericTypeSpec → delegate cleared after callback returns.
GC safety: The m_dlg field is properly marked for heap relocation and handled via Thread.Relocate(), maintaining validity through garbage collection cycles.
Automated fixes for code style.
…7950d-65a8-49af-8b6f-153d015047c0 Code style fixes for nanoframework/nf-interpreter PR#3235
2 participants
Description
Motivation and Context
How Has This Been Tested?
Screenshots
Types of changes
Checklist
Summary by CodeRabbit