[Mono-dev] [PATCH] Generic Type Definition / Open Instantiation mismatch patch.

Tue Sep 25 08:15:07 EDT 2007

Hi Juraj,

Thanks for pointing out the double "if()", I I did some locking cleanup and
all paths were called with the loader lock held, so no need to lock and
re-check.

Cheers,
Rodrigo

On 9/25/07, Juraj Skripsky <js at hotfeet.ch> wrote:
>
> Hi Rodrigo,
>
> Thank for working on this! I wasn't aware of this bug before and it bit
> me a couple of times (with me slowly growing desperate) when using Mono
> with Db4o and generic collections.
>
> I've applied it to my local tree and it works fine so far. I'll try to
> do some more testing and will keep you posted about any problems I run
> into.
>
> Just a cosmetic detail - you are a duplicated if() block in
> mono_class_setup_generic_type_container():
>
> +       if (container->gtd_class)
> +               return;
> +
> +
> +       if (container->gtd_class) {
> +               return;
> +       }
>
> - Juraj
>
>
> On Fri, 2007-09-21 at 12:22 -0300, Rodrigo Kumpera wrote:
> > Hey guys,
> >
> > The patch if finally done and it works perfectly well against head.
> > This patch turned to be very different from what it started to look
> > like and fortunately this meant a reduction of more than an order of
> > magnitude in its size, from 3.000 lines to a bit more than 230.
> >
> > To have the patch make sense I need to first define what it fixes.
> > Right now mono treat the generic type definition and the open
> > instantiation over its own parameters (or just open instantiation from
> > now on) as two different classes. For example, given the type "class
> > Example<T,M>", the generic type definition is "Example`2" and the open
> > instantiation is "Example`2<T,M>" where T and M are the parameters of
> > Example`2. If that doesn't make much sense, the following code might
> > help:
> >
> > Type gtd = typeof (Example<,>);
> > Type openInst = gtd.MakeGenericType (gtd.GetGenericArguments
> > ()); //openInst and gtd should be the same instance
> >
> > This property is very important for all parts of mono that play with
> > raw (non inflated) generic code. Analyzing the IL of generic code
> > ilustrate this issue well better:
> >
> > .class public A`1<T> extends [mscorlib]System.Object
> > {
> >     .field  private  !0 fld
> >
> >     .method public instance void Example ()
> >     {
> >         .maxstack 2
> >         .locals init (
> >             !T      V_0)
> >         ldarg.0
> >         ldfld !0 class A`1<!0>::fld
> >         stloc.0
> >         ret
> >     }
> > }
> >
> > The current situation is that the non-inflated method Example will be
> > bound to a different MonoClass instance than the token of ldfld. Which
> > will lead the runtime to view it as an error. Right now only the
> > verifier is affected by this situation but I think the generics code
> > sharing work might profit from the fix too.
> >
> > The original idea of how to fix it was to change the this_arg and
> > byref_arg of the MonoClass struct to have type MONO_TYPE_GENERICINST.
> > With that set, when a generic type was initialized, the open
> > instantation MonoGenericClass would be bound to it. This works quite
> > well except that it introduced a lot of issues. The main one that the
> > container_type of generic instantiations would no longer be a
> > MONO_TYPE_CLASS.
> >
> > First all code than handled MONO_TYPE_GENERICINST need to be changed
> > to couple with this fact, this required changes across most of
> > metadata and even in mini.c, some places expect a MONO_TYPE_CLASS and
> > don't even guard the retrieval from the data union of MonoType. This
> > led to many hard to spot bugs.
> >
> > The last issue was with TypeBuilders, the way they are currently set,
> > the associated MonoClass cannot be a MONO_TYPE_GENERICINST until
> > CreateType () is called. Changing (this|byval)_arg.type during
> > mono_reflection_create_runtime_class turned to be a bad idea, as it
> > breaks the invariant that once the MonoClass is first initialized it
> > won't change (this|byval)_arg.type in such drastic way. I had to patch
> > too many places trying to get around this issue.
> >
> > The bottom line is that this approach won't work and the resulting
> > patch would introduce a big chunk of new bugs. In the other hand, the
> > runtime seens to be fine with breaking another premise, that all
> > MonoClass instances bould to MonoGenericClass have type
> > MONO_TYPE_GENERICINST.
> >
> > This is how it works: when the generic type definition is initialized
> > at the end of either mono_reflection_create_runtime_class or
> > mono_class_setup_mono_type, we lookup the open instantiation and bind
> > it - IOW we set MonoGenericClass::cached_class to the
> > MonoClass instance of the generic type definition.
> >
> > There is one finall twist on this patch, the way
> > System.Reflection.Emit in mono is incompatible with .net. On mono,
> > TypeBuilder::MakeGenericType returns a Type that is functional (one
> > you can call GetConstructors). This means that you cannot have the
> > same behavior that happens with types loaded from an assembly. To get
> > arround this fact, either I could either "fix" gmcs to work with the
> > crippled .net SRE API, or special case for TypeBuilder. I have choosen
> > the later one as it makes more sense.
> >
> > This mean that in mono_metadata_lookup_generic_class we check if we
> > are looking up the open instantiation of a TypeBuilder and return a
> > MonoGenericClass than won't be the same for the created type.
> >
> > What could receive some love in the patch is the way the check for the
> > TypeBuilder is done could be optimized by doing pre-calculating it in
> > mono_metadata_get_generic_inst. This could be done if you guys find
> > that's a good idea, I'm just not sure if it's work the trouble. Right
> > now the tests are using NUnit, maybe some should be moved to the
> > regression suite.
> >
> >
> > Rodrigo
> >
> > _______________________________________________
> > 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/20070925/06aa52f6/attachment.html 