Skip to content

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

Merged
merged 6 commits into from
May 22, 2025
Merged

Implement ldtoken in the interpreter #115723

merged 6 commits into from
May 22, 2025

Conversation

kg
Copy link
Member

@kg kg commented May 19, 2025

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.)

@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label May 19, 2025
@kg kg mentioned this pull request May 19, 2025
61 tasks
@filipnavara filipnavara added area-CodeGen-Interpreter-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels May 20, 2025
kg added 2 commits May 20, 2025 06:59
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
@kg kg force-pushed the interp-ldtoken branch from 85a8f95 to 9d7bc28 Compare May 20, 2025 14:57
Add commented out test for ldtoken on fields and methods
@kg kg marked this pull request as ready for review May 20, 2025 19:20
@Copilot Copilot AI review requested due to automatic review settings May 20, 2025 19:20
@kg kg requested review from BrzVlad and janvorli as code owners May 20, 2025 19:20
Copy link
Contributor

@Copilot Copilot AI left a 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:
Copy link
Preview

Copilot AI May 20, 2025

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.

@kg kg changed the title [WIP] Implement ldtoken in the interpreter Implement ldtoken in the interpreter May 21, 2025
@kg
Copy link
Member Author

kg commented May 21, 2025

Looks like this is broken specifically on unix somehow, looking into it.

@kg kg requested a review from janvorli May 21, 2025 21:15
@kg
Copy link
Member Author

kg commented May 21, 2025

Requesting a quick re-review for the callstub changes.

Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@kg
Copy link
Member Author

kg commented May 22, 2025

/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

@kg kg merged commit e163124 into dotnet:main May 22, 2025
100 of 102 checks passed
SimaTian pushed a commit that referenced this pull request May 27, 2025
* 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.)
@github-actions github-actions bot locked and limited conversation to collaborators Jun 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants