[cdac] Make cDAC delegate to the DAC instead of the other way around (d…

…otnet#108772) - `SOSDacImpl` in cDAC implements all the interfaces implemented by `ClrDataAccess` - `CLRDataCreateInstance` returns cDAC if it is enabled - `cdac_reader_get_sos_interface` takes the legacy DAC implementation - Legacy DAC no longer calls into cDAC for its `ISOSDac*` implementations - cDAC delegates to legacy DAC (if it has one) for APIs it has not implemented and asserts that the results are the same for APIs it has implemented Behavioural differences of note: - Since asserts comparing results are in the cDAC instead of DAC now, they will come through as a fail fast with or without a debugger attached - as opposed to before, when they were silently ignored when a debugger was attached. The assert message will print to the debugger output (if attached) or OutputDebugString/syslog. - If the cDAC implementation of an API errors, it does not fall back to calling the legacy DAC. I think this is reasonable given that we plan to put the ability to choose cDAC vs legacy DAC at a higher level. If cDAC implementation fails, it should fail and not try to hide it. - `ClrDataAccess` lifetime is handled by cDAC (when enabled), so actual release is dependent on finalization of the COM object. I ran the SOS tests in diagnostics repo against this change locally both with and without the cDAC enabled.
Faithlife · Oct 14, 2024 · adba54d · adba54d
1 parent 8a7fa48
commit adba54d
Show file tree

Hide file tree

Showing 14 changed files with 1,503 additions and 947 deletions.
diff --git a/src/coreclr/debug/daccess/cdac.cpp b/src/coreclr/debug/daccess/cdac.cpp
@@ -43,7 +43,7 @@ namespace
     }
 }
 
-CDAC CDAC::Create(uint64_t descriptorAddr, ICorDebugDataTarget* target)
+CDAC CDAC::Create(uint64_t descriptorAddr, ICorDebugDataTarget* target, IUnknown* legacyImpl)
 {
     HMODULE cdacLib;
     if (!TryLoadCDACLibrary(&cdacLib))
@@ -59,20 +59,18 @@ CDAC CDAC::Create(uint64_t descriptorAddr, ICorDebugDataTarget* target)
         return {};
     }
 
-    return CDAC{cdacLib, handle, target};
+    return CDAC{cdacLib, handle, target, legacyImpl};
 }
 
-CDAC::CDAC(HMODULE module, intptr_t handle, ICorDebugDataTarget* target)
+CDAC::CDAC(HMODULE module, intptr_t handle, ICorDebugDataTarget* target, IUnknown* legacyImpl)
     : m_module{module}
     , m_cdac_handle{handle}
     , m_target{target}
+    , m_legacyImpl{legacyImpl}
 {
     _ASSERTE(m_module != NULL && m_cdac_handle != 0 && m_target != NULL);
 
     m_target->AddRef();
-    decltype(&cdac_reader_get_sos_interface) getSosInterface = reinterpret_cast<decltype(&cdac_reader_get_sos_interface)>(::GetProcAddress(m_module, "cdac_reader_get_sos_interface"));
-    _ASSERTE(getSosInterface != nullptr);
-    getSosInterface(m_cdac_handle, &m_sos);
 }
 
 CDAC::~CDAC()
@@ -88,7 +86,10 @@ CDAC::~CDAC()
         ::FreeLibrary(m_module);
 }
 
-IUnknown* CDAC::SosInterface()
+void CDAC::CreateSosInterface(IUnknown** sos)
 {
-    return m_sos;
+    decltype(&cdac_reader_create_sos_interface) createSosInterface = reinterpret_cast<decltype(&cdac_reader_create_sos_interface)>(::GetProcAddress(m_module, "cdac_reader_create_sos_interface"));
+    _ASSERTE(createSosInterface != nullptr);
+    int ret = createSosInterface(m_cdac_handle, m_legacyImpl, sos);
+    _ASSERTE(ret == 0);
 }
diff --git a/src/coreclr/debug/daccess/cdac.h b/src/coreclr/debug/daccess/cdac.h
@@ -7,7 +7,7 @@
 class CDAC final
 {
 public: // static
-    static CDAC Create(uint64_t descriptorAddr, ICorDebugDataTarget *pDataTarget);
+    static CDAC Create(uint64_t descriptorAddr, ICorDebugDataTarget *pDataTarget, IUnknown* legacyImpl);
 
 public:
     CDAC() = default;
@@ -19,25 +19,25 @@ class CDAC final
         : m_module{ other.m_module }
         , m_cdac_handle{ other.m_cdac_handle }
         , m_target{ other.m_target.Extract() }
-        , m_sos{ other.m_sos.Extract() }
+        , m_legacyImpl{ other.m_legacyImpl }
     {
         other.m_module = NULL;
         other.m_cdac_handle = 0;
         other.m_target = NULL;
-        other.m_sos = NULL;
+        other.m_legacyImpl = NULL;
     }
 
     CDAC& operator=(CDAC&& other)
     {
         m_module = other.m_module;
         m_cdac_handle = other.m_cdac_handle;
         m_target = other.m_target.Extract();
-        m_sos = other.m_sos.Extract();
+        m_legacyImpl = other.m_legacyImpl;
 
         other.m_module = NULL;
         other.m_cdac_handle = 0;
         other.m_target = NULL;
-        other.m_sos = NULL;
+        other.m_legacyImpl = NULL;
 
         return *this;
     }
@@ -49,17 +49,18 @@ class CDAC final
         return m_module != NULL && m_cdac_handle != 0;
     }
 
-    // This does not AddRef the returned interface
-    IUnknown* SosInterface();
+    void CreateSosInterface(IUnknown** sos);
 
 private:
-    CDAC(HMODULE module, intptr_t handle, ICorDebugDataTarget* target);
+    CDAC(HMODULE module, intptr_t handle, ICorDebugDataTarget* target, IUnknown* legacyImpl);
 
 private:
     HMODULE m_module;
     intptr_t m_cdac_handle;
     NonVMComHolder<ICorDebugDataTarget> m_target;
-    NonVMComHolder<IUnknown> m_sos;
+
+    // Assumes the legacy impl lives for the lifetime of this class - currently ClrDataAccess, which contains this class
+    IUnknown* m_legacyImpl;
 };
 
 #endif // CDAC_H
diff --git a/src/coreclr/debug/daccess/daccess.cpp b/src/coreclr/debug/daccess/daccess.cpp
@@ -5501,31 +5501,6 @@ ClrDataAccess::Initialize(void)
     IfFailRet(GetDacGlobalValues());
     IfFailRet(DacGetHostVtPtrs());
 
-// TODO: [cdac] TryGetSymbol is only implemented for Linux, OSX, and Windows.
-#ifdef CAN_USE_CDAC
-    CLRConfigNoCache enable = CLRConfigNoCache::Get("ENABLE_CDAC");
-    if (enable.IsSet())
-    {
-        DWORD val;
-        if (enable.TryAsInteger(10, val) && val == 1)
-        {
-            uint64_t contractDescriptorAddr = 0;
-            if (TryGetSymbol(m_pTarget, m_globalBase, "DotNetRuntimeContractDescriptor", &contractDescriptorAddr))
-            {
-                m_cdac = CDAC::Create(contractDescriptorAddr, m_pTarget);
-                if (m_cdac.IsValid())
-                {
-                    // Get SOS interfaces from the cDAC if available.
-                    IUnknown* unk = m_cdac.SosInterface();
-                    (void)unk->QueryInterface(__uuidof(ISOSDacInterface), (void**)&m_cdacSos);
-                    (void)unk->QueryInterface(__uuidof(ISOSDacInterface2), (void**)&m_cdacSos2);
-                    (void)unk->QueryInterface(__uuidof(ISOSDacInterface9), (void**)&m_cdacSos9);
-                }
-            }
-        }
-    }
-#endif
-
     //
     // DAC is now setup and ready to use
     //
@@ -7146,9 +7121,53 @@ CLRDataCreateInstance(REFIID iid,
 #ifdef LOGGING
     InitializeLogging();
 #endif
-    hr = pClrDataAccess->QueryInterface(iid, iface);
 
-    pClrDataAccess->Release();
+    // TODO: [cdac] Remove when cDAC deploys with SOS - https://github.com/dotnet/runtime/issues/108720
+    NonVMComHolder<IUnknown> cdacInterface = nullptr;
+#ifdef CAN_USE_CDAC
+    CLRConfigNoCache enable = CLRConfigNoCache::Get("ENABLE_CDAC");
+    if (enable.IsSet())
+    {
+        DWORD val;
+        if (enable.TryAsInteger(10, val) && val == 1)
+        {
+            // TODO: [cdac] TryGetSymbol is only implemented for Linux, OSX, and Windows.
+            uint64_t contractDescriptorAddr = 0;
+            if (TryGetSymbol(pClrDataAccess->m_pTarget, pClrDataAccess->m_globalBase, "DotNetRuntimeContractDescriptor", &contractDescriptorAddr))
+            {
+                IUnknown* thisImpl;
+                HRESULT qiRes = pClrDataAccess->QueryInterface(IID_IUnknown, (void**)&thisImpl);
+                _ASSERTE(SUCCEEDED(qiRes));
+                CDAC& cdac = pClrDataAccess->m_cdac;
+                cdac = CDAC::Create(contractDescriptorAddr, pClrDataAccess->m_pTarget, thisImpl);
+                if (cdac.IsValid())
+                {
+                    // Get SOS interfaces from the cDAC if available.
+                    cdac.CreateSosInterface(&cdacInterface);
+                    _ASSERTE(cdacInterface != nullptr);
+
+                    // Lifetime is now managed by cDAC implementation of SOS interfaces
+                    pClrDataAccess->Release();
+                }
+
+                // Release the AddRef from the QI.
+                pClrDataAccess->Release();
+            }
+        }
+    }
+#endif
+    if (cdacInterface != nullptr)
+    {
+        hr = cdacInterface->QueryInterface(iid, iface);
+    }
+    else
+    {
+        hr = pClrDataAccess->QueryInterface(iid, iface);
+
+        // Lifetime is now managed by caller
+        pClrDataAccess->Release();
+    }
+
     return hr;
 }
 

diff --git a/src/coreclr/debug/daccess/dacimpl.h b/src/coreclr/debug/daccess/dacimpl.h
@@ -1231,22 +1231,6 @@ class ClrDataAccess
 
     HRESULT Initialize(void);
 
-    HRESULT GetThreadDataImpl(CLRDATA_ADDRESS threadAddr, struct DacpThreadData *threadData);
-    HRESULT GetThreadStoreDataImpl(struct DacpThreadStoreData *data);
-    HRESULT GetModuleDataImpl(CLRDATA_ADDRESS addr, struct DacpModuleData *moduleData);
-    HRESULT GetNestedExceptionDataImpl(CLRDATA_ADDRESS exception, CLRDATA_ADDRESS *exceptionObject, CLRDATA_ADDRESS *nextNestedException);
-    HRESULT GetMethodTableDataImpl(CLRDATA_ADDRESS mt, struct DacpMethodTableData *data);
-    HRESULT GetMethodTableForEEClassImpl (CLRDATA_ADDRESS eeClassReallyMT, CLRDATA_ADDRESS *value);
-    HRESULT GetMethodTableNameImpl(CLRDATA_ADDRESS mt, unsigned int count, _Inout_updates_z_(count) WCHAR *mtName, unsigned int *pNeeded);
-    HRESULT GetObjectDataImpl(CLRDATA_ADDRESS addr, struct DacpObjectData *objectData);
-    HRESULT GetObjectExceptionDataImpl(CLRDATA_ADDRESS objAddr, struct DacpExceptionObjectData *data);
-    HRESULT GetObjectStringDataImpl(CLRDATA_ADDRESS obj, unsigned int count, _Inout_updates_z_(count) WCHAR *stringData, unsigned int *pNeeded);
-    HRESULT GetPEFileBaseImpl(CLRDATA_ADDRESS moduleAddr, CLRDATA_ADDRESS *base);
-    HRESULT GetPEFileNameImpl(CLRDATA_ADDRESS moduleAddr, unsigned int count, _Inout_updates_z_(count) WCHAR *fileName, unsigned int *pNeeded);
-    HRESULT GetUsefulGlobalsImpl(struct DacpUsefulGlobalsData *globalsData);
-    HRESULT GetMethodDescDataImpl(CLRDATA_ADDRESS methodDesc, CLRDATA_ADDRESS ip, struct DacpMethodDescData *data, ULONG cRevertedRejitVersions, DacpReJitData * rgRevertedRejitData, ULONG * pcNeededRevertedRejitData);
-    HRESULT GetMethodDescNameImpl(CLRDATA_ADDRESS methodDesc, unsigned int count, _Inout_updates_z_(count) WCHAR *name, unsigned int *pNeeded);
-
     BOOL IsExceptionFromManagedCode(EXCEPTION_RECORD * pExceptionRecord);
 #ifndef TARGET_UNIX
     HRESULT GetWatsonBuckets(DWORD dwThreadId, GenericModeBlock * pGM);
@@ -1433,10 +1417,9 @@ class ClrDataAccess
     ULONG32 m_instanceAge;
     bool m_debugMode;
 
+    // This currently exists on the DAC as a way of managing lifetime of loading/freeing the cdacreader
+    // TODO: [cdac] Remove when cDAC deploys with SOS - https://github.com/dotnet/runtime/issues/108720
     CDAC m_cdac;
-    NonVMComHolder<ISOSDacInterface> m_cdacSos;
-    NonVMComHolder<ISOSDacInterface2> m_cdacSos2;
-    NonVMComHolder<ISOSDacInterface9> m_cdacSos9;
 
 #ifdef FEATURE_MINIMETADATA_IN_TRIAGEDUMPS