Fix loading token for TypeSpec #3189
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 update the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant HeapBlock
participant TypeSystem
Caller->>HeapBlock: SetReflection(tsInst, caller)
HeapBlock->>HeapBlock: Initialize as reflection type
HeapBlock->>HeapBlock: Parse tsInst with SignatureParser
alt First element is generic parameter
HeapBlock->>TypeSystem: Resolve generic parameter using caller
TypeSystem-->>HeapBlock: Return mapped type argument
HeapBlock->>HeapBlock: Set reflection data accordingly
else No valid caller or not generic parameter
HeapBlock->>HeapBlock: Assert failure (TODO)
end
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 (
|
|
@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: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/CLR/Core/CLR_RT_HeapBlock.cpp(1 hunks)src/CLR/Core/CLR_RT_HeapBlock_Array.cpp(1 hunks)src/CLR/Core/Interpreter.cpp(1 hunks)src/CLR/Diagnostics/Info.cpp(2 hunks)src/CLR/Include/nanoCLR_Runtime__HeapBlock.h(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- 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 (8)
src/CLR/Core/Interpreter.cpp (2)
3213-3214: LGTM: Variable renaming improves code clarity.The renaming from
sigtotsInstmakes the variable purpose more explicit and aligns with the naming conventions used elsewhere in the codebase for TypeSpec instances.
3219-3219: Correct implementation of generic context passing.The addition of
stack->m_call.genericTypeas the second parameter toSetReflectionproperly provides the caller's generic type context, which is essential for correct TypeSpec token resolution in generic scenarios. This change aligns perfectly with the PR objective to ensure reflection is set with the instantiated type.src/CLR/Core/CLR_RT_HeapBlock_Array.cpp (1)
107-107: LGTM: Method call correctly updated for new signature.The addition of
caller->genericTypeas the second parameter provides the necessary generic type context for proper TypeSpec resolution, which aligns with the PR's objective to fix generic type handling in reflection operations.src/CLR/Include/nanoCLR_Runtime__HeapBlock.h (1)
1350-1350: Verify all call sites have been updated for the new method signature.The new signature with
CLR_RT_TypeSpec_Instanceand optional caller parameter is well-designed for improved generic type resolution. Ensure all existing call sites across the codebase have been updated to match this signature change.#!/bin/bash # Description: Verify all SetReflection call sites use the new signature with two parameters # Search for SetReflection calls and check their parameter count rg -A 5 -B 2 "\.SetReflection\(" --type cpp --type csrc/CLR/Diagnostics/Info.cpp (3)
538-538: LGTM! Consistent assembly index usage.The change to use
assemblyIndexinstead ofownerAsmensures consistency with the local context and aligns with the TypeSpec token handling improvements.
579-587: Enhanced type name formatting with proper initialization.The changes improve type name construction by:
- Zero-initializing the buffer to ensure clean state
- Adding
CLR_RT_TypeSystem::TYPENAME_FLAGS_FULLflag for complete type information- Including
elem.Levelsparameter for proper array/pointer level handlingThese enhancements align well with the PR objective of improving TypeSpec token handling for better reflection accuracy.
575-576: Verify the assembly access pattern change.The change from
tsInst.assemblyto accessing viag_CLR_RT_TypeSystem.m_assemblies[genericType->Assembly() - 1]appears to improve reliability of assembly resolution. However, this introduces potential array bounds issues ifgenericType->Assembly()returns 0 or an invalid index.Run the following script to verify that assembly indices are properly validated throughout the codebase:
#!/bin/bash # Description: Search for similar assembly access patterns and bounds checking # Search for assembly access patterns via m_assemblies array rg -A 5 -B 5 'g_CLR_RT_TypeSystem\.m_assemblies\[.*Assembly\(\)' # Search for bounds checking on Assembly() calls rg -A 3 -B 3 'Assembly\(\)\s*-\s*1' # Look for assembly index validation patterns ast-grep --pattern 'if ($COND) { $$$ Assembly() - 1 $$$ }'src/CLR/Core/CLR_RT_HeapBlock.cpp (1)
235-235: Consider adding parameter validation for safer generic type resolution.The method signature looks correct for the intended TypeSpec handling improvements, but consider adding parameter validation to prevent potential issues.
src/CLR/Diagnostics/Info.cpp
Outdated
| const CLR_RECORD_TYPESPEC *rec = GetTypeSpec(token); | ||
| CLR_RT_TypeSpec_Index localSpec; | ||
| localSpec.Set(assemblyIndex, token); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused local variables.
The newly introduced variables rec and localSpec are declared but never used in the visible code. This creates unnecessary memory allocation and potential confusion.
Apply this diff to remove the unused variables:
- const CLR_RECORD_TYPESPEC *rec = GetTypeSpec(token);
- CLR_RT_TypeSpec_Index localSpec;
- localSpec.Set(assemblyIndex, token);📝 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.
| const CLR_RECORD_TYPESPEC *rec = GetTypeSpec(token); | |
| CLR_RT_TypeSpec_Index localSpec; | |
| localSpec.Set(assemblyIndex, token); |
🤖 Prompt for AI Agents
In src/CLR/Diagnostics/Info.cpp around lines 533 to 535, the variables 'rec' and
'localSpec' are declared but never used, leading to unnecessary memory usage and
confusion. Remove the declarations and any related assignments of these two
variables to clean up the code.
src/CLR/Core/CLR_RT_HeapBlock.cpp
Outdated
| else | ||
| { | ||
| // TODO | ||
| _ASSERTE(false); | ||
| } |
There was a problem hiding this comment.
Implement handling for non-generic parameter types.
The TODO section for non-generic parameter types with _ASSERTE(false) indicates incomplete implementation. This will fail for any non-generic TypeSpec.
Based on the previous implementation that used CLR_RT_TypeDescriptor, consider implementing:
- else
- {
- // TODO
- _ASSERTE(false);
- }
+ else
+ {
+ // Handle non-generic types using TypeDescriptor approach
+ CLR_RT_TypeDescriptor desc;
+ if (SUCCEEDED(desc.InitializeFromTypeSpec(tsInst)))
+ {
+ m_data.reflection.data.type = desc.m_handlerCls;
+ m_data.reflection.levels = elem.Levels;
+ }
+ else
+ {
+ NANOCLR_SET_AND_LEAVE(CLR_E_WRONG_TYPE);
+ }
+ }Do you want me to help complete the implementation for non-generic types based on the previous TypeDescriptor approach?
📝 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.
| else | |
| { | |
| // TODO | |
| _ASSERTE(false); | |
| } | |
| else | |
| { | |
| // Handle non-generic types using TypeDescriptor approach | |
| CLR_RT_TypeDescriptor desc; | |
| if (SUCCEEDED(desc.InitializeFromTypeSpec(tsInst))) | |
| { | |
| m_data.reflection.data.type = desc.m_handlerCls; | |
| m_data.reflection.levels = elem.Levels; | |
| } | |
| else | |
| { | |
| NANOCLR_SET_AND_LEAVE(CLR_E_WRONG_TYPE); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/CLR/Core/CLR_RT_HeapBlock.cpp around lines 278 to 282, the else block
handling non-generic parameter types is incomplete and currently asserts false,
which will cause failures. Replace the _ASSERTE(false) with proper handling
logic for non-generic TypeSpec parameters by implementing the approach used
previously with CLR_RT_TypeDescriptor. This involves creating a TypeDescriptor
instance for the parameter and processing it accordingly to support non-generic
types fully.
| else | ||
| { | ||
| // TODO | ||
| _ASSERTE(false); | ||
| } |
There was a problem hiding this comment.
Remove TODO section with always-failing assertion.
The TODO comment with _ASSERTE(false) will always trigger in debug builds when a caller is not provided for generic parameters, making this code path unusable.
- else
- {
- // TODO
- _ASSERTE(false);
- }
+ else
+ {
+ // No caller provided for generic parameter resolution
+ NANOCLR_SET_AND_LEAVE(CLR_E_WRONG_TYPE);
+ }📝 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.
| else | |
| { | |
| // TODO | |
| _ASSERTE(false); | |
| } | |
| else | |
| { | |
| // No caller provided for generic parameter resolution | |
| NANOCLR_SET_AND_LEAVE(CLR_E_WRONG_TYPE); | |
| } |
🤖 Prompt for AI Agents
In src/CLR/Core/CLR_RT_HeapBlock.cpp around lines 272 to 276, remove the TODO
comment and the _ASSERTE(false) statement that always triggers in debug builds.
Replace this section with proper handling or a more appropriate fallback to
avoid unusable code paths when a caller is not provided for generic parameters.
| if (ok) | ||
| { | ||
| m_data.reflection.data.type = tdArg; | ||
| m_data.reflection.levels = elem.Levels; | ||
| } |
There was a problem hiding this comment.
Handle failed generic parameter resolution properly.
When FindGenericParamAtTypeSpec returns false (ok == false), the reflection data is left uninitialized, potentially leaving the heap block in an inconsistent state.
if (ok)
{
m_data.reflection.data.type = tdArg;
m_data.reflection.levels = elem.Levels;
}
+ else
+ {
+ NANOCLR_SET_AND_LEAVE(CLR_E_TYPE_UNAVAILABLE);
+ }📝 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.
| if (ok) | |
| { | |
| m_data.reflection.data.type = tdArg; | |
| m_data.reflection.levels = elem.Levels; | |
| } | |
| if (ok) | |
| { | |
| m_data.reflection.data.type = tdArg; | |
| m_data.reflection.levels = elem.Levels; | |
| } | |
| else | |
| { | |
| NANOCLR_SET_AND_LEAVE(CLR_E_TYPE_UNAVAILABLE); | |
| } |
🤖 Prompt for AI Agents
In src/CLR/Core/CLR_RT_HeapBlock.cpp around lines 266 to 270, the code does not
handle the case when FindGenericParamAtTypeSpec returns false, leaving
reflection data uninitialized. Add proper handling for the failure case by
initializing or resetting the reflection data fields to a safe default state to
avoid inconsistent heap block state.
| bool ok = g_CLR_RT_TypeSystem.m_assemblies[caller->Assembly() - 1] | ||
| ->FindGenericParamAtTypeSpec(caller->TypeSpec(), gpIndex, tdArg, dtArg); |
There was a problem hiding this comment.
Add bounds checking to prevent array access violation.
The code accesses g_CLR_RT_TypeSystem.m_assemblies[caller->Assembly() - 1] without bounds checking. If Assembly() returns 0, this will cause an underflow and potential memory corruption.
+ if (caller->Assembly() == 0 || caller->Assembly() > g_CLR_RT_TypeSystem.m_assemblies.size())
+ {
+ NANOCLR_SET_AND_LEAVE(CLR_E_WRONG_TYPE);
+ }
bool ok = g_CLR_RT_TypeSystem.m_assemblies[caller->Assembly() - 1]
->FindGenericParamAtTypeSpec(caller->TypeSpec(), gpIndex, tdArg, dtArg);📝 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.
| bool ok = g_CLR_RT_TypeSystem.m_assemblies[caller->Assembly() - 1] | |
| ->FindGenericParamAtTypeSpec(caller->TypeSpec(), gpIndex, tdArg, dtArg); | |
| if (caller->Assembly() == 0 || caller->Assembly() > g_CLR_RT_TypeSystem.m_assemblies.size()) | |
| { | |
| NANOCLR_SET_AND_LEAVE(CLR_E_WRONG_TYPE); | |
| } | |
| bool ok = g_CLR_RT_TypeSystem.m_assemblies[caller->Assembly() - 1] | |
| ->FindGenericParamAtTypeSpec(caller->TypeSpec(), gpIndex, tdArg, dtArg); |
🤖 Prompt for AI Agents
In src/CLR/Core/CLR_RT_HeapBlock.cpp around lines 264 to 265, add bounds
checking before accessing g_CLR_RT_TypeSystem.m_assemblies with index
caller->Assembly() - 1. Verify that caller->Assembly() is greater than 0 and
less than or equal to the size of m_assemblies to prevent underflow and
out-of-bounds access. Only perform the array access if these conditions are met;
otherwise, handle the error or return safely.
- Parsing a typespec when dumping an ldtoken now outputs correct type. - Parsing a typespec when executing ldtoken IL now sets reflection with instanciated type. - Add parameter with caller to SetReflection from TypeSpec. - Fix calls.
josesimoes
force-pushed
the
fix-ldtoken-typespec
branch
from
522b99f to
a26cd9d
Compare
Description
Motivation and Context
How Has This Been Tested?
Span<T>.[build with MDP buildId 56229]
Screenshots
Types of changes
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Refactor