[cDAC] Implement basic stackwalking #111759
Conversation
max-charlamb
force-pushed
the
cdac-stackwalk
branch
from
4f1fbee to
5d9317a
Compare
max-charlamb
force-pushed
the
cdac-stackwalk
branch
from
8009551 to
a794374
Compare
| #elif defined(FEATURE_CDAC_UNWINDER) | ||
|
|
||
| // TODO: add asserts on cDAC build without importing more headers. | ||
| #define UNWINDER_ASSERT(x) |
There was a problem hiding this comment.
Is there a way to add an assert here without importing common.h headers?
There was a problem hiding this comment.
Make a callback that calls the managed Debug.Fail() in the case where the assert fails? This would allow you to have a centralized handling of asserts in the cDAC (turn them into logging, etc.)
| @@ -367,7 +367,7 @@ extends: | |||
| - windows_x64 | |||
| jobParameters: | |||
| templatePath: 'templates-official' | |||
| buildArgs: -s tools+libs -pack -c $(_BuildConfig) /p:TestAssemblies=false /p:TestPackages=true | |||
| buildArgs: -s tools.illink+libs -pack -c $(_BuildConfig) /p:TestAssemblies=false /p:TestPackages=true | |||
There was a problem hiding this comment.
Discussed with @jkoritzinsky , cDAC should not currently be built in the Libraries_WithPackages run. This started failing because the native cDAC components were not available.
| // Creates a stack walk and returns a handle | ||
| IStackWalkHandle CreateStackWalk(ThreadData threadData); | ||
| // Iterates the stackWalkHandle to the next frame. If successful, returns true. Otherwise false. | ||
| bool Next(IStackWalkHandle stackWalkHandle); | ||
| // Gets the current frame from a stack walk and returns a IStackDataFrameHandle to it. | ||
| IStackDataFrameHandle GetCurrentFrame(IStackWalkHandle stackWalkHandle); |
There was a problem hiding this comment.
Instead of defining a handle interface to drive enumeration and nothing else, could we instead provide the following method?
IEnumerable<IStackDataFrameHandle> EnumerateFrames(ThreadData threadData);This method could lazily walk the frames (like how I implemented reading stress log messages in my StressLog cdac PR), which would preserve existing performance characteristics.
| IStackWalkHandle CreateStackWalk(ThreadData threadData); | ||
| ``` | ||
|
|
||
| Creates a stack walk handle for the given thread data. This initializes the context and frame iterator for the stack walk. |
There was a problem hiding this comment.
I don't see documentation about how to do all of this. We need that before we can actually merge this change, not just C# implementation in the actual cDAC codebase. Where we use the a well documented standard like the Win32 unwinder, you can just drop in a link, but for everything else... it needs correct documentation/algorithms
| } | ||
| public unsafe byte[] GetBytes() | ||
| { | ||
| Span<T> structSpan = MemoryMarshal.CreateSpan(ref Context, 1); |
There was a problem hiding this comment.
This should just be new Span(ref Context) .... I also think you should probably make T have the unmanaged constraint.
|
|
||
| namespace Microsoft.Diagnostics.DataContractReader.Contracts.StackWalkHelpers; | ||
|
|
||
| public class ContnextHolder<T> : IPlatformAgnosticContext where T : struct, IPlatformContext |
There was a problem hiding this comment.
ContextHolder ... not ContnextHolder
| @@ -31,6 +31,14 @@ | |||
| <!-- <InstallRuntimeComponentDestination Include="sharedFramework" Condition="'$(RuntimeFlavor)' == 'coreclr'"/> --> | |||
| </ItemGroup> | |||
|
|
|||
| <ItemGroup> | |||
| <!-- TODO: Link libraries on non-windows platforms. --> | |||
There was a problem hiding this comment.
This should either be done as part of this PR, or you should have an open issue about this.
There was a problem hiding this comment.
This is non-trivial, and I think best left another PR.
Created #112416 and linking it in the comment.
| DataType.InlinedCallFrame, | ||
| DataType.SoftwareExceptionFrame, | ||
|
|
||
| DataType.ResumableFrame, |
There was a problem hiding this comment.
A giant list here, doesn't feel like the right implementation. This will change with different versions of the runtime, and in general we try to avoid putting business logic into the Data portion of the contracts.
There was a problem hiding this comment.
I'll move this logic to the FrameIterator so it won't be on the IData type.
I was thinking about a better way to do this, but I'm not sure there is a great way because we need a list of DataTypes which inherit from Frame.
I was also considering moving the VPtr (soon to be enum values) from global in the datadescriptors to fields on the DataType. That isn't quite right either, because it isn't an offset, but an actual value. Maybe datadescriptors could support the notion of a "static" member in the future.
There was a problem hiding this comment.
When we move to enum values... we can likely port the HasValidFrameIdentifier api, which is actually much simpler than this, since it can operate with a low and a high value for valid frame identifiers.
Contributes to #110758
Implements bulk of the cDAC stackwalking mechanics.
IStackWalkcontractSupports
amd64andarm64architectures. Tested on Windows targetingwin-amd64andlinux-arm64dumps.Supports
InlineCallFramesandSoftwareExceptionFrames.Does not support (will be added in future PRs:
IXCLRDataStackWalk::RequestparametersCLRDATA_REQUEST_REVISIONandCLRDATA_STACK_WALK_REQUEST_SET_FIRST_FRAMEClrDataAccess::GetFrameName, currently falls back to the DAC implementation. Used by SOS to get the name of a given Frame.