[cDAC] Support x86 stackwalking #116072

max-charlamb · 2025-05-28T15:19:06Z

Changes

IExecutionManager

GetFuncletStartAddress

Relatively simple, uses the existing GetUnwindInfo implementation.
Added docs

GetGCInfo

Uses new datadescriptor RealCodeHeader::GCInfo
Added doc explaining how this is found for R2R and jitted code.
R2R section still needs work to handle platforms != x86 due to complexities calculating unwind code size.

GetRelativeOffset

Added docs, simple API to extract field off of CodeBlock, similar to GetMethodDesc and GetStartAddress

Extension IsFunclet

Implemented simply in terms of IExecutionManager.StartAddress(cbh) != IExecutionManager.GetFuncletStartAddress(cbh)
Added docs

IStackWalk

Added support for X86 Platform

IPlatformAgnosticContext.GetContextForPlatform support x86
Added X86 context based on https://github.com/microsoft/clrmd/blob/main/src/Microsoft.Diagnostics.Runtime/DataReaders/Registers/X86Context.cs

Managed Implementation of X86 GCInfo decoding

Based on src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/x86/

Managed Implementation of X86 Unwinder

Based on implementation in gc_unwind_x86.inl
GCInfo uses
- InfoHdr to decode the header. This involves reading an initial header from a lookup table then modifying with a series of instructions.
- GCSlotTable
- NoGcRegionTable
- GCTransition
- CallPattern to decode common call pattern encodings. Uses look up table similar to InfoHdr.

Added X86FrameHandler

The following Frames were verified:
- SoftwareExceptionFrame
- InlinedCallFrame
- TransitionFrames
- FaultingExceptionFrame
Frames not currently verified/implemented:
- ResumableFrame
- HijackFrame
- TailCallFrame

Questions

GCInfo Version

How do we deal with this? Currently the runtime looks at the major version to determine what version for R2R code. I think we should parse this, but not support old versions
Need to add constant in doc file

dotnet-policy-service · 2025-05-28T15:20:03Z

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

filipnavara · 2025-05-28T18:34:39Z

Currently the runtime looks at the major version to determine what version for R2R code. I think we should parse this, but not support old versions

We recently switched x86 to use funclets which is a major R2R breaking change. I feel that there's no point in supporting anything older, both cDAC and x86/funclets are on the same shipping schedule.

filipnavara · 2025-05-28T22:07:24Z

src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/x86/InfoHdr.cs

@@ -163,7 +163,7 @@ public class InfoHdrDecoder {
        private const uint HAS_UNTRACKED = 0xFFFFFFFF;
        private const uint HAS_GS_COOKIE_OFFSET = 0xFFFFFFFF;
        private const uint HAS_SYNC_OFFSET = 0xFFFFFFFF;
-        private const uint HAS_REV_PINVOKE_FRAME_OFFSET = 0xFFFFFFFF;
+        private const uint HAS_REV_PINVOKE_FRAME_OFFSET = 0xFFFFFFFF; // is this wrong??


No, it's not wrong. These are just placeholders during header parsing. The x86 GC info has a bunch of optional flags (sometimes encoded as diff but let's not go there). One of the flags - FLIP_REV_PINVOKE_FRAME - flips whether the REV_PINVOKE_FRAME_OFFSET exists in particular header. If it exists then the sentinel value HAS_REV_PINVOKE_FRAME_OFFSET is used for a moment. Once the first part of header is read it is followed by all the optional fields and then HAS_REV_PINVOKE_FRAME_OFFSET gets replaced by reading the real value.

This constant (and the others around it) are not used after the whole header is read.

Oh, you probably meant that it's wrong because the value in the C++ code is different... yeah, that's a fair point. The managed decoder completely lacks the INVALID_REV_PINVOKE_OFFSET constant and the zero value is valid (and thus it cannot currently be distinguished from the invalid one).

Yes.

I think this R2R GCDecoder handles the case where there isn't a Reverse Pinvoke incorrectly. The InfoHdr should have a value of INVALID_REV_PINVOKE_OFFSET = 0xFFFFFFFF, but it ends up with a value of 0x0. I don't think this is a huge deal, but something that impacts the unwinder logic.

edit: fixed wrong value

You're right, this should be fixed. I think the native code uses HAS_REV_PINVOKE_FRAME_OFFSET = 0xFFFFFFFE and INVALID_REV_PINVOKE_OFFSET = 0xFFFFFFFF. The actual values are not part of the format and they are both invalid so it doesn't really matter except for consistency.

max-charlamb · 2025-05-29T16:33:42Z

/azp run runtime-diagnostics

azure-pipelines · 2025-05-29T16:33:58Z

Azure Pipelines successfully started running 1 pipeline(s).

filipnavara · 2025-05-29T16:50:28Z

src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/x86/InfoHdr.cs

@@ -163,7 +163,7 @@ public class InfoHdrDecoder {
        private const uint HAS_UNTRACKED = 0xFFFFFFFF;
        private const uint HAS_GS_COOKIE_OFFSET = 0xFFFFFFFF;
        private const uint HAS_SYNC_OFFSET = 0xFFFFFFFF;
-        private const uint HAS_REV_PINVOKE_FRAME_OFFSET = 0xFFFFFFFF;
+        private const uint HAS_REV_PINVOKE_FRAME_OFFSET = 0xFFFFFFFF; // is this wrong??


Suggested change

private const uint HAS_REV_PINVOKE_FRAME_OFFSET = 0xFFFFFFFF; // is this wrong??

private const uint HAS_REV_PINVOKE_FRAME_OFFSET = 0xFFFFFFFE;

filipnavara · 2025-05-29T16:52:26Z

...ostics.DataContractReader.Contracts/Contracts/StackWalk/Context/X86/GCInfoDecoding/GCInfo.cs

+
+    private void CalculateDepth()
+    {
+        using StreamWriter outputFile = new StreamWriter("C:\\Users\\maxcharlamb\\OneDrive - Microsoft\\Desktop\\out.txt", true);


Just a reminder to remove the debug prints before it goes for review. ;-)

max-charlamb · 2025-05-29T17:13:22Z

/azp run runtime-diagnostics

azure-pipelines · 2025-05-29T17:13:31Z

Azure Pipelines successfully started running 1 pipeline(s).

jkoritzinsky · 2025-05-29T17:21:41Z

...ractReader.Contracts/Contracts/ExecutionManager/ExecutionManagerCore.ReadyToRunJitManager.cs

+            uint unwindDataSize = sizeof(uint); // TODO(cdac): This is platform specific and maybe needs its own contract. Current value is for x86
+            gcInfo = unwindInfo + unwindDataSize;
+            gcVersion = GCINFO_VERSION; // TODO(cdac): This depends on the major version of the runtime.


I think the best way to handle this versioning would be to split it out into its own contract. That way we can use the contract versioning mechanism to handle versioning the format.

The current contract versioning mechanism wouldn't exactly fit, but could be adapted. Multiple GCInfo formats could potentially exist within a project (old R2R code). I don't know if this is a supported scenario.

I don't know if this is a supported scenario.

Generally speaking it is supported by the runtime. There's only enforcement of minimum R2R format version, and IIRC version 2 and 3 of GC info could co-exist in the same process for a while.

Im pretty sure that versioning the GC Info format is a backwards-incompatible change for R2R (and forces a minimum major version bump). There's only one decoder in the runtime and SOS only has a copy of the most recent GCInfo layout.

GC info version 2 and 3 had the same format. IIRC they differed in some offset adjustment and I'm somewhat convinced that they did co-exist for a moment without a minimum R2R version bump. I think there are some GC info version changes that are backwards compatible even if it was rare. That said, I am happy to defer to the subject expert on this one - @VSadov - who probably remembers the exact details of the version changes he has made.

jkoritzinsky · 2025-05-29T17:23:27Z

...stics.DataContractReader.Contracts/Contracts/StackWalk/Context/X86/GCInfoDecoding/InfoHdr.cs

Would it be possible to share the GC Info decoding source between ILCompiler.Reflection.ReadyToRun and the cdac? Like providing an abstraction for reading memory and including the source in both projects (like how the various ILCompiler projects include various source files instead of everything being factored into specific libraries).

I think it's a path worth exploring. We don't often make changes to the GC info format but it happens, and in the last month it happened both for the x86 GC info (added no-GC regions) and the regular GC info (deprecated PSPSym). We already have three places that need to be synced up - runtime itself, ILCompiler's managed version of the code, and the dotnet/diagnostics copy of the runtime files. Adding a fourth should be done only after careful consideration of other options.

jkoritzinsky · 2025-05-29T17:25:10Z

src/native/managed/cdac/mscordaccore_universal/Legacy/ClrDataStackWalk.cs

@@ -4,6 +4,7 @@
 using System;
 using System.Collections.Generic;
 using System.Diagnostics;
+using System.IO;


Suggested change

using System.IO;

jkoritzinsky · 2025-05-29T17:26:37Z

...Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/Context/X86Context.cs

+/// <summary>
+/// X86-specific windows thread context.
+/// </summary>
+[StructLayout(LayoutKind.Explicit, Pack = 1)]


We shouldn't need both Pack=1 and explicit layout. Can we switch this to sequential layout, or can the layout not be specified as a C structure?

jkoritzinsky · 2025-05-29T17:28:36Z

...stics.DataContractReader.Contracts/Contracts/StackWalk/Context/X86/GCInfoDecoding/InfoHdr.cs

+    //        Prolog size
+    //        |
+    //        |   Epilog size
+    //        |   |
+    //        |   |  Epilog count
+    //        |   |  |
+    //        |   |  |  Epilog at end
+    //        |   |  |  |
+    //        |   |  |  |  EDI saved
+    //        |   |  |  |  |
+    //        |   |  |  |  |  ESI saved
+    //        |   |  |  |  |  |
+    //        |   |  |  |  |  |  EBX saved
+    //        |   |  |  |  |  |  |
+    //        |   |  |  |  |  |  |  EBP saved
+    //        |   |  |  |  |  |  |  |
+    //        |   |  |  |  |  |  |  |  EBP-frame
+    //        |   |  |  |  |  |  |  |  |
+    //        |   |  |  |  |  |  |  |  |  Interruptible method
+    //        |   |  |  |  |  |  |  |  |  |
+    //        |   |  |  |  |  |  |  |  |  |  doubleAlign
+    //        |   |  |  |  |  |  |  |  |  |  |
+    //        |   |  |  |  |  |  |  |  |  |  |  security flag
+    //        |   |  |  |  |  |  |  |  |  |  |  |
+    //        |   |  |  |  |  |  |  |  |  |  |  |  handlers
+    //        |   |  |  |  |  |  |  |  |  |  |  |  |
+    //        |   |  |  |  |  |  |  |  |  |  |  |  |  localloc
+    //        |   |  |  |  |  |  |  |  |  |  |  |  |  |
+    //        |   |  |  |  |  |  |  |  |  |  |  |  |  |  edit and continue
+    //        |   |  |  |  |  |  |  |  |  |  |  |  |  |  |
+    //        |   |  |  |  |  |  |  |  |  |  |  |  |  |  |  varargs
+    //        |   |  |  |  |  |  |  |  |  |  |  |  |  |  |  |
+    //        |   |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  ProfCallbacks
+    //        |   |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |
+    //        |   |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  genericsContext
+    //        |   |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |
+    //        |   |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  genericsContextIsMethodDesc
+    //        |   |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |
+    //        |   |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  returnKind
+    //        |   |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |
+    //        |   |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  Arg count
+    //        |   |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |                                 Counted occurrences
+    //        |   |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |   Frame size                    |
+    //        |   |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |   |                             |
+    //        |   |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |   |   untrackedCnt              |   Header encoding
+    //        |   |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |   |   |                         |   |
+    //        |   |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |   |   |  varPtrTable            |   |
+    //        |   |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |   |   |   |                     |   |
+    //        |   |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |   |   |   |  gsCookieOffs       |   |
+    //        |   |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |   |   |   |   |                 |   |
+    //        |   |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |   |   |   |   | syncOffs        |   |
+    //        |   |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |   |   |   |   |  |  |           |   |
+    //        |   |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |   |   |   |   |  |  |           |   |
+    //        |   |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |   |   |   |   |  |  |           |   |
+    //        v   v  v  v  v  v  v  v  v  v  v  v  v  v  v  v  v  v  v  v  v   v   v   v   v  v  v           v   v


Use named parameters in the new(...) expressions below instead of the ascii art?

(The ASCII art is from the pre-existing code that in turn mirrors the native code... which at some point was generated. The generation was done using a special build with debug prints that were then harvested to calculate the table of most common GC info encoding. The most common encodings then get the short codes which are expanded with this table.)

jkoritzinsky · 2025-05-29T17:30:58Z

...stics.DataContractReader.Contracts/Contracts/StackWalk/Context/X86/GCInfoDecoding/InfoHdr.cs

+    public uint NoGCRegionCount { get; set; } = 0;
+
+    public bool HasArgTabOffset { get; set; }
+    public uint ArgTabOffset { get; set; }


Use private set throughout?

jkoritzinsky · 2025-05-29T17:32:12Z

...stics.DataContractReader.Contracts/Contracts/StackWalk/Context/X86/GCInfoDecoding/InfoHdr.cs

+
+    public bool HasArgTabOffset { get; set; }
+    public uint ArgTabOffset { get; set; }
+    public List<int> Epilogs { get; set; }


Use ImmutableArray here so you don't need to do defensive copying to avoid writing to collections saved in other instances.

Max Charlamb added 8 commits May 27, 2025 13:24

Implement partial ExecutionManager GetFuncletStartAddress

b5140fe

allow nativeAOT compilation on x86

89e9505

implement GCInfo and RelativeAddress lookup

02f14ef

wip

99a7f56

wip working PushedArgDepth

6829dda

finish the bones of x86 unwinder

f71b823

continued testing and bug fixes

3fe4a8a

fix issues with ICFs

6b58012

github-actions bot added the area-Diagnostics-coreclr label May 28, 2025

dotnet-policy-service bot assigned max-charlamb May 28, 2025

Max Charlamb added 7 commits May 28, 2025 11:43

add docs for ExecutionManager changes

fba0091

add doc for IsFunclet

7f6b388

fix some build issues

2917ef9

move gcinfo decoding into folder

34a7169

move x86 context next to other contexts

1b19fb2

add some doc comments

3092546

remove debug print helpers

3b02419

This was referenced May 28, 2025

[Apple-mobile] System.Tests.StringTests.[Starts,Ends]WithNoMatch_StringComparison test failure #108832

Open

Test failure: baseservices/threading/regressions/115178/115178/115178.cmd #116060

Open

filipnavara reviewed May 28, 2025

View reviewed changes

Max Charlamb added 2 commits May 29, 2025 12:35

use constant

05e5909

add x86 stackwalk docs

5e813b6

filipnavara reviewed May 29, 2025

View reviewed changes

jkoritzinsky reviewed May 29, 2025

View reviewed changes

This was referenced May 29, 2025

System.Net.Quic.Tests.MsQuicTests.WriteTests failed with "System.Net.Quic.QuicException : The connection timed out from inactivity." #105177

Open

[wasm] at System.Net.Http.BrowserHttpInterop.CancellationHelper #112533

Open

	private const uint HAS_REV_PINVOKE_FRAME_OFFSET = 0xFFFFFFFF; // is this wrong??
	private const uint HAS_REV_PINVOKE_FRAME_OFFSET = 0xFFFFFFFE;

[cDAC] Support x86 stackwalking #116072

Are you sure you want to change the base?

[cDAC] Support x86 stackwalking #116072

Conversation

max-charlamb commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

IExecutionManager

GetFuncletStartAddress

GetGCInfo

GetRelativeOffset

Extension IsFunclet

IStackWalk

Added support for X86 Platform

Managed Implementation of X86 GCInfo decoding

Managed Implementation of X86 Unwinder

Added X86FrameHandler

Questions

GCInfo Version

Uh oh!

dotnet-policy-service bot commented May 28, 2025

Uh oh!

filipnavara commented May 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filipnavara May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

max-charlamb May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

max-charlamb commented May 29, 2025

Uh oh!

azure-pipelines bot commented May 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

max-charlamb commented May 29, 2025

Uh oh!

azure-pipelines bot commented May 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

max-charlamb commented May 28, 2025 •

edited

Loading

filipnavara May 28, 2025 •

edited

Loading

max-charlamb May 29, 2025 •

edited

Loading