Skip to content

[cDAC] Implement DacDbi API GetMetaDataFileInfoFromPEFile#129484

Merged
rcj1 merged 11 commits into
mainfrom
copilot/implement-dacdbi-cdac-api
Jun 24, 2026
Merged

[cDAC] Implement DacDbi API GetMetaDataFileInfoFromPEFile#129484
rcj1 merged 11 commits into
mainfrom
copilot/implement-dacdbi-cdac-api

Conversation

@rcj1

@rcj1 rcj1 commented Jun 16, 2026

Copy link
Copy Markdown
Contributor
  • Adding API
bool GetFileHeadersInfo(ModuleHandle handle, out uint timeStamp, out uint imageSize);
  • Implement the DacDbi API.

Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

1 similar comment
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the cDAC Loader contract to obtain module path information from PEAssembly/PEImage (including a diagnostic filename hint for in-memory modules) and adds PE header parsing (timestamp + SizeOfImage) to support implementing the DacDbi GetMetaDataFileInfoFromPEFile API in the legacy layer.

Changes:

  • Replace GetPath(ModuleHandle) with GetPath(TargetPointer peAssemblyPtr, bool fallbackToHint = false) and add GetFileHeadersInfo(...) on ILoader.
  • Move exported “path” data from Module to PEImage (Path + ModuleFileNameHint) and plumb native CDAC descriptors accordingly.
  • Add/adjust unit + dump tests and update Loader contract documentation to reflect the new API shape.
Show a summary per file
File Description
src/native/managed/cdac/tests/UnitTests/MockDescriptors/MockDescriptors.RuntimeMutableTypeSystem.cs Removes mocked Module.Path field from layout.
src/native/managed/cdac/tests/UnitTests/MockDescriptors/MockDescriptors.Loader.cs Removes mocked Module.Path plumbing and related builder inputs.
src/native/managed/cdac/tests/UnitTests/LoaderTests.cs Adds tests + PE-image target builder for new Loader APIs.
src/native/managed/cdac/tests/DumpTests/LoaderDumpTests.cs Updates dump test to call GetPath(GetPEAssembly(handle)).
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.cs Updates legacy SOS path queries to use PEAssembly-based GetPath.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs Implements GetMetaDataFileInfoFromPEFile using new Loader APIs.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataModule.cs Updates module path/in-memory detection to use PEAssembly-based GetPath.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/PEImage.cs Adds PEImage.Path + PEImage.ModuleFileNameHint fields to contract data.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/Module.cs Removes Module.Path field from contract data.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/ImageOptionalHeader.cs Adds SizeOfImage raw offset field.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/ImageFileHeader.cs Adds TimeDateStamp raw offset field.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Loader_1.cs Implements new Loader APIs and removes GetPath(ModuleHandle).
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ILoader.cs Public interface change: adds new members and removes GetPath(ModuleHandle).
src/coreclr/vm/peimage.h Exposes PEImage path + hint buffers via CDAC offsets.
src/coreclr/vm/datadescriptor/datadescriptor.inc Removes Module.Path descriptor; adds PEImage.Path + ModuleFileNameHint.
src/coreclr/vm/ceeload.h Removes cdac_data<Module>::Path offset.
src/coreclr/inc/sstring.h Adds cdac_data<SString>::Buffer helper offset.
src/coreclr/inc/sbuffer.h Adds friend access needed for cdac_data<SString> offset computation.
docs/design/datacontracts/Loader.md Updates contract docs for new GetPath / GetFileHeadersInfo and data fields.

Copilot's findings

  • Files reviewed: 19/19 changed files
  • Comments generated: 7

Comment thread src/native/managed/cdac/tests/UnitTests/LoaderTests.cs
@github-actions

This comment has been minimized.

rcj1 and others added 2 commits June 18, 2026 14:29
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings June 18, 2026 21:32

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

  • Files reviewed: 24/24 changed files
  • Comments generated: 5

@github-actions

This comment has been minimized.

@rcj1 rcj1 requested review from barosiak and noahfalk June 18, 2026 22:21
@github-actions

This comment has been minimized.

Comment thread src/coreclr/inc/sstring.h Outdated
Copilot AI review requested due to automatic review settings June 22, 2026 23:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

  • Files reviewed: 24/24 changed files
  • Comments generated: 4

Comment thread src/coreclr/debug/daccess/daccess.cpp
@github-actions

This comment has been minimized.

rcj1 and others added 2 commits June 23, 2026 09:03
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings June 23, 2026 16:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

  • Files reviewed: 24/24 changed files
  • Comments generated: 3

Copilot AI review requested due to automatic review settings June 23, 2026 16:44

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

  • Files reviewed: 24/24 changed files
  • Comments generated: 5

Comment thread src/native/managed/cdac/tests/UnitTests/LoaderTests.cs

@noahfalk noahfalk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo a few comments inline

Comment thread docs/design/datacontracts/Loader.md Outdated
Comment thread docs/design/datacontracts/Loader.md Outdated
Copilot AI review requested due to automatic review settings June 24, 2026 16:38

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

  • Files reviewed: 19/19 changed files
  • Comments generated: 6

Comment thread docs/design/datacontracts/Loader.md
@github-actions

Copy link
Copy Markdown
Contributor

Copilot Code Review

Holistic Assessment

Motivation: Justified. This implements the GetMetaDataFileInfoFromPEFile DacDbi API in the cDAC, refactored from VMPTR_PEAssembly to VMPTR_Module (renamed to GetModuleMetaDataFileInfo). The module-level indirection simplifies callers and is architecturally sound since the caller always has a module, not a raw PEAssembly.

Approach: Sound. The new ILoader.GetFileHeadersInfo contract method follows established patterns (TryGetPEImage, FindNTHeaders), and the DacDbiImpl implementation correctly validates against legacy DAC in debug builds.

Summary: ⚠️ Needs Human Review. The implementation is correct and well-tested. Two advisory items warrant a human look: the path-fallback pattern is duplicated across three call sites, and the cDAC does not replicate the native COR header presence check inside GetFileHeadersInfo. Neither is blocking — the debug validation against the legacy DAC will catch any mismatch at runtime.


Detailed Findings

Detailed Findings

✅ Contract implementation — GetFileHeadersInfo is correct

The new Loader_1.GetFileHeadersInfo properly:

  • Initializes out parameters to 0 on all paths.
  • Returns false for null PEImage, null loaded layout, and Webcil images (which lack NT headers).
  • Reuses existing TryGetPEImage and FindNTHeaders helpers, consistent with sibling methods like TryGetLoadedImageContents and IsModuleMapped.
  • PE header offsets are correct: TimeDateStamp at offset 4 in IMAGE_FILE_HEADER, SizeOfImage at offset 56 in IMAGE_OPTIONAL_HEADER (same for PE32 and PE32+).

✅ DacDbi interface change — API rename is safe

Per the cDAC instructions (.github/instructions/cdac.instructions.md), the DacDbi COM interface is internal and unstable for .NET 11 dev branches. The rename from GetMetaDataFileInfoFromPEFile(VMPTR_PEAssembly) to GetModuleMetaDataFileInfo(VMPTR_Module) is correctly propagated across the IDL, native C++ headers, and managed interface.

✅ Debug validation

The #if DEBUG block in DacDbiImpl.GetModuleMetaDataFileInfo correctly validates result, timestamp, image size, and path against the legacy DAC. This will catch behavioral divergences at runtime.

✅ Tests are well-structured

Three test cases cover the important scenarios:

  • Happy path: timestamp and image size read correctly.
  • Null loaded image layout: returns false with zeroed outputs.
  • Webcil format: returns false with zeroed outputs.

The shared CreatePETarget helper correctly constructs a minimal PE image in memory using BinaryPrimitives.WriteUInt32LittleEndian at the real PE format offsets.

⚠️ Path-fallback pattern is duplicated — advisory

The try-GetPath → catch VirtualReadExceptionGetFileName → if-empty → GetFileName pattern now appears in three places:

  1. DacDbiImpl.GetModulePath (line ~231)
  2. DacDbiImpl.GetModuleMetaDataFileInfo (line ~3435)
  3. ClrDataModule.GetFileName (line ~506)

Consider extracting this into a shared helper (e.g., GetBestAvailablePath(ModuleHandle)) to reduce duplication. This is advisory — not blocking for this PR, but worth considering as a follow-up.

💡 Design doc pseudocode diverges from implementation

Loader.md pseudocode for GetFileHeadersInfo uses TryGetLoadedImageContents and omits the Webcil format check. The actual implementation uses TryGetPEImage + explicit Webcil check (correctly, since Webcil lacks NT headers). Consider updating the pseudocode to reflect the Webcil guard, since it documents a real behavioral contract.

Note

This review was generated by GitHub Copilot.

Generated by Code Review for issue #129484 · ● 44.2M ·

@rcj1

rcj1 commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

/ba-g #129820

@rcj1 rcj1 merged commit 05cecc1 into main Jun 24, 2026
139 of 142 checks passed
@rcj1 rcj1 deleted the copilot/implement-dacdbi-cdac-api branch June 24, 2026 23:04
@dotnet-milestone-bot dotnet-milestone-bot Bot added this to the 11.0-preview7 milestone Jun 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants