Hi, On Sat, Jun 27, 2009 at 2:49 AM, Raja R Harinath <harin...@hurrynot.org>wrote:
> Hi, > > Rodrigo Kumpera <kump...@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
_______________________________________________ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list