[Mono-dev] COM Callable Wrapper Patch
Jon Chambers
joncham at gmail.com
Mon Dec 4 19:42:37 EST 2006
Paolo,
Thanks for reviewing the changes. I've addressed most of the issues
you have brought up below. See comments inline and new patch.
On 12/4/06, Paolo Molaro <lupus at ximian.com> wrote:
>
> 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.
Fixed.
> 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.
The are lazily initialized now.
> 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?
Removed.
> +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.
Static and now documented.
> +/**
> > + * 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.
The object is not put in hashtable, but just used as the key in this case. I
changed the hash table to use mono_object_hash() as the hash function where
the object is the key. I've removed the MonoObject* field from the MonoCCW
struct and just look it up using the gc_handle when needed instead.
> + // 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.
I've added the loader lock.
> + 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.
I've put in spaces in my uses of array accessors.
> +
> > + // 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.
These are unmanaged->managed transition wrappers, so I compile these to get
the function pointers to fill the COM vtables.
> + 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.
Removed cast.
> +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.
Added check.
> +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.
I've removed the object reference. I only allocate a weak handle and set the
reference count to 0. If the unmanaged client code decides to addref and
hold onto the CCW, I then allocate a strong handle. Once the reference count
goes back to 0 I go back to a weak handle.
> + 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?
Object reference removed.
> +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.
Added named defines for basic error codes.
> 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).
Added better comment. If an object has a CCW associated with it and has no
finalizer we can safely return after destroying the CCW since we are only
being finalized since COM Interop registered the object for finalization to
clean up the CCW. In all other cases we continue on (including the case you
brought up where an object has a finalizer and has been exposed to unmanaged
code via a CCW) to call the objects finalizer. I've added a FIXME for
resurrection, and also if the user calls SuppressFinalize the CCW will leak
currently. I see what is done for delegates in that case, but this is a bit
different. I currently have no way to determine that an object with a
finalizer needs it's CCW destroyed but don't need it's finalizer run.
Thanks!
>
> lupus
>
> --
> -----------------------------------------------------------------
> lupus at debian.org debian/rules
> lupus at ximian.com Monkeys do it better
> _______________________________________________
> Mono-devel-list mailing list
> Mono-devel-list at lists.ximian.com
> http://lists.ximian.com/mailman/listinfo/mono-devel-list
Also, there are some places I know I need to make threadsafe. Should I
create a new critical section, or just use the marshal one?
Thanks,
Jonathan
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.ximian.com/pipermail/mono-devel-list/attachments/20061204/5d912659/attachment.html
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ccw11.diff
Type: text/x-patch
Size: 60864 bytes
Desc: not available
Url : http://lists.ximian.com/pipermail/mono-devel-list/attachments/20061204/5d912659/attachment.bin
More information about the Mono-devel-list
mailing list