Fix profiler ELT callbacks for Runtime Async suspension - #131985
Fix profiler ELT callbacks for Runtime Async suspension#131985tommcdon wants to merge 2 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
|
Azure Pipelines: Successfully started running 6 pipeline(s). 10 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This PR updates CoreCLR JIT codegen for Runtime Async suspension points to emit profiler Leave callbacks on suspend (to match the existing Enter callbacks that occur on initial invocation and resume), while preserving the async continuation value across the profiler helper call. It also adds a new profiler-based regression test to validate the callback pairing behavior.
Changes:
- Update
CodeGen::genReturnSuspendto call the profiling leave helper onGT_RETURN_SUSPEND, with target-specific handling to preserve the async continuation register across the callback. - Add a new native profiler (
RuntimeAsyncELTProfiler) that counts and validates enter/leave sequencing for a runtime-async method. - Add a new managed profilee (
runtimeasyncelt) and wire the new profiler into the native profiler test build/activation.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/jit/codegencommon.cpp | Emit profiling Leave callback on runtime-async suspend while preserving the continuation across the helper call. |
| src/tests/profiler/native/runtimeasyncelt/runtimeasynceltprofiler.h | Declares a new native profiler test type tracking enter/leave counts and sequencing. |
| src/tests/profiler/native/runtimeasyncelt/runtimeasynceltprofiler.cpp | Implements the enter/leave hooks, target method selection, and pass/fail reporting. |
| src/tests/profiler/native/CMakeLists.txt | Adds the new profiler source to the native profiler test build. |
| src/tests/profiler/native/classfactory.cpp | Registers the new profiler CLSID in the native class factory. |
| src/tests/profiler/elt/runtimeasyncelt.csproj | Adds the new managed profilee project configured for runtime-async. |
| src/tests/profiler/elt/runtimeasyncelt.cs | Managed profilee that exercises runtime-async suspension/resume patterns. |
| if (FAILED(hrStatus) || GetFunctionIDName(functionId) != WCHAR("Work")) | ||
| { | ||
| return S_OK; | ||
| } |
There was a problem hiding this comment.
Addressed in 58c682a. The profiler now tracks Work, WorkVoid, and WorkDouble independently and requires exact paired callback counts of 25, 3, and 3 respectively, with balanced depth and no sequence failures. The expanded test passes on Windows x64 and Linux x64.
| #ifdef PROFILING_SUPPORTED | ||
| if (m_compiler->compIsProfilerHookNeeded()) | ||
| { | ||
| #ifdef TARGET_ARM | ||
| // The ARM32 Leave helper preserves REG_PROFILER_RET_SCRATCH, which is also the dedicated | ||
| // Runtime Async continuation register. For integer/soft-float returns it preserves R0 by | ||
| // moving it through this scratch register; for void and hard-float returns R0 is overwritten | ||
| // and the scratch register itself is preserved. | ||
| static_assert(REG_ASYNC_CONTINUATION_RET == REG_PROFILER_RET_SCRATCH); | ||
| bool r0InUse; | ||
| if (m_compiler->info.compRetType == TYP_VOID) | ||
| { | ||
| r0InUse = false; | ||
| } | ||
| else if (varTypeIsFloating(m_compiler->info.compRetType) || | ||
| m_compiler->IsHfa(m_compiler->info.compMethodInfo->args.retTypeClass)) | ||
| { | ||
| r0InUse = m_compiler->info.compIsVarArgs || m_compiler->opts.compUseSoftFP; | ||
| } | ||
| else | ||
| { | ||
| r0InUse = true; | ||
| } | ||
| if (r0InUse) | ||
| { | ||
| inst_Mov(TYP_REF, REG_INTRET, reg, /* canSkip */ true); | ||
| gcInfo.gcMarkRegPtrVal(REG_INTRET, TYP_REF); | ||
| genProfilingLeaveCallback(CORINFO_HELP_PROF_FCN_LEAVE); | ||
| inst_Mov(TYP_REF, REG_ASYNC_CONTINUATION_RET, REG_INTRET, /* canSkip */ true); | ||
| gcInfo.gcMarkRegSetNpt(genRegMask(REG_INTRET)); | ||
| } | ||
| else | ||
| { | ||
| inst_Mov(TYP_REF, REG_ASYNC_CONTINUATION_RET, reg, /* canSkip */ true); | ||
| gcInfo.gcMarkRegPtrVal(REG_ASYNC_CONTINUATION_RET, TYP_REF); | ||
| genProfilingLeaveCallback(CORINFO_HELP_PROF_FCN_LEAVE); | ||
| } | ||
| #else | ||
| // The Leave helper preserves the normal return register, but may overwrite the dedicated | ||
| // Runtime Async continuation register because it is an argument register on supported | ||
| // ABIs. Temporarily use the normal return register to carry the continuation across the | ||
| // callback, then restore the dedicated register. | ||
| inst_Mov(TYP_REF, REG_INTRET, reg, /* canSkip */ true); | ||
| gcInfo.gcMarkRegPtrVal(REG_INTRET, TYP_REF); | ||
| genProfilingLeaveCallback(CORINFO_HELP_PROF_FCN_LEAVE); | ||
| inst_Mov(TYP_REF, REG_ASYNC_CONTINUATION_RET, REG_INTRET, /* canSkip */ true); | ||
| gcInfo.gcMarkRegSetNpt(genRegMask(REG_INTRET)); | ||
| #endif | ||
| } | ||
| else | ||
| #endif |
There was a problem hiding this comment.
Can you factor this into a function? The ifdefs make this quite hard to read now.
There was a problem hiding this comment.
Addressed in 58c682a. I factored the target-specific preservation and Leave emission into genProfilingLeaveCallbackForAsyncSuspend, so genReturnSuspend now only selects the profiled versus unprofiled path.
| // The Leave helper preserves the normal return register, but may overwrite the dedicated | ||
| // Runtime Async continuation register because it is an argument register on supported | ||
| // ABIs. Temporarily use the normal return register to carry the continuation across the | ||
| // callback, then restore the dedicated register. |
There was a problem hiding this comment.
Are these ELT hooks usually able to look at register state? Does this e.g. break the ability to look at return values from leave callbacks in async functions?
There was a problem hiding this comment.
A suspension Leave marks the end of a physical execution segment, not logical method completion, so there is no semantic method return value at that callback. The normal return registers may contain non-semantic state there. The final completion path is unchanged and still uses normal return codegen, so its Leave callback continues to expose the actual return value. I added this distinction to the helper comment in 58c682a.
There was a problem hiding this comment.
How is the leave hook expected to detect that it is being called while the return register happens to contain a continuation instead of a return value?
There was a problem hiding this comment.
I think this exposes an issue - profilers might not be able to tell the difference between a suspension vs. a synchronous method return. I propose we return CORPROF_E_DATAINCOMPLETE with no return-value range for suspension, and then a synchronous/final return returns S_OK with the real return value. Does that sound reasonable?
There was a problem hiding this comment.
Yes, that sounds good. I am not so familiar with this, how do you think we should pass that info through?
FWIW the enter callback similarly will not have arguments available on resumption, so I expect we want to do something similar there.
There was a problem hiding this comment.
For Enter, I think we can inspect the Continuation argument, and if it is non-null, GetFunctionEnter3Info would return CORPROF_E_DATAINCOMPLETE.
And for Leave, we can use REG_ASYNC_CONTINUATION_RET to detect suspension and similarly return CORPROF_E_DATAINCOMPLETE if non-null.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Emit profiler
Leavecallbacks when Runtime Async methods suspend, pairing the existingEntercallbacks emitted on initial invocation and resume. This makes ELT callbacks consistently represent physical execution segments.Preserves the continuation across profiler callbacks on all supported ABIs and adds Windows/Linux regression coverage for
Task,Task<long>, andTask<double>methods.Fixes #122488.