ILGenerator. Whats wrong with this Code

Question

I am trying to build a dynamic Property Accessor. Want something which is like really fast as close to calling the actually Property. Dont want to go the Reflection route as its very slow. So i opted to using DynamicAssembly and inject IL using ILGenerator. Below is the ILGenerator related code which seems to work

        Label nulllabel = getIL.DefineLabel();
        Label returnlabel = getIL.DefineLabel();
        //_type = targetGetMethod.ReturnType;
        if (methods.Count > 0)
        {
            getIL.DeclareLocal(typeof(object));
            getIL.DeclareLocal(typeof(bool));

            getIL.Emit(OpCodes.Ldarg_1); //Load the first argument

            //(target object)

            //Cast to the source type

            getIL.Emit(OpCodes.Castclass, this.mTargetType);
            //Get the property value

            foreach (var methodInfo in methods)
            {
                getIL.EmitCall(OpCodes.Call, methodInfo, null);

                if (methodInfo.ReturnType.IsValueType)
                {
                    getIL.Emit(OpCodes.Box, methodInfo.ReturnType);
                    //Box if necessary
                }
            }

            getIL.Emit(OpCodes.Stloc_0); //Store it
            getIL.Emit(OpCodes.Br_S,returnlabel);

            getIL.MarkLabel(nulllabel);
            getIL.Emit(OpCodes.Ldnull);
            getIL.Emit(OpCodes.Stloc_0);

            getIL.MarkLabel(returnlabel);
            getIL.Emit(OpCodes.Ldloc_0);
        }
        else
        {
            getIL.ThrowException(typeof(MissingMethodException));
        }
        getIL.Emit(OpCodes.Ret);

So above get the first argument which is the object that contains the property. the methods collection contains the nested property if any. for each property i use EmitCall which puts the the value on the stack and then i try to box it. This works like a charm.

The only issue is if you have a property like Order.Instrument.Symbol.Name and assume that Instrument object is null. Then the code will throw an null object exception.

So this what i did, i introduced a null check

            foreach (var methodInfo in methods)
            {
                getIL.EmitCall(OpCodes.Call, methodInfo, null);

                getIL.Emit(OpCodes.Stloc_0);
                getIL.Emit(OpCodes.Ldloc_0);

                getIL.Emit(OpCodes.Ldnull);
                getIL.Emit(OpCodes.Ceq);
                getIL.Emit(OpCodes.Stloc_1);
                getIL.Emit(OpCodes.Ldloc_1);
                getIL.Emit(OpCodes.Brtrue_S, nulllabel);
                getIL.Emit(OpCodes.Ldloc_0);

                if (methodInfo.ReturnType.IsValueType)
                {
                    getIL.Emit(OpCodes.Box, methodInfo.ReturnType);
                    //Box if necessary
                }
            }

Now this code breaks saying That the object/memory is corrupted etc. So what exactly is wrong with this code. Am i missing something here.

Thanks in Advance.

Answer 1

Previously, if you had consecutive properties P returning string and then Q returning int, you would get something like this:

...   
call P // returns string
call Q // requires a string on the stack, returns an int
box
...

Now you have something like this:

...
call P // returns string
store  // stores to object
...    // load, compare to null, etc.
load   // loads an *object*
call Q // requires a *string* on the stack
store  // stores to object *without boxing*
...

So I see two clear problems:

1. You are calling methods in such a way that the target is only known to be an object, not a specific type which has that method.
2. You are not boxing value types before storing them to a local of type object.

These can be solved by reworking your logic slightly. There are also a few other minor details you could clean up:

1. Rather than ceq followed by brtrue, just use beq.
2. There's no point in doing Stloc_1 followed by Ldloc_1 rather than just using the value on the stack since that local isn't used anywhere else.

Incorporating these changes, here's what I'd do:

Type finalType = null;

foreach (var methodInfo in methods)
{
    finalType = methodInfo.ReturnType;

    getIL.EmitCall(OpCodes.Call, methodInfo, null);
    if (!finalType.IsValueType)
    {
        getIL.Emit(OpCodes.Dup);
        getIL.Emit(OpCodes.Ldnull);
        getIL.Emit(OpCodes.Beq_S, nulllabel);
    }
}

if (finalType.IsValueType)
{
    getIL.Emit(OpCodes.Box, methodInfo.ReturnType);
    //Box if necessary
}

getIL.Emit(OpCodes.Br_S, returnLabel);

getIL.MarkLabel(nulllabel);
getIL.Emit(OpCodes.Pop);    
getIL.Emit(OpCodes.Ldnull);

getIL.MarkLabel(returnlabel);

Note that we can get rid of both locals since we now just duplicate the top value on the stack before comparing against null.