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

[Juraj Skripsky]

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