Repository navigation
[cDAC] Remove GetObjectContents from DAC/DBI - #128615
Conversation
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
There was a problem hiding this comment.
Pull request overview
This PR extends the cDAC object contract with a size query API and uses it to implement DacDbiImpl.GetObjectContents, with accompanying documentation and unit tests to validate the new behavior across architectures.
Changes:
- Added
GetObjectSize(TargetPointer address)to theIObjectcontract and implemented it inObject_1. - Implemented
DacDbiImpl.GetObjectContentsusing the contract-reported object size, with DEBUG parity validation against the legacy DAC. - Added unit tests for
GetObjectSize(plain objects + arrays) andGetObjectContents, plus updated the Object contract design doc.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IObject.cs | Adds new GetObjectSize method to the public Object contract interface. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Object_1.cs | Implements object size calculation based on base size and component count/size. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs | Implements GetObjectContents by returning { address, size } from cDAC. |
| src/native/managed/cdac/tests/ObjectTests.cs | Adds unit tests for GetObjectSize on plain objects and arrays across architectures. |
| src/native/managed/cdac/tests/DacDbiImplTests.cs | Adds unit test for GetObjectContents across architectures using a mocked Object contract. |
| docs/design/datacontracts/Object.md | Documents the new GetObjectSize API and its pseudocode. |
Co-authored-by: Copilot Autofix powered by AI <[email protected]>
noahfalk
left a comment
noahfalk
left a comment
There was a problem hiding this comment.
Looking at the caller for GetObjectContents() it appears to be doing a whole bunch of unnecessary work to effectively implement just a type cast. I think we'd be better off simplifying by getting rid of the API rather than porting it.
I agree, when I tried to remove GetObjectContents I tried to work around needing the type cast at all. Given that this is a cast from VMPTR to CORDB_ADDRESS and not the other way around, I think this is a good idea. |
barosiak
changed the title
barosiak
changed the title
|
/ba-g failures unrelated to changes |
Summary
Remove
GetObjectContentsfrom the DAC/DBI interface. The caller indivalue.cppis replaced with a directVmPtrToCookiecast, making the DAC method redundant.Changes
dacdbiimpl.cppIDacDbiInterface,dacdbi.idland mentions in comments.