[Mono-dev] [patch] partial implementation of System.Reflection.Emit.DynamicILInfo

[Casey Marshall]

On Fri, 2008-03-14 at 17:03 +0100, Paolo Molaro wrote:
> On 03/07/08 Casey Marshall wrote:
> > The attached patches add a partial implementation of DynamicILInfo --
> > basically enough that you can specify the IL code and locals for a
> > method, and have that method runnable.
> > 
> > I might work on this more if I have time, *plus* I'm not sure if this
> > here is too naive, and would need to change a lot for a full
> > implementation.
> 
> A few comments below.
> 
> > Index: mono/metadata/object-internals.h
> > ===================================================================
> > --- mono/metadata/object-internals.h	(revision 97635)
> > +++ mono/metadata/object-internals.h	(working copy)
> > @@ -1004,6 +1004,15 @@
> >  } MonoReflectionGuidAttribute;
> >  
> >  typedef struct {
> > +    MonoObject object;
> > +    MonoArray *code;
> > +    gint32 code_len;
> > +    gint32 max_stack;
> > +    MonoArray *exceptions;
> > +    MonoArray *localsig;
> 
> For newly introduced types, please put all the reference fields at the
> start.
> 

OK, noted.

> > +} MonoReflectionDynamicILInfo;
> > +
> > +typedef struct {
> >  	MonoObject object;
> >  	MonoMethod *mhandle;
> >  	MonoString *name;
> > @@ -1019,8 +1028,10 @@
> >  	MonoArray *refs;
> >  	GSList *referenced_by;
> >  	MonoReflectionType *owner;
> > +    MonoReflectionDynamicILInfo *dynil;
> 
> Use TABs to indent.
> 

Apologies for all that damage; I think I have emacs indenting correctly
now.

> > Index: mono/metadata/reflection.c
> > ===================================================================
> > --- mono/metadata/reflection.c	(revision 97635)
> > +++ mono/metadata/reflection.c	(working copy)
> > @@ -1454,7 +1455,7 @@
> >  	rmb->attrs = mb->attrs;
> >  	rmb->iattrs = 0;
> >  	rmb->call_conv = mb->call_conv;
> > -	rmb->code = NULL;
> > +    rmb->code = NULL;
> 
> Your patch introduces whitespace damage.
> 
> > @@ -8682,6 +8684,16 @@
> >  			num_locals = rmb->ilgen->locals ? mono_array_length (rmb->ilgen->locals) : 0;
> >  			if (rmb->ilgen->ex_handlers)
> >  				num_clauses = method_count_clauses (rmb->ilgen);
> > +		} else if (rmb->dynil) {
> > +			char *ptr = mono_array_addr (rmb->dynil->localsig, guint8, 0);
> > +			code = mono_array_addr (rmb->dynil->code, guint8, 0);
> > +			code_size = rmb->dynil->code_len;
> > +			max_stack = rmb->dynil->max_stack;
> > +			if (*ptr == 0x07)
> > +			{
> 
> Put the opening brace at the end of the previous line.
> 

OK.

> > +		if (rmb->dynil)
> > +		{
> > +			const char *ptr = mono_array_addr (rmb->dynil->localsig, guint8, 0);
> > +			ptr += 2;
> > +			for (i = 0; i < num_locals; i++)
> > +			{
> > +				MonoType *t = mono_metadata_parse_type_full(NULL, NULL, MONO_PARSE_LOCAL,
> > +															0, ptr, &ptr);
> 
> Unfortunately it isn't this simple: the above will work only for very
> simple types. Extensive testing may be needed to see which tokens are
> assigned for user types.
> 

Hmm, OK. I was wondering if there was something in mono's core already
that did all the token resolution (it seems like there would need to be,
if the code is to be jitted).

This token assignment looks rather hairy to me, anyway. It's all pretty
fuzzy how this is supposed to work.

> > Index: class/corlib/System.Reflection.Emit/DynamicMethod.cs
> > ===================================================================
> > --- class/corlib/System.Reflection.Emit/DynamicMethod.cs	(revision 97638)
> > +++ class/corlib/System.Reflection.Emit/DynamicMethod.cs	(working copy)
> > @@ -61,6 +61,7 @@
> >  		private object[] refs;
> >  		private IntPtr referenced_by;
> >  		private Type owner;
> > +        private DynamicILInfo dynil;
> 
> 
> Whitespace damage here as well.
> 
> It would be nice if you added also the tests for the new functionality.
> This is good for a start, though.
> Note that for runtime changes for us to be able to accept them you need
> to explicitly license them under the MIT/X11 license.

This shouldn't be a problem.

> Thanks!
> 

Thanks for the feedback.