[cDAC] Implement GetNativeCodeSequencePointsAndVarInfo for cDAC#129267
[cDAC] Implement GetNativeCodeSequencePointsAndVarInfo for cDAC#129267barosiak wants to merge 9 commits into
Conversation
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
There was a problem hiding this comment.
Pull request overview
This PR updates the DAC↔DBI contract for GetNativeCodeSequencePointsAndVarInfo to use a callback-based enumeration model (instead of DBI-allocated output buffers), and wires up the managed cDAC (DacDbiImpl) implementation plus supporting interop structs and unit tests.
Changes:
- Replaces out-pointer buffer parameters with callback delegates for native var info and sequence points (IDL + native headers + native implementation + DBI consumer).
- Adds managed interop representations for native debug-info types and implements the cDAC-side method that enumerates var info and sequence points via callbacks.
- Adds unit tests for var location kind mapping and SourceTypes flag conversion.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/native/managed/cdac/tests/UnitTests/DacDbiImplTests.cs | Adds unit tests for VarLoc kind/field mapping and SourceTypes flag conversion. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/IDacDbiInterface.cs | Adds managed interop structs/enums and updates method signature to callback-based pattern. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.NativeCodeInfo.cs | Adds conversion helpers, fixed-arg counting helper, and DEBUG legacy validation. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs | Implements callback-based GetNativeCodeSequencePointsAndVarInfo using cDAC contracts. |
| src/coreclr/inc/dacdbi.idl | Updates the COM IDL to define callback typedefs and the new method signature. |
| src/coreclr/inc/cordebuginfo.h | Documents that specific debug-info types are mirrored in managed code. |
| src/coreclr/debug/inc/dacdbiinterface.h | Updates the native interface signature and adds callback typedefs. |
| src/coreclr/debug/di/module.cpp | Updates CordbNativeCode::LoadNativeInfo to collect entries via callbacks and initialize its internal containers. |
| src/coreclr/debug/daccess/dacdbiimpl.h | Updates the native DAC implementation signature to the new callback-based shape. |
| src/coreclr/debug/daccess/dacdbiimpl.cpp | Rewrites the DAC implementation to enumerate var info / sequence points and invoke callbacks. |
| VLT_INVALID, | ||
| } | ||
|
|
||
| // The native VarLoc struct contains a union with a void* member (vlMemory), |
There was a problem hiding this comment.
I think we can remove this member
There was a problem hiding this comment.
This will probably require changing the layout in src/coreclr/tools/Common/JitInterface/CorInfoTypes.VarInfo.cs as well as the JITEEVersionIdentifier
There was a problem hiding this comment.
Removed. Is that what you had in mind?
| string? paramName = GetParameterName(mdReader, methodDef, mdIndex); | ||
| OutputBufferHelpers.CopyStringToBuffer(name, bufLen, nameLen, paramName ?? string.Empty); | ||
| uint token = rts.GetMethodToken(mdh); | ||
| MetadataReader? mdReader = (EcmaMetadataUtils.GetRowId(token) != 0) |
There was a problem hiding this comment.
What happens without the null row check? And how does the native equivalent deal with this?
There was a problem hiding this comment.
Array Get/Set/Address methods aren't no-metadata but carry a nil MethodDef token, which would throw when resolving the param name from an invalid row.
FindParamOfMethod just returns an error HRESULT for a nil token, so the row-id check skips name resolution to match.
…eSequencePointsAndVarInfo
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| public virtual bool IsAsyncMethod(MethodDescHandle methodDesc); | ||
|
|
||
| // Gets the raw signature bytes for a MethodDesc by checking the stored signature, | ||
| // then the async variant signature, then ECMA metadata. Returns false if no |
There was a problem hiding this comment.
The implementation details of where you look for these are not part of the contract, please remove from this comment block
| { | ||
| [Field] public uint Flags { get; } | ||
| [Field] public TargetPointer Sig { get; } | ||
| [Field] public uint cSig { get; } |
There was a problem hiding this comment.
| [Field] public uint cSig { get; } | |
| [Field] public uint CSig { get; } |
nit
There was a problem hiding this comment.
We shouldn't use Hungarian notation in C# anyways. See VASigCookie where these are renamed to SignaturePointer/SignatureLength.
There was a problem hiding this comment.
Looking at this more, we may want to actually have an IData type for Signature instead of accessing the fields directly across multiple types (VASigCookie and now AsyncMethodData)
| if (methodDesc.Classification is MethodClassification.Dynamic or MethodClassification.EEImpl or MethodClassification.Array) | ||
| { | ||
| signature = AsStoredSigMethodDesc(methodDesc).Signature; | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Use existing IsStoredSigMethodDesc
| // Gets the raw signature bytes for a MethodDesc by checking stored signature, async variant signature, then metadata. | ||
| // Returns false if no signature could be resolved. | ||
| bool TryGetMethodSignature(MethodDescHandle methodDesc, out ReadOnlySpan<byte> signature) => throw new NotImplementedException(); |
There was a problem hiding this comment.
Does it make more sense to implement just the Async variant signature lookup? These other operations are already exposed. Similar idea to #129484 (comment)
It seems like we could expose this helper functionality on MethodSignatureHelpers
There was a problem hiding this comment.
I prefer consolidating them just because I do not see one being used without the other and they all serve the same purpose.
There was a problem hiding this comment.
Then we should remove the stored sig variant (IsStoredSigMethodDesc).
Summary
Implement
GetNativeCodeSequencePointsAndVarInfoin the cDACDacDbiImpl. The interface is changed from output-pointer parameters (NativeVarData*,SequencePoints*) to a callback pattern (FP_NATIVEVARINFO_CALLBACK,FP_SEQUENCEPOINT_CALLBACK), since the managed cDAC cannot use the nativeforDbiallocator.Also adds
TryGetMethodSignaturetoIRuntimeTypeSystemto fixGetArgCountandClrDataFramemethods failing on methods without metadata tokens (e.g. async variants), and removes the unused vlMemory member from ICorDebugInfo::VarLoc (with a JIT/EE version GUID bump).Changes
dacdbi.idl/dacdbiinterface.h/dacdbiimpl.h- Add callback typedefs, update method signaturedacdbiimpl.cpp- Rewrite method to use callbacks, removeGetNativeVarData/GetSequencePointshelpersmodule.cpp- RewriteLoadNativeInfowith callback structIDacDbiInterface.cs- AddVarLocType,VarLoc,NativeVarInfo,DbiOffsetMappingstructs, DbiSourceTypes enum and update method signatureDacDbiImpl.cs- ImplementGetNativeCodeSequencePointsAndVarInfoDacDbiImpl.NativeCodeInfo.cs- conversion helpers and DEBUG validationDacDbiImplTests.cs- unit tests for VarLoc type mapping, field-level conversion, and SourceTypes flag conversionIRuntimeTypeSystem.cs/RuntimeTypeSystem_1.cs- AddTryGetMethodSignatureAsyncMethodData.cs/method.hpp/siginfo.hpp/datadescriptor.inc- ExposeSigandcSigfields fromAsyncMethodDataClrDataFrame.cs- ExtractGetFrameMethodDeschelper, update callers to useTryGetMethodSignatureTypeNameBuilder.cs/MethodSignatureHelpers.cs- UseTryGetMethodSignaturefor signature resolution