[Mono-dev] New code to build interface vtables

[Massimiliano Mantione]

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