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

[Rodrigo Kumpera]

We could use some macros to make it easier to deal with error checking
instead of doing
a "if (!mono_error_ok (error)) goto handle_failure;" after every method call
that can fail.

We came up with two suggestions on irc:

#define check_ok() do { if (!mono_error_ok (error)) goto handle_failure;}
while (0)
and
#define check_error error); if (!mono_error_ok (error)) goto handle_failure;
do {} while (0)

The second one is a bit more magic, but results in code much more readable,
one
just has do call a method passing it as an argument: "do_foo (a, b,
check_error);".

I would go with either macro, both make code easier to read than having tons
of ifs'
everywhere.

Besides that, having mono_error_set_ok assert if an error has already
signaled would
reduce one of our current problems, which is error leakage.

Rodrigo


On Fri, Jun 26, 2009 at 1:35 PM, Paolo Molaro <lupus at ximian.com> wrote:

> 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
> _______________________________________________
> Mono-devel-list mailing list
> Mono-devel-list at lists.ximian.com
> http://lists.ximian.com/mailman/listinfo/mono-devel-list
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.ximian.com/pipermail/mono-devel-list/attachments/20090626/7fd7f6de/attachment-0001.html