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. |
| @@ -416,6 +416,9 @@ public static void RunInterpreterTests() | |||
| if (!TestArray()) | |||
| 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."); |
Copilot uses AI. Check for mistakes.
| 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.
Copilot uses AI. Check for mistakes.
| 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. |
Copilot uses AI. Check for mistakes.
am11
added
area-CodeGen-Interpreter-coreclr
and removed
area-Codegen-Interpreter-mono
labels
|
Tagging subscribers to this area: @BrzVlad, @janvorli, @kg |
| @@ -3500,6 +3500,16 @@ int InterpCompiler::GenerateCode(CORINFO_METHOD_INFO* methodInfo) | |||
| m_pLastNewIns->SetDVar(m_pStackPointer[-1].var); | |||
| m_ip++; | |||
| 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
No description provided.