Represent managed-return-values as native vars#128479
Conversation
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
This PR updates CoreCLR JIT/VM debug-info plumbing to represent managed call return values as explicit ICorDebugInfo::NativeVarInfo entries (via a new CALL_RETURN_ILNUM sentinel plus a callReturnValueILOffset field), instead of relying on CALL_INSTRUCTION IP mapping entries. It also updates the compression/writer paths to encode this new data.
Changes:
- Extended
NativeVarInfo(and the shared JIT interface model) withcallReturnValueILOffset, and addedCALL_RETURN_ILNUM/ updatedMAX_ILNUM. - Updated debug info encoding/compression to treat
CALL_RETURN_ILNUMspecially (encode call IL offset; implicit native end offset). - Updated the JIT to collect per-call return-value location info and removed call-site
CALL_INSTRUCTIONmapping emission in multiple emitters.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/vm/debuginfostore.cpp | Adds special-case encoding/decoding for CALL_RETURN_ILNUM and implicit fields. |
| src/coreclr/tools/Common/JitInterface/CorInfoTypes.VarInfo.cs | Extends NativeVarInfo model with callReturnValueILOffset. |
| src/coreclr/tools/Common/JitInterface/CorInfoTypes.cs | Adds CALL_RETURN_ILNUM and updates MAX_ILNUM. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/DebugInfoTableNode.cs | Updates R2R var blob writer to encode call-return entries differently. |
| src/coreclr/jit/scopeinfo.cpp | Adds emission of call-return var entries; updates getSiVarLoc signature/use. |
| src/coreclr/jit/emitxarch.cpp | Removes call-site mapping emission previously used for return value reporting. |
| src/coreclr/jit/emitwasm.cpp | Removes call-site mapping emission previously used for return value reporting. |
| src/coreclr/jit/emitriscv64.cpp | Removes call-site mapping emission previously used for return value reporting. |
| src/coreclr/jit/emitloongarch64.cpp | Removes call-site mapping emission previously used for return value reporting. |
| src/coreclr/jit/emitarm64.cpp | Removes call-site mapping emission previously used for return value reporting. |
| src/coreclr/jit/emitarm.cpp | Removes call-site mapping emission previously used for return value reporting. |
| src/coreclr/jit/emit.h | Replaces EmitCallParams.debugInfo with returnValueCall. |
| src/coreclr/jit/ee_il_dll.cpp | Updates var reporting storage to carry call IL offset; improves display of call-return entries. |
| src/coreclr/jit/compiler.h | Extends VarResultInfo and updates eeSetLVinfo signature for call return metadata. |
| src/coreclr/jit/codegenxarch.cpp | Switches call emission plumbing to pass returnValueCall. |
| src/coreclr/jit/codegenwasm.cpp | Switches call emission plumbing to pass returnValueCall. |
| src/coreclr/jit/codegenlinear.cpp | Initializes storage for emitted call-return info. |
| src/coreclr/jit/codegeninterface.h | Adds EmittedCallReturnInfo container and updates getSiVarLoc signature. |
| src/coreclr/jit/codegencommon.cpp | Captures return-value location after emitting a call and records it for scope info. |
| src/coreclr/jit/codegenarmarch.cpp | Switches call emission plumbing to pass returnValueCall. |
| src/coreclr/inc/cordebuginfo.h | Adds CALL_RETURN_ILNUM and extends NativeVarInfo with callReturnValueILOffset. |
Comments suppressed due to low confidence (2)
src/coreclr/jit/scopeinfo.cpp:516
getSiVarLocnow takesunsigned offsetand then addsvarDsc->GetStackOffset()(a signed stack offset that can be negative). This can underflow/wrap for typical frame-pointer locals (negative offsets), and then gets converted tointwhen constructingsiVarLoc, producing incorrect stack locations in debug info.
CodeGenInterface::siVarLoc CodeGenInterface::getSiVarLoc(const LclVarDsc* varDsc, unsigned offset, unsigned stackLevel) const
{
// For stack vars, find the base register, and offset
regNumber baseReg;
offset += varDsc->GetStackOffset();
if (!varDsc->lvFramePointerBased)
{
baseReg = REG_SPBASE;
offset += stackLevel;
}
else
{
baseReg = REG_FPBASE;
}
return CodeGenInterface::siVarLoc(varDsc, baseReg, offset, isFramePointerUsed());
}
src/coreclr/jit/emit.h:496
EmitCallParamsremoved thedebugInfofield, but there are still target-specific codegen paths assigningparams.debugInfo(e.g., incodegenloongarch64.cppandcodegenriscv64.cpp). This will break builds for those targets unless they are updated to the newreturnValueCallmechanism.
struct EmitCallParams
{
EmitCallType callType = EC_COUNT;
CORINFO_METHOD_HANDLE methHnd = NO_METHOD_HANDLE;
#ifdef DEBUG
// Used to report call sites to the EE
CORINFO_SIG_INFO* sigInfo = nullptr;
#endif
void* addr = nullptr;
ssize_t argSize = 0;
emitAttr retSize = EA_PTRSIZE;
// For multi-reg args with GC returns in the second arg
emitAttr secondRetSize = EA_UNKNOWN;
bool hasAsyncRet = false;
BitVec ptrVars = BitVecOps::UninitVal();
regMaskTP gcrefRegs = RBM_NONE;
regMaskTP byrefRegs = RBM_NONE;
regNumber ireg = REG_NA;
regNumber xreg = REG_NA;
unsigned xmul = 0;
ssize_t disp = 0;
bool isJump = false;
bool noSafePoint = false;
// If this call should have managed return value debug info associated with it, this is the call to associate it with.
GenTreeCall* returnValueCall = nullptr;
#ifdef TARGET_WASM
CORINFO_WASM_TYPE_SYMBOL_HANDLE wasmSignature = nullptr;
#endif
};
noahfalk
left a comment
There was a problem hiding this comment.
Thanks @jakobbotsch! I don't have the background knowledge to usefully review the JIT code but overall this looked like what I was hoping for. Since the debug info reader and ICorDebug code will need to stay synced with the writer let me see if I can get someone on my team to handle the remainder so we could put the whole thing in as one atomic commit.
|
I believe MRV is only used in live debugging scenarios, which requires consuming these changes in the DBI. I think we could consume the change in |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 32 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
src/coreclr/jit/emit.h:496
EmitCallParamsdropped theDebugInfo debugInfofield, but there are still backend codegen paths assigning toparams.debugInfo(e.g.,codegenriscv64.cpp/codegenloongarch64.cpp). As-is this will fail to compile for those targets; they likely need to be updated to usereturnValueCall(or to stop emitting managed return value info on those backends).
struct EmitCallParams
{
EmitCallType callType = EC_COUNT;
CORINFO_METHOD_HANDLE methHnd = NO_METHOD_HANDLE;
#ifdef DEBUG
// Used to report call sites to the EE
CORINFO_SIG_INFO* sigInfo = nullptr;
#endif
void* addr = nullptr;
ssize_t argSize = 0;
emitAttr retSize = EA_PTRSIZE;
// For multi-reg args with GC returns in the second arg
emitAttr secondRetSize = EA_UNKNOWN;
bool hasAsyncRet = false;
BitVec ptrVars = BitVecOps::UninitVal();
regMaskTP gcrefRegs = RBM_NONE;
regMaskTP byrefRegs = RBM_NONE;
regNumber ireg = REG_NA;
regNumber xreg = REG_NA;
unsigned xmul = 0;
ssize_t disp = 0;
bool isJump = false;
bool noSafePoint = false;
// If this call should have managed return value debug info associated with it, this is the call to associate it with.
GenTreeCall* returnValueCall = nullptr;
#ifdef TARGET_WASM
CORINFO_WASM_TYPE_SYMBOL_HANDLE wasmSignature = nullptr;
#endif
};
|
Added the debugger and cDAC support :) |
|
@jakobbotsch fyi |
Thanks for doing that! I thought this would need change to the interface. |
Rework interpreter managed-return-value (MRV) support to use the CALL_RETURN_ILNUM native-variable contract introduced in dotnet#128479 instead of the obsolete CALL_INSTRUCTION source-map encoding. The interpreter now emits one ICorDebugInfo::CALL_RETURN_ILNUM NativeVarInfo entry per value-returning managed call site through the standard setVars path. Each entry describes the call's destination var (an FP-relative stack slot) as the return value home, with callReturnValueILOffset set to the call's IL offset and startOffset set to the post-call native offset. The generic DI consumer (GetReturnValueVariableHomes / GetNativeVariable) handles the VLT_STK location identically to ordinary interpreter locals, so all of the PR's former DI-side instruction-decoding additions are obsolete and dropped (reverted to main): module.cpp, rsthread.cpp, rspriv.h, dacdbiimpl.cpp, and the dacdbistructures isInterpreted plumbing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PR dotnet#128479 introduced emitting managed return values as native vars but used storeVariableInRegisters(REG_FLOATRET, REG_NA) for float returns, which sets vlType=VLT_REG (integer register). The DBI then fails to read the return value because it looks in an integer register instead of XMM0. Fix by using VLT_REG_FP with a 0-based FP register index, which is what the DBI's VLT_REG_FP handler expects (it adds REGISTER_AMD64_XMM0 to convert to CorDebugRegister). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Update representation of managed return values to follow #128397.