SOS: load DBI through dbgshim - #5980
Conversation
5d53e54 to
3e724e8
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates SOS’s DBI activation path so CoreCLR ICorDebugProcess creation is routed through dbgshim (preferring the bundled universal DBI + cDAC first), while keeping the existing direct mscordbi activation path for Desktop CLR. It also updates managed hosting to use the same dbgshim-based activation and packages the universal DBI alongside existing SOS assets.
Changes:
- Native SOS: add dbgshim-based
ICorDebugProcessactivation (CoreCLR) and factor Desktop CLR activation into a dedicated helper. - SOS.Hosting: introduce a managed
ICLRDebuggingLibraryProvider2implementation and route CorDebug activation through a newIClrDataProcessActivator.CreateCorDebugProcessAPI. - Packaging: include
mscordbi_universalin the shared SOS packaging (gated byPackageWithCDac).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/SOS/Strike/platform/runtimeimpl.h | Adds CreateDesktopCorDebugProcess declaration to separate Desktop CLR activation path. |
| src/SOS/Strike/platform/runtimeimpl.cpp | Implements Desktop CLR DBI activation helper and routes CoreCLR DBI activation through dbgshim + library provider. |
| src/SOS/SOS.Hosting/RuntimeWrapper.cs | Updates managed DBI activation to use IClrDataProcessActivator for CoreCLR and improves HRESULT propagation. |
| src/SOS/SOS.Hosting/RuntimeLibraryProvider.cs | Adds managed ICLRDebuggingLibraryProvider2 implementation to resolve DBI/DAC paths for dbgshim. |
| src/SOS/SOS.Hosting/ClrDataProcessActivator.cs | Refactors dbgshim initialization and adds CreateCorDebugProcess to activate ICorDebugProcess via dbgshim. |
| src/sos-packaging.props | Packages mscordbi_universal for supported RIDs when PackageWithCDac is enabled. |
| src/Microsoft.Diagnostics.DebugServices/IClrDataProcessActivator.cs | Extends the activator interface with a new CorDebug activation API. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| ArrayHolder<WCHAR> dacModulePath = new WCHAR[MAX_LONGPATH + 1]; | ||
| if (MultiByteToWideChar(CP_ACP, 0, dacFilePath, -1, dacModulePath, MAX_LONGPATH) <= 0) | ||
| { | ||
| return HRESULT_FROM_WIN32(GetLastError()); | ||
| } |
| modulePath = IntPtr.Zero; | ||
|
|
||
| string path = fileName?.IndexOf("mscordbi", StringComparison.OrdinalIgnoreCase) >= 0 | ||
| ? _getDbiPath() | ||
| : _getDacPath(); | ||
| if (string.IsNullOrEmpty(path)) | ||
| { | ||
| Trace.TraceError($"RuntimeLibraryProvider: could not resolve {fileName}"); | ||
| return HResult.E_NOINTERFACE; | ||
| } | ||
|
|
||
| modulePath = Marshal.StringToCoTaskMemUni(path); | ||
| Trace.TraceInformation($"RuntimeLibraryProvider: resolved {fileName} to {path}"); | ||
| return HResult.S_OK; |
noahfalk
left a comment
noahfalk
left a comment
There was a problem hiding this comment.
LGTM modulo the policy handling
| // This also verifies the DAC signature before the DBI is passed the DAC path or handle. | ||
| CDacLoadPolicy policy = _runtime.RuntimeType == RuntimeType.Desktop | ||
| ? CDacLoadPolicy.UseLegacyDac | ||
| : _services.GetService<ISettingsService>()?.CDacLoadPolicy ?? CDacLoadPolicy.PreferCDac; |
There was a problem hiding this comment.
I don't think SOS should ever be changing the policy, we should insert some logic in the implementation of CreateCorDebugProcess so that it generates the expected result for each policy option on desktop.
cDacOnly -> loading fails
other policies -> load DAC as usual
|
|
||
| DbgShimCDacLoadPolicy loadPolicy = GetRuntimeConfiguration() == IRuntime::WindowsDesktop | ||
| ? DbgShimCDacLoadPolicy::LegacyDacOnly | ||
| : (DbgShimCDacLoadPolicy)GetCDacLoadPolicy(); |
There was a problem hiding this comment.
Same as above. We should handle the policy we are given rather than changing it which implies cDacOnly on desktop is expected to fail.
There was a problem hiding this comment.
yeah - I had the same change locally. Trying to clean up this change.
Depends on #5966.
Route CoreCLR
ICorDebugProcessactivation through dbgshim so the bundled universal DBI and cDAC are tried first, with SOS's existing DAC/DBI resolver retained as the provider fallback. Keep desktop CLR on its existing directmscordbipath because the public dbgshim factory is CoreCLR-SKU-specific.Ship
mscordbi_universalfrom the cDAC transport package beside dbgshim andmscordaccore_universalin the shared SOS packaging used by dotnet-sos, dotnet-dump, and the WinDbg SOS package.Validation:
clrstack -iwith forced cDAC used the bundled DBIclrstack -iused the matching runtime DAC/DBI