[cDAC] Rename GetPEFileMDInternalRW to HasReadWriteMetadata and implement in cDAC

[Copilot]

Renames GetPEFileMDInternalRW to HasReadWriteMetadata and returns metadata writability directly instead of exposing an internal target address.

Changes

• Updated native and managed DacDbi interfaces and callers.
• Matched cDAC behavior to GetMDInternalRWAddress() by checking both the writable-import flag and non-null metadata pointer.
• Added the writable-import field to the cDAC data descriptor and documentation.
• Added coverage for writable, read-only, and invalid-output cases.

[Copilot]

Copilot was unable to review this pull request because there is no eligible user to bill. To allow Copilot reviews on bot-authored pull requests, enable direct organization billing in your organization's Copilot settings.

[azure-pipelines]

Azure Pipelines:
Successfully started running 5 pipeline(s).
11 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[azure-pipelines]

Azure Pipelines:
Successfully started running 5 pipeline(s).
11 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

Copilot's findings

Comments suppressed due to low confidence (2)

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs:5451

• The null-output case throws ArgumentException, which will typically surface as COR_E_ARGUMENT (0x80131500) rather than E_INVALIDARG. This contradicts both the new unit test expectation and the native DAC implementation (returns E_INVALIDARG). Use an exception with HRESULT=E_INVALIDARG (or return HResults.E_INVALIDARG) so the managed and native behaviors match.

            if (pHasReadWriteMetadata is null)
                throw new ArgumentException("Output pointer cannot be null.", nameof(pHasReadWriteMetadata));

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IEcmaMetadata.cs:14

• This adds a new method to the public IEcmaMetadata contract surface. Per repo policy, new public APIs should be backed by an api-approved issue (or the API should be made non-public). The PR description currently doesn't reference any approved API issue.

    static string IContract.Name { get; } = nameof(EcmaMetadata);
    bool HasReadWriteMetadata(TargetPointer peAssembly) => throw new NotImplementedException();
    TargetSpan GetReadOnlyMetadataAddress(ModuleHandle handle) => throw new NotImplementedException();
    TargetSpan GetReadWriteSavedMetadataAddress(ModuleHandle handle) => throw new NotImplementedException();

• Files reviewed: 15/15 changed files
• Comments generated: 0 new

[dotnet-policy-service]

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

[Copilot]

Copilot's findings

Comments suppressed due to low confidence (3)

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IEcmaMetadata.cs:13

• A new public API member was added to the public IEcmaMetadata contract interface (HasReadWriteMetadata) but the PR description doesn’t link an api-approved issue. Per repo policy, new public API surface must be covered by an approved API proposal/issue, or the new member should be made internal until approval is obtained.

public interface IEcmaMetadata : IContract
{
    static string IContract.Name { get; } = nameof(EcmaMetadata);
    bool HasReadWriteMetadata(TargetPointer peAssembly) => throw new NotImplementedException();
    TargetSpan GetReadOnlyMetadataAddress(ModuleHandle handle) => throw new NotImplementedException();

src/native/managed/cdac/tests/UnitTests/DacDbiImplTests.cs:1678

• The new HasReadWriteMetadata logic requires both (1) MDImportIsRW != 0 and (2) MDImport != null, but this test currently covers only the “MDImport only” and “both set” cases. Please add a case where MDImportIsRW is set but MDImport is null to ensure the second condition is actually validated.

        MockMemorySpace.HeapFragment readOnlyPEAssembly = allocator.Allocate(peAssemblyLayout.Stride, "ReadOnlyPEAssembly");
        MockMemorySpace.HeapFragment readWritePEAssembly = allocator.Allocate(peAssemblyLayout.Stride, "ReadWritePEAssembly");
        helpers.WritePointer(
            readOnlyPEAssembly.Data.AsSpan().Slice(peAssemblyLayout.Fields[nameof(Data.PEAssembly.MDImport)].Offset, helpers.PointerSize),
            0x1800);

src/coreclr/vm/peassembly.h:172

• PEAssembly::HasReadWriteMetadata currently returns only the m_MDImportIsRW_Debugger_Use_Only flag. Other call sites (e.g., GetMDInternalRWAddress and the managed cDAC contract) effectively treat RW metadata as present only when the underlying MDInternalRW pointer is non-null. Implementing this helper in terms of GetMDInternalRWAddress() keeps the semantics consistent and avoids future callers accidentally using the flag alone.

    BOOL HasReadWriteMetadata()
    {
        LIMITED_METHOD_DAC_CONTRACT;
        return m_MDImportIsRW_Debugger_Use_Only;
    }

• Files reviewed: 15/15 changed files
• Comments generated: 0 new

[Copilot]

Copilot was unable to review this pull request because there is no eligible user to bill. To allow Copilot reviews on bot-authored pull requests, enable direct organization billing in your organization's Copilot settings.

[Copilot]

Copilot's findings

Comments suppressed due to low confidence (2)

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs:5450

• Using exceptions for expected argument validation here adds avoidable overhead (allocation + stack walk) on an API boundary that may be called frequently. Prefer returning HResults.E_INVALIDARG directly when pHasReadWriteMetadata is null, and reserve exceptions for truly exceptional failures.

            if (pHasReadWriteMetadata is null)
                throw new ArgumentException("Output pointer cannot be null.", nameof(pHasReadWriteMetadata));

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs:5454

• This implementation relies solely on the cDAC EcmaMetadata contract. For targets where that contract version does not implement HasReadWriteMetadata, this will fall into the exception path and return the exception HResult (commonly E_NOTIMPL), even if the legacy DAC path could have answered the question. If cross-version support is required here, consider a fallback: when the contract call is not available/returns E_NOTIMPL and LegacyFallbackHelper.CanFallback() is true, derive the boolean from the legacy API (e.g., call the old GetPEFileMDInternalRW and check for non-null).

            bool hasReadWriteMetadata = _target.Contracts.EcmaMetadata.HasReadWriteMetadata(new TargetPointer(vmPEAssembly));
            *pHasReadWriteMetadata = hasReadWriteMetadata ? Interop.BOOL.TRUE : Interop.BOOL.FALSE;

• Files reviewed: 15/15 changed files
• Comments generated: 2

[rcj1]

/ba-g timeouts