Suppress EDI delimiter for async frames in NativeAOT stack traces#130677
Suppress EDI delimiter for async frames in NativeAOT stack traces#130677steveisok wants to merge 1 commit into
Conversation
Classic (state-machine) async methods that throw emitted an extraneous "--- End of stack trace from previous location ---" delimiter under NativeAOT, whereas CoreCLR suppresses it for async frames. NativeAOT has no reflection available when formatting a stack trace, so the "is async" information must be baked into the stack trace metadata at compile time. Plumb a new IsAsync flag through the AOT stack trace metadata pipeline, mirroring the existing IsHidden flag: the emission policy detects runtime-async (V2) methods and the MoveNext method of a compiler-generated (V1) IAsyncStateMachine, the flag flows through MetadataManager and the stack trace mapping node into the runtime decode, and StackFrame honors it by skipping the delimiter for async frames. Fixes dotnet#129155 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 3 pipeline(s). 12 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @agocke, @dotnet/ilc-contrib |
|
@rcj1 trying to get something started for you. Feel free to take it over. |
There was a problem hiding this comment.
Pull request overview
This PR adds an IsAsync flag to NativeAOT stack trace metadata and uses it at runtime to suppress the --- End of stack trace from previous location --- delimiter for async frames, aligning formatting behavior with CoreCLR.
Changes:
- Extend the NativeAOT stack trace metadata pipeline (emission policy → metadata records → mapping blob → runtime decode) with an
IsAsyncflag. - Update NativeAOT stack frame formatting to skip the EDI delimiter when the last “foreign stack trace” frame is async.
- Enable the existing async stack trace test assertion on NativeAOT by removing the conditional skip.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Diagnostics.StackTrace/tests/StackTraceTests.cs | Removes the NativeAOT-specific conditional so the async stack trace delimiter assertion runs everywhere. |
| src/coreclr/tools/Common/Internal/Runtime/StackTraceData.cs | Adds a new stack trace command bit (IsAsync) used in the emitted mapping blob. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/StackTraceEmissionPolicy.cs | Detects async frames (runtime-async and IAsyncStateMachine.MoveNext) and sets MethodStackTraceVisibilityFlags.IsAsync. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/MetadataManager.cs | Plumbs async visibility into StackTraceRecordFlags.IsAsync for stack trace records. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/StackTraceMethodMappingNode.cs | Emits the IsAsync command bit into the stack trace mapping blob. |
| src/coreclr/nativeaot/System.Private.StackTraceMetadata/src/Internal/StackTraceMetadata/StackTraceMetadata.cs | Decodes the new IsAsync bit from the mapping blob and exposes it through stack trace callbacks. |
| src/coreclr/nativeaot/System.Private.CoreLib/src/System/Diagnostics/StackFrame.NativeAot.cs | Consumes isAsync from callbacks and suppresses the EDI delimiter for async frames. |
| src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/Runtime/Augments/StackTraceMetadataCallbacks.cs | Extends callback contract to return isAsync alongside existing stack trace metadata. |
| private const int IsHiddenFlag = 0x2; | ||
| private const int IsAsyncFlag = 0x1; | ||
| private const int FlagsMask = IsHiddenFlag | IsAsyncFlag; |
There was a problem hiding this comment.
This concern is legitimate, I commented on this in Tom's PR.
|
Ah! I forgot to look there |
| // Async state machines only expose their exception-throwing code through IAsyncStateMachine.MoveNext. | ||
| if (method.Name != "MoveNext"u8) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| if (!_iAsyncStateMachineTypeComputed) | ||
| { | ||
| _iAsyncStateMachineType = method.Context.SystemModule.GetType("System.Runtime.CompilerServices"u8, "IAsyncStateMachine"u8, throwIfNotFound: false); | ||
| _iAsyncStateMachineTypeComputed = true; | ||
| } | ||
|
|
||
| if (_iAsyncStateMachineType == null) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| foreach (DefType interfaceType in method.OwningType.RuntimeInterfaces) | ||
| { | ||
| if (interfaceType == _iAsyncStateMachineType) | ||
| { | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Does this meaningfully improve the stack trace experience in async v1? The EDI separator being present falls out from all of this reflection based code that we can't use with native AOT:
It is likely that when/if we decide we want to fix asyncv1 stack traces, the EDI separators disappearing will also naturally fall out from all the extra metadata we'll need to emit and all of this special casing will be reverted (or... if we let copilot do it, the special casing will probably stay because copilot is usually not capable of identifying redundant code).
I'd delete this code.
| private const int IsHiddenFlag = 0x2; | ||
| private const int IsAsyncFlag = 0x1; | ||
| private const int FlagsMask = IsHiddenFlag | IsAsyncFlag; |
There was a problem hiding this comment.
This concern is legitimate, I commented on this in Tom's PR.
… ctor Introduce async v2 (runtime-async) continuation stitching for stack traces so logical async call chains remain visible across suspension points. Public API: - Environment.AsyncStackTrace: string property mirroring Environment.StackTrace that returns the current trace with runtime-async continuation frames spliced in and non-async infrastructure frames hidden. - StackTrace(int skipFrames, bool fNeedFileInfo, bool fIncludeAsyncContinuationFrames): new public constructor exposing the same stitching over the StackTrace object model. Implementation: - CoreCLR native stitching in debugdebugger.cpp (ExtractContinuationData, fnUnwrapToRAT, fnFindRATWaiter), recognizing RuntimeAsyncTaskContinuation. - NativeAOT managed stitching (continuation-IP collection, waiter-chain walk) with an IsAsync flag plumbed through the AOT stack-trace metadata pipeline. - Mono accepts and ignores the stitching flag (runtime-async unsupported). The NativeAOT IsAsync metadata flag is aligned with dotnet#130677: shared naming, an emission policy covering runtime-async (v2) and v1 MoveNext state machines, and EDI delimiter suppression for async frames. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d562c8ba-a68c-41e9-afc9-7807ed98098b
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "6f621c01124d308649c947e6da9b51d98a32c6b9",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "ed84dd37d6728e9d0d50a37a323eddcc5c57cd71",
"last_reviewed_commit": "6f621c01124d308649c947e6da9b51d98a32c6b9",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "ed84dd37d6728e9d0d50a37a323eddcc5c57cd71",
"last_recorded_worker_run_id": "29684008416",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "6f621c01124d308649c947e6da9b51d98a32c6b9",
"review_id": 4730636799
}
]
} |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Holistic Review
Motivation: Classic (V1) state-machine async methods that throw were emitting an extraneous --- End of stack trace from previous location --- delimiter under NativeAOT, diverging from CoreCLR, which suppresses it for async frames. Because NativeAOT has no reflection when formatting a stack trace, the "is async" fact must be baked into stack-trace metadata at compile time. This is a legitimate, well-scoped correctness fix that also re-enables a previously ActiveIssue-quarantined test (#129155).
Approach: A new IsAsync flag is plumbed end-to-end, mirroring the existing IsHidden flag: EcmaMethodStackTraceEmissionPolicy detects runtime-async (V2) methods and the MoveNext method of a compiler-generated IAsyncStateMachine (V1); the flag flows through MethodStackTraceVisibilityFlags → StackTraceRecordFlags → the StackTraceDataCommand.IsAsync command byte → runtime decode in StackTraceMetadata, and finally StackFrame.AppendToStackTrace skips the delimiter when _isAsync is set. The layering mirrors the established pattern well and the emission-policy detection (interface-based IAsyncStateMachine check, cached type lookup, MoveNext name gate) is sound and appropriately narrow.
Summary: The design is correct and the fix is minimal and consistent with existing conventions. However, the runtime-side bit packing in StackTraceMetadata.StackTraceData introduces an ARM32 correctness bug: the new IsAsyncFlag = 0x1 reuses bit 0 of the packed RVA word, which on ARM32 carries the Thumb bit and is therefore always set for method entrypoints. This can trip the Rva init assert in checked builds, spuriously mark nearly all ARM32 frames async, and/or break the RVA binary search so stack-trace metadata is lost. See the inline comment on the flag definition. Aside from that one issue, the change looks good. Recommend addressing the bit-0 collision (use a bit the RVA never occupies, or store flags outside the RVA word) before merge.
Detailed Findings
-
[ARM32 Thumb-bit collision — blocking]
IsAsyncFlag = 0x1inStackTraceDataoverlaps the ARM32 Thumb bit that method entrypoint symbols carry (ObjectWritersetsthumbBit = 1for ARM methods, andStackTraceMethodMappingNodeemitsRELPTR32relocs to those entrypoints). The pre-existing code deliberately used onlyIsHiddenFlag = 0x2, leaving bit 0 as RVA payload. Details and remediation options in the inline comment. -
[Non-blocking observation] The emission-policy
IsAsyncFramegates onmethod.Name != "MoveNext"u8before the interface check, which correctly avoids misclassifying non-asyncMoveNext(e.g.IEnumerator) implementations because it further requires the owning type to implementIAsyncStateMachine. No change needed.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 149.4 AIC · ⌖ 11.1 AIC · ⊞ 10K
| public struct StackTraceData : IComparable<StackTraceData> | ||
| { | ||
| private const int IsHiddenFlag = 0x2; | ||
| private const int IsAsyncFlag = 0x1; |
There was a problem hiding this comment.
Bit 0 collides with the ARM32 Thumb bit. The original code intentionally used IsHiddenFlag = 0x2 (bit 1) and reserved bit 0 as part of the RVA. On ARM32, method entrypoint symbols are defined with the Thumb bit set (+1) per the AAELF ABI — see ObjectWriter.EmitObjectFile where thumbBit = _nodeFactory.Target.Architecture == TargetArchitecture.ARM && isMethod ? 1 : 0. The StackTraceMethodMappingNode emits IMAGE_REL_BASED_RELPTR32 relocs against factory.MethodEntrypoint(...), so the decoded methodRva in PopulateRvaToTokenMap will have bit 0 set on ARM32.
Repurposing bit 0 as IsAsyncFlag breaks this on ARM32:
- The
Rva { init }Debug.Assert((value & FlagsMask) == 0)will fire in debug/checked builds becausemethodRvais odd. IsAsyncwould be spuriouslytruefor essentially every ARM32 method (bit 0 is always set).Rvanow masks off bit 0 on the stored side, so if the runtime lookup RVA still carries the Thumb bit, theArray.BinarySearchinTryGetStackTraceDatamismatches and stack-trace metadata (method names, hidden/async flags) is lost.
Use a spare high bit instead of bit 0. StackTraceDataCommand.IsAsync = 0x20 is a distinct value in the emitted command byte and is unrelated to this packed-RVA field, so pick a flag bit here that does not overlap the RVA's low bit — e.g. keep the async flag in a bit that ARM32 RVAs never use, or store the flags outside the RVA word entirely.
… ctor Introduce async v2 (runtime-async) continuation stitching for stack traces so logical async call chains remain visible across suspension points, and extend it across the v2 -> v1 boundary so classic async framework frames (e.g. an ASP.NET middleware pipeline) are recovered as well. Public API: - Environment.AsyncStackTrace: string property mirroring Environment.StackTrace that returns the current trace with runtime-async continuation frames spliced in and non-async infrastructure frames hidden. - StackTrace(int skipFrames, bool fNeedFileInfo, bool fIncludeAsyncContinuationFrames): new public constructor exposing the same stitching over the StackTrace object model. Implementation: - CoreCLR native stitching in debugdebugger.cpp (ExtractContinuationData) walks the task waiter chain. At each hop the waiter is either a RuntimeAsyncTask (v2, whose stored continuation chain is appended) or an AsyncStateMachineBox (v1, whose state-machine MoveNext frame is recovered and mapped by the formatter back to the original async method). Resolution follows the RuntimeAsyncTask waiter field precisely to avoid callee back-references. - NativeAOT managed stitching (continuation-IP collection, waiter-chain walk) with an IsAsync flag plumbed through the AOT stack-trace metadata pipeline. - Mono accepts and ignores the stitching flag (runtime-async unsupported). The NativeAOT IsAsync metadata flag is aligned with dotnet#130677: shared naming, an emission policy covering runtime-async (v2) and v1 MoveNext state machines, and EDI delimiter suppression for async frames. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d562c8ba-a68c-41e9-afc9-7807ed98098b
Classic (state-machine) async methods that throw emitted an extraneous "--- End of stack trace from previous location ---" delimiter under NativeAOT, whereas CoreCLR suppresses it for async frames. NativeAOT has no reflection available when formatting a stack trace, so the "is async" information must be baked into the stack trace metadata at compile time.
Plumb a new IsAsync flag through the AOT stack trace metadata pipeline, mirroring the existing IsHidden flag: the emission policy detects runtime-async (V2) methods and the MoveNext method of a compiler-generated (V1) IAsyncStateMachine, the flag flows through MetadataManager and the stack trace mapping node into the runtime decode, and StackFrame honors it by skipping the delimiter for async frames.
Fixes #129155