Add Stub IXCLRDataFunctionTableAccess to cDAC

[noahfalk]

windbg needs an implementation of function table lookup to migrate away from DAC. In DAC this comes from the exported function OutOfProcessFunctionTableCallbackEx but since I didn't want tie the behavior to a specific dll export I created a new interface for it instead.

This just creates the stub of an implementation which will still need to be filled in.

[azure-pipelines]

Azure Pipelines:
Successfully started running 4 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.

[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.

[noahfalk]

fyi @hoyosjs

[Copilot]

Pull request overview

Adds a new COM surface for function-table lookup in the cDAC “Legacy” layer by introducing IXCLRDataFunctionTableAccess and wiring SOSDacImpl to implement it (currently as an E_NOTIMPL stub), along with a unit test that exercises QueryInterface from IXCLRDataProcess.

Changes:

• Introduces a new [GeneratedComInterface] COM interface: IXCLRDataFunctionTableAccess with GetFunctionTable(...).
• Implements IXCLRDataFunctionTableAccess on SOSDacImpl with a stub GetFunctionTable returning E_NOTIMPL.
• Adds a unit test validating QueryInterface returns the new interface and that calling the stub returns E_NOTIMPL.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

File	Description
src/native/managed/cdac/tests/UnitTests/FunctionTableAccessTests.cs	Adds a unit test that QIs IXCLRDataProcess for IXCLRDataFunctionTableAccess and calls the stub method.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.IXCLRDataProcess.cs	Wires SOSDacImpl to implement IXCLRDataFunctionTableAccess and returns E_NOTIMPL from the new method.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/IXCLRData.cs	Declares the new IXCLRDataFunctionTableAccess COM interface and GUID.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

[max-charlamb]

lgtm mod the copilot comment that the C style macros aren't included in xclrdata.h. I don't think this should ever really matter, but its nice for consistency.

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "f44790a5b5341791c0dbf72a7d1dce50d37d5559",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "e5fc9669cc6a67694d46987c753cc13d995b2623",
  "last_reviewed_commit": "f44790a5b5341791c0dbf72a7d1dce50d37d5559",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "e5fc9669cc6a67694d46987c753cc13d995b2623",
  "last_recorded_worker_run_id": "29690719329",
  "review_attempt_commit": "",
  "review_attempt_base_ref": "",
  "review_attempt_count": 0,
  "max_review_attempts": 5,
  "review_history_format": "holistic-review-disclosure-v1",
  "review_history": [
    {
      "commit": "f44790a5b5341791c0dbf72a7d1dce50d37d5559",
      "review_id": 4730926021
    }
  ]
}

[github-actions]

Holistic Review

Motivation: windbg needs a way to look up dynamic function tables (currently provided by the DAC export OutOfProcessFunctionTableCallbackEx) so it can migrate off the DAC. Rather than tying the behavior to a specific DLL export, the PR introduces a new COM interface for it. This PR establishes only the stub; the actual lookup logic is intentionally deferred.

Approach: A new IXCLRDataProcess3 : IXCLRDataProcess2 interface exposing a single GetFunctionTable method is added to xclrdata.idl, with the matching regenerated MIDL artifacts in pal/prebuilt (xclrdata_i.cpp GUID definition and xclrdata.h interface declaration). The managed cDAC side adds a [GeneratedComInterface] mirror in IXCLRData.cs, implements it on SOSDacImpl returning E_NOTIMPL (while zero-initializing the bytesNeeded/entries outputs), and adds a unit test verifying QueryInterface navigation across IXCLRDataProcess/2/3 (COM identity consistency) plus the E_NOTIMPL contract.

Summary: ✅ LGTM. This is a well-scoped, self-consistent stub. The new GUID ...c798b9 follows the existing sequential convention (...b7, ...b8, ...b9 for IXCLRDataProcess/2/3) and is unique across the IDL, prebuilt, and managed definitions. The IDL, prebuilt .h, and managed signatures agree on parameter order and semantics, the doc comment clearly specifies the intended eventual behavior, and the test exercises both QI identity and the stub contract. These are Legacy/** (SOSDacImpl shim / DacDbi) changes, so per repo cDAC guidance no datacontract doc update or public-API-review is required, and COM ABI/GUID concerns do not apply. One minor, non-blocking observation: the IDL comment states bytesNeeded and entries are "reported on every valid call," while the implementation sets them on the E_NOTIMPL failure path — harmless and arguably safer, just a slight wording mismatch to keep in mind when the method is later implemented.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 56 AIC · ⌖ 10.3 AIC · ⊞ 10K

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

src/coreclr/pal/prebuilt/inc/xclrdata.h:2906

• The new IXCLRDataProcess3 definition in the prebuilt header only includes the C++ interface form and is missing the corresponding "C style interface" (Vtbl struct + COBJMACROS) that exists for IXCLRDataProcess2. This makes the header inconsistent and breaks C consumers when CINTERFACE is defined; it also suggests the prebuilt file wasn’t regenerated from the IDL.

#ifndef __IXCLRDataProcess3_INTERFACE_DEFINED__
#define __IXCLRDataProcess3_INTERFACE_DEFINED__

/* interface IXCLRDataProcess3 */
/* [uuid][local][object] */

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/IXCLRData.cs:372

• This introduces a new public interface (IXCLRDataProcess3) in a shipped NuGet (IsPackable=true). If this type is only used for internal COM surface (WinDbg/native callers) and tests already have InternalsVisibleTo, consider making it internal to avoid expanding managed public API surface (and the associated API-approval requirements).

[GeneratedComInterface]
[Guid("5c552ab6-fc09-4cb3-8e36-22fa03c798b9")]
public unsafe partial interface IXCLRDataProcess3 : IXCLRDataProcess2
{

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.IXCLRDataProcess.cs:35

• bytesNeeded and entries are documented as required output parameters (see xclrdata.idl) and other SOSDacImpl methods generally return E_POINTER when required out-pointers are null. The current stub silently tolerates null pointers, which would leave callers with undefined outputs and makes debugging harder.

    int IXCLRDataProcess3.GetFunctionTable(
        ClrDataAddress tableAddress,
        uint bufferSize,
        byte* buffer,
        uint* bytesNeeded,
        uint* entries)
    {
        if (bytesNeeded is not null)
            *bytesNeeded = 0;
        if (entries is not null)
            *entries = 0;

        return HResults.E_NOTIMPL;
    }

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/IXCLRData.cs:372

• This adds a new public API surface (IXCLRDataProcess3) in a packable assembly. If this type isn't intended as a supported public contract, consider making it internal (tests can still access it via the existing InternalsVisibleTo) to avoid taking on a public API compatibility burden / API-approval requirements.

[GeneratedComInterface]
[Guid("5c552ab6-fc09-4cb3-8e36-22fa03c798b9")]
public unsafe partial interface IXCLRDataProcess3 : IXCLRDataProcess2
{

src/coreclr/pal/prebuilt/inc/xclrdata.h:2928

• xclrdata.h is marked as an ALWAYS GENERATED MIDL output, and the new IXCLRDataProcess3 block is missing the usual #else /* C style interface */ section (present on IXCLRDataProcess2, etc.). This looks like a partial/manual edit or an incomplete regeneration and can break C/CINTERFACE consumers.

#ifndef __IXCLRDataProcess3_INTERFACE_DEFINED__
#define __IXCLRDataProcess3_INTERFACE_DEFINED__

/* interface IXCLRDataProcess3 */
/* [uuid][local][object] */


EXTERN_C const IID IID_IXCLRDataProcess3;

#if defined(__cplusplus) && !defined(CINTERFACE)

    MIDL_INTERFACE("5c552ab6-fc09-4cb3-8e36-22fa03c798b9")
    IXCLRDataProcess3 : public IXCLRDataProcess2
    {
    public:
        virtual HRESULT STDMETHODCALLTYPE GetFunctionTable(
            /* [in] */ CLRDATA_ADDRESS tableAddress,
            /* [in] */ ULONG32 bufferSize,
            /* [size_is][out] */ BYTE *buffer,
            /* [out] */ ULONG32 *bytesNeeded,
            /* [out] */ ULONG32 *entries) = 0;

    };

#endif

#endif 	/* __IXCLRDataProcess3_INTERFACE_DEFINED__ */

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/IXCLRData.cs:380

• This introduces a new public COM interface (IXCLRDataProcess3) in the managed cDAC Legacy surface. New public API surface in dotnet/runtime requires an approved API proposal/issue (api-approved) or the API should be kept internal until approval is obtained. If this is intended to be internal-only, consider making the interface (and corresponding implementation exposure) internal and using InternalsVisibleTo for the test project; otherwise, please link the approved API issue in the PR description.

[GeneratedComInterface]
[Guid("5c552ab6-fc09-4cb3-8e36-22fa03c798b9")]
public unsafe partial interface IXCLRDataProcess3 : IXCLRDataProcess2
{
    [PreserveSig]
    int GetFunctionTable(
        ClrDataAddress tableAddress,
        uint bufferSize,
        byte* buffer,
        uint* bytesNeeded,
        uint* entries);
}