[Mono-dev] COM Interop in Mono

[Paolo Molaro]

On 11/16/05 Jonathan S. Chambers wrote:
> Attached is a diff of some current progress. These changes are all in
> place right now, they would of course need moved to an external library.

Thanks for the patch.
I don't think this stuff should be moved to a library, at least not
until it's windows-only. Moving to a module may be needed later if we
support other COM-like systems using the same interface, but that is
likely way off.


> Index: metadata/class.c
> ===================================================================
> --- metadata/class.c	(revision 52794)
> +++ metadata/class.c	(working copy)
> @@ -2682,6 +2682,12 @@
>  		g_assert (class->field.count == 0);
>  	}
>  
> +	/* reserve space to store COM object pointer in RCW */
> +	if (class->flags & TYPE_ATTRIBUTE_IMPORT && !MONO_CLASS_IS_INTERFACE(class)) {
> +		class->instance_size += 2 * sizeof (gpointer);

Reserve room just for one pointer.

> Index: metadata/object-internals.h
> ===================================================================
> --- metadata/object-internals.h	(revision 52794)
> +++ metadata/object-internals.h	(working copy)
> @@ -993,6 +993,17 @@
>  	guint32 location;
>  } MonoManifestResourceInfo;
>  
> +
> +typedef struct {
> +	MonoObject object;
> +	MonoString *guid;
> +} MonoReflectionGuidAttribute;
> +
> +typedef struct {
> +	MonoObject object;
> +	guint16 intType;
> +} MonoInterfaceTypeAttribute;

Add also a:

typedef struct {
	MonoObject object;
	gpointer comptr;
} MonoCOMWrapper;

and always use it instead of doing pointer arithmetric etc.

> Index: metadata/marshal.c
> ===================================================================
> --- metadata/marshal.c	(revision 52794)
> +++ metadata/marshal.c	(working copy)
> @@ -6309,6 +6309,279 @@
>  	mono_mb_emit_byte (mb, CEE_RET);
>  }
>  
> +void
> +component_get_object_and_fnc_ptr(MonoObject *this, MonoMethod* method, gpointer* pObj, gpointer* pFunc)

Most of these functions should be static. If they need to be exported,
they should be in an header file and have the usual mono_ prefix.

> +{
> +	IUnknown * pUnk = NULL;
> +	int ** vtable;
> +	int i = 0;
> +	int offset = 0;
> +	GUID clsid;
> +
> +	for (i = 0; i < method->klass->interface_count; i++)
> +	{

The brace needs to go in the previous line, several of these in the
patch.

> +		int first;
> +		MonoClass* itf = *(method->klass->interfaces+i);

Change to method->klass->interfaces [i];

> +
> +		first = itf->method.first;
> +		if (first <= method->slot && first+itf->method.count > method->slot)

Needs spaces around operators like +.

> +		{
> +			static MonoClass *GuidAttribute;
> +			static MonoClass *InterfaceTypeAttribute;
> +			MonoCustomAttrInfo *cinfo;
> +			MonoReflectionGuidAttribute *attr;
> +			MonoInterfaceTypeAttribute* itf_attr;
> +
> +			offset = method->slot - first;
> +
> +			if (!GuidAttribute)
> +				GuidAttribute = mono_class_from_name (mono_defaults.corlib, "System.Runtime.InteropServices", "GuidAttribute");

You need GuidAttribute already in two places: please add it to the
mono_defaults struct. Aso, please use lower case variable names.

> +			if (!InterfaceTypeAttribute)
> +				InterfaceTypeAttribute = mono_class_from_name (mono_defaults.corlib, "System.Runtime.InteropServices", "InterfaceTypeAttribute");
> +
> +			if (GuidAttribute) {

No need for this check here: if the attribute is not in corlib there are
bigger issues and we'd have the proper code to check it somewhere _once_
instead of at each loop iteration.

> +				cinfo = mono_custom_attrs_from_class (itf);
> +				if (cinfo) {
> +					attr = (MonoReflectionGuidAttribute*)mono_custom_attrs_get_attr (cinfo, GuidAttribute);
> +					itf_attr = (MonoInterfaceTypeAttribute*)mono_custom_attrs_get_attr (cinfo, InterfaceTypeAttribute);
> +					if (attr) {
> +						LPOLESTR temp;
> +						wchar_t buf[50];
> +						wsprintf(buf,L"{%s}",attr->guid->chars);

You're not allowed to access MonoString->chars directly. Use
mono_string_chars(). This code though, should avoid creating the object
and just access the guid data inside the proper cinfo->data.
Of course this code is invariant in the loop, so it should be moved out
of it.

> +						CLSIDFromString(buf, &clsid);
> +						((IUnknown*)*((int*)this+sizeof(MonoObject)))->lpVtbl->QueryInterface(((IUnknown*)*((int*)this+sizeof(MonoObject))), &clsid, &pUnk);

this should be a MonoCOMWrapper, so you can simplify here.

> +
> +						if (pUnk)
> +						{
> +							*pObj = pUnk;
> +							vtable = pUnk->lpVtbl;
> +
> +							if (itf_attr && itf_attr->intType == 1)
> +								offset += 3;
> +							else
> +								offset += 7;

Please assign a meaningful name to these magic numbers.

> +void
> +component_create (MonoObject * this)
> +{
> +	void * pUnk;
> +	int i = 0;
> +	GUID clsid;
> +
> +	
> +	static MonoClass *GuidAttribute;
> +	MonoCustomAttrInfo *cinfo;
> +	MonoReflectionGuidAttribute *attr;
> +	static int coinit = 0;
> +
> +	if (!coinit)
> +	{
> +		CoInitialize(NULL);

How expensive is this call? Maybe it can be called unconditionally on
windows in the mono_runtime_init() function.

> +		coinit = 1;
> +	}
> +
> +	if (!GuidAttribute)
> +			GuidAttribute = mono_class_from_name (mono_defaults.corlib, "System.Runtime.InteropServices", "GuidAttribute");
> +
> +	if (GuidAttribute) {

Same comments as above, move the lookup and check somewhere else.

> +MonoMethod *
> +component_get_native_wrapper (MonoMethod *method)
> +{
> +	MonoMethodSignature *sig, *csig;
> +	MonoMethodBuilder *mb;
> +	MonoMethod *res;
> +	GHashTable *cache;
> +
> +	g_assert (method != NULL);
> +	g_assert (method->klass->flags & TYPE_ATTRIBUTE_IMPORT);
> +	g_assert (method->iflags & (METHOD_IMPL_ATTRIBUTE_INTERNAL_CALL | METHOD_IMPL_ATTRIBUTE_RUNTIME));
> +
> +	cache = method->klass->image->native_wrapper_cache;
> +	sig = mono_method_signature (method);
> +
> +	mb = mono_mb_new (method->klass, method->name, MONO_WRAPPER_MANAGED_TO_NATIVE);
> +	mb->method->save_lmf = 1;
> +
> +	g_print ("Finding internal method for COM object '%s.%s::%s'.\n",method->klass->name_space, method->klass->name,method->name);
> +
> +	if (!strcmp(method->name,".ctor"))
> +	{
> +		csig = sig;
> +		
> +		if (sig->hasthis)
> +			mono_mb_emit_byte (mb, CEE_LDARG_0);
> +
> +		/*for (i = 0; i < sig->param_count; i++)
> +			mono_mb_emit_ldarg (mb, i + sig->hasthis);
> +		*/
> +
> +		mono_mb_emit_native_call (mb, csig, component_create);
> +		emit_thread_interrupt_checkpoint (mb);
> +		mono_mb_emit_byte (mb, CEE_RET);
> +
> +		csig = mono_metadata_signature_dup (csig);
> +		csig->pinvoke = 0;
> +		res = mono_mb_create_and_cache (cache, method,
> +										mb, csig, csig->param_count + 16);
> +		mono_mb_free (mb);
> +		return res;
> +	}
> +	else
> +	{
> +		/* get the function pointer from the object/vtable */
> +		static MonoMethodSignature *comsig = NULL;
> +		MonoMethodSignature *callsig = NULL;		
> +		int i = 0;
> +
> +		csig = sig;
> +
> +		if (!comsig) {
> +			comsig = mono_metadata_signature_alloc (method->klass->image, 3);
> +			comsig->hasthis = 1;
> +			comsig->params [0] = &mono_defaults.int_class->byval_arg;
> +			comsig->params [1] = &mono_defaults.int_class->this_arg;
> +			comsig->params [2] = &mono_defaults.int_class->this_arg;
> +			comsig->ret = &mono_defaults.void_class->byval_arg;
> +			comsig->pinvoke = 0;
> +		}
> +
> +		callsig = mono_metadata_signature_alloc (method->klass->image, csig->param_count + (MONO_TYPE_IS_VOID(csig->ret) ? 0: 1));
> +		callsig->hasthis = 1;
> +		callsig->ret = &mono_defaults.int_class->byval_arg;
> +
> +		/* regular params */
> +		for (i = 0; i < csig->param_count; i++)
> +			callsig->params[i] = csig->params[i];
> +		
> +		/* return val as last param by ref*/
> +		if (!MONO_TYPE_IS_VOID(csig->ret))
> +			callsig->params[csig->param_count] = &mono_class_from_mono_type(csig->ret)->this_arg;
> +
> +		/* not sure about this */
> +		callsig->pinvoke = 1;
> +
> +		/* COM is stdcall */
> +		callsig->call_convention = MONO_CALL_STDCALL;
> +		
> +		/* for func ptr */
> +		mono_mb_add_local (mb, &mono_defaults.int_class->byval_arg);
> +		mono_mb_add_local (mb, &mono_defaults.int_class->byval_arg);

Always store the return value from mono_mb_add_local() in an
appropriately named local var and always use that, instead of magic
numbers when you need to emit ldloc etc.

Thanks for working on this!

lupus

-- 
-----------------------------------------------------------------
lupus at debian.org                                     debian/rules
lupus at ximian.com                             Monkeys do it better