[Mono-dev] COM Callable Wrapper Patch
Paolo Molaro
lupus at ximian.com
Mon Dec 4 12:53:12 EST 2006
On 12/01/06 Jon Chambers wrote:
> Index: mono/mono/metadata/loader.c
> ===================================================================
> --- mono/mono/metadata/loader.c (revision 68741)
> +++ mono/mono/metadata/loader.c (working copy)
> @@ -1377,7 +1377,7 @@
> !(cols [1] & METHOD_IMPL_ATTRIBUTE_INTERNAL_CALL) &&
> !(result->iflags & METHOD_IMPL_ATTRIBUTE_RUNTIME) && container) {
> gpointer loc = mono_image_rva_map (image, cols [0]);
> - g_assert (loc);
> + g_assert (loc) ;
This change is incorrect and should be dropped.
> Index: mono/mono/metadata/domain.c
> ===================================================================
> --- mono/mono/metadata/domain.c (revision 68741)
> +++ mono/mono/metadata/domain.c (working copy)
> @@ -812,14 +812,24 @@
>
> mono_defaults.variant_class = mono_class_from_name (
> mono_defaults.corlib, "System", "Variant");
> + g_assert (mono_defaults.variant_class != 0);
I think we should move away from asserting here. I most cases
the types here should be initialized lazily to avoid increasing startup
time and memory usage. Likely all the COM-related types can be loaded
lazily the first time some COM feature is used.
It's still fine to have the fields in mono_defaults.
> Index: mono/mono/metadata/marshal.c
> ===================================================================
> --- mono/mono/metadata/marshal.c (revision 68741)
> +++ mono/mono/metadata/marshal.c (working copy)
> @@ -386,6 +448,10 @@
> return rt;
> }
>
> +
> +
> +/* End COM Interop related stuff until seperate file */
> +
> void
> mono_marshal_init (void)
> {
> @@ -446,6 +512,8 @@
> register_icall (cominterop_get_method_interface, "cominterop_get_method_interface", "object ptr", FALSE);
> register_icall (cominterop_get_function_pointer, "cominterop_get_function_pointer", "ptr ptr int32", FALSE);
> register_icall (cominterop_object_is_rcw, "cominterop_object_is_rcw", "int32 object", FALSE);
> + register_icall (cominterop_get_ccw, "cominterop_get_ccw", "ptr object ptr", FALSE);
> + register_icall (cominterop_get_ccw_object, "cominterop_get_ccw_object", "object ptr int32", FALSE);
> }
> }
>
> @@ -1796,7 +1864,7 @@
> static MonoMethod* com_interop_proxy_get_proxy = NULL;
> static MonoMethod* get_transparent_proxy = NULL;
> int real_proxy;
> - guint32 pos_failed = 0;
> + guint32 pos_null = 0, pos_ccw = 0, pos_end = 0;
> MonoClass *klass = mono_class_from_mono_type (type);
>
> mono_mb_emit_ldloc (mb, 1);
> @@ -1805,8 +1873,17 @@
>
> mono_mb_emit_ldloc (mb, 0);
> mono_mb_emit_byte (mb, CEE_LDIND_I);
> - pos_failed = mono_mb_emit_short_branch (mb, CEE_BRFALSE_S);
> + pos_null = mono_mb_emit_short_branch (mb, CEE_BRFALSE_S);
>
> + // load dst to store later
> + mono_mb_emit_ldloc (mb, 1);
> +
> + mono_mb_emit_ldloc (mb, 0);
> + mono_mb_emit_byte (mb, CEE_LDIND_I);
> + mono_mb_emit_icon (mb, TRUE);
> + mono_mb_emit_icall (mb, cominterop_get_ccw_object);
> + pos_ccw = mono_mb_emit_short_branch (mb, CEE_BRTRUE_S);
> +
> if (!com_interop_proxy_class)
> com_interop_proxy_class = mono_class_from_name (mono_defaults.corlib, "Mono.Interop", "ComInteropProxy");
> if (!com_interop_proxy_get_proxy)
> @@ -1821,18 +1898,35 @@
> mono_mb_emit_ptr (mb, &mono_defaults.com_object_class->byval_arg);
> mono_mb_emit_icall (mb, type_from_handle);
> mono_mb_emit_managed_call (mb, com_interop_proxy_get_proxy, NULL);
> - mono_mb_emit_stloc (mb, real_proxy);
> + //mono_mb_emit_stloc (mb, real_proxy);
>
>
> - mono_mb_emit_ldloc (mb, 1);
> - mono_mb_emit_ldloc (mb, real_proxy);
> + //mono_mb_emit_ldloc (mb, 1);
> + //mono_mb_emit_ldloc (mb, real_proxy);
Why is this code commented out?
> +GHashTable* ccw_hash = NULL;
> +GHashTable* ccw_entry_hash = NULL;
These must be static. Also, document with a comment what the key and
value types are for each hashtable.
> +/**
> + * cominterop_get_ccw:
> + * @object: a pointer to the object
> + * @itf: interface type needed
> + *
> + * Returns: a value indicating if the object is a
> + * Runtime Callable Wrapper (RCW) for a COM object
> + */
> +static gpointer
> +cominterop_get_ccw (MonoObject* object, MonoClass* itf)
> +{
> + int i;
> + MonoCCW *ccw = NULL;
> + MonoCCWEntry* ccw_entry = NULL;
> + gpointer *vtable = NULL;
> + static gpointer iunknown[3] = {NULL, NULL, NULL};
> + static gpointer idispatch[4] = {NULL, NULL, NULL, NULL};
> + MonoClass* iface = NULL;
> + MonoClass* klass = NULL;
> + EmitMarshalContext m;
> + int start_slot = 3;
> + int method_count = 0;
> +
> + if (!object)
> + return NULL;
> +
> + klass = mono_object_get_class (object);
> +
> + if (!ccw_hash)
> + ccw_hash = g_hash_table_new (mono_aligned_addr_hash, NULL);
> + if (!ccw_entry_hash)
> + ccw_entry_hash = g_hash_table_new (mono_aligned_addr_hash, NULL);
> +
> + ccw = g_hash_table_lookup (ccw_hash, object);
> +
> + if (!iunknown[0]) {
> + iunknown[0] = cominterop_ccw_queryinterface;
> + iunknown[1] = cominterop_ccw_addref;
> + iunknown[2] = cominterop_ccw_release;
> + }
> +
> + if (!idispatch[0]) {
> + idispatch[0] = cominterop_ccw_get_type_info_count;
> + idispatch[1] = cominterop_ccw_get_type_info;
> + idispatch[2] = cominterop_ccw_get_ids_of_names;
> + idispatch[3] = cominterop_ccw_invoke;
> + }
> +
> + if (!ccw) {
> + ccw = g_new0 (MonoCCW, 1);
> + ccw->vtable_hash = g_hash_table_new (mono_aligned_addr_hash, NULL);
> + ccw->ref_count = 0;
> + ccw->object = object;
> + g_hash_table_insert (ccw_hash, object, ccw);
You can't insert a managed object inside a GHashTable, since objects
can move and the GC can't see inside GHashTable, you'll get a broken
pointer and a crash. If the hash must not keep alive the object and you
used a GHashTable as a way to implement weak references, you must use
the runtime support for weak references instead. Also, remember that for
object hashes you must use the mono_object_hash() function.
> + // register for finalization to clean up ccw
> + mono_object_register_finalizer (object);
> + }
> +
> + iface = itf;
> + if (iface == mono_defaults.iunknown_class) {
> + start_slot = 3;
> + }
> + else if (iface == mono_defaults.idispatch_class) {
> + start_slot = 7;
> + }
> + else {
> + while (iface) {
> + method_count += iface->method.count;
> + if (iface->interface_count) {
> + iface = iface->interfaces[0];
> + }
> + else {
> + start_slot = cominterop_get_com_slot_begin (iface);
> + iface = NULL;
> + }
> + }
> + }
> +
> + ccw_entry = g_hash_table_lookup (ccw->vtable_hash, itf);
> +
> + if (!ccw_entry) {
> + int vtable_index = method_count-1+start_slot;
> +
> + vtable = mono_mempool_alloc0 (klass->image->mempool, sizeof (gpointer)*(method_count+start_slot));
You need to take the loader lock to allocate from image->mempool.
> + memcpy(vtable, iunknown, sizeof(iunknown));
> + if (start_slot == 7)
> + memcpy(vtable+3, idispatch, sizeof(idispatch));
> +
> + iface = itf;
> + while (iface) {
> + for (i = iface->method.count-1; i >= 0;i--) {
> + int param_index = 0;
> + MonoMethodBuilder *mb;
> + MonoMarshalSpec ** mspecs;
> + MonoMethod *wrapper_method, *adjust_method;
> + MonoMethod *method = iface->methods[i];
> + MonoMethodSignature* sig_adjusted;
> + MonoMethodSignature* sig = mono_method_signature (method);
> +
> +
> + mb = mono_mb_new (iface, method->name, MONO_WRAPPER_CCW);
> + adjust_method = cominterop_get_managed_wrapper_adjusted (method);
> + sig_adjusted = mono_method_signature (adjust_method);
> +
> + mspecs = g_new (MonoMarshalSpec*, sig_adjusted->param_count + 1);
> + mono_method_get_marshal_info (method, mspecs);
> +
> +
> + // move managed args up one
> + for (param_index = sig->param_count; param_index >= 1; param_index--)
> + mspecs[param_index+1] = mspecs[param_index];
Here and in the other places, put a space before the [ of an array
access.
> +
> + // first arg is IntPtr for interface
> + mspecs[1] = NULL;
> +
> + // move return spec to last param
> + if (!MONO_TYPE_IS_VOID (sig->ret))
> + mspecs[sig_adjusted->param_count] = mspecs[0];
> +
> + mspecs[0] = NULL;
> +
> + cominterop_setup_marshal_context (&m, adjust_method);
> + m.mb = mb;
> + mono_marshal_emit_managed_wrapper (mb, sig_adjusted, mspecs, &m, adjust_method, NULL);
> + mono_loader_lock ();
> + mono_marshal_lock ();
> + wrapper_method = mono_mb_create_method (mb, sig_adjusted, sig_adjusted->param_count + 16);
> + mono_marshal_unlock ();
> + mono_loader_unlock ();
> + vtable[vtable_index--] = mono_compile_method (wrapper_method);
Do you really need the methods to be compiled right away?
It would be good to delay compilation to when the methods are actually
used.
> + for (param_index = sig_adjusted->param_count; param_index >= 0; param_index--)
> + if (mspecs [param_index])
> + mono_metadata_free_marshal_spec (mspecs [param_index]);
> + g_free (mspecs);
> + }
> + if (iface->interface_count)
> + iface = iface->interfaces[0];
> + else
> + iface = NULL;
> + }
> +
> + ccw_entry = g_new0 (MonoCCWEntry, 1);
> + ccw_entry->ccw = ccw;
> + ccw_entry->vtable = vtable;
> + g_hash_table_insert (ccw->vtable_hash, itf, ccw_entry);
> + g_hash_table_insert (ccw_entry_hash, ccw_entry, ccw);
> + }
> +
> + return ccw_entry;
> +}
> +
> +
> +
> +static gboolean
> +mono_marshal_free_ccw_entry (gpointer key, gpointer value, gpointer user_data)
> +{
> + MonoCCWEntry* entry = (MonoCCWEntry*)value;
No need to use a cast here.
> +gboolean
> +mono_marshal_free_ccw (MonoObject* object)
> +{
> + MonoCCW* ccw = NULL;
> + // no ccw's were created
> + if (!ccw_hash)
> + return FALSE;
> +
You might want to add also a check for g_hash_table_size () == 0 here.
> +static int STDCALL
> +cominterop_ccw_addref (MonoCCWEntry* ccwe)
> +{
> + MonoCCW* ccw = ccwe->ccw;
> + g_assert (ccw);
> + g_assert (ccw->object);
> + g_assert (ccw->ref_count >= 0);
> + ccw->ref_count++;
> + if (ccw->ref_count == 1) {
> + /* since we now have a ref count, alloc a handle*/
> + ccw->gc_handle = mono_gchandle_new (ccw->object, FALSE);
> + }
Can you explain this MonoCCW data structure here? It has a pointer
to a reference which isn't GC-tracked and so it's bad. It looks to be
created with a 0 refcount and an handle is created for the object, so
there doesn't seem to be any need to store also an object in it.
Also, the reference count increase is done in a thread-unsafe way.
> + return ccw->ref_count;
> +}
> +
> +static int STDCALL
> +cominterop_ccw_release (MonoCCWEntry* ccwe)
> +{
> + MonoCCW* ccw = ccwe->ccw;
> + g_assert (ccw);
> + g_assert (ccw->object);
> + g_assert (ccw->ref_count > 0);
> + ccw->ref_count--;
> + if (ccw->ref_count == 0) {
> + /* allow gc of object */
> + guint32 handle = ccw->gc_handle;
> + g_assert (handle);
> + ccw->gc_handle = 0;
> + mono_gchandle_free (handle);
Why doesn't this clear ccw->object?
> +static int STDCALL
> +cominterop_ccw_get_type_info_count (MonoCCWEntry* ccwe, guint32 *pctinfo)
> +{
> + if (pctinfo)
> + *pctinfo = 1;
> + return 0x80004001L;
Better use a named define here for clarity of what the number means.
> Index: mono/mono/metadata/gc.c
> ===================================================================
> --- mono/mono/metadata/gc.c (revision 68741)
> +++ mono/mono/metadata/gc.c (working copy)
> @@ -61,6 +61,7 @@
> {
> MonoObject *exc = NULL;
> MonoObject *o, *o2;
> + MonoMethod* finalizer = NULL;
> o = (MonoObject*)((char*)obj + GPOINTER_TO_UINT (data));
>
> #ifndef HAVE_SGEN_GC
> @@ -100,8 +101,15 @@
> return;
> }
>
> - mono_runtime_invoke (mono_class_get_finalizer (o->vtable->klass), o, NULL, &exc);
> + finalizer = mono_class_get_finalizer (o->vtable->klass);
>
> + /* if we area ccw, and have no finalizer method just return
> + * else run regular finalizer after freeing ccw */
> + if (mono_marshal_free_ccw (o) && !finalizer)
> + return;
So what happens if the object has a finalizer and wants to use COM
features in it? Is that not possible? We need a comment here.
You may also want to check what happens with resurrection (ie the
objects stores itself in a static field inside the finalizer).
Thanks!
lupus
--
-----------------------------------------------------------------
lupus at debian.org debian/rules
lupus at ximian.com Monkeys do it better
More information about the Mono-devel-list
mailing list