Fix load and store field IL instructions for value types in stack #3186
Conversation
- Now dealing properly with value types for structs assigned from stack and not from heap. - Fixes in store and load field.
josesimoes
added
the
Area: Interpreter
WalkthroughThe changes update the handling of field load ( Changes
Sequence Diagram(s)sequenceDiagram
participant Thread as CLR_RT_Thread
participant Stack as Evaluation Stack
participant TypeDesc as CLR_RT_TypeDescriptor
Note over Thread: ldfld or stfld instruction
Thread->>Stack: Pop object reference
alt By-reference type
Stack->>Stack: Dereference object
alt Special case (DateTime/TimeSpan)
Thread->>TypeDesc: ExtractObjectAndDataType(dereferenced object)
else Other struct
Stack->>Stack: Treat as raw struct, set type to VALUETYPE
Stack->>Stack: Assert non-null
end
else Not by-reference
Stack->>Stack: Assert non-null
Thread->>TypeDesc: ExtractObjectAndDataType(object)
end
Note over Thread: Proceed with field load/store
✨ Finishing Touches
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 (
|
Automated fixes for code style.
…d71d-e808-4447-aa01-cfb322a45f6c Code style fixes for nanoframework/nf-interpreter PR#3186
josesimoes
force-pushed
the
fix-field-access-stack-objects
branch
from
941c314 to
aa84a32
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: 0
🧹 Nitpick comments (1)
src/CLR/Core/Interpreter.cpp (1)
696-722: Consistent implementation for field stores mirrors the load logic.The store field implementation correctly applies the same by-reference detection and handling logic as the load operation, ensuring symmetric behavior. The special case handling for DateTime and TimeSpan is consistently applied.
One minor observation: there's code duplication between the load and store implementations that could potentially be extracted into a helper function for better maintainability, though this is not critical given the relatively small scope.
Consider extracting the by-reference detection and object resolution logic into a helper function to reduce duplication:
static HRESULT ResolveFieldAccessObject(CLR_RT_HeapBlock*& obj, NanoCLRDataType& dt) { bool instanceIsByRef = (obj->DataType() == DATATYPE_BYREF) || (obj->DataType() == DATATYPE_ARRAY_BYREF); if (instanceIsByRef) { if (obj->Dereference()->DataType() == DATATYPE_DATETIME || obj->Dereference()->DataType() == DATATYPE_TIMESPAN) { NANOCLR_CHECK_HRESULT(CLR_RT_TypeDescriptor::ExtractObjectAndDataType(obj, dt)); } else { dt = DATATYPE_VALUETYPE; obj = obj->Dereference(); FAULT_ON_NULL(obj); } } else { FAULT_ON_NULL(obj); NANOCLR_CHECK_HRESULT(CLR_RT_TypeDescriptor::ExtractObjectAndDataType(obj, dt)); } return S_OK; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/CLR/Core/Interpreter.cpp(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: nf-interpreter (Nightly build) (Check_Build_Options)
- GitHub Check: nf-interpreter (Check_Build_Options)
🔇 Additional comments (1)
src/CLR/Core/Interpreter.cpp (1)
572-598: Well-implemented by-reference type handling for field loads.The logic correctly distinguishes between by-reference struct instances and regular objects, with appropriate special case handling for DateTime and TimeSpan types. The error handling with
FAULT_ON_NULLafter dereferencing is good defensive programming.The implementation addresses the core issue described in the PR objectives by ensuring value types assigned from the stack are handled correctly.
…tack (nanoframework#3186)" This reverts commit 4854f6b.
Description
Motivation and Context
How Has This Been Tested?
Span<T>.[build with MDP buildId 56229]
Screenshots
Types of changes
Checklist
Summary by CodeRabbit