[cDAC] Implement DacDbi APIs EnumerateClassFields and EnumerateInstantiationFields#128829
[cDAC] Implement DacDbi APIs EnumerateClassFields and EnumerateInstantiationFields#128829Copilot wants to merge 4 commits into
Conversation
…h/alloc, full DEBUG validation) Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
There was a problem hiding this comment.
Pull request overview
The PR description frames this as a small follow-up cleanup ("refactor two file-local FieldDataAccumulator helpers to use the existing generic CallbackAccumulator<FieldData> and revert an unnecessary DAC/DBI protocol counter bump"), but the actual diff is a much larger change that also lands the DacDbi FieldData callback-based protocol, the new cDAC IEditAndContinue contract, and supporting data descriptors.
Changes:
- Replace
IDacDbiInterface::GetClassInfo/GetInstantiationFieldInfowith callback-basedEnumerateClassFields/EnumerateInstantiationFields(managed + native) and refactorrsclass.cpp/rstype.cppto drive them viaCallbackAccumulator<FieldData>; widenFieldData::m_fldInstanceOffsettoULONG64andm_pFldStaticAddresstoCORDB_ADDRESS. - Add a new cDAC
IEditAndContinuecontract (EnumerateAddedFieldDescs), newEnCEEClassData/EnCAddedFieldElement/UnorderedArrayBaseData classes + descriptors,Module.EnCClassList(optionalFieldAddress), and supporting source-generator changes for nullableFieldAddress. - Expose
GetNumInstanceFieldBytes/IsFieldDescRVA/IsFieldDescEnCNewonIRuntimeTypeSystem, addEEClass.BaseSizePadding, extractCUnorderedArrayBaseso cDAC can read it, and add mocks + xUnit tests for the new EnC contract.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/inc/dacdbi.idl | Replace GetClassInfo/GetInstantiationFieldInfo with Enumerate* callback-based variants; drop DacDbiArrayList_FieldData/ClassInfo decls. |
| src/coreclr/debug/inc/dacdbiinterface.h | New FP_FIELDDATA_CALLBACK typedef and EnumerateClassFields / EnumerateInstantiationFields virtuals; updated docs. |
| src/coreclr/debug/inc/dacdbistructures.h | Widen m_fldInstanceOffset to ULONG64 and m_pFldStaticAddress to CORDB_ADDRESS; update setter signatures. |
| src/coreclr/debug/inc/dacdbistructures.inl | Adjust FieldData inline accessors to match new CORDB_ADDRESS/ULONG64 types. |
| src/coreclr/debug/daccess/dacdbiimpl.h | Replace GetClassInfo/GetInstantiationFieldInfo decls; change CollectFields to callback form; drop GetTotalFieldCount/InitClassData. |
| src/coreclr/debug/daccess/dacdbiimpl.cpp | Implement callback-based Enumerate*; update ComputeFieldData to use CORDB_ADDRESS; remove preallocation/count logic. |
| src/coreclr/debug/di/rsclass.cpp | Drive EnumerateClassFields via CallbackAccumulator<FieldData> and Init the field list from acc.items. |
| src/coreclr/debug/di/rstype.cpp | Same refactor for EnumerateInstantiationFields. |
| src/coreclr/inc/utilcode.h | Extract CUnorderedArrayBase (count + TADDR table) from CUnorderedArrayWithAllocator and add cdac_data shim. |
| src/coreclr/vm/encee.h | Add cdac_data for EnCEEClassData and EnCAddedFieldElement. |
| src/coreclr/vm/ceeload.h | Add cdac_data<Module>::EnCClassList under FEATURE_METADATA_UPDATER. |
| src/coreclr/vm/class.h | Add cdac_data<EEClass>::BaseSizePadding. |
| src/coreclr/vm/datadescriptor/datadescriptor.inc | Add UnorderedArrayBase, EnCEEClassData, EnCAddedFieldElement descriptors; EEClass.BaseSizePadding; FieldOffsetNewEnc global; EditAndContinue contract entry. |
| .../Abstractions/Contracts/IEditAndContinue.cs | New contract interface (EnumerateAddedFieldDescs) and empty struct. |
| .../Abstractions/Contracts/IRuntimeTypeSystem.cs | Add GetNumInstanceFieldBytes, IsFieldDescRVA, IsFieldDescEnCNew; make fieldDef nullable on GetFieldDescOffset. |
| .../Abstractions/ContractRegistry.cs | Expose EditAndContinue accessor. |
| .../Contracts/CoreCLRContracts.cs | Register EditAndContinue v1. |
| .../Contracts/Contracts/EditAndContinue_1.cs | New v1 implementation enumerating EnC-added FieldDescs. |
| .../Contracts/Contracts/RuntimeTypeSystem_1.cs | Implement GetNumInstanceFieldBytes/IsFieldDescEnCNew; promote IsFieldDescRVA to interface; null-handle big-RVA fieldDef. |
| .../Contracts/Data/{Module,EEClass,EnCEEClassData,EnCAddedFieldElement,UnorderedArrayBase}.cs | New Data classes / fields backing the EnC descriptors. |
| .../Contracts/DataType.cs / Constants.cs | New DataType values and FieldOffsetNewEnc global. |
| .../DataGenerator/Parser.cs, Emitter.cs | Source-gen support for optional/nullable [FieldAddress] properties. |
| .../Legacy/Dbi/{IDacDbiInterface.cs,DacDbiImpl.cs} | Define FieldData struct; implement new managed Enumerate* with optional debug-only legacy validation. |
| src/native/managed/cdac/tests/... | New mock infrastructure (MockDescriptors.EditAndContinue.cs), EnC contract xUnit tests, and TestOptionalFieldAddr data-generator tests. |
| docs/design/datacontracts/{EditAndContinue,RuntimeTypeSystem}.md | Document the new contract and the new IRuntimeTypeSystem APIs/globals. |
Copilot's findings
- Files reviewed: 36/36 changed files
- Comments generated: 1
| // this may be called multiple times. Each call will discard previous values in m_fieldList and reinitialize | ||
| // the list with updated information | ||
| RSLockHolder lockHolder(pProcess->GetProcessLock()); | ||
| IfFailThrow(pProcess->GetDAC()->GetInstantiationFieldInfo(m_pClass->GetModule()->GetRuntimeAssembly(), | ||
|
|
||
| CallbackAccumulator<FieldData> acc; |
| // Mirrors native: storage not yet available; carry the FieldDesc pointer through | ||
| // m_vmFieldDesc but leave all "is" flags false. |
| bool isNullable = prop.Type is INamedTypeSymbol named | ||
| && named.IsGenericType | ||
| && named.ConstructedFrom.ToDisplayString() == "System.Nullable<T>"; |
| if (DWord2 == _target.ReadGlobal<uint>("FieldOffsetBigRVA") && fieldDef.HasValue) | ||
| { | ||
| return (uint)fieldDef.GetRelativeVirtualAddress(); | ||
| return (uint)fieldDef.Value.GetRelativeVirtualAddress(); | ||
| } |
noahfalk
left a comment
There was a problem hiding this comment.
Looked good other than a few comments inline.
| @@ -0,0 +1,64 @@ | |||
| # Contract EditAndContinue | |||
There was a problem hiding this comment.
| # Contract EditAndContinue | |
| # Contract RuntimeMutableTypeSystem |
Naming nit: I believe we have scenarios other than ENC (such as HotReload) that can modify types on the fly too.
| @@ -0,0 +1,64 @@ | |||
| # Contract EditAndContinue | |||
|
|
|||
| This contract exposes the runtime's Edit-and-Continue (EnC) bookkeeping. | |||
There was a problem hiding this comment.
| This contract exposes the runtime's Edit-and-Continue (EnC) bookkeeping. | |
| This contract exposes runtime type system information about changes that occured after the initial type load, such as from ENC or HotReload. |
| bool IsFieldDescThreadStatic(TargetPointer fieldDescPointer); | ||
| bool IsFieldDescStatic(TargetPointer fieldDescPointer); | ||
| bool IsFieldDescRVA(TargetPointer fieldDescPointer); | ||
| bool IsFieldDescEnCNew(TargetPointer fieldDescPointer); |
There was a problem hiding this comment.
Could we put this in the ENC contract? A runtime that doesn't implement the ENC contract shouldn't have discoverable ENC FieldDescs.
|
|
||
| ### FieldDesc | ||
|
|
||
| The version 1 FieldDesc APIs depend on the following globals: |
There was a problem hiding this comment.
The comment implies we have a separately versioning FieldDesc contract but I don't believe we do right? Unless we plan to define a separate contract this sounds like it should get merged with other RuntimeTypeSystem references.
| // Free the chunk of memory. | ||
| if (m_pTable != NULL) | ||
| ALLOCATOR::Free(this, m_pTable); | ||
| if (m_pTable != (TADDR)NULL) |
There was a problem hiding this comment.
Rather than modify all these members is anything stopping us from leaving m_pTable conditionally typed as T*/TADDR?
Adding the following RuntimeTypeSystem APIs: