Skip to content

Commit

Permalink
Misc dbgshim and SOS fixes (#3072)
Browse files Browse the repository at this point in the history 
* Fix some issues found with netcoredbg

* Fix some sign-extension issues on alpine arm32

* Fix DbgShim.UnitTest's Microsoft.Diagnostics.DbgShimAPI+NativeRuntimeStartupCallbackDelegate being called after collected by the GC

* Code review feedback
  • Loading branch information
@mikem8361
mikem8361 authored May 14, 2022
1 parent 6b2b94c commit 2269f2d
Show file tree
Hide file tree
Showing 6 changed files with 57 additions and 28 deletions.
1 change: 1 addition & 0 deletions src/Microsoft.Diagnostics.DebugServices/MemoryServiceExtensions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ public static Stream CreateMemoryStream(this IMemoryService memoryService, ulong
{
Debug.Assert(address != 0);
Debug.Assert(size != 0);
Debug.Assert((address & ~memoryService.SignExtensionMask()) == 0);
return new TargetStream(memoryService, address, size);
}

Expand Down
4 changes: 4 additions & 0 deletions src/SOS/SOS.Hosting/SymbolServiceWrapper.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,13 +53,15 @@ private delegate void SymbolFileCallback(

private readonly ISymbolService _symbolService;
private readonly IMemoryService _memoryService;
private readonly ulong _ignoreAddressBitsMask;

public SymbolServiceWrapper(ISymbolService symbolService, IMemoryService memoryService)
{
Debug.Assert(symbolService != null);
Debug.Assert(memoryService != null);
_symbolService = symbolService;
_memoryService = memoryService;
_ignoreAddressBitsMask = memoryService.SignExtensionMask();
Debug.Assert(_symbolService != null);

VTableBuilder builder = AddInterface(IID_ISymbolService, validate: false);
Expand Down Expand Up @@ -147,11 +149,13 @@ private IntPtr LoadSymbolsForModule(
ISymbolFile symbolFile = null;
if (loadedPeAddress != 0)
{
loadedPeAddress &= _ignoreAddressBitsMask;
Stream peStream = _memoryService.CreateMemoryStream(loadedPeAddress, loadedPeSize);
symbolFile = _symbolService.OpenSymbolFile(assemblyPath, isFileLayout, peStream);
}
if (inMemoryPdbAddress != 0)
{
inMemoryPdbAddress &= _ignoreAddressBitsMask;
Stream pdbStream = _memoryService.CreateMemoryStream(inMemoryPdbAddress, inMemoryPdbSize);
symbolFile = _symbolService.OpenSymbolFile(pdbStream);
}
Expand Down
21 changes: 15 additions & 6 deletions src/dbgshim/dbgshim.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,11 @@ struct ClrRuntimeInfo
ClrRuntimeInfo()
{
ModuleHandle = NULL;
#ifdef TARGET_UNIX
ContinueStartupEvent = NULL;
#else
ContinueStartupEvent = INVALID_HANDLE_VALUE;
#endif

EngineMetrics.cbSize = sizeof(EngineMetrics);
EngineMetrics.dwDbiVersion = CorDebugLatestVersion;
Expand Down Expand Up @@ -396,8 +400,10 @@ class RuntimeStartupHelper
exit:
if (FAILED(hr))
{
_ASSERTE(pCordb == NULL);

if (pCordb != NULL)
{
pCordb->Release();
}
// Invoke the callback on error
m_callback(NULL, m_parameter, hr);
}
Expand Down Expand Up @@ -502,19 +508,19 @@ class RuntimeStartupHelper
// Don't need to wake up and wait for the worker thread if called on it
if (m_threadId != GetCurrentThreadId())
{
// Wait for work thread to exit
WaitForSingleObject(m_threadHandle, INFINITE);
// Wait for work thread to exit for 60 seconds
WaitForSingleObject(m_threadHandle, 60 * 1000);
}
}

HRESULT InvokeStartupCallback(bool *pCoreClrExists)
{
ClrRuntimeInfo clrRuntimeInfo;
IUnknown *pCordb = NULL;
HRESULT hr = S_OK;

PAL_CPP_TRY
{
IUnknown *pCordb = NULL;

*pCoreClrExists = FALSE;

Expand Down Expand Up @@ -592,7 +598,10 @@ class RuntimeStartupHelper
SetEvent(clrRuntimeInfo.ContinueStartupEvent);
}
}

if (FAILED(hr) && (pCordb != NULL))
{
pCordb->Release();
}
return hr;
}

Expand Down
7 changes: 2 additions & 5 deletions src/shared/pal/src/thread/process.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1441,11 +1441,8 @@ class PAL_RuntimeStartupHelper
// Don't need to wait for the worker thread if unregister called on it
if (m_threadId != (DWORD)THREADSilentGetCurrentThreadId())
{
// Wait for work thread to exit
if (WaitForSingleObject(m_threadHandle, INFINITE) != WAIT_OBJECT_0)
{
ASSERT("WaitForSingleObject\n");
}
// Wait for work thread to exit for 60 seconds
WaitForSingleObject(m_threadHandle, 60 * 1000);
}
}

Expand Down
39 changes: 28 additions & 11 deletions src/tests/DbgShim.UnitTests/DbgShimAPI.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,43 +85,60 @@ public static HResult CreateProcessForLaunch(string commandLine, bool suspendPro

public delegate void RuntimeStartupCallbackDelegate(ICorDebug cordbg, object parameter, HResult hresult);

public static HResult RegisterForRuntimeStartup(int pid, object parameter, out IntPtr unregisterToken, RuntimeStartupCallbackDelegate callback)
public static HResult RegisterForRuntimeStartup(int pid, object parameter, out (IntPtr, GCHandle) unregister, RuntimeStartupCallbackDelegate callback)
{
IntPtr nativeCallback = RuntimeStartupCallback(parameter, callback, out IntPtr nativeParameter);
return _registerForRuntimeStartup((uint)pid, nativeCallback, nativeParameter, out unregisterToken);
IntPtr nativeCallback = RuntimeStartupCallback(parameter, callback, out GCHandle gchNativeCallback, out IntPtr nativeParameter);
HResult hr = _registerForRuntimeStartup((uint)pid, nativeCallback, nativeParameter, out IntPtr unregisterToken);
unregister = (unregisterToken, gchNativeCallback);
return hr;
}

public static HResult RegisterForRuntimeStartupEx(int pid, string applicationGroupId, object parameter, out IntPtr unregisterToken, RuntimeStartupCallbackDelegate callback)
public static HResult RegisterForRuntimeStartupEx(int pid, string applicationGroupId, object parameter, out (IntPtr, GCHandle) unregister, RuntimeStartupCallbackDelegate callback)
{
IntPtr nativeCallback = RuntimeStartupCallback(parameter, callback, out IntPtr nativeParameter);
return _registerForRuntimeStartupEx((uint)pid, applicationGroupId, nativeCallback, nativeParameter, out unregisterToken);
IntPtr nativeCallback = RuntimeStartupCallback(parameter, callback, out GCHandle nativeCallbackHandle, out IntPtr nativeParameter);
HResult hr = _registerForRuntimeStartupEx((uint)pid, applicationGroupId, nativeCallback, nativeParameter, out IntPtr unregisterToken);
unregister = (unregisterToken, nativeCallbackHandle);
return hr;
}

public static HResult RegisterForRuntimeStartup3(int pid, string applicationGroupId, object parameter, IntPtr libraryProvider, out IntPtr unregisterToken, RuntimeStartupCallbackDelegate callback)
public static HResult RegisterForRuntimeStartup3(int pid, string applicationGroupId, object parameter, IntPtr libraryProvider, out (IntPtr, GCHandle) unregister, RuntimeStartupCallbackDelegate callback)
{
if (_registerForRuntimeStartup3 == default)
{
throw new NotSupportedException("RegisterForRuntimeStartup3 not supported");
}
IntPtr nativeCallback = RuntimeStartupCallback(parameter, callback, out IntPtr nativeParameter);
return _registerForRuntimeStartup3((uint)pid, applicationGroupId, libraryProvider, nativeCallback, nativeParameter, out unregisterToken);
IntPtr nativeCallback = RuntimeStartupCallback(parameter, callback, out GCHandle nativeCallbackHandle, out IntPtr nativeParameter);
HResult hr = _registerForRuntimeStartup3((uint)pid, applicationGroupId, libraryProvider, nativeCallback, nativeParameter, out IntPtr unregisterToken);
unregister = (unregisterToken, nativeCallbackHandle);
return hr;
}

private delegate void NativeRuntimeStartupCallbackDelegate(IntPtr cordbg, IntPtr parameter, HResult hresult);

private static IntPtr RuntimeStartupCallback(object parameter, RuntimeStartupCallbackDelegate callback, out IntPtr nativeParameter)
private static IntPtr RuntimeStartupCallback(object parameter, RuntimeStartupCallbackDelegate callback, out GCHandle nativeCallbackHandle, out IntPtr nativeParameter)
{
NativeRuntimeStartupCallbackDelegate native = (IntPtr cordbg, IntPtr param, HResult hresult) => {
GCHandle gch = GCHandle.FromIntPtr(param);
callback(ICorDebug.Create(cordbg), gch.Target, hresult);
gch.Free();
};
// Need to keep native callback delegate alive until UnregisterForRuntimeStartup
nativeCallbackHandle = GCHandle.Alloc(native);

// Need to keep parameter alive until the callback is invoked
GCHandle gchParameter = GCHandle.Alloc(parameter);
nativeParameter = GCHandle.ToIntPtr(gchParameter);

// Return the function pointer for the native callback
return Marshal.GetFunctionPointerForDelegate(native);
}

public static HResult UnregisterForRuntimeStartup(IntPtr unregisterToken) => _unregisterForRuntimeStartup(unregisterToken);
public static HResult UnregisterForRuntimeStartup((IntPtr unregisterToken, GCHandle nativeCallbackHandle) unregister)
{
HResult hr = _unregisterForRuntimeStartup(unregister.unregisterToken);
unregister.nativeCallbackHandle.Free();
return hr;
}

private const int HRESULT_ERROR_PARTIAL_COPY = unchecked((int)0x8007012b);
private const int HRESULT_ERROR_BAD_LENGTH = unchecked((int)0x80070018);
Expand Down
13 changes: 7 additions & 6 deletions src/tests/DbgShim.UnitTests/DbgShimTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
using System.Diagnostics;
using System.IO;
using System.Linq;
using System.Runtime.InteropServices;
using System.Text;
using System.Threading;
using System.Threading.Tasks;
Expand Down Expand Up @@ -347,15 +348,15 @@ private static async Task<DebuggeeInfo> StartDebuggee(string configXml, bool lau
Assert.True(await debuggeeInfo.WaitForDebuggee());
}
Trace.TraceInformation("CreateProcessForLaunch pid {0} DONE", processId);
return debuggeeInfo;
return debuggeeInfo;
}

private static void TestRegisterForRuntimeStartup(DebuggeeInfo debuggeeInfo, int api)
{
TestConfiguration config = debuggeeInfo.TestConfiguration;
AutoResetEvent wait = new AutoResetEvent(false);
(IntPtr, GCHandle) unregister = (IntPtr.Zero, default);
string applicationGroupId = null;
IntPtr unregisterToken = IntPtr.Zero;
HResult result = HResult.S_OK;
HResult callbackResult = HResult.S_OK;
Exception callbackException = null;
Expand Down Expand Up @@ -386,14 +387,14 @@ private static void TestRegisterForRuntimeStartup(DebuggeeInfo debuggeeInfo, int
switch (api)
{
case 1:
result = DbgShimAPI.RegisterForRuntimeStartup(debuggeeInfo.ProcessId, parameter: IntPtr.Zero, out unregisterToken, callback);
result = DbgShimAPI.RegisterForRuntimeStartup(debuggeeInfo.ProcessId, parameter: IntPtr.Zero, out unregister, callback);
break;
case 2:
result = DbgShimAPI.RegisterForRuntimeStartupEx(debuggeeInfo.ProcessId, applicationGroupId, parameter: IntPtr.Zero, out unregisterToken, callback);
result = DbgShimAPI.RegisterForRuntimeStartupEx(debuggeeInfo.ProcessId, applicationGroupId, parameter: IntPtr.Zero, out unregister, callback);
break;
case 3:
LibraryProviderWrapper libraryProvider = new(config.RuntimeModulePath(), config.DbiModulePath(), config.DacModulePath());
result = DbgShimAPI.RegisterForRuntimeStartup3(debuggeeInfo.ProcessId, applicationGroupId, parameter: IntPtr.Zero, libraryProvider.ILibraryProvider, out unregisterToken, callback);
result = DbgShimAPI.RegisterForRuntimeStartup3(debuggeeInfo.ProcessId, applicationGroupId, parameter: IntPtr.Zero, libraryProvider.ILibraryProvider, out unregister, callback);
break;
default:
throw new ArgumentException(nameof(api));
Expand All @@ -410,7 +411,7 @@ private static void TestRegisterForRuntimeStartup(DebuggeeInfo debuggeeInfo, int
Assert.True(wait.WaitOne());
Trace.TraceInformation("RegisterForRuntimeStartup pid {0} after callback wait", debuggeeInfo.ProcessId);

AssertResult(DbgShimAPI.UnregisterForRuntimeStartup(unregisterToken));
AssertResult(DbgShimAPI.UnregisterForRuntimeStartup(unregister));
Assert.Null(callbackException);

switch (api)
Expand Down

0 comments on commit 2269f2d

Please sign in to comment.