Revert "Skipping EDI in the runtime-async case as well"

[steveisok]

Reverts #128735

[dotnet-policy-service]

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

[Copilot]

Pull request overview

This PR reverts a prior change in StackTrace.ToString related to detecting async frames and suppressing the EDI (“--- End of stack trace from previous location ---”) boundary marker, restoring the older async-detection behavior.

Changes:

• Reverts async detection in StackTrace.ToString to no longer use MethodImplAttributes.Async, relying again on compiler-generated state machine type checks.
• Removes runtime-async EDI-focused test methods and expected-pattern test data.
• Drops the assertion that runtime-async exception text must not contain the EDI boundary marker.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
src/libraries/System.Private.CoreLib/src/System/Diagnostics/StackTrace.cs	Reverts async frame detection affecting whether EDI boundaries are emitted during stack trace formatting.
src/libraries/System.Diagnostics.StackTrace/tests/StackTraceTests.cs	Removes EDI-specific runtime-async tests and relaxes validation by dropping the EDI marker assertion.

[github-actions]

Note

This review was generated by Copilot.

🤖 Copilot Code Review — PR #129123

Holistic Assessment

Motivation: Justified. PR #128735 broke NativeAOT CI (comment from @MichalStrehovsky). The NativeAOT StackFrame.AppendToStackTrace in StackFrame.NativeAot.cs has its own EDI boundary formatting at line 233 that unconditionally appends the --- End of stack trace from previous location --- marker — the original fix only addressed the CoreCLR path in StackTrace.cs but not the NativeAOT path, causing the newly-added EdiOuter tests to fail on NativeAOT.

Approach: A revert is the correct response. The original fix was incomplete (it only handled CoreCLR, not NativeAOT) and was blocking all NativeAOT CI legs. A proper fix should handle both code paths, and there was also unresolved discussion about whether the right approach is filtering EDI markers during stack trace formatting vs. not inserting them in the first place for infrastructure use.

Summary: ✅ LGTM. This is a clean, mechanically-correct revert of #128735. Every addition from the original PR is precisely undone: the MethodImplAttributes.Async detection in StackTrace.cs, the EdiOuter/EdiMiddle/EdiInner test methods and data, and the Assert.DoesNotContain assertion. The revert unblocks NativeAOT CI as requested.

---

Detailed Findings

✅ Revert Completeness — Diff is exact inverse

Compared the revert diff against the original PR #128735 diff. Every hunk is precisely reversed:

• StackTrace.cs: bool isAsync = (mb.MethodImplementationFlags & MethodImplAttributes.Async) != 0 → bool isAsync = false, and the !isAsync && guard is removed from the if condition.
• StackTraceTests.cs: All added test methods (EdiOuter, EdiMiddle, EdiInner), test data entries, and the Assert.DoesNotContain assertion are removed.

No extraneous changes are included.

✅ NativeAOT Root Cause — Confirmed

The NativeAOT AppendToStackTrace in StackFrame.NativeAot.cs (line 233) unconditionally emits the EDI boundary marker when _isLastFrameFromForeignExceptionStackTrace is set — it has no isAsync skip logic. The test added by #128735 asserted absence of this marker for all runtimes, which is why NativeAOT tests failed.

💡 Follow-up — Issue #128723 will need a new fix

This revert re-opens the underlying problem reported in #128723 (EDI markers appearing in runtime-async stack traces). A future fix should address both the CoreCLR path (StackTrace.cs:361) and the NativeAOT path (StackFrame.NativeAot.cs:233), or take the alternative approach discussed in the original PR of avoiding the marker insertion at the infrastructure level.

Generated by Code Review for issue #129123 · ● 4.4M · ◷