[Mono-dev] COM Callable Wrapper Patch
Jon Chambers
joncham at gmail.com
Wed Feb 7 11:36:37 EST 2007
Hello,
Here is another attempt at this patch. It is a large amount of changes, but
the good news is that this largely supports COM Callable Wrappers. So, the
functionality here provides ~60% of the COM support in the CLR and ~90% of
the COM support used by most applications. If I need to break this patch
down (although alot of the changes are atomic) or change some things let me
know.
The only additional change from previous patches that would affect code
outside of COM Interop uses is the fact that I cause wrapper methods to have
local variables intialized. I was marshalling a local struct variable and
ran into it being not initialized. If this is a problem, I can work around
it.
This code is contributed under the MIT/X11 license, and I signed off in the
ChangeLogs within the runtime code.
Thanks,
Jonathan
On 12/5/06, Paolo Molaro <lupus at ximian.com> wrote:
>
> On 12/04/06 Jon Chambers wrote:
> > >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.
>
> The key can change if it is an object address, so you may end up not
> finding the value from a previous object or finding it for an object
> that wasn't registered yet.
>
> > >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.
>
> Ok, please add the description as a comment.
>
> > >> + 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.
>
> For delegates we just check and return in SuppressFinalize.
> You could add a similar check there with the usual fast path for when no
> com support has been used.
>
> > 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?
>
> Yes, please.
>
> > Index: mono/mono/metadata/marshal.c
> > ===================================================================
> > --- mono/mono/metadata/marshal.c (revision 68977)
> > +++ mono/mono/metadata/marshal.c (working copy)
> > @@ -1823,18 +1904,31 @@
> > @@ -2102,20 +2196,20 @@
> > case MONO_MARSHAL_CONV_OBJECT_INTERFACE:
> > case MONO_MARSHAL_CONV_OBJECT_IDISPATCH:
> > case MONO_MARSHAL_CONV_OBJECT_IUNKNOWN: {
> > - guint32 pos_failed = 0, pos_rcw = 0;
> > - char * msg;
> > + guint32 pos_null = 0, pos_rcw = 0, pos_end = 0;
> >
> > + /* COM types are initialized lazily */
> > + mono_init_com_types ();
> > +
> > mono_mb_emit_ldloc (mb, 1);
> > - //mono_mb_emit_ldloc (mb, 0);
> > - mono_mb_emit_ptr (mb, 0);
> > - //mono_mb_emit_byte (mb, CEE_LDIND_U1);
> > + mono_mb_emit_byte (mb, CEE_LDNULL);
> > mono_mb_emit_byte (mb, CEE_STIND_I);
>
> ldnull loads an object and an object can't be stored with stind.i: if
> you really need to store a NULL pointer (and not a null reference), load
> a 0 constant and convert it with conv.u. Otherwise use stind.ref to
> store a reference.
>
> > @@ -3295,6 +3413,20 @@
> > return res;
> > }
> >
> > +/* Maps a managed object to its unmanaged representation
> > + * i.e. it's COM Callable Wrapper (CCW).
> > + * Key: MonoObject*
> > + * Value: MonoCCW*
> > + */
> > +static GHashTable* ccw_hash = NULL;
>
> As explained above, you can't really use an object address as the key,
> since it changes over time. One possible implementation idea: use
> mono_object_hash() as the key and a list as the value: you then walk the
> list and get the object to compare from the GC handle. There may be also
> faster ways for sure and if this is accessed during finalizations we
> should check that the Boehm GC doesn't null the weak refs before running
> the finalizers (but I guess that if you find a null target in the gc
> handle you can free the ccw struct right away...)
>
> > + if (!ccw_entry) {
> > + int vtable_index = method_count-1+start_slot;
> > + mono_loader_lock ();
> > + vtable = mono_mempool_alloc0 (klass->image->mempool,
> sizeof (gpointer)*(method_count+start_slot));
> > + mono_loader_unlock ();
> > + memcpy(vtable, iunknown, sizeof(iunknown));
> > + if (start_slot == 7)
> > + memcpy(vtable+3, idispatch, sizeof(idispatch));
>
> Put a space before the opening parenthesis of a function call.
>
> > +static gboolean
> > +cominterop_class_guid_equal (gpointer guid, MonoClass* klass)
> > +{
> > + static MonoClass *GuidAttribute = NULL;
> > + MonoCustomAttrInfo *cinfo;
> > + MonoReflectionGuidAttribute *attr;
> > + MonoString *guid_string = mono_string_new (mono_domain_get (),
> mono_guid_to_string ((guint8*)guid));
>
> If guid is a guint8*, change the type from gpointer to it wherever it is
> used.
>
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.ximian.com/pipermail/mono-devel-list/attachments/20070207/1451b8db/attachment.html
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch3.diff
Type: text/x-patch
Size: 102134 bytes
Desc: not available
Url : http://lists.ximian.com/pipermail/mono-devel-list/attachments/20070207/1451b8db/attachment.bin
More information about the Mono-devel-list
mailing list