[Mono-dev] [PATCH] Handle more gracefully invalid generic instantiations

[Paolo Molaro]

On 06/26/09 Rodrigo Kumpera wrote:
> The attached patch changes class.c/inflate_generic_type to not abort the
> runtime when facing a bad instantiation.
> 
> My only issue is that I'm not sure if mono_class_inflate_generic_type* and
> mono_class_inflate_generic_class should
> set a loader error when returning null. I don't think this is necessary as
> this is something their callers should do. But maybe
> mono_class_inflate_generic_method_full should.

In general only the low-level code knows the reason for the failure, so
the low-level code should propagate this info (and the exact reason for
the failure is often useful when debugging). Note how instead your patch
removes some potentially useful info.
For that reason I'm not much a fan of having functions return a boolean,
because that gives very little information.
On the other hand, using something like GError, with it's forced
malloc() usage may not be an appropriate pattern for use in the whole
runtime.

A compromise suggestion could be something like this:
	typedef struct {
		unsigned short error_code; // this can be MONO_EXCEPTION_*
		unsigned short flags;
		void *data0; // same as exception_data in MonoClass or other fields in MonoLoaderError
		void *data1;
		void *data2;
		void *datan;
		char message [128];
	} MonoError;

A few considerations:
*) the struct would be passed by ref as the last argument of the runtime
functions
*) it can be stack allocated, so the malloc is avoided and it can be
used also in signals etc. This means that message should be kept small,
no more than 256, though.
*) eventually it should be also the mechanism for reporting errors with
the embedding interface, so there are a few dummy fields for future
expansion, but the internals should be considered private and once
published the struct won't be changed.
*) setting the error should be done with a few simple functions like:

	void mono_error_set_ok        (MonoError *error); // no error
	void mono_error_set_bad_image (MonoError *error, MonoImage *image, char *msg_format, ...);
	etc.
*) I think it would be a good idea to always require the error
argument to be non-null.
*) error checking would be done with the (macro equivalent) of:
	gboolean mono_error_ok (MonoError *error);

See below how the first snippet of code would become.

> --- a/mono/metadata/class.c
> +++ b/mono/metadata/class.c
> @@ -490,20 +490,24 @@ mono_class_is_open_constructed_type (MonoType *t)
>  	}
>  }
>  
> -static MonoType*
> -inflate_generic_type (MonoImage *image, MonoType *type, MonoGenericContext *context)
> +/*
> + * This function returns TRUE if it could proper inflate @type.
> + * The resulting type can be found in @res  
> + */
> +static gboolean
> +inflate_generic_type (MonoImage *image, MonoType *type, MonoGenericContext *context, MonoType **res)
>  {
> +#define SUCCESS(VAL) do { *res = VAL; return TRUE; } while (0)
> +#define ERROR() do { *res = NULL; return FALSE; } while (0)
>  	switch (type->type) {
>  	case MONO_TYPE_MVAR: {
>  		MonoType *nt;
>  		int num = mono_type_get_generic_param_num (type);
>  		MonoGenericInst *inst = context->method_inst;
>  		if (!inst || !inst->type_argv)
> -			return NULL;
> +			SUCCESS (NULL);
>  		if (num >= inst->type_argc)
> -			g_error ("MVAR %d (%s) cannot be expanded in this context with %d instantiations",
> -				num, mono_generic_param_info (type->data.generic_param)->name, inst->type_argc);
> -
> +			ERROR ();

static MonoType*
inflate_generic_type (MonoImage *image, MonoType *type, MonoGenericContext *context, MonoError *error)
{
	mono_error_set_ok (error);
	switch (type->type) {
	case MONO_TYPE_MVAR: {
		MonoType *nt;
		int num = mono_type_get_generic_param_num (type);
		MonoGenericInst *inst = context->method_inst;
		if (!inst || !inst->type_argv)
			return NULL;
		if (num >= inst->type_argc)
			mono_error_set_bad_image (error, image, "MVAR %d (%s) cannot be expanded in this context with %d instantiations",
				num, mono_generic_param_info (type->data.generic_param)->name, inst->type_argc);
			return NULL;
...

Comments welcome (also by future users of the embedding interface).

lupus

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