microsoft / clrmd

Microsoft.Diagnostics.Runtime is a set of APIs for introspecting processes and dumps.
MIT License
1.05k stars 255 forks source link

ClrMD should stop using HResult as a PInvoke return #1015

Closed leculver closed 2 years ago

leculver commented 2 years ago

From https://github.com/dotnet/runtime/issues/70099. .Net Core apparently runs on x86 for Linux which means that the HResult struct we use to make life easier in handling HRESULT from the dac's COM interfaces breaks when used as a return value in place of int.

We need to change all usages of HResult to not be directly in a PInvoke/reverse PInvoke signature. E.G. vtable->QueryInterface = (delegate* unmanaged[Stdcall]<IntPtr, in Guid, out IntPtr, HResult>)Marshal.GetFunctionPointerForDelegate(qi); needs to be int> instead of HResult>.

This will be a breaking change, so we'll need to version bump to 2.2. It shouldn't effect most people though, since very few people use the *.DacInterface namespace.

@t-mustafin I've ported the issue you reported here. Note that even if ClrMD changes this behavior we still need to check the CLR diagnostics codebase for the same pattern. I think @mikem8361 uses HResult in a few places in SOS...but I'm not sure whether he directly uses it as a return value for a pinvoke.

mikem8361 commented 2 years ago

Yes, SOS uses HResult in pinvoke/reverse pinvoke sigatures of its own. I created https://github.com/dotnet/diagnostics/issues/3101 to track the SOS work.

leculver commented 2 years ago

@t-mustafin I have completed the work in ClrMD to stop using HResult as a return from PInvoke. We still use it inside the library itself, but that shouldn't cause problems.

Can you please sync the repository this week and test this out by using ClrMD on x86 Linux? Unfortunately SOS won't work until CLR Diagnostics has a chance to update SOS, but it should be straight forward to run something like this and see if it blows up:

using DataTarget dt = DataTarget.LoadDump("/path/to/coredump");
using ClrRuntime runtime = dt.ClrVersions.Single().CreateRuntime();
runtime.Heap.EnumerateObjects.ToArray();
t-mustafin commented 2 years ago

@t-mustafin I have completed the work in ClrMD to stop using HResult as a return from PInvoke. We still use it inside the library itself, but that shouldn't cause problems.

Can you please sync the repository this week and test this out by using ClrMD on x86 Linux? Unfortunately SOS won't work until CLR Diagnostics has a chance to update SOS, but it should be straight forward to run something like this and see if it blows up:

using DataTarget dt = DataTarget.LoadDump("/path/to/coredump");
using ClrRuntime runtime = dt.ClrVersions.Single().CreateRuntime();
runtime.Heap.EnumerateObjects.ToArray();

I would check it next week.

t-mustafin commented 2 years ago

This changes + patch to diagnostics work for me.