-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Implement ldtoken in the interpreter #115723
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Add a 'self' value to the InterpMethod header so we can detect invalid InterpMethod pointers early instead of crashing downstream Cleanup, expand test Repair merge damage
Add commented out test for ldtoken on fields and methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request implements support for a new LDTOKEN opcode in the interpreter, including a basic test for type tokens and additional integrity checks for interpreter methods.
- Added new opcode definition and implementation for LDTOKEN across interpreter and code generator files
- Introduced integrity assertions in InterpMethod and updated relevant tests to include Ldtoken verification
- Extended the code generation logic in the compiler to handle token resolution and helper function lookup for LDTOKEN
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
src/tests/JIT/interpreter/Interpreter.cs | Added TestLdtoken to call the new opcode and verify type tokens |
src/coreclr/vm/interpexec.cpp | Implemented the LDTOKEN opcode and added integrity assertions |
src/coreclr/interpreter/intops.def | Registered the new INTOP_LDTOKEN opcode |
src/coreclr/interpreter/interpretershared.h | Updated InterpMethod by adding a self pointer for validation |
src/coreclr/interpreter/compiler.cpp | Extended code generation for CEE_LDTOKEN with token resolution |
Comments suppressed due to low confidence (2)
src/tests/JIT/interpreter/Interpreter.cs:972
- TestLdtoken currently verifies only type tokens. Consider adding a comment or expanding the test cases to clarify that method and field tokens are not supported at this time.
public static bool TestLdtoken()
src/coreclr/interpreter/compiler.cpp:3746
- Verify that the instruction pointer increment (m_ip += 5) in the CEE_LDTOKEN case matches the expected operand width for this opcode and is consistent with the interpreter's ip advancement logic.
m_ip += 5;
@@ -1423,6 +1426,21 @@ do { \ | |||
STELEM(double, double); | |||
break; | |||
} | |||
case INTOP_LDTOKEN: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review the ldtoken opcode implementation to ensure that the helper function resolution using the INTERP_INDIRECT_HELPER_TAG flag works correctly for all token types.
Copilot uses AI. Check for mistakes.
Looks like this is broken specifically on unix somehow, looking into it. |
Requesting a quick re-review for the callstub changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
/ba-g one lane is stuck (Build android-x64 Release AllSubsets_CoreCLR). This PR only modifies the interpreter, which is limited to debug/checked configurations, not Release |
* Implements a new LDTOKEN interpreter opcode with support for method/field/type tokens. Only type tokens have been tested, it's not currently possible to test the other two. * Adds an integrity check header to InterpMethod because the behavior if we jump to something that isn't a valid InterpMethod is very messy and harder to debug. * Fixes a bug in the CallStub generator for sysv (we weren't handling certain argument types.)
Implements a new LDTOKEN opcode with support for method/field/type tokens. Only type tokens have been tested, it doesn't seem to be possible to generate the other two with C#.
Also adds an integrity check header to InterpMethod because the behavior if we jump to something that isn't a valid InterpMethod is very messy and harder to debug.
Fixes a bug in the CallStub generator for sysv as well (we weren't handling certain argument types.)