On 01/06/06 Jonathan S. Chambers wrote: > Here's a first attempt at a patch for COM Interop. This only > provides support for RCWs (Runtime Callable Wrappers), i.e. using > unmanaged COM components from managed code. This should support both > classes used from Interop Assemblies (where metadata is known), as well > as bare interface pointers (with no metadata) who are wrapped using > __ComObject.
> Index: mono/mono/metadata/class.c > =================================================================== > --- mono/mono/metadata/class.c (revision 55123) > +++ mono/mono/metadata/class.c (working copy) > @@ -2271,6 +2271,8 @@ > if (cmethod != default_finalize) { > class->has_finalize = 1; > } > + else if ( class->flags & TYPE_ATTRIBUTE_IMPORT && > !MONO_CLASS_IS_INTERFACE(class) ) > + class->has_finalize = 1; > } Please follow the mono coding convention, these lines should read: if (cmethod != default_finalize) { class->has_finalize = 1; } else if (class->flags & TYPE_ATTRIBUTE_IMPORT && !MONO_CLASS_IS_INTERFACE (class)) { class->has_finalize = 1; } i.e. no space after ( and before ), space before (, the else on the same line as } etc. There are several instances of these issues in the patch. > Index: mono/mono/metadata/metadata.h > =================================================================== > --- mono/mono/metadata/metadata.h (revision 55123) > +++ mono/mono/metadata/metadata.h (working copy) > @@ -144,6 +144,7 @@ > MONO_MARSHAL_CONV_BOOL_VARIANTBOOL, > MONO_MARSHAL_CONV_BOOL_I4, > MONO_MARSHAL_CONV_STR_BSTR, > + MONO_MARSHAL_CONV_BSTR_STR, > MONO_MARSHAL_CONV_STR_LPSTR, > MONO_MARSHAL_CONV_LPSTR_STR, > MONO_MARSHAL_CONV_LPTSTR_STR, > @@ -165,8 +166,11 @@ > MONO_MARSHAL_CONV_ARRAY_LPARRAY, > MONO_MARSHAL_CONV_OBJECT_INTERFACE, > MONO_MARSHAL_CONV_OBJECT_IDISPATCH, > + MONO_MARSHAL_CONV_IDISPATCH_OBJECT, > MONO_MARSHAL_CONV_OBJECT_IUNKNOWN, > + MONO_MARSHAL_CONV_IUNKNOWN_OBJECT, > MONO_MARSHAL_CONV_OBJECT_STRUCT, > + MONO_MARSHAL_CONV_STRUCT_OBJECT, > MONO_MARSHAL_CONV_DEL_FTN, > MONO_MARSHAL_CONV_FTN_DEL, > MONO_MARSHAL_FREE_ARRAY Add items only to the end of this enum. > Index: mono/mono/metadata/object-internals.h > =================================================================== > --- mono/mono/metadata/object-internals.h (revision 55123) > +++ mono/mono/metadata/object-internals.h (working copy) > @@ -184,9 +184,18 @@ > MonoObject *unwrapped_server; > gint32 target_domain_id; > MonoString *target_uri; > + MonoObject *object_identity; > + MonoObject *obj_TP; > + MonoObject *stub_data; > } MonoRealProxy; > > typedef struct { > + MonoRealProxy real_proxy; > + gpointer itf_hash_table; > + MonoString *type_name; > +} MonoComProxy; > + > +typedef struct { > MonoObject object; > MonoRealProxy *rp; > MonoRemoteClass *remote_class; > @@ -994,6 +1003,27 @@ > guint32 location; > } MonoManifestResourceInfo; > > +typedef struct { > + MonoObject object; > + guint16 intType; > +} MonoInterfaceTypeAttribute; > + > +typedef struct { > + MonoObject object; > + gpointer comptr; > +} MonoCOMWrapper; > + > +typedef struct { > + MonoMarshalByRefObject object; > + gpointer intType; > +} Mono__ComObject; > + > + > +typedef struct { > + MonoObject object; > + guint32 argnum; > +} MonoLCIDConversionAttribute; > + Do you really need all those types in this header? It seems they are used only in marshal.c, so put them there. > /* Keep in sync with System.GenericParameterAttributes */ > typedef enum { > GENERIC_PARAMETER_ATTRIBUTE_NON_VARIANT = 0, > Index: mono/mono/metadata/marshal.c > =================================================================== > --- mono/mono/metadata/marshal.c (revision 55123) > +++ mono/mono/metadata/marshal.c (working copy) > @@ -68,6 +68,15 @@ > static void > emit_struct_conv (MonoMethodBuilder *mb, MonoClass *klass, gboolean > to_object); > > +void > +component_get_object_and_fnc_ptr(MonoObject *this, MonoMethod* method, > gpointer* pObj, gpointer* pFunc); > + > +gpointer > +component_get_interface(MonoObject *this, MonoClass* klass); > + > +void > +component_create (MonoObject * this, gpointer ptr); These functions should be static, I don't see a need to export them to the world. > + > #ifdef DEBUG_RUNTIME_CODE > static char* > indenter (MonoDisHelper *dh, MonoMethod *method, guint32 ip_offset) > @@ -136,7 +145,6 @@ > register_icall (mono_string_new_wrapper, > "mono_string_new_wrapper", "obj ptr", FALSE); > register_icall (mono_string_to_utf8, "mono_string_to_utf8", > "ptr obj", FALSE); > register_icall (mono_string_to_lpstr, "mono_string_to_lpstr", > "ptr obj", FALSE); > - register_icall (mono_string_to_bstr, "mono_string_to_bstr", > "ptr obj", FALSE); Can you explain why you are removing this function? > @@ -348,6 +359,61 @@ > } > } > > +static void > +release_com_objects (gpointer key, gpointer value, gpointer user_data) > +{ > + MonoObject *exc = NULL; > + static MonoMethod *marshal_release = NULL; > + static MonoMethod *coinitialize = NULL; > + static int co_init = 0; > + gpointer pa [1]; > + if (!marshal_release) > + marshal_release = mono_class_get_method_from_name > (mono_class_from_name(mono_defaults.corlib, "System.Runtime.InteropServices", > "Marshal"), "Release", 1); > + g_assert(marshal_release); > + > + if (!coinitialize) > + { > + gpointer pa [2]; > + int int_null = 0; > + int coinit_mta = 2; > + coinitialize = mono_class_get_method_from_name > (mono_class_from_name(mono_defaults.corlib, "System", "COMInteropHelpers"), > "CoInitializeEx", 2); > + g_assert(coinitialize); > + pa[0] = &int_null; > + pa[1] = &coinit_mta; > + mono_runtime_invoke(coinitialize, NULL, pa, NULL); > + } Why is an initialization function called from a release one? Maybe I'm missing some COM-fu here, but it looks weird. If we created some COM objects, initialization should happen at creation time, not at release... Please explain. > + pa[0] = &value; > + > + mono_runtime_invoke(marshal_release, NULL, pa, &exc); Coding convention: pa [0] = &value; mono_runtime_invoke (marshal_release, NULL, pa, &exc); > +void > +mono_free_com_object (MonoObject *object) > +{ > + gpointer pUnk = NULL; > + static MonoClass* transparent_proxy = NULL; > + > + if (!transparent_proxy) > + transparent_proxy = mono_class_from_name (mono_defaults.corlib, > "System.Runtime.Remoting.Proxies", "TransparentProxy"); Use mono_defaults.transparent_proxy_class. > + if (object->vtable->klass == transparent_proxy) { > + /* TODO */ > + } > + else { > + /* ((MonoCOMWrapper*)this)->comptr; > + * This is not working for some reason. > + */ > + GHashTable* itf_hash = *((int*)object+sizeof(MonoObject)); > + g_hash_table_foreach(itf_hash, release_com_objects, object); What is not working here? The commented code? It should use object, of course. Anyway, all such pointer arithmetric should be replaced with field accesses to the proper struct. > + if (klass == mono_defaults.variant_class) > + { > + int msize = mono_class_value_size (klass, NULL); > + static MonoMethod *get_native_variant_for_object = NULL; > + if (!get_native_variant_for_object) > + get_native_variant_for_object = > mono_class_get_method_from_name_flags (mono_defaults.marshal_class, > "GetNativeVariantForObject", 2, METHOD_ATTRIBUTE_PUBLIC); > + g_assert (get_native_variant_for_object); > + > + mono_mb_emit_ldloc (mb, 0); > + mono_mb_emit_ldloc (mb, 1); Please don't use magic numbers for the locals, store the number that mono_mb_add_local() returns and use that. > @@ -4564,6 +4662,8 @@ > int pos = 0, pos2; > > klass = t->data.klass; > + if (!klass) > + klass = mono_defaults.variant_class; Why this change? It needs an explanation because it looks like it's covering some bug. > + } > mono_mb_emit_stloc (mb, 3); > > /* free the string */ > mono_mb_emit_ldloc (mb, 0); > - mono_mb_emit_icall (mb, g_free); > + > + if (mono_marshal_get_string_encoding ( m->piinfo, spec) == > MONO_NATIVE_BSTR) { > + static MonoMethod *free_bstr = NULL; > + if (!free_bstr) > + free_bstr = > mono_class_get_method_from_name_flags (mono_defaults.marshal_class, > "FreeBSTR", 1, METHOD_ATTRIBUTE_PUBLIC); There is already a lookup for the same method above: please store it in just one location. > +static MonoType* > +mono_marshal_com_return_type(MonoMethodSignature *sig, MonoMarshalSpec *spec) > +{ > + int type; > + MonoType* ret = NULL; > + > + /* convert the result */ > + if (!sig->ret->byref) { > + type = sig->ret->type; Please reduce the indentation: if (sig->ret->byref) { ... assert ... } type = sig->ret->type; ... [...] > +void > +component_get_object_and_fnc_ptr(MonoObject *this, MonoMethod* method, > gpointer* pObj, gpointer* pFunc) > +{ Please try to put all the COM-related functions close to each other in marshal.c, so that we can #ifdef them out when not needed more simply. > + int offset = 0; > + MonoClass* itf = NULL; > + static MonoClass* transparent_proxy = NULL; > + static MonoClass *interface_type_attribute = NULL; > + > + if (!transparent_proxy) > + transparent_proxy = mono_class_from_name (mono_defaults.corlib, > "System.Runtime.Remoting.Proxies", "TransparentProxy"); See mono_defaults again. > +gpointer > +component_get_interface(MonoObject *this, MonoClass* klass) > +{ > + GHashTable* itf_hash; > + gpointer itf; > + MonoCustomAttrInfo *cinfo = NULL; > + > + g_assert(klass->interface_id); > + > + > + if (this->vtable->klass == mono_defaults.transparent_proxy_class) { > + MonoTransparentProxy *tp = ((MonoTransparentProxy *)this); > + MonoComProxy *cp = tp->rp; > + g_assert (cp->itf_hash_table != NULL); > + itf_hash = (GHashTable*)cp->itf_hash_table; > + } > + else { > + itf_hash = *((int*)this+sizeof(MonoObject)); > + } > + itf = g_hash_table_lookup(itf_hash, klass); > + > + > + if (itf == NULL) > + { > + MonoObject * itf_obj; > + static MonoMethod *marshal_get_com_object_for_object = NULL; > + gpointer pa [2]; > + if (!marshal_get_com_object_for_object) > + marshal_get_com_object_for_object = > mono_class_get_method_from_name (mono_class_from_name(mono_defaults.corlib, > "System.Runtime.InteropServices", "Marshal"), "GetComInterfaceForObject", 2); > + g_assert(marshal_get_com_object_for_object); > + pa[0] = this; > + pa[1] = type_from_handle(&klass->byval_arg); > + itf_obj = > mono_runtime_invoke(marshal_get_com_object_for_object, NULL, pa, NULL); > + itf = *(int*)mono_object_unbox(itf_obj); Why this cast? GetComInterfaceForObject() returns a IntPtr and you cast it to an int: this is not 64 bit safe. It should be: itf = *(gpointer*)mono_object_unbox (itf_obj); > +gpointer > +ves_icall_System_Runtime_InteropServices_Marshal_GetIUnknownForObject > (MonoObject *obj) > +{ > + gpointer pUnk = NULL; > + static MonoClass* com_proxy = NULL; > + GHashTable* itf_hash; > + > + if (!com_proxy) > + com_proxy = mono_class_from_name (mono_defaults.corlib, > "System", "ComProxy"); You seem to use this in several places, you may want to add it to mono_defaults. > > +void mono_free_com_object (MonoObject *object); Please rename to mono_com_object_free. > Index: mono/mono/metadata/gc.c > =================================================================== > --- mono/mono/metadata/gc.c (revision 55123) > +++ mono/mono/metadata/gc.c (working copy) > @@ -99,6 +99,16 @@ > return; > } > > + /* COM objects need to have release called on the IUnknown pointer > + Not sure if this is the best way to do it. > + */ > + > + if ( o->vtable->klass->flags & TYPE_ATTRIBUTE_IMPORT && > !MONO_CLASS_IS_INTERFACE(o->vtable->klass) ) > + { Why the interface check? Interfaces can't be instantiated. Thanks for the patch! After a few small cleanups it can go in svn. Congrats for getting this working! lupus -- ----------------------------------------------------------------- [EMAIL PROTECTED] debian/rules [EMAIL PROTECTED] Monkeys do it better _______________________________________________ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list