Fix handling VAR data type #3169
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 refactor how generic type parameter resolution (for Changes
Sequence Diagram(s)sequenceDiagram
participant ExecutionEngine
participant SignatureParser
participant TypeSystem
participant Debugger
ExecutionEngine->>SignatureParser: Initialize parser for locals signature
ExecutionEngine->>SignatureParser: Advance to generic parameter position
SignatureParser-->>ExecutionEngine: Return class and data type
Debugger->>SignatureParser: Initialize parser for method/locals signature
Debugger->>SignatureParser: Advance to argument/local index, then generic param
SignatureParser-->>Debugger: Return class and data type
TypeSystem->>SignatureParser: Initialize parser for TypeSpec signature
TypeSystem->>SignatureParser: Advance to generic parameter position
SignatureParser-->>TypeSystem: Return TypeDef_Index and data type
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
josesimoes
force-pushed
the
fix-processing-datatype-var
branch
from
95cbf83 to
06fb34d
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/CLR/Core/Execution.cpp(1 hunks)src/CLR/Core/TypeSystem.cpp(1 hunks)src/CLR/Debugger/Debugger.cpp(2 hunks)src/CLR/Include/nanoCLR_Runtime.h(0 hunks)
💤 Files with no reviewable changes (1)
- src/CLR/Include/nanoCLR_Runtime.h
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALX)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALTHREE)
- GitHub Check: nf-interpreter (Check_Code_Style)
- GitHub Check: nf-interpreter (Nightly build) (Check_Build_Options)
- GitHub Check: nf-interpreter (Check_Build_Options)
🔇 Additional comments (3)
src/CLR/Core/Execution.cpp (1)
1974-1994: Improved generic parameter resolution for type-level generics.The change enhances handling of
DATATYPE_VARby directly parsing generic parameters from the signature blob instead of using the removedFindGenericParamAtTypeSpec()function. This properly resolves type-level generic parameters in locals signatures.src/CLR/Debugger/Debugger.cpp (1)
2854-2864: UncheckedAdvance()results may crash the debuggerEvery call to
parser.Advance(elem)returns anHRESULT.
In the new loop that walks toGenericParamPositionthe return value is ignored:for (int i = 0; i < res.GenericParamPosition; i++) { parser.Advance(elem); // <- ignored }If the signature is malformed or shorter than expected the function will return a failing
HRESULT, leavingelemundefined and propagating bogustargetClass/targetDataType.
Always propagate the error:for (int i = 0; i < res.GenericParamPosition; i++) { - parser.Advance(elem); + NANOCLR_CHECK_HRESULT(parser.Advance(elem)); }src/CLR/Core/TypeSystem.cpp (1)
989-1018: Implementation improved for generic parameter resolution in VAR data typesThe new implementation directly parses the signature stream to resolve generic parameters, which is a more robust approach than the previous method. This enables proper support for type-level generic arguments beyond just method-generic parameters.
The code now:
- Creates a signature parser initialized with the TypeSpec
- Advances once to consume the GENERICINST or VAR entry
- Walks forward through the signature elements to the correct generic parameter position
- Extracts the corresponding TypeDef_Index directly from the signature
- Properly populates all instance fields with the resolved type information
This implementation properly handles error cases and includes helpful comments that explain the parsing logic.
| // this element.Class is the TypeDef_Index for 'T' | ||
| targetClass = elem.Class; | ||
| targetDataType = elem.DataType; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
targetClass / targetDataType may remain uninitialised
When any of the above Advance operations fail the two out-parameters are left untouched yet later used to patch tmp.
Ensure they are initialised (e.g. to CLR_RT_TypeDef_Index::Invalid() / DATATYPE_VOID) right before the parsing starts, and only used when the parsing was successful.
src/CLR/Debugger/Debugger.cpp
Outdated
| if (cmd->m_kind == CLR_DBG_Commands::Debugging_Value_GetStack::c_Argument) | ||
| { | ||
| parser.Initialize_MethodSignature(&md); | ||
|
|
||
| // skip the return value entry | ||
| iElement++; | ||
|
|
||
| // if there's an implicit 'this', adjust index | ||
| if (parser.Flags & PIMAGE_CEE_CS_CALLCONV_HASTHIS) | ||
| { | ||
| if (iElement == 0) | ||
| { | ||
| // requested the 'this' argument – invalid for a primitive | ||
| return false; | ||
| } | ||
| iElement--; | ||
| } | ||
| } |
There was a problem hiding this comment.
iElement is reused after it has under-flown – element selection becomes undefined
iElement was decremented to 0 in the first walk (do { Advance } while(iElement--)).
After the loop it holds UINT32_MAX; re-using it here:
iElement++; // 0x00000000 → 0x00000001 ?? NO – value is 0xFFFFFFFF
...
if (parser.Flags & PIMAGE_CEE_CS_CALLCONV_HASTHIS)
{
if (iElement == 0) ...
iElement--;
}means the subsequent element navigation is performed with an invalid large index, so the newly built parser will advance way past the end of the signature, or – worse – trap on the first Advance.
Fix by using a fresh local variable for the second traversal:
-// advance to the requested argument/local
-do { parser.Advance(elem); } while (iElement-- > 0);
+uint32_t localIndex = cmd->m_index; // fresh copy
+if (kind == c_Argument) { localIndex++; /* skip ret */ }
+...
+while (localIndex-- > 0) { NANOCLR_CHECK_HRESULT(parser.Advance(elem)); }
src/CLR/Debugger/Debugger.cpp
Outdated
| // type‐level generic parameter in a generic TypeDef: re‐parse the signature | ||
| CLR_RT_SignatureParser parser; | ||
|
|
||
| // isGenericInstance = true; | ||
| // for arguments, initialize over the method signature and skip the return slot | ||
| if (cmd->m_kind == CLR_DBG_Commands::Debugging_Value_GetStack::c_Argument) | ||
| { | ||
| parser.Initialize_MethodSignature(&md); | ||
|
|
||
| // skip the return value entry | ||
| iElement++; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
parser is re-declared and shadows an outer variable – easy to mis-read and introduces lifetime bugs
There is already a CLR_RT_SignatureParser parser{} defined a few lines above (2787).
Declaring a second variable with the same name inside this inner scope:
CLR_RT_SignatureParser parser; // shadows outer parsersilently hides the outer instance and makes the code harder to reason about.
If a future maintainer inserts code below this block that relies on the first parser, the compiler will accept it but the logic will be wrong.
-CLR_RT_SignatureParser parser;
+CLR_RT_SignatureParser sigParser;Rename the inner parser (or simply re-use the outer one after Initialize_*()), so that shadowing cannot occur.
josesimoes
force-pushed
the
fix-processing-datatype-var
branch
6 times, most recently
from
68c3e43 to
88c6138
Compare
- Now it's parsing it in the code instead of calling FindGenericParamAtTypeSpec(). - Remove declaration and implementation of FindGenericParamAtTypeSpec().
josesimoes
force-pushed
the
fix-processing-datatype-var
branch
from
88c6138 to
4391171
Compare
Description
Motivation and Context
How Has This Been Tested?
[build with MDP buildId 55899]
Screenshots
Types of changes
Checklist
Summary by CodeRabbit