JIT: bug in devirtualization where type information gets lost

[AndyAyersMS]

Following test case, if compiled /o+ /debug:pdbonly, and run in a debug/check CoreCLR, will trigger an assert in resolveVirtualMethod:

Consistency check failed: FAILED: slotNumber < GetNumVtableSlots()

This it triggered by aggressive use of dup in the generated IL.

Likely two fixes needed.

First, have the VM side validate that the method being invoked is really a virtual method of the derived class, and if not, fail devirtualization. Would be nice to get the diagnostic information back too since this case represents a modelling error in the jit, so perhaps in addition to failing, resolveVirtualMethod should return some kind of status code.

Second, need to fix the jit not to lose the type information in this case.

This can also result in silent bad code, if it turns out the slot number presented to B is a valid one, and the method found there happens to be final.

using System;

public class Base
{
    public int value;
    public void B0() { value += 12; }
    public virtual void B1() { value += 33; }
}

// Ensure that D1 and D2 have enough virtuals that the slot number for
// the virtual M1 is greater than any virtual's slot number in B.

public class D1 : Base
{
    public virtual void MA() { }
    public virtual void MB() { }
    public virtual void MC() { }
    public virtual void MD() { }

    public virtual void M1() { value += 44; }
}

public class D2 : Base
{
    public virtual void MA() { }
    public virtual void MB() { }
    public virtual void MC() { }
    public virtual void MD() { }

    public virtual void M1() { value += 55; }
}

// Aggressive use of 'dup' here by CSC will confuse the jit, and it
// will effectively substitute 'b' for uses of d1 and d2. This is not
// value-incorrect but loses type information.
//
// This loss of type information subsequently triggers an assert in
// devirtualization because b does not have M1 as virtual method.

public class Test
{
    public static int Main(string[] args)
    {
        Base b;

        if (args.Length > 0)
        {
            D1 d1 = new D1();
            b = d1;
            d1.B1();
            d1.M1();
        }
        else
        {
            D2 d2 = new D2();
            b = d2;
            d2.B1();
            d2.M1();
        }

        b.B0();
        return b.value;
    }
}

[AndyAyersMS]

Looking at the jit side -- the problematic IL is:

newobj       0x6000009
dup         
stloc.0     
dup         
callvirt     0x6000002

Where local 0 has a type that is a supertype of the newobj. In impImportBlockCode the jit deliberately substitutes the local for the result of the dup.

                // Convert a (dup, stloc) sequence into a (stloc, ldloc) sequence in the following cases:
                // - If this is non-debug code - so that CSE will recognize the two as equal.
                //   This helps eliminate a redundant bounds check in cases such as:
                //       ariba[i+3] += some_value;
                // - If the top of the stack is a non-leaf that may be expensive to clone.

Because we're in the importer we may be able to access a more accurate type for the value being dup'd and spill to a temp local instead. So we'd effectively act as if the IL were

newobj       0x6000009
stloc        newtemp  // gets type from newobj
ldloc        newtemp
ldloc        newtemp        
stloc.0     
ldloc        newtemp
callvirt     0x6000002

which would avoid losing type information and still lead to the same CSE opportunities; the only downside is re-introducing the local temp that CSC worked so hard to get rid of. In this particular case we already create a temp for the result of newobj so no additional temp is actually needed.

The jit also uses the same dup transmogrification to turn ldloc; dup; stloc into 'ldloc; stloc; ldloc` but here there is no type loss and it can probably stay as is.

So the logic would be something like: when importing a dup, if the TOS is not a local or a simple constant, pop the TOS and spill it to a new temp with type taken from the stack model, then reload the temp twice.