Add README for generics public preview #3157
Conversation
- Fix packaging and versioning of win32 CLR. ***NO_CI***
- Fix condition to publish packages on buils from develop branch. ***NO_CI***
…into develop ***NO_CI***
…into develop ***NO_CI***
- Remove task publishing to private Azure feed. - Update condition to publish nanoCLR on develop builds. ***NO_CI***
- Add links to both main README files. - Expand targets tables to include preview versions. - Minor formating fixes.
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (4)
You can disable this status message by setting the WalkthroughAdds comprehensive runtime support for generics across the CLR: new native CorLib bindings (Span/ReadOnlySpan/Unsafe/String/Type/GC/etc.), TypeSystem and interpreter/runtime changes to resolve and instantiate generic types, HeapBlock/type-descriptor matching, GC and profiler updates, plus docs, devcontainer image bumps, and CI/pipeline adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Managed Code
participant Interpreter
participant Exec as ExecutionEngine
participant TypeSys as TypeSystem/SignatureParser
participant Heap as HeapBlock/Array
participant GC as GarbageCollector
Caller->>Interpreter: execute IL (newobj / ldtoken / box / castclass)
Interpreter->>TypeSys: resolve token (with generic context if present)
TypeSys-->>Interpreter: type/TypeSpec (GENERICINST/VAR/MVAR)
Interpreter->>Exec: request object creation with TypeSpec
Exec->>Heap: New(Generic)InstanceObject / CreateInstanceWithStorage
Heap-->>Exec: instance reference
Exec->>GC: register statics / protect temporaries
GC-->>Exec: confirm protection (may schedule compaction/collection)
Exec-->>Caller: return new object/reference
Estimated code review effort🎯 5 (Critical) | ⏱️ ~240 minutes Areas to focus review on:
Possibly related PRs
Suggested labels
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
README.md (2)
18-22: Minor punctuation improvement in new preview section
Add a comma for clarity in the sentence on line 21.
Suggested change:- If you want to use this feature please refer to the documentation [here](README-GENERICS.md). + If you want to use this feature, please refer to the documentation [here](README-GENERICS.md).🧰 Tools
🪛 LanguageTool
[typographical] ~21-~21: It seems that a comma is missing.
Context: ...enerics. If you want to use this feature please refer to the documentation [here](READM...(IF_PLEASE_COMMA)
40-40: Remove stray semicolon in separator line
The line-----;appears to include an unintended semicolon and may disrupt Markdown rendering. It should be:- -----; + -----README.zh-cn.md (1)
18-21: Remove trailing spaces after Chinese sentence
Line 20 ends with two spaces; remove the extra whitespace to clean up formatting.README-GENERICS.md (2)
42-43: Clarify comparative phrasing
The clause “similarly to how they would on the full .NET runtime” lacks an explicit verb and may read awkwardly.
Suggested revision:- …write more flexible and reusable code on microcontrollers, similarly to how they would on the full .NET runtime. + …write more flexible and reusable code on microcontrollers, similar to how they can on the full .NET runtime.🧰 Tools
🪛 LanguageTool
[grammar] ~42-~42: It seems that a verb is missing.
Context: ...microcontrollers, similarly to how they would on the full .NET runtime. Keep in mind ...(MD_DT_JJ)
75-75: Add hyphens for compound adjectives
Use hyphens in “pre- and post-generics projects” for correct compound adjective styling.
Suggested revision:- …not possible to support pre and post generics projects… + …not possible to support pre‑ and post‑generics projects…🧰 Tools
🪛 LanguageTool
[grammar] ~75-~75: It appears that hyphens are missing.
Context: ...extension, it's not possible to support pre and post generics projects with the same extension. There...(PRE_AND_POST_NN)
[grammar] ~75-~75: It appears that hyphens are missing.
Context: ...ersions. It is not an option to install pre and post generics extensions in VS2019 and VS2022, for ex...(PRE_AND_POST_NN)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README-GENERICS.md(1 hunks)README.md(4 hunks)README.zh-cn.md(2 hunks)
🧰 Additional context used
🪛 LanguageTool
README.zh-cn.md
[style] ~119-~119: Using many exclamation marks might seem excessive (in this case: 90 exclamation marks for a text that’s 39429 characters long)
Context: ...-|---|---| | TI_CC1352R1_LAUNCHXL_868 | |
| | TI_CC1352R1_LAUNCHXL_915 |
|
| | TI_CC3220SF_LAUNCHXL | [
README.md
[typographical] ~21-~21: It seems that a comma is missing.
Context: ...enerics. If you want to use this feature please refer to the documentation [here](READM...
(IF_PLEASE_COMMA)
[style] ~131-~131: Using many exclamation marks might seem excessive (in this case: 91 exclamation marks for a text that’s 43399 characters long)
Context: ...-|---|---| | TI_CC1352R1_LAUNCHXL_868 | |
| | TI_CC1352R1_LAUNCHXL_915 |
|
| | TI_CC3220SF_LAUNCHXL | [
README-GENERICS.md
[grammar] ~42-~42: It seems that a verb is missing.
Context: ...microcontrollers, similarly to how they would on the full .NET runtime. Keep in mind ...
(MD_DT_JJ)
[grammar] ~75-~75: It appears that hyphens are missing.
Context: ...extension, it's not possible to support pre and post generics projects with the same extension. There...
(PRE_AND_POST_NN)
[grammar] ~75-~75: It appears that hyphens are missing.
Context: ...ersions. It is not an option to install pre and post generics extensions in VS2019 and VS2022, for ex...
(PRE_AND_POST_NN)
🔇 Additional comments (11)
README.md (1)
44-45: Preview column added correctly
The new “Preview” column header and its alignment row are clear and consistent with existing table formatting.README.zh-cn.md (1)
32-33: 新增“预览”列标题格式正确
表头新增的“预览”列及其对齐符号符合 Markdown 规范，清晰易读。README-GENERICS.md (9)
1-4: Great new documentation file
The title and overview section clearly introduce the purpose and context of the generics public preview.
5-12: Clear explanation of generics benefits
The bullet points for type safety, reusability, and performance are well‑written and concise.
13-25: Well‑formed generic method example
The sample code forMax<T>demonstrates usage effectively and follows standard C# conventions.
27-38: Effective generic class illustration
TheStack<T>class example is simple, accurate, and easy to follow.
44-58: Firmware flashing instructions are clear
Thenanoff --previewexample and notes accurately guide users through applying preview firmware.
61-70: VSIX installation steps are well detailed
The instructions for downloading and installing the preview Visual Studio extension are precise and user‑friendly.
77-86: NuGet package update guidance is solid
The steps for enabling prerelease packages and aligning versions are clear and actionable.
88-98: Feedback and known issues section is comprehensive
The caveats around debugging quirks and memory usage set proper expectations for users.
99-100: Strong closing call to action
The thank‑you note and invitation for feedback provide a positive wrap‑up.
- For consistency with generated code. ***NO_CI***
***NO_CI***
***NO_CI***
***NO_CI***
***NO_CI***
…into develop ***NO_CI***
***NO_CI***
…into develop ***NO_CI***
- Now updating on different version, not higher. ***NO_CI***
- Upload of nuget packages is not happening in develop branch anymore (except if forced with build var). ***NO_CI***
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
README-GENERICS.md (5)
1-1: Enhance title for clarity and consistency
To align with other nanoFramework documentation, consider renaming the title to something like “Generics Public Preview for .NET nanoFramework CLR” to immediately convey context.
40-42: Clarify wording in paragraph for readability
The phrase “similarly to how they would on the full .NET runtime” is missing a verb and reads awkwardly. For example:- Developers can now write more flexible and reusable code on microcontrollers, similarly to how they would on the full .NET runtime. + Developers can now write more flexible and reusable code on microcontrollers, just as they would on the full .NET runtime.🧰 Tools
🪛 LanguageTool
[grammar] ~42-~42: It seems that a verb is missing.
Context: ...microcontrollers, similarly to how they would on the full .NET runtime. Keep in mind ...(MD_DT_JJ)
71-71: Hyphenate “double-click”
Use “double-click” instead of “double click” to match conventional spelling.- After downloading the file, double click on it and the extension installer will open. + After downloading the file, double-click on it and the extension installer will open.🧰 Tools
🪛 LanguageTool
[grammar] ~71-~71: This expression is usually spelled with a hyphen.
Context: ...l Studio:** After downloading the file, double click on it and the extension installer will ...(DOUBLE_CLICK_HYPHEN)
75-75: Improve hyphenation and punctuation in warning
This sentence has multiple hyphenation and comma issues. For clarity:- > **Warning:** Despite we've tried to make that possible using preview features in VS extension, it's not possible to support pre and post generics projects with the same extension. + > **Warning:** Despite our efforts to make this possible using the VS extension preview, it’s not possible to support pre- and post-generics projects with the same extension.Also add a comma after “for example” in the next sentence:
- It is not an option to install pre and post generics extensions in VS2019 and VS2022, for example as both use… + It is not an option to install pre- and post-generics extensions in VS2019 and VS2022, for example, as both use…🧰 Tools
🪛 LanguageTool
[uncategorized] ~75-~75: You might be missing the article “the” here.
Context: ...that possible using preview features in VS extension, it's not possible to support...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[grammar] ~75-~75: It appears that hyphens are missing.
Context: ...extension, it's not possible to support pre and post generics projects with the same extension. There...(PRE_AND_POST_NN)
[grammar] ~75-~75: It appears that hyphens are missing.
Context: ...ersions. It is not an option to install pre and post generics extensions in VS2019 and VS2022, for ex...(PRE_AND_POST_NN)
[uncategorized] ~75-~75: A comma might be missing here.
Context: ...cs extensions in VS2019 and VS2022, for example as both use the same msbuild components...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
1-100: Consider adding a Table of Contents
This document is lengthy. A TOC at the top (linking to sections like “Overview,” “Firmware Flashing,” etc.) will improve navigation for readers.🧰 Tools
🪛 LanguageTool
[grammar] ~42-~42: It seems that a verb is missing.
Context: ...microcontrollers, similarly to how they would on the full .NET runtime. Keep in mind ...(MD_DT_JJ)
[grammar] ~71-~71: This expression is usually spelled with a hyphen.
Context: ...l Studio:** After downloading the file, double click on it and the extension installer will ...(DOUBLE_CLICK_HYPHEN)
[uncategorized] ~75-~75: You might be missing the article “the” here.
Context: ...that possible using preview features in VS extension, it's not possible to support...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[grammar] ~75-~75: It appears that hyphens are missing.
Context: ...extension, it's not possible to support pre and post generics projects with the same extension. There...(PRE_AND_POST_NN)
[grammar] ~75-~75: It appears that hyphens are missing.
Context: ...ersions. It is not an option to install pre and post generics extensions in VS2019 and VS2022, for ex...(PRE_AND_POST_NN)
[uncategorized] ~75-~75: A comma might be missing here.
Context: ...cs extensions in VS2019 and VS2022, for example as both use the same msbuild components...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README-GENERICS.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
README-GENERICS.md
[grammar] ~42-~42: It seems that a verb is missing.
Context: ...microcontrollers, similarly to how they would on the full .NET runtime. Keep in mind ...
(MD_DT_JJ)
[grammar] ~71-~71: This expression is usually spelled with a hyphen.
Context: ...l Studio:** After downloading the file, double click on it and the extension installer will ...
(DOUBLE_CLICK_HYPHEN)
[uncategorized] ~75-~75: You might be missing the article “the” here.
Context: ...that possible using preview features in VS extension, it's not possible to support...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[grammar] ~75-~75: It appears that hyphens are missing.
Context: ...extension, it's not possible to support pre and post generics projects with the same extension. There...
(PRE_AND_POST_NN)
[grammar] ~75-~75: It appears that hyphens are missing.
Context: ...ersions. It is not an option to install pre and post generics extensions in VS2019 and VS2022, for ex...
(PRE_AND_POST_NN)
[uncategorized] ~75-~75: A comma might be missing here.
Context: ...cs extensions in VS2019 and VS2022, for example as both use the same msbuild components...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ST_NUCLEO64_F091RC)
- 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)
Ellerbach
left a comment
Ellerbach
left a comment
There was a problem hiding this comment.
That look super clear! Thanks a lot!
…into develop ***NO_CI***
- Pin AZDO vm image to windows-2022. (new windows-2025 is having issues). ***NO_CI***
…into develop ***NO_CI***
…into develop ***NO_CI***
…e related with generics (nanoframework#3240)
***NO_CI***
- Remove pre-check of MDP specific build. Can be handled at step level, allowing re-running a job. ***NO_CI***
…eter into add-generics-readme
…into add-generics-readme
|
@josesimoes there are issues with the code style on the source files. Make sure to follow the project code style. Check the details here on how it works and the tools required to help you with that. |
There was a problem hiding this comment.
Actionable comments posted: 16
♻️ Duplicate comments (1)
CMake/Modules/AzureRTOS_EFM32GG11_GCC_options.cmake (1)
65-65: Same LTO removal as TI_SimpleLink target.This mirrors the change in
TI_SimpleLink_CC13X2_GCC_options.cmake, indicating a coordinated decision to disable LTO across multiple targets. The same code size verification considerations apply here.See the comment on
TI_SimpleLink_CC13X2_GCC_options.cmakeline 54 for verification steps.
🧹 Nitpick comments (24)
README.md (1)
98-98: Consider adding a hyphen to "chip-based."Grammar linting suggests: "STM32 boards and chip-based" should be "STM32 boards and chip-based" (with hyphen). The current text reads "STM32 boards and chip based" which would benefit from a hyphen to clarify that "chip" modifies "based" as a compound adjective.
Apply this diff:
-### STM32 boards and chip based +### STM32 boards and chip-basedsrc/CLR/Core/Cache.cpp (1)
199-272: Consider extracting diagnostics to a helper function.The diagnostic block is useful for debugging memory layout issues but spans 74 lines within
Initialize(). While the DEBUG-only scope is appropriate, extracting this to a separate function (e.g.,VerifyMemoryLayout()) would improve readability and maintainability.Example refactor:
#ifdef DEBUG static void VerifyMemoryLayout(Link* entries, Link* entriesMRU, Payload* payloads) { // Move diagnostic code here } #endif void CLR_RT_EventCache::VirtualMethodTable::Initialize() { NATIVE_PROFILE_CLR_CORE(); CLR_UINT32 index; m_entries = (Link *)&g_scratchVirtualMethodTableLink[0]; m_entriesMRU = (Link *)&g_scratchVirtualMethodTableLinkMRU[0]; m_payloads = (Payload *)&g_scratchVirtualMethodPayload[0]; #ifdef DEBUG VerifyMemoryLayout(m_entries, m_entriesMRU, m_payloads); #endif // Rest of initialization... }src/CLR/Debugger/Debugger.cpp (1)
2455-2459: Unreachable code should be removed.Line 2458
return true;is unreachable becauseGetValuereturns on line 2456. This pattern appears multiple times in the file (lines 2476-2478, 2929-2931, 3041-3044, 3097-3100, 3109-3113, 3134-3137, 3194-3197, 3225-3228, 3254-3257, 3470-3473).- // WP_ReplyToCommand(msg, g_CLR_DBG_Debugger->GetValue(msg, pThread, nullptr, nullptr), false, nullptr, 0); - return g_CLR_DBG_Debugger->GetValue(msg, pThread, nullptr, nullptr); - - return true; + return g_CLR_DBG_Debugger->GetValue(msg, pThread, nullptr, nullptr);src/CLR/Core/CLR_RT_UnicodeHelper.cpp (1)
807-863: Makem_wCharArrayusage consistently usenullptrThe constructor and
Release()now usenullptr, but there are still assignments toNULLin the error path ofAssign()(Line 845) and inRelease()(Line 860). Not a bug, but it’s inconsistent within the same type.To keep the style uniform and fully modernize the pointer usage, consider:
if (!m_unicodeHelper.ConvertFromUTF8(-1, false)) { CLR_RT_Memory::Release(m_wCharArray); - m_wCharArray = NULL; + m_wCharArray = nullptr; m_length = 0; NANOCLR_SET_AND_LEAVE(CLR_E_INVALID_PARAMETER); }void UnicodeString::Release() { - if (m_wCharArray != nullptr) - { - CLR_RT_Memory::Release(m_wCharArray); - m_wCharArray = NULL; - } + if (m_wCharArray != nullptr) + { + CLR_RT_Memory::Release(m_wCharArray); + m_wCharArray = nullptr; + } m_length = 0; }src/CLR/Core/NativeEventDispatcher/NativeEventDispatcher.cpp (2)
59-80: Explicit initialization ofeventand driver fields improves safetyInitializing
eventtonullptrmakes the cleanup path safe if allocation fails, and settingpDrvCustomData/driverMethodstonullptravoids leaking stale pointers when reusing cached instances. You might optionally move these default assignments intoInitialize()(or similar) to centralize the type’s invariants, but the current placement is functionally fine.
160-179: Consider guarding against nullPop/Pushresults inRemoveFromHALQueueNow that
testRecandnewRecare explicitly initialized tonullptr, it’s clearer thatPop()/Push()are assumed never to returnnullptrhere. If that assumption can be violated under contention or capacity limits,testRec->m_contextandnewRec->AssignFrom(*testRec)would dereference null. If the queue API can fail, consider adding null checks (or assertions) before dereferencing; if it truly cannot, an assertion documenting that contract would help future readers.src/CLR/Core/CLR_RT_SystemAssembliesTable.cpp (1)
35-36: Minor: Comment still references NULL.The comment on line 35 says "return NULL" but the code now returns
nullptr. Consider updating for consistency.- // This will return NULL if there is no registered interop assembly of that name + // This will return nullptr if there is no registered interop assembly of that name return LookUpAssemblyNativeDataByName(g_CLR_InteropAssembliesNativeData, lpszAssemblyName);src/CLR/Core/GarbageCollector_Compaction.cpp (1)
161-165: Fix comment indentation.The comment block has inconsistent indentation compared to the surrounding code structure.
- ////////////////////////////////////////////////////// - // - // At this point, we have at least ONE movable block. - // - ////////////////////////////////////////////////////// + ////////////////////////////////////////////////////// + // + // At this point, we have at least ONE movable block. + // + //////////////////////////////////////////////////////src/CLR/Core/Interpreter.cpp (5)
3019-3205: Generic static field access + .cctor reschedule helper: behavior is reasonable; verify token size assumptionThe new handling in
LDSFLD,LDSFLDA, andSTSFLD:
- Resolves the field with
field.ResolveToken(arg, assm, &stack->m_call)so VAR/MVAR can be interpreted in the current generic context.- For generic static fields (
field.genericTypevalid), callsfield.assembly->GetStaticFieldByFieldDef(...)with both the TypeSpec context and the current method call context to locate the correct per‑closed‑type slot.- If that lookup returns
nullptr, usesHandleGenericCctorRescheduleto detect a scheduled‑but‑not‑executed generic.cctorand returnsCLR_E_RESCHEDULEafter rewindingipby 3 bytes (opcode + compressed field token), causing the interpreter to retry the instruction after the.cctorruns.- Falls back to
AccessStaticField(field)for non‑generic static fields.This is a sensible way to gate static field access on generic
.cctorcompletion without introducing runtime races. Two things worth confirming:
- The assumption that the compressed field token is always 2 bytes (so rewinding by 3 total bytes reaches the opcode) must match your metadata encoding; if that ever changes,
HandleGenericCctorReschedulewill need to adjust.- That
GetStaticFieldByFieldDefreally does returnnullptrspecifically for “field storage not yet created because cctor hasn’t run”, and not for other error modes that should be treated as hard failures.If both hold, this logic should behave correctly and deterministically for generic statics.
3545-3821: Array element generics handling (LDELEM/STELEM) is careful; context restore is importantThe updated
LDELEM/STELEMlogic:
- Uses
InitializeArrayReference+FixArrayReferenceForValueTypesto build a by‑ref to the target slot.- Temporarily sets
stack->m_call.arrayElementTypefrom the runtime element type (ExtractTypeIndexFromObject) before callingResolveToken(arg, assm, &stack->m_call)so that generic VAR tokens can be closed against the actual array element type.- Restores the previous
arrayElementTypeafter resolving and/or loading the element, so the generic context doesn’t leak to unrelated instructions.- In
STELEM, computes the element size from the resolvedTypeDef(primitive vs value type) and usesStoreToReferencewith that size, after promoting the value.This is a good pattern for reconciling IL’s generic element type with the runtime array type and should avoid mismatched VAR resolutions as long as
arrayElementTypeis never relied on outside this short scope (which you do restore).
4295-4335:locallocimplementation: ensure allocated blocks are reliably freed when the frame unwindsThe new
CEE_LOCALLOChandling:
- Validates that
sizeRaw >= 0and enforces a maximum count viam_localAllocCount/c_Max_Localloc_Count.- Allocates from the platform heap via
platform_malloc(size)and zero‑initializes the memory.- Tracks each allocation in
stack->m_localAllocs[stack->m_localAllocCount++]and pushes an unmanaged pointer to the eval stack.This is fine functionally, but it hinges on complementary cleanup:
- On every path that pops or destroys the
CLR_RT_StackFrame(normalret, exceptions, inline unwind, app‑domain transitions, thread abort, etc.), thosem_localAllocs[...]must be freed exactly once, andm_localAllocCountreset.- If that cleanup is missing or incomplete in
CLR_RT_StackFrameteardown code, locallocs will leak platform heap memory.Please double‑check that the frame destructor/pop logic frees all entries in
m_localAllocsfor every exit path.If helpful, I can sketch a small helper (e.g.,
CLR_RT_StackFrame::FreeLocallocs()) and show where it should be called from Pop/teardown.
1098-1157: Opcode stack-change debug checks: confirmStackPop/StackPushencoding before relying on themUnder
NANOCLR_OPCODE_STACKCHANGES, you add pre‑ and post‑opcode checks that:
- Use
c_CLR_RT_OpcodeLookup[op].StackPop()/StackPush()/StackChanges()to compute expected pops/pushes.- Treat any opcode with
stackPop == 0 || stackPush == 0as “variable stack op” and skip validation.- For other opcodes, pre‑check for stack underflow and overflow, and post‑check that the actual depth change matches
StackChanges().This is a useful diagnostic, but it depends heavily on how
StackPop()/StackPush()encode:
- If
VarPop/VarPushare encoded with the same sentinel value asPop0/Push0(e.g., both returning 0), theisVariableStackOplogic will skip validation for all opcodes that legitimately pop or push zero items, which may not be what you intend.- Conversely, if
Var*use a distinct sentinel (e.g., negative values), the equality‑to‑0 test is wrong and variable‑stack opcodes will be (incorrectly) validated, leading to false stack‑underflow/overflow errors.Please confirm the actual encoding of these helpers and adjust
isVariableStackOpaccordingly (e.g., explicitly checking for a known sentinel for VarPop/VarPush) so that the debug checks only skip truly variable‑stack instructions.
2200-2515: Interface/generic call handling: correct but potentially expensive TypeSpec searchFor CALL/CALLVIRT you now:
- Pre-populate
calleeInst.arrayElementTypefromstack->m_call.arrayElementTypeso that token resolution can use this information.- For CALLVIRT on a MethodRef owned by a TypeRef (typical interface calls), with no existing generic context, walk the caller assembly’s
TypeSpectable to find a closed generic whose generic type definition matches the runtimethisobject’s class, and treat that aseffectiveCallerGeneric.- Pass
effectiveCallerGenericintocalleeInst.ResolveToken, then carry the resulting genericType into the dispatched method via virtual dispatch, preserving/overridingarrayElementTypeas needed (including the SZArrayHelper special case).- On push of the new frame, store the final effective generic TypeSpec into
m_genericTypeSpecStorageand pointm_call.genericTypethere.This gives you the correct closed generic context for interface dispatch on generic instances (e.g.,
IEnumerable<T>/IList<T>onList<int>), which is hard to get right; the logic looks sound.The only caveat is cost: in the worst case, the
for (int i = 0; i < assm->tablesSize[TBL_TypeSpec]; i++)linear scan per CALLVIRT could be noticeable if there are many TypeSpecs and many such calls in hot paths.If this becomes a hotspot, you might eventually want a small cache or index (e.g., map from
(assembly, TypeDef)to TypeSpec index) rather than scanning the whole table each time. For now, the correctness benefit probably outweighs the overhead.src/CLR/Core/Execution.cpp (1)
996-1119: Review generic .cctor scheduling logic for edge cases.The function iterates through all TypeSpecs in an assembly to find closed generic types that need .cctor execution. A few observations:
Line 1074: If
FindOrCreateGenericCctorRecordreturns nullptr (out of memory), the function continues to the next TypeSpec. This is reasonable behavior but could lead to incomplete initialization if memory is critically low.Lines 1081-1086: The check for already-scheduled/executed .cctors uses bitwise AND. This is correct but consider whether a race condition could occur if multiple threads access the same record.
Lines 1112-1113: If
PushThreadProcDelegatefails, the scheduled flag is cleared. Good defensive programming.Consider adding diagnostic logging for out-of-memory scenarios to aid debugging:
if (record == nullptr) { #if defined(NANOCLR_GC_VERBOSE) CLR_Debug::Printf("Failed to allocate .cctor record for generic type at TypeSpec %d\r\n", iTs); #endif continue; }src/CLR/Core/CLR_RT_HeapBlock_GenericInstance.cpp (1)
11-82: Placeholder implementation with unrelated commented-out code.The
CreateInstancestub correctly suppresses unused parameter warnings with(void)casts. However, the extensive commented-out code (lines 21-80) appears to be copy-pasted from delegate creation logic and is unrelated to generic instance creation. This dead code adds confusion for future maintainers.Consider removing the commented-out code entirely or replacing it with a concise TODO comment describing the intended implementation.
(void)reference; (void)tsIndex; - // reference.SetObjectReference( nullptr ); - - // CLR_UINT32 length = 0; - - // #if defined(NANOCLR_DELEGATE_PRESERVE_STACK) - // if(call) - // { - // NANOCLR_FOREACH_NODE_BACKWARD__DIRECT(CLR_RT_StackFrame,ptr,call) - // { - // length++; - // } - // NANOCLR_FOREACH_NODE_BACKWARD_END(); - // } - - // // - // // Limit depth to three callers. - // // - // if(length > 3) length = 3; - // #else - // (void)call; - // #endif - - // CLR_UINT32 totLength = (CLR_UINT32)(sizeof(CLR_RT_HeapBlock_Delegate) + length * - // sizeof(CLR_RT_MethodDef_Index)); - - // CLR_RT_HeapBlock_Delegate* dlg = - // (CLR_RT_HeapBlock_Delegate*)g_CLR_RT_ExecutionEngine.ExtractHeapBytesForObjects( DATATYPE_DELEGATE_HEAD, 0, - // totLength ); CHECK_ALLOCATION(dlg); - - // reference.SetObjectReference( dlg ); - - // dlg->ClearData(); - // dlg->m_cls.Clear(); - // dlg->m_ftn = ftn; - // #if defined(NANOCLR_DELEGATE_PRESERVE_STACK) - // dlg->m_numOfStackFrames = length; - // #endif - - // dlg->m_object.SetObjectReference( nullptr ); - - // #if defined(NANOCLR_APPDOMAINS) - // dlg->m_appDomain = g_CLR_RT_ExecutionEngine.GetCurrentAppDomain(); - // #endif - - // #if defined(NANOCLR_DELEGATE_PRESERVE_STACK) - // if(call) - // { - // CLR_RT_MethodDef_Index* callStack = dlg->GetStackFrames(); - - // NANOCLR_FOREACH_NODE_BACKWARD__DIRECT(CLR_RT_StackFrame,ptr,call) - // { - // if(length-- == 0) break; - - // *callStack++ = ptr->m_call; - // } - // NANOCLR_FOREACH_NODE_BACKWARD_END(); - // } - // #endif - - // NANOCLR_NOCLEANUP(); + // TODO: Implement generic instance creation + NANOCLR_NOCLEANUP_NOLABEL();src/CLR/CorLib/corlib_native_System_String.cpp (1)
1469-1572: Consider extracting numeric formatting to reduce goto usage.The
goto process_numericpattern at line 1571 works correctly but reduces readability. Since the numeric formatting logic (lines 1470-1563) is substantial, consider extracting it to a helper function that can be called from both theDATATYPE_VALUETYPEbranch and the direct numeric type branch.This would eliminate the goto and improve maintainability:
// Helper function signature static const char* FormatNumericArg( CLR_RT_HeapBlock *deref, NanoCLRDataType dt, const char *formatSpec, char *argBuffer, char *negSign, char *decSep);Then both branches could simply call:
argStr = FormatNumericArg(deref, dt, formatSpec, argBuffer, negSign, decSep);src/CLR/CorLib/corlib_native_System_Span_1.cpp (1)
117-121: Inconsistent signed/unsigned comparisons.Line 118 uses
.u4(unsigned) for length comparison, but line 129 uses.s4(signed) for the empty check. While functionally correct (since lengths should be non-negative), this inconsistency could be confusing.Consider using consistent type access:
- if (sourceSpan[FIELD___length].NumericByRefConst().u4 > destinationSpan[FIELD___length].NumericByRefConst().u4) + if (sourceSpan[FIELD___length].NumericByRefConst().s4 > destinationSpan[FIELD___length].NumericByRefConst().s4)Also applies to: 129-132
src/CLR/CorLib/corlib_native_System_Runtime_CompilerServices_RuntimeHelpers.cpp (3)
272-276: Minor: Redundant|= falseoperation.The expression
isRefContainsRefs |= falsehas no effect - OR-ing with false never changes the value. While harmless, it could be simplified.if (dt <= DATATYPE_LAST_NONPOINTER) { // primitive types aren't and don't contain references - isRefContainsRefs |= false; + // no action needed - primitive types don't contain references }
292-296: Minor: Another redundant|= falseoperation.Same as above - this has no effect on the result.
if (totFields == 0) { // this class has no fields, therefore doesn't contain references - isRefContainsRefs |= false; + // no action needed - no fields means no references }
322-325: Unused variables when NANOCLR_INSTANCE_NAMES is not defined.The
typeNameandfieldNamevariables are only used for debugging purposes whenNANOCLR_INSTANCE_NAMESis defined, which may cause compiler warnings in release builds.This is acceptable if the compiler optimizes out unused variables, but you could consider marking them with
(void)typeName; (void)fieldName;or wrapping the entire block more tightly if warnings are a concern.src/CLR/CorLib/corlib_native_System_Reflection_Assembly.cpp (1)
30-37: Consider usingsnprintffor defense in depth.While the buffer sizing appears safe (2x assembly name max), using
snprintfwould provide additional protection against buffer overflows.- sprintf( + snprintf( buffer, + sizeof(buffer), "%s, Version=%d.%d.%d.%d", assm->name, header->version.majorVersion,src/CLR/Core/CLR_RT_HeapBlock_Array.cpp (1)
390-392: Consider making the storage pointer check a runtime error instead of assert.The
ASSERTonly triggers in debug builds. If this condition could occur in production due to a bug, consider returning an error or handling it gracefully instead of silently allowing thememmovein release builds.if (!arraySrc->m_fReference) { - // check that array is not stored in stack - ASSERT(arraySrc->m_StoragePointer == 0); + // check that array is not stored externally + if (arraySrc->m_StoragePointer != 0) + { + NANOCLR_SET_AND_LEAVE(CLR_E_NOT_SUPPORTED); + } memmove(dataDst, dataSrc, length * sizeElem); }azure-pipelines.yml (1)
76-97: Inconsistent authentication header formatsThe
$authvariable is set on line 76 with format":$(GitHubToken)"(empty username), then redefined on line 97 with"nfbot:$(GitHubToken)"(with username). While line 97 properly overrides for PR builds, the inconsistency may cause confusion.Consider using the same format consistently, or remove the redundant assignment on line 76 if it's not used before being overwritten.
src/CLR/CorLib/corlib_native.h (1)
203-303: Significant code duplication in System_String conditional branches.The
#if (NANOCLR_REFLECTION == TRUE)and#elsebranches contain nearly identical method declarations with minor differences. Key differences:
TrimStart___STRINGandTrimEnd___STRING(parameterless) only exist in the NANOCLR_REFLECTION path- Both branches define
Trim___STRING(parameterless)Consider extracting common declarations to reduce duplication and maintenance burden.
One approach is to declare common methods outside the conditional and only wrap the differing methods:
struct Library_corlib_native_System_String { static const int FIELD_STATIC__Empty = 2; + // Common methods for both paths + NANOCLR_NATIVE_DECLARE(CompareTo___I4__OBJECT); + NANOCLR_NATIVE_DECLARE(get_Chars___CHAR__I4); + // ... other common methods ... + #if (NANOCLR_REFLECTION == TRUE) - NANOCLR_NATIVE_DECLARE(CompareTo___I4__OBJECT); - // ... all methods duplicated ... + // Additional methods only for reflection path + NANOCLR_NATIVE_DECLARE(TrimStart___STRING); + NANOCLR_NATIVE_DECLARE(TrimEnd___STRING); #else - NANOCLR_NATIVE_DECLARE(CompareTo___I4__OBJECT); - // ... all methods duplicated again ... + // Non-reflection specific declarations if any #endif };
| HRESULT CLR_Checks::VerifyStackOK(const CLR_RT_StackFrame &stack, const CLR_RT_HeapBlock *top, const int num) | ||
| { | ||
| NANOCLR_HEADER(); | ||
|
|
||
| // Check if stack has enough space for num elements | ||
| if (top + num > stack.m_evalStackEnd) | ||
| { | ||
| NANOCLR_SET_AND_LEAVE(CLR_E_STACK_OVERFLOW); | ||
| } | ||
|
|
||
| // Check if top is within stack bounds | ||
| if (top < stack.m_evalStack || top >= stack.m_evalStackEnd) | ||
| { | ||
| NANOCLR_SET_AND_LEAVE(CLR_E_INVALID_PARAMETER); | ||
| } | ||
|
|
||
| NANOCLR_NOCLEANUP(); | ||
| } |
There was a problem hiding this comment.
Harden VerifyStackOK against negative counts and tighten check ordering
As written, two edge cases are worth addressing:
- If
numis negative, the “has enough space” check becomes ineffective (top + nummoves backwards), and the function will return success even though the call is logically invalid. - You perform
top + numbefore validating thattopis at least within the eval stack range, which makes debugging bad callers harder and does unnecessary pointer arithmetic on an obviously-invalid argument.
A minimal tightening that keeps the same behavior for valid callers:
HRESULT CLR_Checks::VerifyStackOK(const CLR_RT_StackFrame &stack, const CLR_RT_HeapBlock *top, const int num)
{
NANOCLR_HEADER();
- // Check if stack has enough space for num elements
- if (top + num > stack.m_evalStackEnd)
- {
- NANOCLR_SET_AND_LEAVE(CLR_E_STACK_OVERFLOW);
- }
-
- // Check if top is within stack bounds
- if (top < stack.m_evalStack || top >= stack.m_evalStackEnd)
- {
- NANOCLR_SET_AND_LEAVE(CLR_E_INVALID_PARAMETER);
- }
+ // Sanity‑check count
+ if (num < 0)
+ {
+ NANOCLR_SET_AND_LEAVE(CLR_E_INVALID_PARAMETER);
+ }
+
+ // Check that top is within stack bounds before doing pointer arithmetic
+ if (top < stack.m_evalStack || top >= stack.m_evalStackEnd)
+ {
+ NANOCLR_SET_AND_LEAVE(CLR_E_INVALID_PARAMETER);
+ }
+
+ // Check if stack has enough space for num elements
+ if (top + num > stack.m_evalStackEnd)
+ {
+ NANOCLR_SET_AND_LEAVE(CLR_E_STACK_OVERFLOW);
+ }
NANOCLR_NOCLEANUP();
}
This makes the contract around num explicit and ensures you only do the “capacity” computation after confirming top is within the stack interval.
📝 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.
| HRESULT CLR_Checks::VerifyStackOK(const CLR_RT_StackFrame &stack, const CLR_RT_HeapBlock *top, const int num) | |
| { | |
| NANOCLR_HEADER(); | |
| // Check if stack has enough space for num elements | |
| if (top + num > stack.m_evalStackEnd) | |
| { | |
| NANOCLR_SET_AND_LEAVE(CLR_E_STACK_OVERFLOW); | |
| } | |
| // Check if top is within stack bounds | |
| if (top < stack.m_evalStack || top >= stack.m_evalStackEnd) | |
| { | |
| NANOCLR_SET_AND_LEAVE(CLR_E_INVALID_PARAMETER); | |
| } | |
| NANOCLR_NOCLEANUP(); | |
| } | |
| HRESULT CLR_Checks::VerifyStackOK(const CLR_RT_StackFrame &stack, const CLR_RT_HeapBlock *top, const int num) | |
| { | |
| NANOCLR_HEADER(); | |
| // Sanity‑check count | |
| if (num < 0) | |
| { | |
| NANOCLR_SET_AND_LEAVE(CLR_E_INVALID_PARAMETER); | |
| } | |
| // Check that top is within stack bounds before doing pointer arithmetic | |
| if (top < stack.m_evalStack || top >= stack.m_evalStackEnd) | |
| { | |
| NANOCLR_SET_AND_LEAVE(CLR_E_INVALID_PARAMETER); | |
| } | |
| // Check if stack has enough space for num elements | |
| if (top + num > stack.m_evalStackEnd) | |
| { | |
| NANOCLR_SET_AND_LEAVE(CLR_E_STACK_OVERFLOW); | |
| } | |
| NANOCLR_NOCLEANUP(); | |
| } |
🧰 Tools
🪛 Cppcheck (2.18.0)
[information] 10-10: Include file
(missingIncludeSystem)
[information] 12-12: Include file
(missingIncludeSystem)
[information] 13-13: Include file
(missingIncludeSystem)
[information] 15-15: Include file
(missingIncludeSystem)
[information] 19-19: Include file
(missingIncludeSystem)
[information] 22-22: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
🤖 Prompt for AI Agents
In src/CLR/Core/Checks.cpp around lines 10 to 27, the function VerifyStackOK
should first validate that the incoming top pointer is within
[stack.m_evalStack, stack.m_evalStackEnd) and then validate that num is
non-negative before performing top + num arithmetic; update the checks to (1) if
top < stack.m_evalStack || top >= stack.m_evalStackEnd return
CLR_E_INVALID_PARAMETER, (2) if num < 0 return CLR_E_INVALID_PARAMETER, and (3)
if top + num > stack.m_evalStackEnd return CLR_E_STACK_OVERFLOW, ensuring the
pointer arithmetic is only done after the arguments are known-valid.
| _ASSERTE(sizeof(CLR_RT_HeapBlock_WaitForObject) % 4 == 0); | ||
|
|
||
|
|
||
| CLR_UINT32 totLength = | ||
| (CLR_UINT32)(sizeof(CLR_RT_HeapBlock_WaitForObject) + cObjects * sizeof(struct CLR_RT_HeapBlock)); | ||
|
|
||
|
|
||
| _ASSERTE(sizeof(CLR_RT_HeapBlock_WaitForObject) % 4 == 0); |
There was a problem hiding this comment.
Duplicate assertion detected.
The assertion _ASSERTE(sizeof(CLR_RT_HeapBlock_WaitForObject) % 4 == 0) appears twice: on line 30 and again on line 36. The second instance appears to be unintentional duplication.
- _ASSERTE(sizeof(CLR_RT_HeapBlock_WaitForObject) % 4 == 0);
-
CLR_RT_HeapBlock_WaitForObject *wait = EVENTCACHE_EXTRACT_NODE_AS_BYTES(
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/CLR/Core/CLR_RT_HeapBlock_WaitForObject.cpp around lines 30 to 36, there
is a duplicated assertion `_ASSERTE(sizeof(CLR_RT_HeapBlock_WaitForObject) % 4
== 0)` appearing twice; remove the redundant second assertion (the one at line
36) so the size alignment check appears only once, ensuring no change to
surrounding logic or formatting.
|
|
||
| default: | ||
| { | ||
| // value-type, primitive, struct, etc. | ||
| // this->CopyFrom(rhs); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Incomplete implementation: value-types and primitives are not copied.
The default case at lines 781-786 has the copy operation commented out (// this->CopyFrom(rhs);), meaning value-types, primitives, and structs won't be copied after type validation passes.
This appears to be incomplete. Either implement the copy logic or delegate to the existing Reassign:
default:
{
// value-type, primitive, struct, etc.
- // this->CopyFrom(rhs);
- break;
+ NANOCLR_SET_AND_LEAVE(this->Reassign(rhs));
}
Alternatively, if intentional, add a comment explaining why no copy is needed.
🧰 Tools
🪛 Cppcheck (2.18.0)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
🤖 Prompt for AI Agents
In src/CLR/Core/CLR_RT_HeapBlock.cpp around lines 780 to 787, the default switch
case leaves value-types/primitives/structs unhandled with this->CopyFrom(rhs)
commented out; restore proper copy semantics by either calling
this->CopyFrom(rhs) (or this->Reassign(rhs) if that matches surrounding
semantics) so primitives and structs are copied after type validation, and if
omission was intentional replace the commented code with a clear explanatory
comment describing why no copy is required.
| CLR_RT_HeapCluster *freeRegion_hc = nullptr; | ||
| ; | ||
| CLR_RT_HeapBlock_Node *freeRegion = nullptr; |
There was a problem hiding this comment.
Remove stray semicolon.
Line 97 has an extra standalone semicolon that appears to be a merge artifact.
CLR_RT_HeapCluster *freeRegion_hc = nullptr;
- ;
CLR_RT_HeapBlock_Node *freeRegion = nullptr;
📝 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.
| CLR_RT_HeapCluster *freeRegion_hc = nullptr; | |
| ; | |
| CLR_RT_HeapBlock_Node *freeRegion = nullptr; | |
| CLR_RT_HeapCluster *freeRegion_hc = nullptr; | |
| CLR_RT_HeapBlock_Node *freeRegion = nullptr; |
🧰 Tools
🪛 Cppcheck (2.18.0)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
🤖 Prompt for AI Agents
In src/CLR/Core/GarbageCollector_Compaction.cpp around lines 96 to 98, there is
a stray standalone semicolon on line 97 (merge artifact) between the variable
declarations; remove that extra semicolon so the declarations read cleanly
without an empty statement.
| NANOCLR_FOREACH_NODE_END(); | ||
|
|
||
| return NULL; | ||
| return nullptr; |
There was a problem hiding this comment.
Critical inconsistency: Code changes in a documentation-only PR.
The PR objectives state this is a documentation-only change ("Type: documentation" label, "Change type: documentation only"), but this file contains actual code changes—specifically, NULL-to-nullptr conversions. While these modernization changes are technically correct and safe, they should not be included in a documentation-focused PR.
Recommendation:
Either update the PR description to reflect that it includes code modernization changes, or move these code changes to a separate PR and keep this one strictly for documentation updates.
Also applies to: 56-56, 137-137
🤖 Prompt for AI Agents
In src/CLR/Core/RPC/CLR_RT_HeapBlock_EndPoint.cpp around lines 39, 56, and 137,
this PR includes actual code changes (NULL-to-nullptr conversions) despite being
labeled and described as documentation-only; restore the original code in this
PR and move the nullptr modernizations to a separate code-modernization PR, or
alternatively update the PR title/description and labels to accurately indicate
it contains code changes; pick one: either revert the three nullptr edits back
to NULL here and open a new PR with the modernization, or adjust PR metadata and
description to reflect that code changes are included.
| CLR_RT_SignatureParser parser{}; | ||
| parser.Initialize_TypeSpec(genericInstance); | ||
|
|
||
| CLR_RT_SignatureParser::Element element; | ||
|
|
||
| // only need to consume the 1st element which has the level data we're looking for | ||
| parser.Advance(element); | ||
|
|
||
| stack.SetResult_Boolean(td.m_data == g_CLR_RT_WellKnownTypes.m_Array.m_data); | ||
| // check if element has levels | ||
| stack.SetResult_Boolean(element.Levels > 0); | ||
| } |
There was a problem hiding this comment.
Unchecked parser.Advance() return value.
Similar to GetGenericTypeDefinition, the parser.Advance(element) call at line 372 doesn't check the return value.
Consider adding error handling for consistency:
// only need to consume the 1st element which has the level data we're looking for
- parser.Advance(element);
+ if (parser.Advance(element) != S_OK)
+ {
+ 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.
| CLR_RT_SignatureParser parser{}; | |
| parser.Initialize_TypeSpec(genericInstance); | |
| CLR_RT_SignatureParser::Element element; | |
| // only need to consume the 1st element which has the level data we're looking for | |
| parser.Advance(element); | |
| stack.SetResult_Boolean(td.m_data == g_CLR_RT_WellKnownTypes.m_Array.m_data); | |
| // check if element has levels | |
| stack.SetResult_Boolean(element.Levels > 0); | |
| } | |
| CLR_RT_SignatureParser parser{}; | |
| parser.Initialize_TypeSpec(genericInstance); | |
| CLR_RT_SignatureParser::Element element; | |
| // only need to consume the 1st element which has the level data we're looking for | |
| if (parser.Advance(element) != S_OK) | |
| { | |
| NANOCLR_SET_AND_LEAVE(CLR_E_WRONG_TYPE); | |
| } | |
| // check if element has levels | |
| stack.SetResult_Boolean(element.Levels > 0); | |
| } |
🧰 Tools
🪛 Cppcheck (2.18.0)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
🤖 Prompt for AI Agents
In src/CLR/CorLib/corlib_native_System_Type.cpp around lines 366-376,
parser.Advance(element) is called without checking its boolean return; update
the code to check the return value and handle failure consistently with
GetGenericTypeDefinition: if Advance returns false, set
stack.SetResult_Boolean(false) (or the appropriate error/return path used in the
surrounding function) and return immediately to avoid using an invalid element;
ensure any necessary cleanup or error logging mirrors existing patterns in the
file.
| // consume GENERICINST | ||
| parser.Advance(element); | ||
|
|
||
| // consume class | value type | ||
| parser.Advance(element); | ||
|
|
||
| // check if it has generic parameters | ||
| if (element.GenParamCount == 0) | ||
| { | ||
| isTypeDefinition = false; | ||
| } | ||
| else | ||
| { | ||
| // keep reading the generic parameters | ||
| for (int i = 0; i < element.GenParamCount; i++) | ||
| { | ||
| if (SUCCEEDED(parser.Advance(element))) | ||
| { | ||
| if (element.DataType == DATATYPE_VAR || element.DataType == DATATYPE_MVAR) | ||
| { | ||
| // if we find a VAR or MVAR, then this is a generic type definition | ||
| isTypeDefinition = true; | ||
| break; | ||
| } | ||
| else | ||
| { | ||
| // anything else, this is not a generic type definition | ||
| isTypeDefinition = false; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Unchecked parser.Advance() return values and logic concern.
-
Lines 429 and 432 don't check the return value of
parser.Advance(). -
The logic breaks on the first parameter - if a generic type has
<T, int>, it would correctly identify as a definition, but if it has<int, T>, the current logic would incorrectly returnfalseon the firstintparameter.
Apply this diff to fix both issues:
// consume GENERICINST
- parser.Advance(element);
+ if (parser.Advance(element) != S_OK)
+ {
+ NANOCLR_SET_AND_LEAVE(CLR_E_WRONG_TYPE);
+ }
// consume class | value type
- parser.Advance(element);
+ if (parser.Advance(element) != S_OK)
+ {
+ NANOCLR_SET_AND_LEAVE(CLR_E_WRONG_TYPE);
+ }
// check if it has generic parameters
if (element.GenParamCount == 0)
{
isTypeDefinition = false;
}
else
{
// keep reading the generic parameters
for (int i = 0; i < element.GenParamCount; i++)
{
if (SUCCEEDED(parser.Advance(element)))
{
if (element.DataType == DATATYPE_VAR || element.DataType == DATATYPE_MVAR)
{
// if we find a VAR or MVAR, then this is a generic type definition
isTypeDefinition = true;
break;
}
- else
- {
- // anything else, this is not a generic type definition
- isTypeDefinition = false;
- break;
- }
+ // continue checking other parameters
}
}
}
The else branch with break causes early exit on the first non-VAR/MVAR parameter, which is incorrect for partially-closed generics.
🧰 Tools
🪛 Cppcheck (2.18.0)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
🤖 Prompt for AI Agents
In src/CLR/CorLib/corlib_native_System_Type.cpp around lines 428-460, the code
calls parser.Advance(element) at lines ~429 and ~432 without checking return
values and incorrectly breaks out of the generic-parameter loop on the first
non-VAR/MVAR parameter (making <int,T> mis-classified). Fix by: 1) Check the
HRESULT/SUCCEEDED result of the two initial parser.Advance calls and handle
failures (set isTypeDefinition = false and exit or propagate error) before
proceeding; 2) In the GenParamCount loop, on each iteration verify
parser.Advance succeeded and if it fails handle as above; 3) Only set
isTypeDefinition = true and break when you encounter a DATATYPE_VAR or
DATATYPE_MVAR; otherwise continue scanning remaining parameters and only set
isTypeDefinition = false after the loop completes without finding any VAR/MVAR
(do not break on the first non-VAR/MVAR). Ensure no early exit on non-generic
parameters and consistent error handling for Advance failures.
| for (int i = 0; i < res.GenericParamPosition; i++) | ||
| { | ||
| parser.Advance(elem); | ||
| } | ||
|
|
There was a problem hiding this comment.
Bug: Wrong parser used in generic parameter loop.
Line 2864 uses parser.Advance(elem) but should use typeParser.Advance(elem). The variable parser is the outer signature parser that has already been advanced to the original element position, while typeParser is the newly initialized parser intended for walking to the Nth generic-type parameter.
// now we've landed on a DATATYPE_VAR; walk to the Nth generic‐type parameter
typeParser.Advance(elem);
for (int i = 0; i < res.GenericParamPosition; i++)
{
- parser.Advance(elem);
+ typeParser.Advance(elem);
}
📝 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.
| for (int i = 0; i < res.GenericParamPosition; i++) | |
| { | |
| parser.Advance(elem); | |
| } | |
| for (int i = 0; i < res.GenericParamPosition; i++) | |
| { | |
| typeParser.Advance(elem); | |
| } |
🧰 Tools
🪛 Cppcheck (2.18.0)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
[information] Too many #ifdef configurations - cppcheck only checks 12 configurations. Use --force to check all configurations. For more details, use --enable=information.
(toomanyconfigs)
🤖 Prompt for AI Agents
In src/CLR/Debugger/Debugger.cpp around lines 2862 to 2866, the loop advancing
through generic parameters incorrectly calls parser.Advance(elem) (using the
outer signature parser) instead of advancing the newly created typeParser;
change the call to typeParser.Advance(elem) so the loop walks the Nth
generic-type parameter using the correct parser instance, keeping elem and loop
bounds the same.
| else if (array->m_StoragePointer != 0) | ||
| { | ||
| // TODO need to handle this case properly | ||
| ASSERT(FALSE); | ||
| } |
There was a problem hiding this comment.
Incomplete implementation will cause assertion failure.
This new branch for m_StoragePointer != 0 contains only ASSERT(FALSE) and a TODO comment. If this code path is ever reached:
- In debug builds: assertion failure/crash
- In release builds: undefined behavior as
blkremains uninitialized from this branch
Either implement the proper handling or document when this condition can occur and whether it's truly unreachable.
Would you like me to help research the proper handling for arrays with m_StoragePointer set, or should this branch return an error instead of asserting?
🧰 Tools
🪛 Cppcheck (2.18.0)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
[information] Too many #ifdef configurations - cppcheck only checks 12 configurations. Use --force to check all configurations. For more details, use --enable=information.
(toomanyconfigs)
🤖 Prompt for AI Agents
In src/CLR/Debugger/Debugger.cpp around lines 3069-3073, the branch for
array->m_StoragePointer != 0 currently only ASSERTs and leaves blk
uninitialized; replace that with real handling: read the storage pointer value,
validate it, initialize blk and any related size/length variables from the
pointed storage (or convert it to the same representation used in the other
branch), and handle failures by returning an appropriate error code instead of
asserting; if this condition is truly impossible, add a clear comment explaining
why and replace the ASSERT with a defensive return/error to avoid uninitialized
use in release builds.
| static CLR_RT_TypeSpec_Index s_ownerTypeSpec; | ||
| s_ownerTypeSpec.Set(assemblyIndex, fr->OwnerIndex()); |
There was a problem hiding this comment.
Static variable in DumpToken may cause issues in concurrent scenarios.
s_ownerTypeSpec is declared static inside a conditional block, meaning it persists across calls. While diagnostics code is typically single-threaded, this could lead to unexpected behavior if DumpToken is called concurrently or re-entrantly.
Consider making this a local variable if the static lifetime is not intentionally required.
- if (fr->Owner() == TBL_TypeSpec)
- {
- static CLR_RT_TypeSpec_Index s_ownerTypeSpec;
- s_ownerTypeSpec.Set(assemblyIndex, fr->OwnerIndex());
+ if (fr->Owner() == TBL_TypeSpec)
+ {
+ CLR_RT_TypeSpec_Index ownerTypeSpec;
+ ownerTypeSpec.Set(assemblyIndex, fr->OwnerIndex());
📝 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.
| static CLR_RT_TypeSpec_Index s_ownerTypeSpec; | |
| s_ownerTypeSpec.Set(assemblyIndex, fr->OwnerIndex()); | |
| if (fr->Owner() == TBL_TypeSpec) | |
| { | |
| CLR_RT_TypeSpec_Index ownerTypeSpec; | |
| ownerTypeSpec.Set(assemblyIndex, fr->OwnerIndex()); |
🧰 Tools
🪛 Cppcheck (2.18.0)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
🤖 Prompt for AI Agents
In src/CLR/Diagnostics/Info.cpp around lines 460-461, the variable
s_ownerTypeSpec is declared static inside DumpToken which can persist across
calls and cause concurrency/re-entrancy issues; change it to a regular local
CLR_RT_TypeSpec_Index variable (remove the static keyword), initialize it as you
currently do with Set(assemblyIndex, fr->OwnerIndex()), and use this local
instance so each call to DumpToken has its own independent owner type spec.
3 participants
Description
Motivation and Context
How Has This Been Tested?
Screenshots
Types of changes
Checklist
Summary by CodeRabbit
Documentation
New Features
✏️ Tip: You can customize this high-level summary in your review settings.