Implement sizeof() in the interpreter #115745
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request implements the sizeof() functionality within the interpreter and introduces a corresponding test.
- Added a new test method (TestSizeof) in the interpreter tests.
- Introduced a new case for CEE_SIZEOF in the JIT interpreter compiler.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/tests/JIT/interpreter/Interpreter.cs | Added TestSizeof() and integrated its execution with FailFast. |
| src/coreclr/interpreter/compiler.cpp | Added a new switch-case branch for the CEE_SIZEOF instruction. |
| Environment.FailFast(null); | ||
|
|
||
| if (!TestSizeof()) | ||
| Environment.FailFast(null); |
There was a problem hiding this comment.
Consider providing a descriptive error message in Environment.FailFast to aid debugging if TestSizeof fails.
| Environment.FailFast(null); | |
| Environment.FailFast("TestSizeof failed."); |
| return false; | ||
| if (sizeof(double) != 8) | ||
| return false; | ||
| if (sizeof(MyStruct) != 4) |
There was a problem hiding this comment.
[nitpick] Consider adding a comment explaining the assumptions behind MyStruct's size (e.g. packing or field types) to clarify why the expected size is 4.
| AddIns(INTOP_LDC_I4); | ||
| m_pLastNewIns->data[0] = m_compHnd->getClassSize(clsHnd); | ||
| PushStackType(StackTypeI4, NULL); | ||
| m_pLastNewIns->SetDVar(m_pStackPointer[-1].var); |
There was a problem hiding this comment.
[nitpick] Consider adding a comment to explain the reason for incrementing m_ip by 5, which can help clarify the instruction length and avoid confusion.
| m_pLastNewIns->SetDVar(m_pStackPointer[-1].var); | |
| m_pLastNewIns->SetDVar(m_pStackPointer[-1].var); | |
| // Increment the instruction pointer by 5 to account for the 1-byte opcode | |
| // and the 4-byte operand (e.g., a token or offset) of the CEE_SIZEOF instruction. |
am11
added
area-CodeGen-Interpreter-coreclr
and removed
area-Codegen-Interpreter-mono
labels
|
Tagging subscribers to this area: @BrzVlad, @janvorli, @kg |
| break; | ||
| case CEE_SIZEOF: | ||
| { | ||
| CORINFO_CLASS_HANDLE clsHnd = ResolveClassToken(getU4LittleEndian(m_ip + 1)); |
There was a problem hiding this comment.
It seems it would make sense to add CHECK_STACK(1);
There was a problem hiding this comment.
I see, I thought CHECK_STACK was "make sure this many items are on the stack"
There was a problem hiding this comment.
Ah, I guess I am wrong in this case, I haven't realized the token is not on the stack.
Implements CEE_SIZEOF in the interpreter
4 participants
No description provided.