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

[Rodrigo Kumpera]

Hi,

On Sat, Jun 27, 2009 at 2:49 AM, Raja R Harinath <harinath at hurrynot.org>wrote:

> Hi,
>
> Rodrigo Kumpera <kumpera at gmail.com> writes:
>
> > 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.
> >
> > This patch doesn't fix the whole thing as an audit of all callers of
> mono_class_inflate_generic_type(_with_mempool | _no_copy)
> > and mono_class_inflate_generic_class are required to property handle them
> returning NULL.
> >
> > I got some local tests for these errors, but they are not in good shape
> for been part of this patch.
>
> [snip]
>
> > +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 ();
>
> Hmm, so much machinery for the one use of ERROR!
>
> I think the issue is that we're forced to intertwine this particular
> error check in the middle of code dealing with the mechanics of
> inflating.  It'd be much nicer for inflate_generic_type to have the
> precondition that no VAR or MVAR in 'type' will be out-of-bounds WRT
> 'context'.
>
> The problem is that this precondition check is currently expensive, as
> it would duplicate the same recursive traversal.  However, we _can_ and
> _should_ make it non-recursive -- we can replace the field and
> computation of MonoGenericInst::is_open with something like
> MonoGenericInst::min_context_size (yeah that name is horribly bad.  I've
> been putting of writing that code since I couldn't get a better name).
>
> The reason I think this is better is that we already have code in JIT
> and the verifier that need only that information (i.e. are equivalent to
> that precondition check), but are forced to walk the MonoType tree
> because 'is_open' is too limited.
>
> - Hari
>


Hari, I fail to see how changing MonoGenericInst::is_open would help here.
The type received by inflate_generic_type can be anything such as "!T[]",
which requires a recursive transversal to check for errors as there is no
MonoGenericInst involved.

Cheers,
Rodrigo
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.ximian.com/pipermail/mono-devel-list/attachments/20090630/40c90263/attachment.html