From 84cb8acf6f3a965b355b282a4a5b35191351cb89 Mon Sep 17 00:00:00 2001 From: Tom McDonald Date: Sun, 26 Apr 2026 21:20:53 -0400 Subject: [PATCH 1/4] Interpreter: fix stack walk reporting wrong source line for non-void calls - Filter IL-to-native mapping to statement boundaries - Emit real NOP after call instructions so return address falls within call range - Add INTOP_NOP handler in interpreter executor Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/coreclr/interpreter/compiler.cpp | 15 +++++++++++++-- src/coreclr/vm/eetwain.cpp | 1 + src/coreclr/vm/interpexec.cpp | 3 +++ 3 files changed, 17 insertions(+), 2 deletions(-) diff --git a/src/coreclr/interpreter/compiler.cpp b/src/coreclr/interpreter/compiler.cpp index 045839aa29504a..cddc47bdf67707 100644 --- a/src/coreclr/interpreter/compiler.cpp +++ b/src/coreclr/interpreter/compiler.cpp @@ -1054,7 +1054,9 @@ int32_t* InterpCompiler::EmitCodeIns(int32_t *ip, InterpInst *ins, TArraynativeOffset); - if ((m_ILToNativeMapSize == 0) || (m_pILToNativeMap[m_ILToNativeMapSize - 1].ilOffset != ilOffset)) + // Only emit mapping entries at IL offsets where the evaluation stack is empty or on NOP instructions + if ((ins->flags & INTERP_INST_FLAG_EMPTY_IL_STACK) && + ((m_ILToNativeMapSize == 0) || (m_pILToNativeMap[m_ILToNativeMapSize - 1].ilOffset != ilOffset))) { // This code assumes that instructions for the same IL offset are emitted in a single run without // any other IL offsets in between and that they don't repeat again after the run ends. @@ -1076,7 +1078,7 @@ int32_t* InterpCompiler::EmitCodeIns(int32_t *ip, InterpInst *ins, TArrayflags & INTERP_INST_FLAG_EMPTY_IL_STACK) ? ICorDebugInfo::STACK_EMPTY : ICorDebugInfo::SOURCE_TYPE_INVALID; + m_pILToNativeMap[m_ILToNativeMapSize].source = ICorDebugInfo::STACK_EMPTY; m_ILToNativeMapSize++; } } @@ -1115,14 +1117,23 @@ int32_t *InterpCompiler::EmitBBCode(int32_t *ip, InterpBasicBlock *bb, TArraynativeOffset = (int32_t)(ip - m_pMethodCode); + bool prevWasCall = false; for (InterpInst *ins = bb->pFirstIns; ins != NULL; ins = ins->pNext) { if (InterpOpIsEmitNop(ins->opcode)) { ins->nativeOffset = (int32_t)(ip - m_pMethodCode); + if (prevWasCall) + { + // Emit a real NOP so the return address after a call lands within + // the call's native offset range, not on the next statement boundary. + *ip++ = INTOP_NOP; + prevWasCall = false; + } continue; } + prevWasCall = InterpOpIsDirectCall(ins->opcode) || InterpOpIsIndirectCall(ins->opcode); ip = EmitCodeIns(ip, ins, relocs); } diff --git a/src/coreclr/vm/eetwain.cpp b/src/coreclr/vm/eetwain.cpp index a1ec17c0002fa9..0bebc952d4d64b 100644 --- a/src/coreclr/vm/eetwain.cpp +++ b/src/coreclr/vm/eetwain.cpp @@ -2104,6 +2104,7 @@ static void VirtualUnwindInterpreterCallFrame(TADDR sp, T_CONTEXT *pContext) pFrame = pFrame->pParent; if (pFrame != NULL) { + // The parent frame's IP points past the CALL/CALLVIRT instruction (the resumption point). SetIP(pContext, (TADDR)pFrame->ip); SetSP(pContext, dac_cast(pFrame)); SetFP(pContext, (TADDR)pFrame->pStack); diff --git a/src/coreclr/vm/interpexec.cpp b/src/coreclr/vm/interpexec.cpp index d36bb50efde8d6..10a9d34dda0e48 100644 --- a/src/coreclr/vm/interpexec.cpp +++ b/src/coreclr/vm/interpexec.cpp @@ -1421,6 +1421,9 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr MemoryBarrier(); ip++; break; + case INTOP_NOP: + ip++; + break; #ifndef FEATURE_PORTABLE_ENTRYPOINTS case INTOP_STORESTUBCONTEXT: { From 0e4df8f1b915132f4f6742f4a9598b8e8b58ba5f Mon Sep 17 00:00:00 2001 From: Tom McDonald Date: Mon, 27 Apr 2026 15:58:16 -0400 Subject: [PATCH 2/4] Only emit post-call NOP for debug builds Gate the post-call NOP emission on CORJIT_FLAG_DEBUG_CODE so that release/optimized interpreter code does not pay for the extra instruction. Debug builds need the NOP to preserve the native offset gap between a call and the next statement for correct debugger stack walk line numbers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/coreclr/interpreter/compiler.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/interpreter/compiler.cpp b/src/coreclr/interpreter/compiler.cpp index cddc47bdf67707..e8e76d5a34969e 100644 --- a/src/coreclr/interpreter/compiler.cpp +++ b/src/coreclr/interpreter/compiler.cpp @@ -1123,7 +1123,7 @@ int32_t *InterpCompiler::EmitBBCode(int32_t *ip, InterpBasicBlock *bb, TArrayopcode)) { ins->nativeOffset = (int32_t)(ip - m_pMethodCode); - if (prevWasCall) + if (prevWasCall && m_corJitFlags.IsSet(CORJIT_FLAGS::CORJIT_FLAG_DEBUG_CODE)) { // Emit a real NOP so the return address after a call lands within // the call's native offset range, not on the next statement boundary. From c034a3165f745e1a8b8a22dcbae39a603358abee Mon Sep 17 00:00:00 2001 From: Tom McDonald Date: Mon, 27 Apr 2026 18:48:07 -0400 Subject: [PATCH 3/4] Address PR feedback: use INTOP_DEBUG_SEQ_POINT and fix comments - Replace INTOP_NOP with INTOP_DEBUG_SEQ_POINT for post-call gap emission, since INTOP_NOP is in the non-executable IROP range (per JanV's suggestion) - Remove INTOP_NOP execution handler from interpexec.cpp (no longer needed) - Fix comment in compiler.cpp: remove 'or on NOP instructions' to match code - Fix comment in eetwain.cpp: generalize 'CALL/CALLVIRT' to 'call instruction' Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/coreclr/interpreter/compiler.cpp | 9 +++++---- src/coreclr/vm/eetwain.cpp | 2 +- src/coreclr/vm/interpexec.cpp | 3 --- 3 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/coreclr/interpreter/compiler.cpp b/src/coreclr/interpreter/compiler.cpp index e8e76d5a34969e..6f0f52e9aa5d0d 100644 --- a/src/coreclr/interpreter/compiler.cpp +++ b/src/coreclr/interpreter/compiler.cpp @@ -1054,7 +1054,7 @@ int32_t* InterpCompiler::EmitCodeIns(int32_t *ip, InterpInst *ins, TArraynativeOffset); - // Only emit mapping entries at IL offsets where the evaluation stack is empty or on NOP instructions + // Only emit mapping entries at IL offsets where the evaluation stack is empty if ((ins->flags & INTERP_INST_FLAG_EMPTY_IL_STACK) && ((m_ILToNativeMapSize == 0) || (m_pILToNativeMap[m_ILToNativeMapSize - 1].ilOffset != ilOffset))) { @@ -1125,9 +1125,10 @@ int32_t *InterpCompiler::EmitBBCode(int32_t *ip, InterpBasicBlock *bb, TArraynativeOffset = (int32_t)(ip - m_pMethodCode); if (prevWasCall && m_corJitFlags.IsSet(CORJIT_FLAGS::CORJIT_FLAG_DEBUG_CODE)) { - // Emit a real NOP so the return address after a call lands within - // the call's native offset range, not on the next statement boundary. - *ip++ = INTOP_NOP; + // Emit a debug sequence point so the return address after a call + // lands within the call's native offset range, not on the next + // statement boundary. + *ip++ = INTOP_DEBUG_SEQ_POINT; prevWasCall = false; } continue; diff --git a/src/coreclr/vm/eetwain.cpp b/src/coreclr/vm/eetwain.cpp index 0bebc952d4d64b..2fef103782ad88 100644 --- a/src/coreclr/vm/eetwain.cpp +++ b/src/coreclr/vm/eetwain.cpp @@ -2104,7 +2104,7 @@ static void VirtualUnwindInterpreterCallFrame(TADDR sp, T_CONTEXT *pContext) pFrame = pFrame->pParent; if (pFrame != NULL) { - // The parent frame's IP points past the CALL/CALLVIRT instruction (the resumption point). + // The parent frame's IP points past the call instruction (the resumption point). SetIP(pContext, (TADDR)pFrame->ip); SetSP(pContext, dac_cast(pFrame)); SetFP(pContext, (TADDR)pFrame->pStack); diff --git a/src/coreclr/vm/interpexec.cpp b/src/coreclr/vm/interpexec.cpp index 10a9d34dda0e48..d36bb50efde8d6 100644 --- a/src/coreclr/vm/interpexec.cpp +++ b/src/coreclr/vm/interpexec.cpp @@ -1421,9 +1421,6 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr MemoryBarrier(); ip++; break; - case INTOP_NOP: - ip++; - break; #ifndef FEATURE_PORTABLE_ENTRYPOINTS case INTOP_STORESTUBCONTEXT: { From 1c126e42e450dc046b6b8ea8a250115910b175cf Mon Sep 17 00:00:00 2001 From: Tom McDonald Date: Tue, 28 Apr 2026 09:44:08 -0400 Subject: [PATCH 4/4] Emit debug seq point for all eliminated IL instructions in debug builds Remove the prevWasCall check so that all emit-nop instructions get a real INTOP_DEBUG_SEQ_POINT in debug builds, not just those after call instructions. This ensures any eliminated IL instruction still occupies a bytecode slot for correct debug mapping. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/coreclr/interpreter/compiler.cpp | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/coreclr/interpreter/compiler.cpp b/src/coreclr/interpreter/compiler.cpp index 6f0f52e9aa5d0d..6f10b84a6ea6f2 100644 --- a/src/coreclr/interpreter/compiler.cpp +++ b/src/coreclr/interpreter/compiler.cpp @@ -1117,24 +1117,22 @@ int32_t *InterpCompiler::EmitBBCode(int32_t *ip, InterpBasicBlock *bb, TArraynativeOffset = (int32_t)(ip - m_pMethodCode); - bool prevWasCall = false; for (InterpInst *ins = bb->pFirstIns; ins != NULL; ins = ins->pNext) { if (InterpOpIsEmitNop(ins->opcode)) { ins->nativeOffset = (int32_t)(ip - m_pMethodCode); - if (prevWasCall && m_corJitFlags.IsSet(CORJIT_FLAGS::CORJIT_FLAG_DEBUG_CODE)) + if (m_corJitFlags.IsSet(CORJIT_FLAGS::CORJIT_FLAG_DEBUG_CODE)) { - // Emit a debug sequence point so the return address after a call - // lands within the call's native offset range, not on the next - // statement boundary. + // Emit a debug sequence point so that eliminated IL instructions + // still occupy a bytecode slot. This ensures return addresses after + // calls land within the call's native offset range rather than on + // the next statement boundary. *ip++ = INTOP_DEBUG_SEQ_POINT; - prevWasCall = false; } continue; } - prevWasCall = InterpOpIsDirectCall(ins->opcode) || InterpOpIsIndirectCall(ins->opcode); ip = EmitCodeIns(ip, ins, relocs); }