Fix handling generic type in several IL #3172
Conversation
josesimoes
added
the
Area: Interpreter
|
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 changes introduce enhanced type resolution and type safety features in the runtime, focusing on generic types and fields. New methods for comparing type descriptors, resolving tokens with generic context, and performing type-safe assignments are added. Existing logic for field and array operations is updated to use these new mechanisms, improving correctness in generic and type-sensitive scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant IL_Interpreter
participant ExecutionEngine
participant HeapBlock
participant TypeSystem
IL_Interpreter->>TypeSystem: ResolveToken(token, assm, caller)
TypeSystem-->>IL_Interpreter: TypeDef/FieldDef/TypeSpec
IL_Interpreter->>ExecutionEngine: IsInstanceOfToken(token, obj, caller)
ExecutionEngine->>TypeSystem: InitializeFromSignatureToken(token, caller)
TypeSystem-->>ExecutionEngine: TypeDescriptor (expected)
ExecutionEngine->>HeapBlock: GetTypeDescriptor(obj)
HeapBlock-->>ExecutionEngine: TypeDescriptor (actual)
ExecutionEngine->>ExecutionEngine: TypeDescriptorsMatch(expected, actual)
ExecutionEngine-->>IL_Interpreter: true/false
IL_Interpreter->>HeapBlock: Reassign(rhs, expectedType)
HeapBlock->>HeapBlock: TypeDescriptorsMatch(expected, actual)
HeapBlock-->>IL_Interpreter: HRESULT (success or type error)
sequenceDiagram
participant IL_Interpreter
participant TypeSystem
IL_Interpreter->>TypeSystem: ResolveToken for field/array (with caller context)
TypeSystem-->>IL_Interpreter: FieldDef/TypeDef/TypeDescriptor
IL_Interpreter->>IL_Interpreter: Store/Load array element with type check
IL_Interpreter->>ExecutionEngine: IsInstanceOfToken(token, value, caller)
ExecutionEngine-->>IL_Interpreter: true/false
IL_Interpreter->>IL_Interpreter: Store value if type matches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
josesimoes
force-pushed
the
fix-IL-generics
branch
from
bbf4af9 to
af9f497
Compare
|
@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: 9
🧹 Nitpick comments (6)
src/CLR/Core/Interpreter.cpp (1)
2964-2975: Unused local after refactor – consider pruning
clsis still populated viaExtractTypeIndexFromObject, but the value is no longer used after switching toIsInstanceOfToken.
If the sole purpose of the call is the HRESULT check, you can keep the call and discard the variable:- CLR_RT_TypeDef_Index cls; - ... - NANOCLR_CHECK_HRESULT(CLR_RT_TypeDescriptor::ExtractTypeIndexFromObject(evalPos[0], cls)); + NANOCLR_CHECK_HRESULT( + CLR_RT_TypeDescriptor::ExtractTypeIndexFromObject(evalPos[0], CLR_RT_TypeDef_Index()));(or cast to
(void)cls;in non-debug builds). This avoids “unused-variable” warnings with stricter compilers.src/CLR/Core/Execution.cpp (1)
3220-3250:IsInstanceOfToken()– missing null-object pathNice helper! Consider an early–out for the common
nullcase to avoid an unnecessaryInitializeFromObjectfailure:+ if (obj.DataType() == DATATYPE_OBJECT && obj.Dereference() == nullptr) + { + return false; // null never “is‐a” any type + }This avoids converting a
nullptrintoCLR_E_NULL_REFERENCEinsideInitializeFromObject, and keeps the method strictly boolean.src/CLR/Include/nanoCLR_Runtime.h (3)
1425-1429: Consider returningCLR_RT_GenericParam_Indexinstead of multiple out‐parameters
FindGenericParamAtTypeSpec()returns aboolfor success and then fills two out-params (typeDef,dataType).
Because the method is used to locate a single generic parameter, you could simplify its contract and avoid accidental misuse by:
- Returning the already existing
CLR_RT_GenericParam_Index(or a dedicated struct) that fully describes the parameter found;- Conveying failure with
NANOCLR_INDEX_IS_INVALID()instead of a separate Boolean.This makes call-sites less error-prone and keeps the API aligned with the other
Find*helpers that already return indices.
Feel free to keep the current shape if binary compatibility is required, but consider adding the more expressive overload for new code.
2340-2344: Missingnullptrdefault for newcallerparameter
InitializeFromSignatureToken()now needscaller. When the token comes from metadata outside of a method body, the caller is naturallynullptr.
To avoid sprinkling explicitnullptrat every call-site, add an in-header default:HRESULT InitializeFromSignatureToken( CLR_RT_Assembly *assm, CLR_UINT32 token, const CLR_RT_MethodDef_Instance *caller = nullptr);This is safe – overload resolution remains unchanged and existing updated calls still compile.
4071-4076: Thread-safety / re-entrancy of new instance helpers inExecutionEngine
IsInstanceOfToken()is an instance method, but it only queries metadata and HeapBlock state.
Because EE methods are often invoked concurrently from GC, interpreter and debugger threads, confirm that:
- No internal state is mutated (looks read-only – good).
- The helper is callable while a GC is in progress (
m_heapState == c_HeapState_UnderGC).- The method deliberately avoids the global
s_compensation.Suspend()that other slow helpers use.If any of these invariants can be violated, mark the method
constand/or sprinkle the usualNANOCLR_FOREACH_ASSEMBLYguards.src/CLR/Core/TypeSystem.cpp (1)
1134-1212:FieldDef::ResolveToken– logic OK but leaks on failure pathWhen
FindFieldDef(...)fails the function returnsfalsewithout resetting the partially-initialised instance, leaving the caller with a half-populated object.
InsertClearInstance();before each earlyreturn false;to preserve previous behaviour:if (!assm->FindFieldDef(...)) { - return false; + ClearInstance(); + return false; }Minor, but avoids subtle issues if the same instance is re-used.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/CLR/Core/CLR_RT_HeapBlock.cpp(2 hunks)src/CLR/Core/Execution.cpp(2 hunks)src/CLR/Core/Interpreter.cpp(10 hunks)src/CLR/Core/TypeSystem.cpp(5 hunks)src/CLR/Debugger/Debugger.cpp(1 hunks)src/CLR/Include/nanoCLR_Runtime.h(4 hunks)src/CLR/Include/nanoCLR_Runtime__HeapBlock.h(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/CLR/Core/Execution.cpp (1)
src/CLR/Core/CLR_RT_HeapBlock.cpp (2)
TypeDescriptorsMatch(820-906)TypeDescriptorsMatch(820-822)
⏰ Context from checks skipped due to timeout of 90000ms (18)
- 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_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_NXP_targets NXP_MIMXRT1060_EVK)
- GitHub Check: nf-interpreter (Build_STM32_targets ST_STM32F429I_DISCOVERY)
- GitHub Check: nf-interpreter (Build_TI_SimpleLink_targets TI_CC1352R1_LAUNCHXL_915)
- GitHub Check: nf-interpreter (Check_Code_Style)
- 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 (9)
src/CLR/Debugger/Debugger.cpp (1)
2861-2861: Fixed off-by-one error in generic parameter iteration.The loop condition was changed from
<to<=to ensure the parser correctly advances to include the generic parameter at positionres.GenericParamPosition. This fixes an off-by-one error that would have caused the parser to stop one position short of the target generic parameter.This change aligns with similar fixes in the codebase for handling generic parameters and ensures proper type resolution during debugging.
src/CLR/Include/nanoCLR_Runtime__HeapBlock.h (2)
1363-1363: Strong addition to enhance type safety during heap block reassignment.This method overload adds an important type-checking capability to
Reassignoperations, allowing the runtime to verify that the source object matches the expected type before performing the assignment. This enhances type safety when working with generic types.
1375-1375: Good addition for robust generic type comparison.This static utility method provides a critical foundation for comparing type descriptors, which is essential for proper type checking with generics. It will help ensure type safety across operations involving open and closed generic types.
src/CLR/Core/Interpreter.cpp (2)
2534-2541: Context-awareResolveToken()call is a welcome fixPassing
&stack->m_callintoCLR_RT_FieldDef_Instance::ResolveTokenenables the runtime to resolve open/closed generic fields correctly. The change is straightforward and does not alter control flow – nice job!
2645-2652:DATATYPE_GENERICINSTis now handled – great coverageAdding
case DATATYPE_GENERICINST:to thestfldswitch ensures generic‐instance fields are treated the same way as normal class/value-type fields. This eliminates a whole class of “invalid type” exceptions that could surface when storing into generic objects.src/CLR/Core/CLR_RT_HeapBlock.cpp (2)
777-818: Good implementation of type-safe assignment with robust type checks.The
Reassignoverload provides strong type safety by explicitly checking if the runtime type ofrhsmatches the expected type before performing the assignment. This will help prevent type errors in generic scenarios.However, I noticed that line 812 has commented-out code (
// this->CopyFrom(rhs);) suggesting that value-type copying isn't fully implemented yet. This could be an issue if these methods are called with value types.Is the value-type copying intentionally commented out? Should there be additional code to handle value types and primitives properly?
820-906: Well-structured type descriptor comparison with comprehensive type handling.The
TypeDescriptorsMatchmethod implements detailed logic to compare type descriptors, with special handling for:
- Generic instance promotion and matching
- Primitive type equivalence checks
- Boxed struct type handling
- TypeDef-based comparison for classes and value types
This approach ensures correct type compatibility checking across various scenarios, particularly for generic types.
src/CLR/Include/nanoCLR_Runtime.h (1)
2134-2138: Potential breaking change:ResolveTokennow requires caller context
CLR_RT_FieldDef_Instance::ResolveToken()gained a mandatoryconst CLR_RT_MethodDef_Instance *callerparameter.
Down-stream code that invokes this helper without the extra argument will no longer compile. Double-check:
- All existing invocations inside
Core/andInterpreter/were updated (grep for.ResolveToken().- External/native partners (e.g. HAL, test utilities) are built in CI or clearly flagged as needing updates.
If compatibility is a concern, you can provide an inline overload that forwards
nullptrfor legacy code:bool ResolveToken(CLR_UINT32 tk, CLR_RT_Assembly *assm) { return ResolveToken(tk, assm, /*caller*/ nullptr); }src/CLR/Core/TypeSystem.cpp (1)
1746-1827: Large token switch needs exhaustive error handling
InitializeFromSignatureToken()mixesNANOCLR_SET_AND_LEAVEwith barereturns and missing checks.
Example: ifparser.Initialize_TypeSpec(...)fails the laterparser.Advance(elem)will operate on an invalid parser but no error is raised.Audit each path and wrap parser calls with
NANOCLR_CHECK_HRESULT, or factor the body into smaller helpers so that the macro usage mirrors the rest of the file.Failing to do so can leave the descriptor partially initialised and cause follow-up crashes during execution.
| return TypeDescriptorsMatch(expectedType, actualType); | ||
| } |
There was a problem hiding this comment.
Potential recursive call issue in array element type comparison.
This line appears to be making a recursive call to TypeDescriptorsMatch with the same parameters, which could lead to infinite recursion. For array element type comparison, you typically need to compare the element types, not the array types again.
Consider modifying to compare element types instead:
- return TypeDescriptorsMatch(expectedType, actualType);
+ CLR_RT_TypeDescriptor expectedElementType, actualElementType;
+ expectedType.GetElementType(expectedElementType);
+ actualType.GetElementType(actualElementType);
+ return TypeDescriptorsMatch(expectedElementType, actualElementType);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return TypeDescriptorsMatch(expectedType, actualType); | |
| } | |
| CLR_RT_TypeDescriptor expectedElementType, actualElementType; | |
| expectedType.GetElementType(expectedElementType); | |
| actualType.GetElementType(actualElementType); | |
| return TypeDescriptorsMatch(expectedElementType, actualElementType); | |
| } |
There was a problem hiding this comment.
There is no GetElementType() API in CLR_RT_TypeDescriptor .
I believe this is OK as it is... if you don't have a better suggestion let's leave it as it is.
There was a problem hiding this comment.
@josesimoes Thank you for clarifying that there's no GetElementType() API available. You're right that my suggestion wouldn't work without that API.
However, I still want to caution about the current implementation, as it calls itself recursively with the same parameters:
return TypeDescriptorsMatch(expectedType, actualType);This could potentially lead to infinite recursion if both types are arrays. If you're confident this won't be an issue in your implementation (perhaps because of other constraints that prevent this case), then it's reasonable to leave as is.
Alternatively, you might consider adding explicit element type extraction logic in this case block if needed for correctness.
✏️ Learnings added
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3172
File: src/CLR/Core/CLR_RT_HeapBlock.cpp:899-900
Timestamp: 2025-05-14T16:27:02.525Z
Learning: The CLR_RT_TypeDescriptor type in nanoFramework doesn't have a GetElementType() API for extracting array element types.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
- Rework handlers for ldfld, ldflda, stfld, ldsflda, ldelem, stelem and ldtoken to perperly deal with opened and closed generic types and instances. - Add several new helper APIs to HeapBlock, CLR_RT_TypeDef_Instance, CLR_RT_FieldDef_Instance, CLR_RT_TypeDescriptor and CLR_RT_Assembly to support the latter. - Fix crawling signatures for generic parameters in several places.
josesimoes
force-pushed
the
fix-IL-generics
branch
from
af9f497 to
1c96e58
Compare
Automated fixes for code style.
josesimoes
force-pushed
the
fix-IL-generics
branch
from
ae5b668 to
8f5db09
Compare
Description
Motivation and Context
How Has This Been Tested?
[build with MDP buildId 55899]
Screenshots
Types of changes
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Refactor