[Mono-dev] RE: Com Interop Patch
Paolo Molaro
lupus at ximian.com
Mon Jan 9 11:07:42 EST 2006
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
--
-----------------------------------------------------------------
lupus at debian.org debian/rules
lupus at ximian.com Monkeys do it better
More information about the Mono-devel-list
mailing list