Migrate core part of IDispatch support for built-in COM to managed

[huoyaoyuan]

Opening for discussion per #124303 (comment). @AaronRobinsonMSFT

Currently, the IDispatch support relies heavily on managed reflection stack:

runtime/src/coreclr/vm/dispatchinfo.cpp

Lines 1592 to 1794 in 1969a31

	switch (MemberType)
	{
	case Field:
	{
	// Make sure this invoke is actually for a property put or get.
	if (wFlags & (DISPATCH_METHOD | DISPATCH_PROPERTYGET))
	{
	// Do some more validation now that we know the type of the invocation.
	if (NumNamedArgs != 0)
	COMPlusThrowHR(DISP_E_NONAMEDARGS);
	if (NumArgs != 0)
	COMPlusThrowHR(DISP_E_BADPARAMCOUNT);
	// Retrieve the method descriptor that will be called on.
	MethodDesc *pMD = GetFieldInfoMD(METHOD__FIELD_INFO__GET_VALUE, pObjs->MemberInfo->GetTypeHandle());
	MethodDescCallSite getValue(pMD, &pObjs->MemberInfo);
	// Prepare the arguments that will be passed to Invoke.
	ARG_SLOT Args[] =
	{
	ObjToArgSlot(pObjs->MemberInfo),
	ObjToArgSlot(pObjs->Target),
	};
	// Do the actual method invocation.
	pObjs->RetVal = getValue.Call_RetOBJECTREF(Args);
	}
	else if (wFlags & (DISPATCH_PROPERTYPUT | DISPATCH_PROPERTYPUTREF))
	{
	// Do some more validation now that we know the type of the invocation.
	if (NumArgs != 0)
	COMPlusThrowHR(DISP_E_BADPARAMCOUNT);
	if (NumNamedArgs != 0)
	COMPlusThrowHR(DISP_E_NONAMEDARGS);
	// Retrieve the method descriptor that will be called on.
	MethodDesc *pMD = GetFieldInfoMD(METHOD__FIELD_INFO__SET_VALUE, pObjs->MemberInfo->GetTypeHandle());
	MethodDescCallSite setValue(pMD, &pObjs->MemberInfo);
	// Prepare the arguments that will be passed to Invoke.
	ARG_SLOT Args[] =
	{
	ObjToArgSlot(pObjs->MemberInfo),
	ObjToArgSlot(pObjs->Target),
	ObjToArgSlot(pObjs->PropVal),
	(ARG_SLOT) BindingFlags,
	ObjToArgSlot(pObjs->OleAutBinder),
	ObjToArgSlot(pObjs->CultureInfo),
	};
	// Do the actual method invocation.
	setValue.Call(Args);
	}
	else
	{
	COMPlusThrowHR(DISP_E_MEMBERNOTFOUND);
	}
	break;
	}
	case Property:
	{
	// Make sure this invoke is actually for a property put or get.
	if (wFlags & (DISPATCH_METHOD | DISPATCH_PROPERTYGET))
	{
	if (!IsPropertyAccessorVisible(false, &pObjs->MemberInfo))
	COMPlusThrowHR(DISP_E_MEMBERNOTFOUND);
	// Retrieve the method descriptor that will be called on.
	MethodDesc *pMD = GetPropertyInfoMD(METHOD__PROPERTY__GET_VALUE, pObjs->MemberInfo->GetTypeHandle());
	MethodDescCallSite getValue(pMD, &pObjs->MemberInfo);
	// Prepare the arguments that will be passed to GetValue().
	ARG_SLOT Args[] =
	{
	ObjToArgSlot(pObjs->MemberInfo),
	ObjToArgSlot(pObjs->Target),
	(ARG_SLOT) BindingFlags,
	ObjToArgSlot(pObjs->OleAutBinder),
	ObjToArgSlot(pObjs->ParamArray),
	ObjToArgSlot(pObjs->CultureInfo),
	};
	// Do the actual method invocation.
	pObjs->RetVal = getValue.Call_RetOBJECTREF(Args);
	}
	else if (wFlags & (DISPATCH_PROPERTYPUT | DISPATCH_PROPERTYPUTREF))
	{
	if (!IsPropertyAccessorVisible(true, &pObjs->MemberInfo))
	COMPlusThrowHR(DISP_E_MEMBERNOTFOUND);
	// Retrieve the method descriptor that will be called on.
	MethodDesc *pMD = GetPropertyInfoMD(METHOD__PROPERTY__SET_VALUE, pObjs->MemberInfo->GetTypeHandle());
	MethodDescCallSite setValue(pMD, &pObjs->MemberInfo);
	// Prepare the arguments that will be passed to SetValue().
	ARG_SLOT Args[] =
	{
	ObjToArgSlot(pObjs->MemberInfo),
	ObjToArgSlot(pObjs->Target),
	ObjToArgSlot(pObjs->PropVal),
	(ARG_SLOT) BindingFlags,
	ObjToArgSlot(pObjs->OleAutBinder),
	ObjToArgSlot(pObjs->ParamArray),
	ObjToArgSlot(pObjs->CultureInfo),
	};
	// Do the actual method invocation.
	setValue.Call(Args);
	}
	else
	{
	COMPlusThrowHR(DISP_E_MEMBERNOTFOUND);
	}
	break;
	}
	case Method:
	{
	// Make sure this invoke is actually for a method. We also allow
	// prop gets since it is harmless and it allows the user a bit
	// more freedom.
	if (!(wFlags & (DISPATCH_METHOD | DISPATCH_PROPERTYGET)))
	COMPlusThrowHR(DISP_E_MEMBERNOTFOUND);
	// Retrieve the method descriptor that will be called on.
	MethodDesc *pMD = GetMethodInfoMD(METHOD__METHOD__INVOKE, pObjs->MemberInfo->GetTypeHandle());
	MethodDescCallSite invoke(pMD, &pObjs->MemberInfo);
	// Prepare the arguments that will be passed to Invoke.
	ARG_SLOT Args[] =
	{
	ObjToArgSlot(pObjs->MemberInfo),
	ObjToArgSlot(pObjs->Target),
	(ARG_SLOT) BindingFlags,
	ObjToArgSlot(pObjs->OleAutBinder),
	ObjToArgSlot(pObjs->ParamArray),
	ObjToArgSlot(pObjs->CultureInfo),
	};
	// Do the actual method invocation.
	pObjs->RetVal = invoke.Call_RetOBJECTREF(Args);
	break;
	}
	default:
	{
	COMPlusThrowHR(E_UNEXPECTED);
	}
	}
	}
	else
	{
	// Convert the LCID into a CultureInfo.
	GetCultureInfoForLCID(lcid, &pObjs->CultureInfo);
	pObjs->ReflectionObj = GetReflectionObject();
	// Retrieve the method descriptor that will be called on.
	MethodDesc *pMD = GetInvokeMemberMD();
	MethodDescCallSite invokeMember(pMD, &pObjs->ReflectionObj);
	// Allocate the string that will contain the name of the member.
	if (!pDispMemberInfo)
	{
	WCHAR strTmp[64];
	_snwprintf_s(strTmp, ARRAY_SIZE(strTmp), _TRUNCATE, DISPID_NAME_FORMAT_STRING, id);
	pObjs->MemberName = (OBJECTREF)StringObject::NewString(strTmp);
	}
	else
	{
	pObjs->MemberName = (OBJECTREF)StringObject::NewString(pDispMemberInfo->GetName().GetUnicode());
	}
	// If there are named arguments, then set up the array of named arguments
	// to pass to InvokeMember.
	if (NumNamedArgs > 0)
	SetUpNamedParamArray(pDispMemberInfo, pSrcArgNames, NumNamedArgs, &pObjs->NamedArgArray);
	// If this is a PROPUT or a PROPPUTREF then we need to add the value
	// being set as the last argument in the argument array.
	if (wFlags & (DISPATCH_PROPERTYPUT | DISPATCH_PROPERTYPUTREF))
	pObjs->ParamArray->SetAt(NumParams, pObjs->PropVal);
	// Prepare the arguments that will be passed to Invoke.
	ARG_SLOT Args[] =
	{
	ObjToArgSlot(pObjs->ReflectionObj),
	ObjToArgSlot(pObjs->MemberName),
	(ARG_SLOT) BindingFlags,
	ObjToArgSlot(pObjs->OleAutBinder),
	ObjToArgSlot(pObjs->Target),
	ObjToArgSlot(pObjs->ParamArray),
	ObjToArgSlot(NULL), // @TODO(DM): Look into setting the byref modifiers.
	ObjToArgSlot(pObjs->CultureInfo),
	ObjToArgSlot(pObjs->NamedArgArray),
	};
	// Do the actual method invocation.
	pObjs->RetVal = invokeMember.Call_RetOBJECTREF(Args);
	}

runtime/src/coreclr/vm/dispatchinfo.cpp

Lines 510 to 568 in 1969a31

	LPWSTR DispatchMemberInfo::GetMemberName(OBJECTREF MemberInfoObj, ComMTMemberInfoMap *pMemberMap)
	{
	CONTRACT (LPWSTR)
	{
	THROWS;
	GC_TRIGGERS;
	MODE_COOPERATIVE;
	INJECT_FAULT(COMPlusThrowOM());
	PRECONDITION(MemberInfoObj != NULL);
	PRECONDITION(CheckPointer(pMemberMap, NULL_OK));
	POSTCONDITION(CheckPointer(RETVAL));
	}
	CONTRACT_END;
	NewArrayHolder<WCHAR> strMemberName = NULL;
	ComMTMethodProps *pMemberProps = NULL;
	GCPROTECT_BEGIN(MemberInfoObj);
	{
	// Get the member's properties.
	pMemberProps = GetMemberProps(MemberInfoObj, pMemberMap);
	// If we managed to get the member's properties then extract the name.
	if (pMemberProps)
	{
	int MemberNameLen = (INT)u16_strlen(pMemberProps->pName);
	strMemberName = new WCHAR[MemberNameLen + 1];
	memcpy(strMemberName, pMemberProps->pName, (MemberNameLen + 1) * sizeof(WCHAR));
	}
	else
	{
	// Retrieve the Get method for the Name property.
	MethodDesc *pMD = MemberLoader::FindPropertyMethod(MemberInfoObj->GetMethodTable(), MEMBER_INFO_NAME_PROP, PropertyGet);
	_ASSERTE(pMD && "Unable to find getter method for property MemberInfo::Name");
	MethodDescCallSite propGet(pMD, &MemberInfoObj);
	// Prepare the arguments.
	ARG_SLOT Args[] =
	{
	ObjToArgSlot(MemberInfoObj)
	};
	// Retrieve the value of the Name property.
	STRINGREF strObj = propGet.Call_RetSTRINGREF(Args);
	_ASSERTE(strObj != NULL);
	// Copy the name into the buffer we will return.
	int MemberNameLen = strObj->GetStringLength();
	strMemberName = new WCHAR[strObj->GetStringLength() + 1];
	memcpy(strMemberName, strObj->GetBuffer(), MemberNameLen * sizeof(WCHAR));
	strMemberName[MemberNameLen] = 0;
	}
	}
	GCPROTECT_END();
	strMemberName.SuppressRelease();
	RETURN strMemberName;
	}

The majority of code is manipulating managed objects in native code. The proposal is to convert the managed-manipulating code to managed.

The scope will include:

• DispatchInfo::InvokeMemberWorker: this is the core member that manipulates managed reflection types intensively.
• DispatchMemberInfo: it uses managed MemberInfo and ParameterInfo to represent parameter information. The native signature parsing part is shared with MarshalInfo and should be kept native.
• Most of DispatchInfo: it uses managed reflection stack to retrieve member list, with manual synchronization etc.
• Overrides of DispParamMarshaler: most are invoking managed functions or custom marshalers. SafeArray and VT_RECORD will be kept native.

The parameter marshal and coerce logic will be kept as-is, with some code written in plain transcribed managed code.

After the conversion, the central logic of IDispatch support would be managed reflection code, with native calls for pieces unsuitable for managed code.

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.

[AaronRobinsonMSFT]

@huoyaoyuan Thanks for filing this proposal, I really appreciate it.

The proposal is to convert the managed-manipulating code to managed.

The narrow example above in InvokeMemberWorker() does seem reasonable, but not the entire function. That is doing a lot of argument manipulation that I'd prefer to not touch. If you're suggesting just updating 1592 to 1794, then I can see an argument for that.

Overrides of DispParamMarshaler:

This is the base class for many marshallers. Are you suggesting just update the functions on the base class or the overrides? Many (most?) of the overrides seem to use MethodDescCallSite that should eventually be converted at some point. The base functions all see to call into OleVariant::MarshalOleRefVariantForObject(), which also has MethodDescCallSite. I'm not fully understanding the benefit of moving all of this up, other than it would be needed if the DispatchInfo is converted. I'm not convinced this size of change will be a net win.

After the conversion, the central logic of IDispatch support would be managed reflection code, with native calls for pieces unsuitable for managed code.

This is going to make it much more difficult to debug and likely introduce subtle bugs in an area that is prone to painful regressions. Given our sparse testing for most IDispatch scenarios, see the recent #123961 which was a test for a regression introduced in #106735, I'm reluctant to support such a big change. If there was some tangible win I would get it, but users of this area are after reliabilty not wins.

Do you have a compelling benefit for changing this so broadly as opposed to the narrow changes needed for MethodDescCallSite?

[huoyaoyuan]

That is doing a lot of argument manipulation that I'd prefer to not touch. If you're suggesting just updating 1592 to 1794, then I can see an argument for that.

I have actually completed the entire function. The argument manipulation part can be converted with plain transcription. It can be kept native in large or small pieces, but in my test it can just expose leaf entries for individual elements in native.

The downside of splitting large part of logic between native and managed is buffer management. Passing buffer with managed objects from native to managed is not pleasant.

This is the base class for many marshallers. Are you suggesting just update the functions on the base class or the overrides?

The entire class family. It's invoked with DispatchMemberInfo which depends mostly on managed code. Invoking virtual method from managed code is not simple and they should keep consistent with DispatchMemberInfo. The array/record marshaling type will use QCall to native.

The base functions all see to call into OleVariant::MarshalOleRefVariantForObject(), which also has MethodDescCallSite.

My proposal is to expose a dedicated QCall for it like MarshalNative_GetNativeVariantForObject and not touch it during IDispatch work. It's also used as a majority routine in the argument manipulation part of InvokeMemberWorker.

This is going to make it much more difficult to debug and likely introduce subtle bugs in an area that is prone to painful regressions.

Debuggability is a important point that I didn't consider too much. It can be a case that if the majority of logic is in managed, it would be easier to debug with managed debugger. I'm somehow confident about no behavior won't change because the entire control flow will be kept as-is, with just auxiliary buffer management change.

Do you have a compelling benefit for changing this so broadly as opposed to the narrow changes needed for MethodDescCallSite?

It can simplify object lifetime management a lot. In the proposed way, the entire DispatchInfo class will hold reference to one managed object, which would be fully responsible for the control flow. This will not introduce new lifetime pattern as it already holds a Type.

Let me describe the picture of goal for IDispatch:

• At entry point, native code finds the managed object from the CCW. This will be unchanged.
• Find the type-associated marshal strategy (DispatchInfo) from the object. This will be unchanged.
• The control flow will then be transferred to a central managed method, replacing DispatchInfo::InvokeMember and DispatchInfo::InvokeMemberWorker.
• The central managed method retrieves detailed strategy (DispatchMemberInfo and ParameterInfo) with managed reflection stack. This will simplify a lot because the code for retrieving information from managed object and store in native will be eliminated.
• The central managed method marshals argument VARIANT array to managed. This should be the most concerned part for behavior change. By following strict transcription in this part, the risk can be lowered.
• Leaf marshaling routine (object <-> VARIANT) will be kept as-is in native with dedicated 1:1 entry point. They can be visited later, but not in the IDispatch work.

This is partial work I have completed and can be reviewed for shape:

https://github.com/dotnet/runtime/compare/de90299215b46e09cf84060f206333327adf2113...huoyaoyuan:runtime:371f89c175c80df62fbbd5a41846678edab38802?expand=1