-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Fix debugger interaction in the interpreter #118699
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
Fix debugger interaction in the interpreter #118699
Conversation
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 PR fixes various points in the CoreCLR runtime to properly handle the interpreter precode when interacting with the debugger. The changes ensure that the interpreter precode is considered "part" of the method code during debugging operations and code analysis.
Key changes:
- Modifies code discovery logic to handle interpreter precodes by following them to their actual bytecode addresses
- Updates method compilation checks to use
HasNativeCode()
instead of direct null checks - Removes obsolete interpreter-specific code from event tracing that's now handled more generically
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
src/coreclr/vm/multicorejitplayer.cpp | Updates method compilation check to use HasNativeCode() method |
src/coreclr/vm/method.cpp | Adds interpreter precode handling in NonVirtualEntry2MethodDesc function |
src/coreclr/vm/jitinterface.cpp | Adds interpreter precode detection and bytecode address resolution in EECodeInfo::Init |
src/coreclr/vm/eventtrace.cpp | Removes obsolete interpreter-specific code handling |
src/coreclr/debug/daccess/task.cpp | Refactors code address retrieval in GetRepresentativeEntryAddress |
src/coreclr/debug/daccess/dacdbiimpl.cpp | Fixes hot region initialization to include start address |
src/coreclr/vm/method.cpp
Outdated
return (MethodDesc*)((StubPrecode*)PCODEToPINSTR(entryPoint))->GetMethodDesc(); | ||
StubPrecode stubPrecode = (StubPrecode*)PCODEToPINSTR(entryPoint); | ||
#ifdef FEATURE_INTERPRETER | ||
if (stubPrecode->GetType() == PRECODE_INTERPRETER)) |
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.
There is an extra closing parenthesis that will cause a compilation error.
if (stubPrecode->GetType() == PRECODE_INTERPRETER)) | |
if (stubPrecode->GetType() == PRECODE_INTERPRETER) |
Copilot uses AI. Check for mistakes.
src/coreclr/vm/method.cpp
Outdated
#ifdef FEATURE_INTERPRETER | ||
if (stubPrecode->GetType() == PRECODE_INTERPRETER)) | ||
{ | ||
entryPoint = dac_cast<PTR_InterpreterPrecode>(pStubPrecode)->GetData()->ByteCodeAddr; |
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.
The variable 'pStubPrecode' is used but not defined. It should be 'stubPrecode' to match the variable declared on line 2226.
entryPoint = dac_cast<PTR_InterpreterPrecode>(pStubPrecode)->GetData()->ByteCodeAddr; | |
entryPoint = dac_cast<PTR_InterpreterPrecode>(stubPrecode)->GetData()->ByteCodeAddr; |
Copilot uses AI. Check for mistakes.
else | ||
#endif // FEATURE_INTERPRETER | ||
{ | ||
return (MethodDesc*)(stubPrecode)->GetMethodDesc(); |
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.
The parentheses around 'stubPrecode' are unnecessary and the cast should be removed. It should be 'return stubPrecode->GetMethodDesc();'
return (MethodDesc*)(stubPrecode)->GetMethodDesc(); | |
return stubPrecode->GetMethodDesc(); |
Copilot uses AI. Check for mistakes.
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.
I think it wants (MethodDesc*)(stubPrecode->GetMethodDesc())
, which feels better to me also
Tagging subscribers to this area: @BrzVlad, @janvorli, @kg |
PTR_StubPrecode pStubPrecode = dac_cast<PTR_StubPrecode>(PCODEToPINSTR(codeAddress)); | ||
if (pStubPrecode->GetType() == PRECODE_INTERPRETER) | ||
{ | ||
codeAddress = dac_cast<PTR_InterpreterPrecode>(pStubPrecode)->GetData()->ByteCodeAddr; |
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.
This will end up creating codeinfo for a different IP than the one passed into Init. I can imagine that it is fixing a problem somewhere, but it is likely creating different problems somewhere else. Is this going to break places like
runtime/src/coreclr/vm/threadsuspend.cpp
Lines 5787 to 5789 in 2a95831
EECodeInfo codeInfo(ip); | |
if (!codeInfo.IsValid()) | |
return; |
What is the exact path that motivated this fix? |
@jkotas, the interpreter precode is built as part of the interpreter codegen process, and placed into the NativeCode slot in the exact same place that the jitted code would go. Thus it gets used as the proxy for the interpreter compiled byte code all over the codebase. The path which is most visible is that this impacts the way the debugger looks at the process. The easiest path to hit where this fixes things is the behavior of !clrstack. Without this fix, the DAC fails to validate that any |
runtime/src/coreclr/vm/method.cpp Line 2201 in 6f0ce34
STUB_CODE_BLOCK_STUBPRECODE path should take care of it. Is that right?
If there is a need to map interpreter precode to MethodDesc, my thinking is that it should go through |
I've merged a less complete version of this which is enough for !clrstack. Possibly we may want to pull some more of this logic in the future, but for now, this should be enough. |
Fix various points in the runtime which need to handle the situation of the interpreter precode existing, and allowing the interpreter precode to be considered "part" of the code of the method.
I expect that we may eventually need to treat the interpreter precode as either the "hot" or "cold" code region of the function, but I'm not certain of that yet.