Convert IDacDbiInterface to COM interface#125074
Convert IDacDbiInterface to COM interface#125074max-charlamb wants to merge 4 commits intodotnet:mainfrom
Conversation
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
There was a problem hiding this comment.
Pull request overview
This PR converts IDacDbiInterface (DAC↔DBI contract) from a plain C++ abstract class to a COM-style interface (IUnknown + GUID), with the goal of enabling a future managed (C#) cDAC implementation and standard COM lifetime management.
Changes:
- Make
IDacDbiInterfacea COM interface (MIDL_INTERFACE,IUnknownbase) and remove the customDestroy()lifecycle method in favor ofRelease(). - Implement
QueryInterface/AddRef/ReleaseonDacDbiInterfaceImpland update DBI call sites for the new pointer-based out params. - Add
dacdbi.idldescribing the intended COM contract (including callback interfaces).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/debug/inc/dacdbiinterface.h | Converts IDacDbiInterface to MIDL_INTERFACE + IUnknown, removes Destroy(), adjusts several signatures for COM compatibility. |
| src/coreclr/debug/inc/dacdbi.idl | Adds an IDL definition for the full COM contract and callback interfaces. |
| src/coreclr/debug/di/rstype.cpp | Updates GetExactTypeHandle call to pass an out pointer. |
| src/coreclr/debug/di/rspriv.h | Updates forward-declaration to struct to match MIDL_INTERFACE expansion. |
| src/coreclr/debug/di/process.cpp | Updates metadata query call sites to use pointer out-params; switches DAC teardown from Destroy() to Release(). |
| src/coreclr/debug/di/module.cpp | Updates metadata query call sites to use pointer out-params. |
| src/coreclr/debug/di/divalue.cpp | Updates exception stack frame query to pass DacDbiArrayList by pointer. |
| src/coreclr/debug/daccess/dacdbiimpl.h | Declares IUnknown methods and updates signatures to pointer out-params. |
| src/coreclr/debug/daccess/dacdbiimpl.cpp | Implements IUnknown methods; updates implementations for pointer out-params. |
Comments suppressed due to low confidence (2)
src/coreclr/debug/daccess/dacdbiimpl.cpp:1201
- GetMetaDataFileInfoFromPEFile now takes pTimeStamp/pImageSize as pointers, but the implementation unconditionally dereferences them on the success path and also leaves them uninitialized when vmPEAssembly resolves to NULL. For COM-style APIs (and consistent with other methods in this file that return E_POINTER), please validate pTimeStamp/pImageSize (and other required out params like pResult) up-front, return E_POINTER when null, and ensure out values are always initialized on all return paths.
HRESULT DacDbiInterfaceImpl::GetMetaDataFileInfoFromPEFile(VMPTR_PEAssembly vmPEAssembly, DWORD * pTimeStamp, DWORD * pImageSize, IStringHolder* pStrFilename, OUT bool * pResult)
{
DD_ENTER_MAY_THROW;
HRESULT hr = S_OK;
EX_TRY
{
DWORD dwDataSize;
DWORD dwRvaHint;
PEAssembly * pPEAssembly = vmPEAssembly.GetDacPtr();
_ASSERTE(pPEAssembly != NULL);
if (pPEAssembly == NULL)
{
*pResult = false;
}
src/coreclr/debug/daccess/dacdbiimpl.cpp:3756
- GetStackFramesFromException now takes the DacDbiArrayList as a pointer, but the method doesn’t validate pDacStackFrames before dereferencing it (Alloc / operator[]). Please add an E_POINTER check at the top of the method (and consider initializing it to an empty list on success paths where there are 0 frames).
HRESULT DacDbiInterfaceImpl::GetStackFramesFromException(VMPTR_Object vmObject, DacDbiArrayList<DacExceptionCallStackData>* pDacStackFrames)
{
DD_ENTER_MAY_THROW;
HRESULT hr = S_OK;
EX_TRY
{
PTR_Object objPtr = vmObject.GetDacPtr();
#ifdef _DEBUG
| // | ||
| virtual HRESULT DacSetTargetConsistencyChecks(bool fEnableAsserts) = 0; | ||
|
|
There was a problem hiding this comment.
After removing Destroy() in favor of COM Release(), there’s still a stale reference to Destroy() in the FlushCache() notes earlier in this file ("besides Destroy"). Please update that comment to refer to Release() (or remove the parenthetical) so the interface contract stays accurate.
| @@ -2753,12 +2764,12 @@ HRESULT DacDbiInterfaceImpl::GetExactTypeHandle(DebuggerIPCE_ExpandedTypeData * | |||
|
|
|||
| EX_TRY | |||
| { | |||
| vmTypeHandle = vmTypeHandle.NullPtr(); | |||
| *pVmTypeHandle = VMPTR_TypeHandle::NullPtr(); | |||
|
|
|||
There was a problem hiding this comment.
GetExactTypeHandle switched from an out reference to an out pointer (pVmTypeHandle) but the implementation immediately dereferences it without validating. To match the rest of DacDbiInterfaceImpl’s COM-style behavior, please add a null check for pVmTypeHandle and return E_POINTER before writing through it.
3d9e111 to
f3b3cb8
Compare
f3b3cb8 to
1c619d9
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/coreclr/debug/daccess/dacdbiimpl.cpp:1190
- GetMetaDataFileInfoFromPEFile now takes multiple pointer parameters (pTimeStamp, pImageSize, pStrFilename, pResult) but does not validate them before dereferencing later in the method. This was previously safe with reference parameters; now a null argument will crash. Add E_POINTER checks (and consider setting outputs to safe defaults on failure paths).
HRESULT DacDbiInterfaceImpl::GetMetaDataFileInfoFromPEFile(VMPTR_PEAssembly vmPEAssembly, DWORD * pTimeStamp, DWORD * pImageSize, IStringHolder* pStrFilename, OUT BOOL * pResult)
{
DD_ENTER_MAY_THROW;
HRESULT hr = S_OK;
| @@ -3761,12 +3772,12 @@ HRESULT DacDbiInterfaceImpl::GetStackFramesFromException(VMPTR_Object vmObject, | |||
|
|
|||
| if (dacStackFramesLength > 0) | |||
| { | |||
There was a problem hiding this comment.
GetStackFramesFromException changed its DacDbiArrayList parameter from a reference to a pointer, but the implementation dereferences pDacStackFrames unconditionally (Alloc / operator[]). This introduces a null-deref crash path; validate pDacStackFrames (and any other required pointer params) and return E_POINTER when null.
| { | |
| { | |
| if (pDacStackFrames == NULL) | |
| { | |
| ThrowHR(E_POINTER); | |
| } |
| HRESULT DacDbiInterfaceImpl::IsWinRTModule(VMPTR_Module vmModule, BOOL * pIsWinRT) | ||
| { | ||
| DD_ENTER_MAY_THROW; | ||
|
|
||
| HRESULT hr = S_OK; | ||
| isWinRT = FALSE; | ||
| *pIsWinRT = FALSE; | ||
|
|
||
| return hr; |
There was a problem hiding this comment.
IsWinRTModule changed from an out reference to an out pointer (pIsWinRT) but still writes to it unconditionally. Add a null check and return E_POINTER when pIsWinRT is null (consistent with other APIs in this layer that validate out pointers).
| HRESULT EnumerateThreads([in] FP_THREAD_ENUMERATION_CALLBACK fpCallback, [in] CALLBACK_DATA pUserData); | ||
| HRESULT IsThreadMarkedDead([in] VMPTR_Thread vmThread, [out] BOOL * pResult); | ||
| HRESULT GetThreadHandle([in] VMPTR_Thread vmThread, [out] HANDLE * pRetVal); | ||
| HRESULT GetThreadObject([in] VMPTR_Thread vmThread, [out] VMPTR_OBJECTHANDLE * pRetVal); | ||
| HRESULT GetThreadAllocInfo([in] VMPTR_Thread vmThread, [out] struct DacThreadAllocInfo * threadAllocInfo); |
There was a problem hiding this comment.
This IDL uses the Win32 HANDLE type in the method signature here, but the file only imports unknwn.idl and does not define/import HANDLE anywhere. This is likely to fail MIDL parsing/compilation; import an IDL that defines HANDLE (commonly wtypes.idl / objidl.idl) or add a local typedef for HANDLE in the dummy-type section.
- Add MIDL_INTERFACE with GUID and IUnknown inheritance - Replace Destroy() with QueryInterface/AddRef/Release - Convert all C++ reference parameters to pointers (5 methods) - Update all DacDbiInterfaceImpl implementations - Update all DBI callers (process.cpp, divalue.cpp, module.cpp, rstype.cpp) - Fix class/struct forward declaration mismatch in rspriv.h - Add dacdbi.idl defining the full COM interface contract - Inner interfaces (IStringHolder, IAllocator, IMetaDataLookup) get own GUIDs Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1c619d9 to
3f45dad
Compare
Add prebuilt dacdbi_i.cpp with MIDL_DEFINE_GUID definitions for IDacDbiInterface, IDacDbiStringHolder, IDacDbiAllocator, and IDacDbiMetaDataLookup. On non-Windows the PAL expands __uuidof(type) to IID_##type, so these symbols must exist in the corguids library. On Windows, add dacdbi.idl to the MIDL compilation step in CMakeLists.txt (handled separately from CORGUIDS_IDL_SOURCES since the IDL lives in debug/inc/ rather than inc/). On non-Windows, add the prebuilt dacdbi_i.cpp to the corguids sources. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add missing HANDLE typedef (UINT_PTR) to the IDL dummy types section, fixing MIDL compilation when HANDLE is used in method signatures. All null-check review comments were already addressed in prior commits: - GetExactTypeHandle (E_POINTER check present) - GetStackFramesFromException (E_POINTER check present) - IsWinRTModule (E_POINTER check present) - Stale Destroy comment (already updated to Release) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
|
||
|
|
||
| /* this ALWAYS GENERATED file contains the IIDs and CLSIDs */ | ||
|
|
||
| /* link this file in with the server and any clients */ | ||
|
|
||
|
|
There was a problem hiding this comment.
The new prebuilt MIDL output file is missing the standard .NET Foundation/MIT license header that other files under src/coreclr/pal/prebuilt/idl/ include (e.g., cordebug_i.cpp). Please add the usual 2-line license header at the top to match repository requirements for checked-in source files.
| /* this ALWAYS GENERATED file contains the IIDs and CLSIDs */ | |
| /* link this file in with the server and any clients */ | |
| // Licensed to the .NET Foundation under one or more agreements. | |
| // The .NET Foundation licenses this file to you under the MIT license. | |
| /* this ALWAYS GENERATED file contains the IIDs and CLSIDs */ | |
| /* link this file in with the server and any clients */ |
| HRESULT DacDbiInterfaceImpl::GetMetaDataFileInfoFromPEFile(VMPTR_PEAssembly vmPEAssembly, DWORD & dwTimeStamp, DWORD & dwImageSize, IStringHolder* pStrFilename, OUT bool * pResult) | ||
| HRESULT DacDbiInterfaceImpl::GetMetaDataFileInfoFromPEFile(VMPTR_PEAssembly vmPEAssembly, DWORD * pTimeStamp, DWORD * pImageSize, IStringHolder* pStrFilename, OUT BOOL * pResult) | ||
| { | ||
| if (pTimeStamp == NULL || pImageSize == NULL || pResult == NULL) |
There was a problem hiding this comment.
GetMetaDataFileInfoFromPEFile now returns E_POINTER for some null pointer arguments, but it still unconditionally dereferences pStrFilename later via AssignCopy. If callers pass a null string holder, this will AV instead of returning E_POINTER. Consider including pStrFilename in the null-pointer validation (or documenting it as required and keeping error behavior consistent).
| if (pTimeStamp == NULL || pImageSize == NULL || pResult == NULL) | |
| if (pTimeStamp == NULL || pImageSize == NULL || pStrFilename == NULL || pResult == NULL) |
On non-Windows, __uuidof(IDacDbiInterface) expands to the symbol IID_IDacDbiInterface (PAL macro in rpc.h). The previous commit added the GUID definition to corguids via dacdbi_i.cpp, but the daccess compilation unit needs a visible extern declaration at compile time. Add EXTERN_C const IID IID_IDacDbiInterface; to dacdbiinterface.h, matching the pattern used in pal/prebuilt/inc/sospriv.h for IID_ISOSDacInterface. Also: - Add .NET Foundation license header to dacdbi_i.cpp - Add pStrFilename null check in GetMetaDataFileInfoFromPEFile Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Converts
IDacDbiInterfacefrom a plain C++ abstract class into a proper COM interface, enabling future implementation in managed C# via the cDAC.Changes
IDacDbiInterfacenow inherits fromIUnknownviaMIDL_INTERFACEwith GUIDDB505C1B-A327-4A46-8C32-AF55A56F8E09Destroy()method, replaced with standard COMRelease()lifecycle in all callersDacDbiInterfaceImplboolto COM-compatibleBOOL(4-byte int)dacdbi.idldefining the full COM interface contract including inner callback interfaces (IStringHolder,IAllocator,IMetaDataLookup). Validated with MIDL compiler.Motivation
This is Phase 1 of enabling the cDAC (contract-based Data Access Component) to implement
IDacDbiInterfacein managed C#, following the same pattern asISOSDacInterface→SOSDacImpl. The prior PR #124836 converted all methods to return HRESULT; this PR completes the COM conversion.Testing
Local build verified (daccess.lib, mscordbi.dll, mscordaccore.dll). IDL validated with MIDL compiler. Running CI for full validation.