Draft
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…AC contracts Port two IDacDbiInterface methods from legacy forwarding to cDAC contract-backed behavior in DacDbiImpl: - AreGCStructuresValid: uses IGC.GetGCStructuresValid() contract - IsThreadSuspendedOrHijacked: uses IThread.GetThreadData() and checks TS_SyncSuspended (0x80000) and TS_Hijacked (0x80) thread state flags Both methods include DEBUG validation that compares cDAC results against legacy DAC implementation, following the SOSDacImpl pattern. Also stores Target reference in DacDbiImpl (was previously discarded) and adds necessary using directives for Contracts namespace. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… IsAssemblyFullyTrusted via cDAC Port four more IDacDbiInterface methods from legacy forwarding to cDAC contract-backed behavior in DacDbiImpl: - IsThreadMarkedDead: checks TS_Dead (0x800) flag via IThread contract - TryGetVolatileOSThreadID: returns OSId from IThread contract, handles SWITCHED_OUT_FIBER_OSID (0xbaadf00d) magic value - GetUniqueThreadID: returns OS thread ID via IThread contract - IsAssemblyFullyTrusted: always returns TRUE (matching native behavior; full trust is the only mode in modern .NET) All thread methods include DEBUG validation against legacy implementation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Port methods that have trivial or fixed-return implementations: - GetConnectionID: always returns INVALID_CONNECTION_ID (0) - GetTaskID: always returns INVALID_TASK_ID (0) - EnableNGENPolicy: returns E_NOTIMPL (matching native) - GetNGENCompilerFlags: returns CORDBG_E_NGEN_NOT_SUPPORTED - SetNGENCompilerFlags: returns CORDBG_E_NGEN_NOT_SUPPORTED - IsWinRTModule: always returns FALSE (WinRT not supported) - GetObject: trivial address wrapping (VMPTR passthrough) - GetObjectFromRefPtr: dereferences ObjectRef pointer via Target.ReadPointer Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Port additional trivial methods: - GetVmObjectHandle: VMPTR address wrapping (passthrough) - GetHandleAddressFromVmHandle: VMPTR address unwrapping (passthrough) - GetAppDomainIdFromVmObjectHandle: always returns DefaultADID (1) - GetAppDomainId: returns 0 for null, DefaultADID (1) otherwise Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Implement GetCurrentAppDomain by reading the AppDomain global pointer from the target process, matching the native AppDomain::GetCurrentDomain() behavior. Includes DEBUG validation against legacy. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Port additional methods with platform-specific behavior: - IsTransitionStub: E_NOTIMPL on Unix (mixed-mode debugging not supported) - IsRcw: always FALSE on Linux (FEATURE_COMINTEROP not defined) - RequiresAlign8: E_NOTIMPL on x64 (FEATURE_64BIT_ALIGNMENT only ARM/WASM) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Methods ported in this batch: Deprecated ReJit methods (always return S_OK, matching native): - GetReJitInfo (both overloads) - GetSharedReJitInfo - GetSharedReJitInfoData Loader contract-backed: - GetAssemblyFromDomainAssembly: uses ILoader.GetAssembly(ModuleHandle) Object/type contract-backed: - GetTypeID: uses IObject.GetMethodTableAddress for COR_TYPEID - GetTypeIDForType: uses IRuntimeTypeSystem for canonical MethodTable All contract-backed methods include #if DEBUG validation against legacy. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- GetThreadAllocInfo: uses IThread.GetThreadAllocContext + ThreadData for SOH/UOH byte calculations matching native alloc_bytes - unused adjustment - GetTypeID: uses IObject.GetMethodTableAddress for COR_TYPEID - GetTypeIDForType: uses IRuntimeTypeSystem.GetCanonicalMethodTable Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
In .NET Core's single-AppDomain model, GetAppDomainFromId returns the global AppDomain pointer via ReadGlobalPointer(Constants.Globals.AppDomain). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…DefinesBitField, GetMDStructuresVersion Add Debugger type to cDAC data descriptor: - cdac_data<Debugger> with LeftSideInitialized, Defines, MDStructuresVersion - CDAC_GLOBAL_POINTER(Debugger, &::g_pDebugger) global - Data.Debugger managed data type, DataType.Debugger enum entry Implement 3 methods using the new Debugger data: - IsLeftSideInitialized: reads Debugger.LeftSideInitialized - GetDefinesBitField: reads Debugger.Defines - GetMDStructuresVersion: reads Debugger.MDStructuresVersion All include #if DEBUG validation against legacy. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- GetThreadObject: reads ExposedObject (GCHandle field) from Thread data descriptor with state validation (TS_Dead, TS_Unstarted, TS_Detached) - GetAppDomainObject: always returns NULL (matching native AppDomain::GetRawExposedObjectHandleForDebugger() which returns NULL) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Walk thread list using ThreadStoreData.FirstThread → ThreadData.NextThread chain, filtering dead (TS_Dead) and unstarted (TS_Unstarted) threads. Invoke native callback via unmanaged function pointer. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Single AppDomain in .NET Core - read global AppDomain pointer and invoke callback once. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add DomainAssembly type to datadescriptor.inc with Assembly field - Add cdac_data<DomainAssembly> in domainassembly.h - Add Module.DomainAssembly field to data descriptor and Data.Module - Create managed Data.DomainAssembly type - Port GetDomainAssemblyData, GetModuleForDomainAssembly, GetDomainAssemblyFromModule, EnumerateAssembliesInAppDomain, EnumerateModulesInAssembly to cDAC Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add StringHolderAssignCopy helper for calling native IStringHolder vtable from managed code. Port: - GetAppDomainFullName via ILoader.GetAppDomainFriendlyName - GetModulePath via ILoader.GetPath - GetAssemblyPath via ILoader.GetModuleHandleFromAssemblyPtr + GetPath Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fill ModuleInfo struct using ILoader methods for assembly, PEAssembly, base address, size, dynamic flag, and in-memory flag. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- IsModuleMapped checks FLAG_MAPPED from TryGetLoadedImageContents - GetSymbolsBuffer uses ILoader.TryGetSymbolStream Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add m_StateNC field to cdac_data<Thread> and datadescriptor.inc - Add StateNC property to managed Data.Thread - Port GetPartialUserState mapping Thread.State and StateNC to CorDebugUserState flags (Background, Unstarted, Stopped, WaitSleepJoin, ThreadPool) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Read ExceptionTracker pointer then ThrownObjectHandle from ExceptionInfo. Returns null when no active tracker (unhandled fallback not yet implemented). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add Module.SimpleName field to data descriptor - GetModuleSimpleName reads UTF-8 simple name and calls IStringHolder - GetCurrentException reads ExceptionTracker→ThrownObjectHandle Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Port GetThreadHandle and GetCurrentCustomDebuggerNotification to cDAC. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add global pointers for CLRJitAttachState and g_metadataUpdatesApplied. Read non-pointer globals by dereferencing the stored address with Target.Read<T> instead of ReadGlobalPointer (which returns the address). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Make IStringHolder inherit IUnknown with IID 1D83B63E-...-E53BFB3CF9A3. Add QI/AddRef/Release to StringCopyHolder (stack-allocated, ref count is no-op). Create managed IStringHolder COM projection. Replace manual vtable dispatch in DacDbiImpl with natural COM interop calls. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Create IDebugger contract with IsLeftSideInitialized, GetDefinesBitField, GetMDStructuresVersion, GetAttachStateFlags, and MetadataUpdatesApplied methods. Implement in Debugger_1.cs with factory and registry wiring. Add CDAC_GLOBAL_CONTRACT(Debugger, 1) to datadescriptor.inc. Extend IThread with GetExposedObject, GetThreadHandle, GetCurrentExceptionHandle, GetCurrentCustomDebuggerNotification, and GetPartialUserState. Implement in Thread_1.cs. Extend ILoader with GetModuleSimpleName, GetDomainAssemblyForModule, and GetAppDomain. Implement in Loader_1.cs. Refactor all DacDbiImpl methods that previously read Data.* types directly to instead call the appropriate contract APIs. DacDbiImpl no longer imports the Data namespace. Add Debugger.md data contract documentation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
GetGCHeapInformation uses IGC contract (GetGCStructuresValid, GetGCHeapCount, GetGCIdentifiers) to fill COR_HEAPINFO struct. IsValueType uses IRuntimeTypeSystem.GetSignatureCorElementType to check for CorElementType.ValueType. Score: 58/160 methods ported. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Make IStringHolder public to match public DacDbiImpl/IDacDbiInterface. Add StateNC, ThreadHandle, CurrNotification, GCHandle fields to mock thread type for test compatibility. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…nsions - Thread.md: Add docs for GetExposedObject, GetThreadHandle, GetCurrentExceptionHandle, GetCurrentCustomDebuggerNotification, GetPartialUserState. Add StateNC, ThreadHandle, CurrNotification, GCHandle data descriptors. - Loader.md: Add docs for GetModuleSimpleName, GetDomainAssemblyForModule, GetAppDomain. Add SimpleName, DomainAssembly data descriptors. - ThreadTests.cs: 7 new tests covering GetPartialUserState (3 flag combinations), GetThreadHandle, GetExposedObject, GetCurrentCustomDebuggerNotification, GetCurrentExceptionHandle (with/without exception). - LoaderTests.cs: 3 new tests for GetModuleSimpleName, GetDomainAssemblyForModule, GetAppDomain. - MockDescriptors: Add SimpleName/DomainAssembly to ModuleFields, add helpers SetThreadState, SetThreadHandle, SetGCHandle, SetCurrNotification, SetExceptionThrownObjectHandle, SetAppDomainGlobal, AddModule with simpleName/domainAssembly params. - TestPlaceholderTarget: Implement ReadUtf8String. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds initial infrastructure to expose a COM-like IDacDbiInterface from cDAC (mirroring the existing SOS path), plus new/extended cDAC contracts and tests to support a first batch of DAC-DBI APIs.
Changes:
- Introduces managed COM projections + implementation scaffold for
IDacDbiInterface, and wires up a newcdac_reader_create_dacdbi_interfaceentrypoint. - Adds a new
Debuggercontract and extendsThread/Loadercontracts + coreclr data descriptors/globals needed by the new implementation. - Expands the cDAC test harness and adds unit tests for the new contract APIs.
Reviewed changes
Copilot reviewed 40 out of 40 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/native/managed/cdac/tests/ThreadTests.cs | Adds tests for new IThread APIs (partial user state, handles, notifications, exception handle). |
| src/native/managed/cdac/tests/TestPlaceholderTarget.cs | Implements UTF-8 string reading for loader/module tests. |
| src/native/managed/cdac/tests/MockDescriptors/MockDescriptors.cs | Extends mock type layouts for Thread and Module. |
| src/native/managed/cdac/tests/MockDescriptors/MockDescriptors.Thread.cs | Adds mock mutators for newly exposed Thread fields. |
| src/native/managed/cdac/tests/MockDescriptors/MockDescriptors.Loader.cs | Adds module simple name/domain assembly plumbing and AppDomain global support in mocks. |
| src/native/managed/cdac/tests/LoaderTests.cs | Adds tests for module simple name, domain assembly, and AppDomain global. |
| src/native/managed/cdac/mscordaccore_universal/Entrypoints.cs | Adds unmanaged entrypoint to create IDacDbiInterface via cDAC. |
| src/native/managed/cdac/inc/cdac_reader.h | Declares cdac_reader_create_dacdbi_interface. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader/CachingContractRegistry.cs | Registers the new Debugger contract factory. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/IStringHolder.cs | Adds COM projection for IDacDbiInterface::IStringHolder. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/IDacDbiInterface.cs | Adds COM name-surface for IDacDbiInterface method order validation. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/DacDbiImpl.cs | Implements a subset of IDacDbiInterface using cDAC contracts (fallbacks to legacy where needed). |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/Thread.cs | Adds StateNC, ThreadHandle, CurrNotification to Thread processed-data. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/Module.cs | Adds DomainAssembly and SimpleName to Module processed-data. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/DomainAssembly.cs | Adds processed-data type for DomainAssembly. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/Debugger.cs | Adds processed-data type for Debugger. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Thread_1.cs | Implements new IThread APIs needed by DAC-DBI (exposed object, handles, partial user state, etc.). |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Loader_1.cs | Implements module simple name, domain assembly lookup, and AppDomain global access. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Debugger_1.cs | Implements IDebugger contract v1. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/DebuggerFactory.cs | Adds contract factory for IDebugger. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Constants.cs | Adds globals used by Debugger contract. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/DataType.cs | Adds DomainAssembly and Debugger data types. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IThread.cs | Extends IThread contract interface. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ILoader.cs | Extends ILoader contract interface. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IDebugger.cs | Introduces IDebugger contract abstraction. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/ContractRegistry.cs | Exposes Debugger via the contract registry. |
| src/coreclr/vm/threads.h | Adds cDAC field offsets for new Thread fields. |
| src/coreclr/vm/domainassembly.h | Adds DomainAssembly cDAC descriptor. |
| src/coreclr/vm/datadescriptor/datadescriptor.inc | Adds type fields, new types, new globals, and registers Debugger global contract. |
| src/coreclr/vm/ceeload.h | Adds cDAC field offsets for Module.DomainAssembly and Module.SimpleName. |
| src/coreclr/debug/inc/stringcopyholder.h | Makes StringCopyHolder COM-like for IStringHolder : IUnknown. |
| src/coreclr/debug/inc/dacdbiinterface.h | Adds GUID-based COM declarations for IDacDbiInterface and IStringHolder. |
| src/coreclr/debug/ee/debugger.h | Exposes debugger fields to cDAC via cdac_data<Debugger>. |
| src/coreclr/debug/daccess/dacdbiimpl.h | Forwards IUnknown methods explicitly to preserve COM identity. |
| src/coreclr/debug/daccess/dacdbiimpl.cpp | Adds IID definition + optional cDAC creation path and explicit IUnknown forwarding implementation. |
| src/coreclr/debug/daccess/cdac.h | Adds CreateDacDbiInterface and updates legacyImpl plumbing to void*. |
| src/coreclr/debug/daccess/cdac.cpp | Implements CreateDacDbiInterface and updates SOS creation to cast legacyImpl to IUnknown. |
| docs/design/datacontracts/Thread.md | Documents new Thread contract APIs/fields. |
| docs/design/datacontracts/Loader.md | Documents new Loader contract APIs/fields and AppDomain global. |
| docs/design/datacontracts/Debugger.md | Adds design doc for new Debugger contract. |
Comment on lines
+31
to
+44
| public int IsLeftSideInitialized(int* pResult) | ||
| { | ||
| *pResult = _target.Contracts.Debugger.IsLeftSideInitialized() ? 1 : 0; | ||
| #if DEBUG | ||
| if (_legacy is not null) | ||
| { | ||
| int legacyResult; | ||
| int legacyHr = _legacy.IsLeftSideInitialized(&legacyResult); | ||
| Debug.Assert(legacyHr == HResults.S_OK); | ||
| Debug.Assert(legacyResult == *pResult, $"IsLeftSideInitialized mismatch: cDAC={*pResult} legacy={legacyResult}"); | ||
| } | ||
| #endif | ||
| return HResults.S_OK; | ||
| } |
There was a problem hiding this comment.
DacDbiImpl.IsLeftSideInitialized dereferences pResult and calls into contracts without any null check or exception-to-HRESULT handling. Other COM-facing implementations in this area (e.g., SOSDacImpl) wrap bodies in try/catch and return ex.HResult to avoid crashing the debugger process on contract read failures or null out-params. Please add a pResult null check (E_POINTER/E_INVALIDARG) and a try/catch that converts exceptions to HRESULT.
Comment on lines
+46
to
+61
| public int GetAppDomainFromId(uint appdomainId, ulong* pRetVal) | ||
| { | ||
| // In .NET Core (single AppDomain), the only valid appdomainId is DefaultADID=1. | ||
| // Return the global AppDomain pointer. | ||
| TargetPointer appDomain = new TargetPointer(_target.Contracts.Loader.GetAppDomain().Value); | ||
| *pRetVal = appDomain.Value; | ||
| #if DEBUG | ||
| if (_legacy is not null) | ||
| { | ||
| ulong legacyResult; | ||
| int legacyHr = _legacy.GetAppDomainFromId(appdomainId, &legacyResult); | ||
| Debug.Assert(legacyHr == HResults.S_OK); | ||
| Debug.Assert(legacyResult == *pRetVal, $"GetAppDomainFromId mismatch: cDAC={*pRetVal:x} legacy={legacyResult:x}"); | ||
| } | ||
| #endif | ||
| return HResults.S_OK; |
There was a problem hiding this comment.
GetAppDomainFromId currently ignores appdomainId and always returns the global AppDomain pointer with S_OK. The native DAC implementation fails when the ID is invalid (and the header comment explicitly says it fails if the ID is invalid). Consider validating the ID (e.g., return E_INVALIDARG / CORDBG_E_BAD_APPDOMAIN for anything other than DefaultADID) and handling errors via HRESULT rather than always succeeding.
Comment on lines
+343
to
+345
| IThread threadContract = _target.Contracts.Thread; | ||
| TargetPointer handle = threadContract.GetThreadHandle(new TargetPointer(vmThread)); | ||
| *(ulong*)pRetVal = handle.Value; |
There was a problem hiding this comment.
GetThreadHandle writes the handle to (ulong)pRetVal. This is not pointer-size safe on 32-bit hosts (HANDLE is pointer-sized), and it will also AV if pRetVal is 0. Prefer writing via a pointer-sized type (e.g., nint/IntPtr) and validate pRetVal before dereferencing.
Suggested change
| IThread threadContract = _target.Contracts.Thread; | |
| TargetPointer handle = threadContract.GetThreadHandle(new TargetPointer(vmThread)); | |
| *(ulong*)pRetVal = handle.Value; | |
| if (pRetVal == 0) | |
| { | |
| return HResults.E_INVALIDARG; | |
| } | |
| IThread threadContract = _target.Contracts.Thread; | |
| TargetPointer handle = threadContract.GetThreadHandle(new TargetPointer(vmThread)); | |
| *(nint*)pRetVal = (nint)handle.Value; |
Comment on lines
+17
to
+23
| TargetPointer debuggerPtr = _target.ReadGlobalPointer(Constants.Globals.Debugger); | ||
| if (debuggerPtr == TargetPointer.Null) | ||
| return false; | ||
|
|
||
| Data.Debugger debugger = _target.ProcessedData.GetOrAdd<Data.Debugger>(debuggerPtr); | ||
| return debugger.LeftSideInitialized != 0; | ||
| } |
There was a problem hiding this comment.
Debugger_1 treats ReadGlobalPointer(Constants.Globals.Debugger) as if it were the Debugger instance address. CDAC_GLOBAL_POINTER(Debugger, &::g_pDebugger) publishes the address of the global pointer variable, so the contract should dereference once (ReadPointer(ReadGlobalPointer(...))) to get the Debugger instance pointer, matching patterns used elsewhere (e.g., RCWCleanupList, AppDomain). As written, ProcessedData will read Debugger fields from the global variable’s storage, yielding incorrect results.
Comment on lines
+90
to
+98
| private static unsafe int CreateDacDbiInterface(IntPtr handle, IntPtr legacyImplPtr, nint* obj) | ||
| { | ||
| if (obj == null) | ||
| return HResults.E_INVALIDARG; | ||
| if (handle == IntPtr.Zero || legacyImplPtr == IntPtr.Zero) | ||
| { | ||
| *obj = IntPtr.Zero; | ||
| return HResults.E_NOTIMPL; | ||
| } |
There was a problem hiding this comment.
CreateDacDbiInterface treats legacyImplPtr as mandatory and returns E_NOTIMPL when it is IntPtr.Zero, but cdac_reader.h documents legacyImpl as optional. This prevents creating a cDAC-only IDacDbiInterface (no legacy fallback). Consider allowing legacyImplPtr==0 and still creating the managed implementation (with _legacy == null), returning S_OK when possible.
Comment on lines
+24
to
+31
| HRESULT STDMETHODCALLTYPE QueryInterface(REFIID riid, void **ppvObject) override | ||
| { | ||
| if (ppvObject == NULL) return E_POINTER; | ||
| *ppvObject = NULL; | ||
| return E_NOINTERFACE; | ||
| } | ||
| ULONG STDMETHODCALLTYPE AddRef() override { return 1; } | ||
| ULONG STDMETHODCALLTYPE Release() override { return 1; } |
There was a problem hiding this comment.
StringCopyHolder::QueryInterface currently always returns E_NOINTERFACE. Since IStringHolder now derives from IUnknown, QueryInterface should at least succeed for IID_IUnknown and IID_IDacDbiInterface::IStringHolder (and set *ppvObject accordingly). Returning E_NOINTERFACE for those can break COM clients that QI the holder (even implicitly).
This was referenced Mar 5, 2026
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This shows an example of porting about 35% of the IDacDbiInterface APIs. I've done no review on the new contracts so I don't recommend anyone spend time yet scrutinizing the exact API shape or specific fields that are being exposed.
I think the more interesting angle to look at now is the structure of how cDAC creates an IDacDbiInterface and how things are layered together. The intent is that its very similar to what we are already doing with DacSosInterface so hopefully it feels boring. The basic approach is to make IDacDbiInterface sufficiently COM-like that we can use the same ComWrappers interop.
I'd suggest we don't check this PR in. Instead we can decompose it a bit to first PR the IDacDbiInterface infrastructure, then add APIs in groups with separate PRs as we have been doing elsewhere.