Various fixes for generics support#3338
Merged
josesimoes merged 24 commits intoApr 23, 2026
Merged
Conversation
- Now properly resolving types in calls.
- Setting to OBJECT which will provide a null object ref.
- Required to prevent generic type from pointing to a caller's stack-local parameter. - Add code to use these.
Automated fixes for code style.
…88c4f-a34f-4599-a78a-537c7de3bc40 Code style fixes for nanoframework/nf-interpreter PR#3338
Contributor
📝 WalkthroughWalkthroughNormalize and persist generic-type storage across stack and inline frames; implement constrained. prefix handling for CALLVIRT with BYREF this; change MVAR handling to prefer MethodSpec resolution and treat unresolved MVARs as object during local initialization; adjust generic-context propagation and diagnostic resolution/printing. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Interpreter
participant StackFrame as "Stack/InlineFrame"
participant TypeSystem
participant VM as "Dispatch/Runtime"
Caller->>Interpreter: emit CALLVIRT with CEE_CONSTRAINED token
Interpreter->>StackFrame: read `this` (BYREF) and constrained token
alt constrained + BYREF and target is ref type
Interpreter->>StackFrame: dereference BYREF -> object ref into `this` slot
end
Interpreter->>TypeSystem: resolve callee generic context (filter effectiveCallerGeneric / methodSpec)
TypeSystem-->>Interpreter: resolved callee Instance (genericType/methodSpec normalized)
Interpreter->>StackFrame: Push/PushInline (copy/normalize generic-type storage)
Interpreter->>VM: perform virtual dispatch with normalized context
VM-->>Caller: return
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 (3)
src/CLR/Core/CLR_RT_StackFrame.cpp (1)
326-339:⚠️ Potential issue | 🟠 MajorMake inline-frame generic context save/restore self-contained.
PushInlinesavesm_genericTypeSpecStorage, but the savedm_call.genericTypecan still point at the live stack frame’s storage while the inlined callee may overwrite it.SaveStack/RestoreStackalso omit the new field entirely. Preserve the storage and normalize the pointer in everyCLR_RT_InlineFramesave/restore path.🛠️ Proposed fix
m_inlineFrame->m_frame.m_genericTypeSpecStorage = m_genericTypeSpecStorage; + if (m_inlineFrame->m_frame.m_call.genericType == &m_genericTypeSpecStorage) + { + m_inlineFrame->m_frame.m_call.genericType = &m_inlineFrame->m_frame.m_genericTypeSpecStorage; + } @@ m_genericTypeSpecStorage = m_inlineFrame->m_frame.m_genericTypeSpecStorage; // If the restored m_call.genericType pointed into m_genericTypeSpecStorage, fix the pointer. if (m_call.genericType == &m_inlineFrame->m_frame.m_genericTypeSpecStorage) { m_call.genericType = &m_genericTypeSpecStorage; } @@ m_localAllocCount = frame.m_localAllocCount; for (CLR_INT32 i = 0; i < c_Max_Localloc_Count; i++) { m_localAllocs[i] = frame.m_localAllocs[i]; } + m_genericTypeSpecStorage = frame.m_genericTypeSpecStorage; + if (m_call.genericType == &frame.m_genericTypeSpecStorage) + { + m_call.genericType = &m_genericTypeSpecStorage; + } } @@ frame.m_localAllocCount = m_localAllocCount; for (CLR_INT32 i = 0; i < c_Max_Localloc_Count; i++) { frame.m_localAllocs[i] = m_localAllocs[i]; } + frame.m_genericTypeSpecStorage = m_genericTypeSpecStorage; + if (frame.m_call.genericType == &m_genericTypeSpecStorage) + { + frame.m_call.genericType = &frame.m_genericTypeSpecStorage; + } }Also applies to: 426-480
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CLR/Core/CLR_RT_StackFrame.cpp` around lines 326 - 339, The inline-frame generic context isn't self-contained because PushInline saves m_genericTypeSpecStorage but leaves m_call.genericType pointing to the live frame's storage; update PushInline, SaveStack and RestoreStack so every CLR_RT_InlineFrame saves a copy of m_genericTypeSpecStorage into the inline frame (CLR_RT_InlineFrame::m_frame.m_genericTypeSpecStorage) and then normalize/reset the saved m_call.genericType to point at that copied storage (not the live m_genericTypeSpecStorage); likewise, when restoring, copy the saved genericTypeSpec storage back (or restore pointers) so restored m_call.genericType references the inline frame's storage consistently; apply same fixes to the other save/restore paths referenced around lines 426-480 to ensure all paths preserve and normalize the generic context pointer.src/CLR/Core/TypeSystem.cpp (2)
8250-8288:⚠️ Potential issue | 🔴 CriticalUse the effective generic context before dereferencing
genericType.This branch can be entered because
mdInst.genericTypeis valid while thegenericTypeparameter is null. If a MethodSpec argument isVAR, Line 8285 dereferences*genericTypeand can crash.🐛 Proposed fix
CLR_RT_TypeSpec_Instance contextTs; CLR_RT_SignatureParser::Element paramElement; + const CLR_RT_TypeSpec_Index *contextGenericType = + (mdInst.genericType != nullptr && NANOCLR_INDEX_IS_VALID(*mdInst.genericType)) + ? mdInst.genericType + : genericType; // try to resolve from method context - if (!contextTs.InitializeFromIndex(*genericType)) + if (contextGenericType == nullptr || !NANOCLR_INDEX_IS_VALID(*contextGenericType) || + !contextTs.InitializeFromIndex(*contextGenericType)) { NANOCLR_SET_AND_LEAVE(CLR_E_FAIL); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CLR/Core/TypeSystem.cpp` around lines 8250 - 8288, The code dereferences the pointer genericType when resolving VAR generic arguments (in the block handling msInst from mdInst.methodSpec), which can be null; change the resolution to use the effective generic context: if genericType is null fall back to mdInst.genericType (or the method-spec's genericType from msInst) before calling contextTs.InitializeFromIndex, i.e., compute an effectiveGenericIndex = genericType ? *genericType : mdInst.genericType (or msInst's appropriate field) and pass that to contextTs.InitializeFromIndex; update references in the VAR handling branch (CLR_RT_MethodSpec_Instance msInst, mdInst, contextTs, parser.Advance) to use effectiveGenericIndex instead of dereferencing genericType directly.
1315-1365:⚠️ Potential issue | 🔴 CriticalGuard
callerbefore the nested-VARfallback.With the new
paramElement.DataType != DATATYPE_VARpath, an opencontextTypeSpeccan fall through tocaller->arrayElementTypewhilecalleris null. Also avoid dereferencing a nullcaller->genericType.🐛 Proposed fix
- else if (caller != nullptr && NANOCLR_INDEX_IS_VALID(*caller->genericType)) + else if (caller != nullptr && caller->genericType != nullptr && + NANOCLR_INDEX_IS_VALID(*caller->genericType)) { effectiveContext = caller->genericType; } @@ - else if (NANOCLR_INDEX_IS_VALID(caller->arrayElementType) && genericPosition == 0) + else if (caller != nullptr && NANOCLR_INDEX_IS_VALID(caller->arrayElementType) && + genericPosition == 0) { // Fallback to arrayElementType: covers SZArrayHelper scenarios and // the nested-VAR case where the context TypeSpec is still open.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CLR/Core/TypeSystem.cpp` around lines 1315 - 1365, The code can dereference caller when it's null during the nested-VAR fallback; ensure you guard all uses of caller and caller->genericType. Specifically, update the fallback condition that reads "else if (NANOCLR_INDEX_IS_VALID(caller->arrayElementType) && genericPosition == 0)" to require caller != nullptr (e.g. "else if (caller != nullptr && NANOCLR_INDEX_IS_VALID(caller->arrayElementType) && genericPosition == 0)"), and audit any other places in this block that dereference caller or *caller->genericType (such as the earlier genericType check and uses of caller->arrayElementType.Type/Assembly) to only access them after confirming caller is non-null; keep the existing resolve_generic_argument/genericPosition logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/CLR/Core/Interpreter.cpp`:
- Around line 2479-2504: The constrained prefix token (constrainedTypeToken) can
persist past the intended call site because it's only cleared inside the
DATATYPE_BYREF branch and only when pThis[0] is a BYREF; fix by gating the
constrained handling to the CEE_CALLVIRT path and ensure constrainedTypeToken is
cleared unconditionally after handling (or after deciding not to handle) so it
cannot leak to later calls. Specifically, in the code around
constrainedTypeToken/pThis/CLR_RT_TypeDef_Instance/ResolveToken/DATATYPE_BYREF/Dereference/Assign,
only attempt the dereference-and-assign logic when the current opcode is
CEE_CALLVIRT and, regardless of whether pThis[0].DataType() == DATATYPE_BYREF or
ResolveToken succeeds, set constrainedTypeToken = 0 before exiting that handling
block so the token is always consumed.
In `@src/CLR/Core/TypeSystem.cpp`:
- Around line 2966-2990: The fallback currently looks up generic parameters
using the token assembly (assm) and can pick the wrong GenericParam table across
assemblies and also treats MethodSpec mappings like "!!U -> !T" as unresolved;
change the fallback to resolve GenericParam using the caller's assembly/context
(use the caller's assembly reference from the stack frame instead of assm when
calling FindGenericParamAtMethodDef and indexing crossReferenceGenericParam)
and, before falling back to the declared constraint, detect MethodSpec-level
mappings (via CLR_RT_MethodSpec_Instance msInst / msInst.GetGenericArgument)
that map a method generic to a type generic and use that resolved type
(msElem.Class) when available; update uses of assm->FindGenericParamAtMethodDef,
assm->crossReferenceGenericParam, and the fallback InitializeFromTypeDef code to
operate on the caller's assembly/context.
In `@src/CLR/Include/nanoCLR_Runtime.h`:
- Around line 2280-2282: The issue is that CLR_RT_MethodDef_Instance becomes
self-referential when InitializeFromIndex sets genericType = &m_typeSpecStorage,
so plain by-value copies leave genericType pointing at the source's
m_typeSpecStorage and can dangle; fix this by adding a proper copy helper for
CLR_RT_MethodDef_Instance: implement a copy constructor and copy assignment (or
a Normalize/Fixup method) that when copying checks if src.genericType ==
&src.m_typeSpecStorage and, if so, copies src.m_typeSpecStorage into the
destination's m_typeSpecStorage and sets dst.genericType =
&dst.m_typeSpecStorage (otherwise copy genericType as-is); then replace the
ad-hoc pointer fixes in CLR_RT_StackFrame::Push, RestoreFromInlineStack,
RestoreStack (and remove manual fixes in Interpreter.cpp and Thread.cpp) by
invoking this copy helper or calling the normalization method immediately after
any by-value assignment of m_call to ensure the self-referential relationship is
preserved.
---
Outside diff comments:
In `@src/CLR/Core/CLR_RT_StackFrame.cpp`:
- Around line 326-339: The inline-frame generic context isn't self-contained
because PushInline saves m_genericTypeSpecStorage but leaves m_call.genericType
pointing to the live frame's storage; update PushInline, SaveStack and
RestoreStack so every CLR_RT_InlineFrame saves a copy of
m_genericTypeSpecStorage into the inline frame
(CLR_RT_InlineFrame::m_frame.m_genericTypeSpecStorage) and then normalize/reset
the saved m_call.genericType to point at that copied storage (not the live
m_genericTypeSpecStorage); likewise, when restoring, copy the saved
genericTypeSpec storage back (or restore pointers) so restored
m_call.genericType references the inline frame's storage consistently; apply
same fixes to the other save/restore paths referenced around lines 426-480 to
ensure all paths preserve and normalize the generic context pointer.
In `@src/CLR/Core/TypeSystem.cpp`:
- Around line 8250-8288: The code dereferences the pointer genericType when
resolving VAR generic arguments (in the block handling msInst from
mdInst.methodSpec), which can be null; change the resolution to use the
effective generic context: if genericType is null fall back to
mdInst.genericType (or the method-spec's genericType from msInst) before calling
contextTs.InitializeFromIndex, i.e., compute an effectiveGenericIndex =
genericType ? *genericType : mdInst.genericType (or msInst's appropriate field)
and pass that to contextTs.InitializeFromIndex; update references in the VAR
handling branch (CLR_RT_MethodSpec_Instance msInst, mdInst, contextTs,
parser.Advance) to use effectiveGenericIndex instead of dereferencing
genericType directly.
- Around line 1315-1365: The code can dereference caller when it's null during
the nested-VAR fallback; ensure you guard all uses of caller and
caller->genericType. Specifically, update the fallback condition that reads
"else if (NANOCLR_INDEX_IS_VALID(caller->arrayElementType) && genericPosition ==
0)" to require caller != nullptr (e.g. "else if (caller != nullptr &&
NANOCLR_INDEX_IS_VALID(caller->arrayElementType) && genericPosition == 0)"), and
audit any other places in this block that dereference caller or
*caller->genericType (such as the earlier genericType check and uses of
caller->arrayElementType.Type/Assembly) to only access them after confirming
caller is non-null; keep the existing resolve_generic_argument/genericPosition
logic unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: dafc7198-ba79-4b37-a5fd-67f1a4758846
📒 Files selected for processing (6)
src/CLR/Core/CLR_RT_StackFrame.cppsrc/CLR/Core/Execution.cppsrc/CLR/Core/Interpreter.cppsrc/CLR/Core/TypeSystem.cppsrc/CLR/Diagnostics/Info.cppsrc/CLR/Include/nanoCLR_Runtime.h
- Fix PushInline to normalize saved m_call.genericType pointer into inline frame storage. - Fix SaveStack to save m_genericTypeSpecStorage and normalize pointer. - Fix RestoreStack to restore m_genericTypeSpecStorage and normalize pointer. - Fix null dereference of genericType in VAR handling. - Fix null dereference of caller in arrayElementType fallback.
14 tasks
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/CLR/Core/Interpreter.cpp (1)
2530-2642: 🧹 Nitpick | 🔵 TrivialConsider folding the duplicated "TypeSpec refers to callee owner" lambdas.
matchesCalleeOwner(PushInline path, 2530–2540) andrefersToCalleeOwner(Push path, 2622–2642) implement the same predicate and each re-derivescalleeOwner…TypeDefDataby callingcalleeInst.GetDeclaringType(). A single static helper (or a single lambda hoisted above both paths sharing one cachedcalleeOwnerTypeDefData) would remove the duplication and one extraGetDeclaringTypecall. No behavior change intended.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CLR/Core/Interpreter.cpp` around lines 2530 - 2642, The two duplicated predicates matchesCalleeOwner and refersToCalleeOwner (used in the PushInline and Push paths) both compute calleeOwnerTypeDefData via calleeInst.GetDeclaringType() and repeat the same InitializeFromIndex logic; replace them with a single helper predicate (or hoist one lambda) that takes a CLR_RT_TypeSpec_Index* and uses a shared cached calleeOwnerTypeDefData computed once (via CLR_RT_TypeDef_Instance + calleeInst.GetDeclaringType()) and reuse that helper in both places so you remove the duplicate logic and the extra GetDeclaringType call.src/CLR/Core/TypeSystem.cpp (1)
1396-1405:⚠️ Potential issue | 🟠 MajorResolve
!!U -> !Tthrough the caller generic context when no explicit context is passed.Line 1400 only accepts
contextTypeSpec; valid calls that carry the closed type oncaller->genericTypestill fail afterMethodSpecmaps an MVAR to a VAR. Mirror the effective-context selection used above forDATATYPE_VAR.🐛 Proposed fix
if (element.DataType == DATATYPE_VAR) { // need to defer to generic type parameter + const CLR_RT_TypeSpec_Index *effectiveContext = nullptr; + + if (contextTypeSpec != nullptr && NANOCLR_INDEX_IS_VALID(*contextTypeSpec)) + { + effectiveContext = contextTypeSpec; + } + else if ( + caller->genericType != nullptr && NANOCLR_INDEX_IS_VALID(*caller->genericType)) + { + effectiveContext = caller->genericType; + } + CLR_RT_TypeSpec_Instance contextTs; - if (!contextTypeSpec || !contextTs.InitializeFromIndex(*contextTypeSpec)) + if (effectiveContext == nullptr || !contextTs.InitializeFromIndex(*effectiveContext)) { return false; } if (!contextTs.GetGenericParam(element.GenericParamPosition, element))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CLR/Core/TypeSystem.cpp` around lines 1396 - 1405, When handling element.DataType == DATATYPE_VAR, if contextTypeSpec is null we must fall back to the effective caller generic context (the same selection used earlier) so MVAR mapped to VAR can be resolved; instantiate a CLR_RT_TypeSpec_Instance from caller->genericType when contextTypeSpec is absent and then call InitializeFromIndex and GetGenericParam(element.GenericParamPosition, element) on that instance instead of immediately returning false. Ensure you reference contextTypeSpec, CLR_RT_TypeSpec_Instance contextTs, contextTs.InitializeFromIndex, caller->genericType, and GetGenericParam in the fix so the VAR is resolved through the caller generic context when present.
♻️ Duplicate comments (3)
src/CLR/Include/nanoCLR_Runtime.h (1)
2293-2306:⚠️ Potential issue | 🔴 CriticalMake
CLR_RT_MethodDef_Instancecopies self-normalizing.
Normalize(src)only works after explicit assignments wheresrcis still known. It cannot repair implicit copy construction or by-value parameters such asCLR_RT_StackFrame::MakeCall(CLR_RT_MethodDef_Instance md, ...); those copies can still carry agenericTypepointer to another instance’sm_typeSpecStorage, andPush()can persist that dangling pointer into a stack frame. Prefer a copy constructor and copy-assignment operator that re-anchor automatically.🛠️ Proposed fix
struct CLR_RT_MethodDef_Instance : public CLR_RT_MethodDef_Index { CLR_RT_Assembly *assembly; const CLR_RECORD_METHODDEF *target; @@ //--// + CLR_RT_MethodDef_Instance() = default; + + CLR_RT_MethodDef_Instance(const CLR_RT_MethodDef_Instance &src) + { + CopyFrom(src); + } + + CLR_RT_MethodDef_Instance &operator=(const CLR_RT_MethodDef_Instance &src) + { + if (this != &src) + { + CopyFrom(src); + } + + return *this; + } + + inline void CopyFrom(const CLR_RT_MethodDef_Instance &src) + { + data = src.data; + assembly = src.assembly; + target = src.target; + genericType = src.genericType; + methodSpec = src.methodSpec; + m_typeSpecStorage = src.m_typeSpecStorage; + arrayElementType = src.arrayElementType; +#if defined(NANOCLR_INSTANCE_NAMES) + name = src.name; +#endif + + Normalize(src); + } + // After any plain by-value copy of a CLR_RT_MethodDef_Instance, call // Normalize(src) to re-anchor the self-referential genericType pointer.Run this read-only check to find remaining by-value copy sites that should become safe via the copy constructor/operator:
#!/bin/bash # Description: Find CLR_RT_MethodDef_Instance by-value parameters and explicit copy sites. # Expectation: Either CLR_RT_MethodDef_Instance has self-normalizing copy construction/assignment, # or each by-value signature/copy site is changed to avoid persisting stale genericType pointers. rg -nP -C3 '\bCLR_RT_MethodDef_Instance\s+\w+\s*(,|\))|\bCLR_RT_MethodDef_Instance\s+\w+\s*=|\bm_call\s*=' --type=cpp --type=h rg -nP -C4 'CLR_RT_MethodDef_Instance\s*\(\s*const\s+CLR_RT_MethodDef_Instance\s*&|operator=\s*\(\s*const\s+CLR_RT_MethodDef_Instance\s*&' src/CLR/Include/nanoCLR_Runtime.h🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CLR/Include/nanoCLR_Runtime.h` around lines 2293 - 2306, Add self-normalizing copy semantics to CLR_RT_MethodDef_Instance by implementing a copy constructor and copy-assignment operator that perform a memberwise copy and then re-anchor any self-referential genericType pointer: in both CLR_RT_MethodDef_Instance(const CLR_RT_MethodDef_Instance &src) and operator=(const CLR_RT_MethodDef_Instance &src) copy all fields, then if src.genericType == &src.m_typeSpecStorage set this->genericType = &this->m_typeSpecStorage (otherwise copy the pointer), or simply call Normalize(src) after the copy; update the class declaration to declare these functions so implicit by-value copies (e.g. CLR_RT_StackFrame::MakeCall or Push sites) no longer carry dangling pointers to another instance’s m_typeSpecStorage.src/CLR/Core/Interpreter.cpp (1)
2459-2507:⚠️ Potential issue | 🟡 Minor
constrainedTypeTokenstill leaks on the AppDomain-transition path.The unconditional
constrainedTypeToken = 0at line 2506 lives inside theelsebranch ofif (fAppDomainTransition). WhenfAppDomainTransitionis true the code callsPushAppDomainTransitionand falls through togoto Execute_Restartat line 2691 without touchingconstrainedTypeToken. BecauseExecute_Restartcontinues inside the sameExecute_ILinvocation when the frame is compatible (see line 4770–4781), the stale token can be consumed by a later CALL/CALLVIRT in the callee or in this frame after return, resolving the wrong constrained type.Co-occurrence (BYREF
this+ transparent proxy) is unlikely but cheap to make watertight — clear the token unconditionally before branching onfAppDomainTransition, or duplicate the clear in the AppDomain arm.💡 Proposed fix
+ // Always consume the constrained. prefix token at the call site so it + // cannot leak past this instruction (including the AppDomain path below). + CLR_UINT32 pendingConstrainedToken = constrainedTypeToken; + constrainedTypeToken = 0; + `#if` defined(NANOCLR_APPDOMAINS) if (fAppDomainTransition) { WRITEBACK(stack, evalPos, ip, fDirty); ... } else `#endif` // NANOCLR_APPDOMAINS { ... - if (constrainedTypeToken != 0) + if (pendingConstrainedToken != 0) { if (op == CEE_CALLVIRT && pThis[0].DataType() == DATATYPE_BYREF) { CLR_RT_TypeDef_Instance constrainedType{}; - if (constrainedType.ResolveToken(constrainedTypeToken, assm, &stack->m_call)) + if (constrainedType.ResolveToken(pendingConstrainedToken, assm, &stack->m_call)) { ... } } - constrainedTypeToken = 0; }The prior review note about gating the deref to
CEE_CALLVIRTand ensuring the token is cleared in the non-AppDomain branch has already been addressed by the current structure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CLR/Core/Interpreter.cpp` around lines 2459 - 2507, Clear constrainedTypeToken unconditionally before the AppDomain-transition branch to avoid leaking a stale token across PushAppDomainTransition/Execute_Restart; specifically, set constrainedTypeToken = 0 prior to checking fAppDomainTransition (the block that calls CLR_RT_StackFrame::PushAppDomainTransition) or alternatively duplicate the clear immediately after the PushAppDomainTransition call, ensuring any subsequent execution (including via Execute_Restart) cannot reuse the old constrainedTypeToken during later CEE_CALLVIRT handling of pThis.src/CLR/Core/TypeSystem.cpp (1)
3010-3015:⚠️ Potential issue | 🟠 MajorValidate the GenericParam fallback before indexing the cross-reference table.
FindGenericParamAtMethodDef()can fail or produce an out-of-range parameter index, but Line 3014 indexescrossReferenceGenericParamunconditionally. ReturnCLR_E_WRONG_TYPEinstead of reading the wrong row or running past the table.🐛 Proposed fix
if (caller != nullptr) { CLR_RT_GenericParam_Index gp; - caller->assembly->FindGenericParamAtMethodDef(*caller, elem.GenericParamPosition, gp); + if (caller->assembly == nullptr || + !caller->assembly->FindGenericParamAtMethodDef(*caller, elem.GenericParamPosition, gp) || + gp.GenericParam() >= caller->assembly->tablesSize[TBL_GenericParam]) + { + NANOCLR_SET_AND_LEAVE(CLR_E_WRONG_TYPE); + } + auto &info = caller->assembly->crossReferenceGenericParam[gp.GenericParam()]; - this->InitializeFromTypeDef(info.classTypeDef); + NANOCLR_CHECK_HRESULT(this->InitializeFromTypeDef(info.classTypeDef)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CLR/Core/TypeSystem.cpp` around lines 3010 - 3015, FindGenericParamAtMethodDef can fail or produce an invalid index but the code currently unconditionally indexes caller->assembly->crossReferenceGenericParam[gp.GenericParam()]; update the logic in the block that uses CLR_RT_GenericParam_Index gp so that after calling caller->assembly->FindGenericParamAtMethodDef(*caller, elem.GenericParamPosition, gp) you validate the result and that gp.GenericParam() is within bounds of caller->assembly->crossReferenceGenericParam; if FindGenericParamAtMethodDef fails or the index is out of range, return CLR_E_WRONG_TYPE instead of indexing; keep the subsequent call to InitializeFromTypeDef(info.classTypeDef) only when validation succeeds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/CLR/Core/TypeSystem.cpp`:
- Around line 8003-8013: The resolution path for VAR inside MethodSpec args
currently ignores the provided contextTypeSpec and only checks
contextMethodDef->genericType; update the logic in the block handling
paramElement.DataType == DATATYPE_VAR to first attempt resolving against the
supplied contextTypeSpec (use its CLR_RT_TypeSpec_Instance and call
GetGenericParam with paramElement.GenericParamPosition into argElement) and only
if that fails fall back to initializing contextTs from
contextMethodDef->genericType and calling GetGenericParam there; ensure
paramTypeDef is set from argElement.Class when the contextTypeSpec resolution
succeeds and preserve existing behavior when it does not.
---
Outside diff comments:
In `@src/CLR/Core/Interpreter.cpp`:
- Around line 2530-2642: The two duplicated predicates matchesCalleeOwner and
refersToCalleeOwner (used in the PushInline and Push paths) both compute
calleeOwnerTypeDefData via calleeInst.GetDeclaringType() and repeat the same
InitializeFromIndex logic; replace them with a single helper predicate (or hoist
one lambda) that takes a CLR_RT_TypeSpec_Index* and uses a shared cached
calleeOwnerTypeDefData computed once (via CLR_RT_TypeDef_Instance +
calleeInst.GetDeclaringType()) and reuse that helper in both places so you
remove the duplicate logic and the extra GetDeclaringType call.
In `@src/CLR/Core/TypeSystem.cpp`:
- Around line 1396-1405: When handling element.DataType == DATATYPE_VAR, if
contextTypeSpec is null we must fall back to the effective caller generic
context (the same selection used earlier) so MVAR mapped to VAR can be resolved;
instantiate a CLR_RT_TypeSpec_Instance from caller->genericType when
contextTypeSpec is absent and then call InitializeFromIndex and
GetGenericParam(element.GenericParamPosition, element) on that instance instead
of immediately returning false. Ensure you reference contextTypeSpec,
CLR_RT_TypeSpec_Instance contextTs, contextTs.InitializeFromIndex,
caller->genericType, and GetGenericParam in the fix so the VAR is resolved
through the caller generic context when present.
---
Duplicate comments:
In `@src/CLR/Core/Interpreter.cpp`:
- Around line 2459-2507: Clear constrainedTypeToken unconditionally before the
AppDomain-transition branch to avoid leaking a stale token across
PushAppDomainTransition/Execute_Restart; specifically, set constrainedTypeToken
= 0 prior to checking fAppDomainTransition (the block that calls
CLR_RT_StackFrame::PushAppDomainTransition) or alternatively duplicate the clear
immediately after the PushAppDomainTransition call, ensuring any subsequent
execution (including via Execute_Restart) cannot reuse the old
constrainedTypeToken during later CEE_CALLVIRT handling of pThis.
In `@src/CLR/Core/TypeSystem.cpp`:
- Around line 3010-3015: FindGenericParamAtMethodDef can fail or produce an
invalid index but the code currently unconditionally indexes
caller->assembly->crossReferenceGenericParam[gp.GenericParam()]; update the
logic in the block that uses CLR_RT_GenericParam_Index gp so that after calling
caller->assembly->FindGenericParamAtMethodDef(*caller,
elem.GenericParamPosition, gp) you validate the result and that
gp.GenericParam() is within bounds of
caller->assembly->crossReferenceGenericParam; if FindGenericParamAtMethodDef
fails or the index is out of range, return CLR_E_WRONG_TYPE instead of indexing;
keep the subsequent call to InitializeFromTypeDef(info.classTypeDef) only when
validation succeeds.
In `@src/CLR/Include/nanoCLR_Runtime.h`:
- Around line 2293-2306: Add self-normalizing copy semantics to
CLR_RT_MethodDef_Instance by implementing a copy constructor and copy-assignment
operator that perform a memberwise copy and then re-anchor any self-referential
genericType pointer: in both CLR_RT_MethodDef_Instance(const
CLR_RT_MethodDef_Instance &src) and operator=(const CLR_RT_MethodDef_Instance
&src) copy all fields, then if src.genericType == &src.m_typeSpecStorage set
this->genericType = &this->m_typeSpecStorage (otherwise copy the pointer), or
simply call Normalize(src) after the copy; update the class declaration to
declare these functions so implicit by-value copies (e.g.
CLR_RT_StackFrame::MakeCall or Push sites) no longer carry dangling pointers to
another instance’s m_typeSpecStorage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 37219361-73f7-4271-b6e8-3b02eb50f8e7
📒 Files selected for processing (5)
src/CLR/Core/CLR_RT_StackFrame.cppsrc/CLR/Core/Interpreter.cppsrc/CLR/Core/TypeSystem.cppsrc/CLR/Include/nanoCLR_Runtime.htargets/netcore/src/CLR/Core/CLR_RT_StackFrame.cpp
14 tasks
Automated fixes for code style.
…909ca-2718-4e9c-82cb-82fc3dc75d98 Code style fixes for nanoframework/nf-interpreter PR#3338
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Motivation and Context
How Has This Been Tested?
String.Format()CoreLibrary#269]Screenshots
Types of changes
Checklist