Fix x86 runtime async frame pointer mismatch in GetSpForDiagnosticReporting

[tommcdon]

For certain runtime async frames this resulted in the ICorDebugManagedCallback2::Exception to return a null ICorDebugFrame for DEBUG_EXCEPTION_CATCH_HANDLER_FOUND notifications. The fix addresses this by adjusting GetSpForDiagnosticReporting to account for runtime async variant method stack layout differences on x86.

[dotnet-policy-service]

Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

Adjusts the stack pointer reported to the debugger on x86 for runtime “async call” frames so that ICorDebugManagedCallback2::Exception callbacks (notably DEBUG_EXCEPTION_CATCH_HANDLER_FOUND) can resolve a non-null ICorDebugFrame.

Changes:

• Extends GetSpForDiagnosticReporting to optionally accept a MethodDesc* and apply an extra x86 adjustment for runtime async methods (IsAsyncMethod()).
• Updates exception/debugger callback sites to pass the current MethodDesc* into GetSpForDiagnosticReporting.

Comments suppressed due to low confidence (1)

src/coreclr/vm/exceptionhandling.cpp:2968

• pMD is only referenced under ESTABLISHER_FRAME_ADDRESS_IS_CALLER_SP + TARGET_X86. In other builds (e.g., x64 Unix where -Wall is enabled, often with -Werror), this new parameter can become unused and may trigger an unused-parameter warning. Consider adding UNREFERENCED_PARAMETER(pMD); in the #else path and/or in the non-x86 path to keep all configurations warning-free.

static TADDR GetSpForDiagnosticReporting(REGDISPLAY *pRD, MethodDesc *pMD = NULL)
{
#ifdef ESTABLISHER_FRAME_ADDRESS_IS_CALLER_SP
    TADDR sp = CallerStackFrame::FromRegDisplay(pRD).SP;
#if defined(TARGET_X86)
    sp -= sizeof(TADDR);
    // On x86, runtime async methods have stack parameters that cause CallerSP
    // to sit above the parameter area.  The DBI uses PCTAddr as the frame
    // pointer, which is at the return address (below the parameters).
    // Subtract an extra sizeof(TADDR) to account for the stack parameter.
    if (pMD != NULL && pMD->IsAsyncMethod())
    {
        sp -= sizeof(TADDR);
    }
#endif
    return sp;
#else
    return GetSP(pRD->pCurrentContext);
#endif

[jakobbotsch]

Is this because of the continuation? Or what exactly is the difference here?

[jakobbotsch]

I am confused here because the continuation is not the only possible extra argument we can have -- we can also have generic context (and vararg cookie), but those do not seem to require handling here.

I am wondering if we instead need a fix on the JIT side in the GC information. Perhaps something like

runtime/src/coreclr/vm/gc_unwind_x86.inl

Lines 1902 to 1923 in a181406

	unsigned GetPushedArgSize(hdrInfo * info, PTR_CBYTE table, DWORD curOffs)
	{
	SUPPORTS_DAC;
	unsigned sz;
	if (info->interruptible)
	{
	sz = scanArgRegTableI(skipToArgReg(*info, table),
	curOffs,
	curOffs,
	info);
	}
	else
	{
	sz = scanArgRegTable(skipToArgReg(*info, table),
	curOffs,
	info);
	}
	return sz;
	}

is not working properly for the continuation.

[tommcdon]

Is this because of the continuation? Or what exactly is the difference here?

Correct - this is due to the continuation parameter in the async calling convention being passed on the stack

I am wondering if we instead need a fix on the JIT side in the GC information

Good suggestion- taking a look

[jakobbotsch]

Correct - this is due to the continuation parameter in the async calling convention being passed on the stack

The continuation parameter is not always passed on the stack. It is just a regular argument that follows the regular calling convention, which may or may not put it on the stack.

[noahfalk]

yeah, I would hope that this can be addressed as part of GC or unwind information attached to the method rather than custom post-processing for async frames. I'd worry that any discrepancy we try to address here means we've got a leaky abstraction and this probably isn't the only place it would be leaking.

[janvorli]

@tommcdon I wonder - was this always a problem? I am asking since there was a change in the ifdef in this code in January (#122833) from

#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_X86)
    sp -= sizeof(TADDR); // For X86 with funclets we want the address 1 pointer into the callee.
#endif // defined(FEATURE_EH_FUNCLETS) && defined(TARGET_X86)

I wonder if the ifdef really meant to be for Linux x86 only (which was previously the only one with defined(FEATURE_EH_FUNCLETS)) and was unrelated to the funclets per se.