[Mono-dev] New code to build interface vtables

[Paolo Molaro]

On 01/29/08 Massimiliano Mantione wrote:
> Anyway, the patch passes everything here.

Did you run the corlib tests, too?

> --- mono/metadata/class.c	(revision 94247)
> +++ mono/metadata/class.c	(working copy)
> +#if USE_NEW_INTERFACE_VTABLE_CODE
> +	// Loop on all implemented interfaces...
> +	for (i = 0; i < class->interface_offsets_count; i++) {
> +		MonoClass *parent = class->parent;
> +		int ic_offset;
> +		gboolean interface_is_explicitly_implemented_by_class;
> +		int im_index;
> +		
> +		ic = class->interfaces_packed [i];
> +		ic_offset = mono_class_interface_offset (class, ic);
> +		
> +		// 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.

> +		} else {
> +			interface_is_explicitly_implemented_by_class = TRUE;
> +		}
> +		
> +		// Loop on all interface methods...
> +		for (im_index = 0; im_index < ic->method.count; im_index++) {
> +			MonoMethod *im = ic->methods [im_index];
> +			int im_slot = ic_offset + im->slot;
> +			MonoMethod *override_im = (override_map != NULL) ? g_hash_table_lookup (override_map, im) : NULL;
> +			
> +			// If there is an explicit implementation, just use it right away,
> +			// otherwise look for a matching method
> +			if (override_im == NULL) {
> +				int cm_index;
> +				
> +				// First look for a suitable method among the class methods
> +				for (cm_index = 0; cm_index < class->method.count; cm_index++) {
> +					MonoMethod *cm = class->methods [cm_index];
> +					
> +					TRACE_INTERFACE_VTABLE (printf ("    For slot %d ('%s'.'%s':'%s'), trying method '%s'.'%s':'%s'... [EXPLICIT IMPLEMENTATION = %d][SLOT IS NULL = %d]", im_slot, ic->name_space, ic->name, im->name, cm->klass->name_space, cm->klass->name, cm->name, interface_is_explicitly_implemented_by_class, (vtable [im_slot] == NULL)));
> +					if ((cm->flags & METHOD_ATTRIBUTE_VIRTUAL) && check_interface_method_override (class, im, cm, TRUE, interface_is_explicitly_implemented_by_class, (vtable [im_slot] == NULL), security_enabled)) {
> +						TRACE_INTERFACE_VTABLE (printf ("[check ok]: ASSIGNING"));
> +						vtable [im_slot] = cm;
> +						/* 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;

> +					}
> +					TRACE_INTERFACE_VTABLE (printf ("\n"));
> +				}
> +				
> +				// 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];
> +						
> +						TRACE_INTERFACE_VTABLE ((cm != NULL) && printf ("    For slot %d ('%s'.'%s':'%s'), trying (ancestor) method '%s'.'%s':'%s'... ", im_slot, ic->name_space, ic->name, im->name, cm->klass->name_space, cm->klass->name, cm->name));
> +						if ((cm != NULL) && check_interface_method_override (class, im, cm, FALSE, FALSE, TRUE, security_enabled)) {
> +							TRACE_INTERFACE_VTABLE (printf ("[everything ok]: ASSIGNING"));
> +							vtable [im_slot] = cm;
> +							/* Why do we need this? */
> +							if (vtable [im_slot]->slot < 0) {
> +								vtable [im_slot]->slot = im_slot;
> +							}
> +							break;
> +						}
> +						TRACE_INTERFACE_VTABLE ((cm != NULL) && printf ("\n"));
> +					}
> +				}
> +			} else {
> +				g_assert (vtable [im_slot] == override_im);
> +			}
> +		}
> +	}
> +	
> +	// If the class is not abstract, check that all its interface slots are full.
> +	// The check is done here and not directly at the end of the loop above because
> +	// it can happen (for injected generic array interfaces) that the same slot is
> +	// processed multiple times (those interfaces have overlapping slots), and it
> +	// will not always be the first pass the one that fills the slot.
> +	if (! (class->flags & TYPE_ATTRIBUTE_ABSTRACT)) {
> +		for (i = 0; i < class->interface_offsets_count; i++) {
> +			int ic_offset;
> +			int im_index;
> +			
> +			ic = class->interfaces_packed [i];
> +			ic_offset = mono_class_interface_offset (class, ic);
> +			
> +			for (im_index = 0; im_index < ic->method.count; im_index++) {
> +				MonoMethod *im = ic->methods [im_index];
> +				int im_slot = ic_offset + im->slot;
> +				
> +				TRACE_INTERFACE_VTABLE (printf ("      [class is not abstract, checking slot %d for interface '%s'.'%s', method %s, slot check is %d]\n",
> +						im_slot, ic->name_space, ic->name, im->name, (vtable [im_slot] == NULL)));
> +				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.
Please make the additional tests I suggested above and adjust the code
if needed.
This looks good, thanks!

lupus

-- 
-----------------------------------------------------------------
lupus at debian.org                                     debian/rules
lupus at ximian.com                             Monkeys do it better