Add Variant marshalling

[kant2002]

#1100, #306

[kant2002]

I did not test that yet. Just trying to gather feedback on the direction of the change.

[kant2002]

With half backed implementation I hit this

Seems to be IDispatch is inevitable. Will need advice after this would be done.

[kant2002]

Not tested

• Marshal_XXX_Boolean
• Marshal_XXX_Currency
• Marshal_XXX_Object
• Marshal_XXX_Object_IUnknown

Current tests in the Interop\Pinvoke\Variant depends on the built-in marshalling. Is it possible split tests that built-in marshalling would be not required. Or maybe I can create similar set of test for ComWrappers code path which will share most of code with current tests by reusing files? Do I need go to runtime for that?

[jkotas]

Is it possible split tests that built-in marshalling would be not required.

Yes. Note that there is ongoing work to split the tests that require built-in COM marshalling in dotnet/runtime, so contributing to that would be definitely welcomed:

dotnet/runtime#50500
dotnet/runtime#51265

[jkotas]

Seems to be IDispatch is inevitable. Will need advice after this would be done.

Does this really only need to QueryInterface for IID_IDispatch?

[kant2002]

Does this really only need to QueryInterface for IID_IDispatch?

Yes. Pedantic me want to implement Marshal.GetObjectForComInterface<object, T> which is overkill or this specific case. If I change Variant implementation in runtime to just QI to IDispatch that should just works.

[kant2002]

Locally seems to be working. Build issues seems to be unrelated. But current PR I rely on the changes to runtime which is not yet landed, and as such not integrated.

ComboBox now demands IDispatch implementation from my ComWrappers instance. Sigh.
Will try to use https://github.com/dotnet/samples/blob/main/core/interop/comwrappers/IDispatch/AnyObjectProxy.cs as example how to do that. But overall work on the Variant marshalling seems to be done in technical sense. Ready to accept feedback.

[kant2002]

Actually three lines may not working

https://github.com/dotnet/runtime/blob/01b7e73cd378145264a7cb7a09365b41ed42b240/src/tests/Interop/PInvoke/Variant/VariantTest.cs#L46-L48

It's not clear for me

• First line means that TimeSpan not marshalled, or specifically TimeSpan.Zero cannot be marshalled.
• Third line VariantWrapper not supported at all? Actually I found https://docs.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.variantwrapper?view=net-5.0#remarks but have not incentive to implement that now because I do not fully understand what's going on here.
• Second line. Does any not implemented cases should throw NotSupportedException?

[jkotas]

First line means that TimeSpan not marshalled, or specifically TimeSpan.Zero cannot be marshalled.

Yeah, that looks odd. It would be useful to run it on CoreCLR to see where it is comming from.

Does any not implemented cases should throw NotSupportedException?

It should throw whatever CoreCLR throws. In general, there is no much consistency around NotSupportedException and similar exceptions - see dotnet/runtime#22069 (comment) for some stats.

The full Variant marshalling is very complex. It is fine to only implement a reasonable subset.

[kant2002]

The full Variant marshalling is very complex. It is fine to only implement a reasonable subset.

Can we leave this 3 cases? I would check TimeSpan for sure, but would like to move forward if possible

[kant2002]

@jkotas build issues seems to be unrelated. Do I miss anything else here?

[jkotas]

It is failing with:

2021-05-24T09:16:57.1984007Z   Assertion failed 'genActualType(lclTyp) == genActualType(op1->gtType) || (genActualType(lclTyp) == TYP_I_IMPL && op1->IsLocalAddrExpr() != nullptr) || (genActualType(lclTyp) == TYP_I_IMPL && (op1->gtType == TYP_BYREF || op1->gtType == TYP_REF)) || (genActualType(op1->gtType) == TYP_I_IMPL && lclTyp == TYP_BYREF) || (varTypeIsFloating(lclTyp) && varTypeIsFloating(op1->TypeGet())) || ((genActualType(lclTyp) == TYP_BYREF) && genActualType(op1->TypeGet()) == TYP_REF) : Possibly bad IL with CEE_stloc.s at offset 0013h (op1=ref op2=NULL stkDepth=0)' in 'Internal.CompilerGenerated.<Module>:ReverseDelegateStub__Get(int,long,long,int,long,long,long):int' during 'Importation' (IL size 84)

It looks like bad IL generated by the new marshaler.

[kant2002]

@jkotas this was fixed in ea76dd0 as part of #1102 . Exception was part of System.Management.WmiNetUtilsHelper.GetNames

internal delegate int GetNames(
  int vFunc,
  IntPtr pWbemClassObject,
  [In][MarshalAs(UnmanagedType.LPWStr)] string wszQualifierName,
  [In] int lFlags,
  [In] ref object pQualifierVal, 
->>  [MarshalAs(UnmanagedType.SafeArray, SafeArraySubType = VarEnum.VT_BSTR)] out string[] pNames);

[kant2002]

Other location is

// System.Management.WmiNetUtilsHelper.GetDemultiplexedStub

internal delegate int GetDemultiplexedStub(
  [In][MarshalAs(UnmanagedType.IUnknown)] object pIUnknown,
  [In] bool isLocal, 
-->  [MarshalAs(UnmanagedType.IUnknown)] out object ppIUnknown);

For me this seems as unrelated change. (This one actually exposes another issue with IUnknown marshalling). I need more time to understand what's going on.

[kant2002]

So based on my understanding, problem is with delegates who has [In][Out]ref object as the parameter.
Current implementation of marshaller does not handle that. And seems I would like somehow to opt-out of that temporary, but do not know how. If possible and somebody know any hints, I would appreciate.

This indicates missing coverage in tests. If I have courage, I will find out what case trigger issue, then I may create test. Should I do that, if so, where should I do that?

[jkotas]

Throw exception for the unhandled case before it crashes the JIT.

[kant2002]

Phew, I'm finally understand where to put exception. My understanding that this was related to #176 because I was able to replicate with

Get xxx = Marshal.GetDelegateForFunctionPointer<Get>(IntPtr.Zero);
internal delegate int Get(ref object pVal);

[kant2002]

@jkotas anything here?

[jkotas]

LGTM. Thank you!