[Mono-dev] New code to build interface vtables

[Zoltan Varga]

Hi,

  I think it would be useful to leave the old code for a while, better
yet control it with
an env var so we can tell people using packages to set the env var
when debugging
a bug.

         Zoltan

On Feb 4, 2008 5:39 PM, Massimiliano Mantione <massi at ximian.com> wrote:
>
> On Mon, 2008-02-04 at 15:22 +0100, Paolo Molaro wrote:
> > Did you run the corlib tests, too?
>
> I run "make check" in mono, which runs just about everything,
> also after a full rebuild (done with the new code).
>
> > > +           // Check if this interface is explicitly implemented (instead of just inherited)
> > > +           if (parent != NULL) {
> > > +                   int implemented_interfaces_index;
> > > +                   interface_is_explicitly_implemented_by_class = FALSE;
> > > +                   for (implemented_interfaces_index = 0; implemented_interfaces_index < class->interface_count; implemented_interfaces_index++) {
> > > +                           if (ic == class->interfaces [implemented_interfaces_index]) {
> > > +                                   interface_is_explicitly_implemented_by_class = TRUE;
> > > +                                   break;
> > > +                           }
> > > +                   }
> >
> > You likely need to loop other all the hierarchy here, right? Because
> > this is not restricted to just the immediate parent.
>
> Well, in section 2, 12.2, the standard says "If this class explicitly
> specifies that it implements the interface (i.e., the interfaces that
> appear in this class's InterfaceImpl table, §22.23)".
> In the code, "interface_is_explicitly_implemented_by_class" wants to
> flag this condition, so I only look in "class->interfaces".
>
> > > +                                           /* Why do we need this? */
> > > +                                           if (vtable [im_slot]->slot < 0) {
> > > +                                                   vtable [im_slot]->slot = im_slot;
> > > +                                           }
> >
> > It's much simpler to use:
> >       if (cm->slot < 0)
> >               cm->slot = im_slot;
>
> Yes, right :-)
>
> > > +                           // If the slot is still empty, look in all the inherited virtual methods...
> > > +                           if ((vtable [im_slot] == NULL) && class->parent != NULL) {
> > > +                                   MonoClass *parent = class->parent;
> >
> > I think you need to loop over all the parents here. Please write the
> > appropriate test cases where the interface method is implemented by
> > a non-immediate parent.
> >
> > > +                                   // Reverse order, so that last added methods are preferred
> > > +                                   for (cm_index = parent->vtable_size - 1; cm_index >= 0; cm_index--) {
> > > +                                           MonoMethod *cm = parent->vtable [cm_index];
>
> Always in section 2, 12.2, the standard says "If there are any virtual
> methods in the interface that still have empty slots, see if there are
> any public virtual methods, but not public virtual newslot methods,
> available on this class (directly or inherited)...".
> My interpretation is that since we are looking for virtual methods they
> must be in the vtable, and "parent->vtable" contains all of them (also
> the inherited ones), so one single loop is OK.
> The methods of "class" are already covered by the previous loop.
>
> Just to be on the safe side, I modified iface4.cs to test this as
> well (maybe there was already a test like that somewhere, but it was
> easier adding it anyway), and it passes.
>
> > > +                           if (vtable [im_slot] == NULL) {
> > > +                                   int index;
> > > +                                   char *method_signature;
> > > +                                   for (index = 0; index < onum; ++index) {
> > > +                                           g_print (" at slot %d: %s (%d) overrides %s (%d)\n", im_slot, overrides [index*2+1]->name,
> > > +                                                    overrides [index*2+1]->slot, overrides [index*2]->name, overrides [index*2]->slot);
> > > +                                   }
> > > +                                   method_signature = mono_signature_get_desc (mono_method_signature (im), FALSE);
> > > +                                   printf ("no implementation for interface method %s::%s(%s) in class %s.%s\n",
> > > +                                           mono_type_get_name (&ic->byval_arg), im->name, method_signature, class->name_space, class->name);
> > > +                                   g_free (method_signature);
> > > +                                   for (index = 0; index < class->method.count; ++index) {
> > > +                                           MonoMethod *cm = class->methods [index];
> > > +                                           method_signature = mono_signature_get_desc (mono_method_signature (cm), TRUE);
> > > +
> > > +                                           printf ("METHOD %s(%s)\n", cm->name, method_signature);
> > > +                                           g_free (method_signature);
> > > +                                   }
> >
> > Move this debugging code to it's own external function so it doesn't
> > clutter the code.
>
> OK.
>
> The last question: should I leave
>
> #define USE_NEW_INTERFACE_VTABLE_CODE 1
>
> in place (with the old code) for a few days after committing (just in
> case, for debugging), or should I commit it directly clean?
>
> Thanks a lot!
>   Massi
>
>
>
> _______________________________________________
> Mono-devel-list mailing list
> Mono-devel-list at lists.ximian.com
> http://lists.ximian.com/mailman/listinfo/mono-devel-list
>