Support for loading virtual delegates

Callsites that load a virtual method delegate AV in the JIT due to `getCallInfo` returning a stub call kind instead of virtual call kind, which the JIT isn't expecting. Fix `getCallInfo` to behave like CoreCLR's Crossgen.

Tweak the control flow of 'getCallInfo' to more closely resemble what Crossgen does. This makes things easier to reason about since the logic in `getCallInfo` is complicated.

MethodWithToken is now used as a dictionary key and needs implementations for `Equals` and `GetHashCode`

Switch GVMs to use virtual entrypoint to match CoreCLR. With the test case I added to ReadyToRunUnit, the runtime would AV returning the value from the GVM when using regular method entry.

Remove the string constructor special casing in getCallinfo - it was for the CoreRT runtime.

src/ILCompiler.ReadyToRun/src/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs

@@ -156,12 +156,6 @@ namespace ILCompiler.DependencyAnalysis
             }
         }
 
-        public IMethodNode StringAllocator(MethodDesc constructor, ModuleToken methodToken, SignatureContext signatureContext)
-        {
-            return MethodEntrypoint(constructor, constrainedType: null, originalMethod: null, 
-                methodToken: methodToken, signatureContext: signatureContext, isUnboxingStub: false);
-        }
-
         protected override ISymbolNode CreateReadyToRunHelperNode(ReadyToRunHelperKey helperCall)
         {
             throw new NotImplementedException();
@@ -519,5 +513,29 @@ namespace ILCompiler.DependencyAnalysis
             }
             return sectionStartNode;
         }
+
+        private Dictionary<MethodWithToken, ISymbolNode> _dynamicHelperCellCache = new Dictionary<MethodWithToken, ISymbolNode>();
+
+        public ISymbolNode DynamicHelperCell(MethodWithToken methodWithToken, SignatureContext signatureContext)
+        {
+            ISymbolNode result;
+            if (!_dynamicHelperCellCache.TryGetValue(methodWithToken, out result))
+            {
+                result = new DelayLoadHelperImport(
+                    this,
+                    DispatchImports,
+                    ILCompiler.DependencyAnalysis.ReadyToRun.ReadyToRunHelper.READYTORUN_HELPER_DelayLoad_Helper_Obj,
+                    MethodSignature(
+                        ReadyToRunFixupKind.READYTORUN_FIXUP_VirtualEntry,
+                        methodWithToken.Method,
+                        constrainedType: null,
+                        methodWithToken.Token,
+                        signatureContext: signatureContext,
+                        isUnboxingStub: false,
+                        isInstantiatingStub: false));
+                _dynamicHelperCellCache.Add(methodWithToken, result);
+            }
+            return result;
+        }
     }
 }

src/ILCompiler.ReadyToRun/src/JitInterface/CorInfoImpl.ReadyToRun.cs

@@ -28,6 +28,22 @@ namespace Internal.JitInterface
             Method = method;
             Token = token;
         }
+
+        public override bool Equals(object obj)
+        {
+            return obj is MethodWithToken methodWithToken &&
+                Equals(methodWithToken);
+        }
+
+        public override int GetHashCode()
+        {
+            return Method.GetHashCode() ^ unchecked(17 * Token.GetHashCode());
+        }
+
+        public bool Equals(MethodWithToken methodWithToken)
+        {
+            return Method == methodWithToken.Method && Token.Equals(methodWithToken.Token);
+        }
     }
 
     public struct GenericContext : IEquatable<GenericContext>
@@ -376,7 +392,12 @@ namespace Internal.JitInterface
                 case CorInfoHelpFunc.CORINFO_HELP_NEWARR_1_DIRECT:
                     id = ReadyToRunHelper.NewArray;
                     break;
-
+                case CorInfoHelpFunc.CORINFO_HELP_VIRTUAL_FUNC_PTR:
+                    id = ReadyToRunHelper.VirtualFuncPtr;
+                    break;
+                case CorInfoHelpFunc.CORINFO_HELP_READYTORUN_VIRTUAL_FUNC_PTR:
+                    id = ReadyToRunHelper.VirtualFuncPtr;
+                    break;
                 case CorInfoHelpFunc.CORINFO_HELP_LMUL:
                     id = ReadyToRunHelper.LMul;
                     break;
@@ -768,7 +789,6 @@ namespace Internal.JitInterface
 
             bool resolvedConstraint = false;
             bool forceUseRuntimeLookup = false;
-            bool targetIsFatFunctionPointer = false;
 
             MethodDesc methodAfterConstraintResolution = method;
             if (constrainedType == null)
@@ -846,6 +866,11 @@ namespace Internal.JitInterface
             bool directCall = false;
             bool resolvedCallVirt = false;
 
+            if ((flags & CORINFO_CALLINFO_FLAGS.CORINFO_CALLINFO_LDFTN) != 0)
+            {
+                directCall = true;
+            }
+            else
             if (targetMethod.Signature.IsStatic)
             {
                 // Static methods are always direct calls
@@ -871,103 +896,93 @@ namespace Internal.JitInterface
 
             pResult->codePointerOrStubLookup.lookupKind.needsRuntimeLookup = false;
 
-            bool allowInstParam = (flags & CORINFO_CALLINFO_FLAGS.CORINFO_CALLINFO_ALLOWINSTPARAM) != 0;
-
-            if (directCall && !allowInstParam && targetMethod.GetCanonMethodTarget(CanonicalFormKind.Specific).RequiresInstArg())
+            if (directCall)
             {
-                // JIT needs a single address to call this method but the method needs a hidden argument.
-                // We need a fat function pointer for this that captures both things.
-                targetIsFatFunctionPointer = true;
+                bool allowInstParam = (flags & CORINFO_CALLINFO_FLAGS.CORINFO_CALLINFO_ALLOWINSTPARAM) != 0;
+                MethodDesc canonMethod = targetMethod.GetCanonMethodTarget(CanonicalFormKind.Specific);
 
-                // JIT won't expect fat function pointers unless this is e.g. delegate creation
-                Debug.Assert((flags & CORINFO_CALLINFO_FLAGS.CORINFO_CALLINFO_LDFTN) != 0);
-
-                pResult->kind = CORINFO_CALL_KIND.CORINFO_CALL_CODE_POINTER;
-
-                if (pResult->exactContextNeedsRuntimeLookup)
+                if (pResult->exactContextNeedsRuntimeLookup && !allowInstParam && targetMethod.GetCanonMethodTarget(CanonicalFormKind.Specific).RequiresInstArg())
                 {
+                    CORINFO_RUNTIME_LOOKUP_KIND lookupKind = GetGenericRuntimeLookupKind(canonMethod);
+                    ReadyToRunHelperId helperId = ReadyToRunHelperId.MethodHandle;
+                    object helperArg = new MethodWithToken(canonMethod, new ModuleToken(_tokenContext, pResolvedToken.token));
+                    GenericContext methodContext = new GenericContext(entityFromContext(pResolvedToken.tokenContext));
+                    ISymbolNode helper = _compilation.SymbolNodeFactory.GenericLookupHelper(lookupKind, helperId, helperArg, methodContext, _signatureContext);
+                    pResult->codePointerOrStubLookup.constLookup = CreateConstLookupToSymbol(helper);
                     pResult->codePointerOrStubLookup.lookupKind.needsRuntimeLookup = true;
                     pResult->codePointerOrStubLookup.lookupKind.runtimeLookupFlags = 0;
                     pResult->codePointerOrStubLookup.runtimeLookup.indirections = CORINFO.USEHELPER;
-
-                    // Do not bother computing the runtime lookup if we are inlining. The JIT is going
-                    // to abort the inlining attempt anyway.
-                    MethodDesc contextMethod = methodFromContext(pResolvedToken.tokenContext);
-                    if (contextMethod == MethodBeingCompiled)
-                    {
-                        pResult->codePointerOrStubLookup.lookupKind.runtimeLookupKind = GetGenericRuntimeLookupKind(contextMethod);
-                        pResult->codePointerOrStubLookup.lookupKind.runtimeLookupFlags = (ushort)ReadyToRunHelperId.MethodEntry;
-                        pResult->codePointerOrStubLookup.lookupKind.runtimeLookupArgs = (void*)ObjectToHandle(GetRuntimeDeterminedObjectForToken(ref pResolvedToken));
-                    }
+                    pResult->kind = CORINFO_CALL_KIND.CORINFO_CALL_CODE_POINTER;
                 }
                 else
                 {
-                    pResult->codePointerOrStubLookup.constLookup =
-                        CreateConstLookupToSymbol(_compilation.NodeFactory.FatFunctionPointer(targetMethod));
-                }
-            }
-            else if (directCall)
-            {
-                bool referencingArrayAddressMethod = false;
+                    bool referencingArrayAddressMethod = false;
 
-                if (targetMethod.IsIntrinsic)
-                {
-                    // For multidim array Address method, we pretend the method requires a hidden instantiation argument
-                    // (even though it doesn't need one). We'll actually swap the method out for a differnt one with
-                    // a matching calling convention later. See ArrayMethod for a description.
-                    referencingArrayAddressMethod = targetMethod.IsArrayAddressMethod();
-                }
-
-                MethodDesc concreteMethod = targetMethod;
-                targetMethod = targetMethod.GetCanonMethodTarget(CanonicalFormKind.Specific);
-
-                pResult->kind = CORINFO_CALL_KIND.CORINFO_CALL;
-
-                if (targetMethod.IsConstructor && targetMethod.OwningType.IsString)
-                {
-                    // Calling a string constructor doesn't call the actual constructor.
-                    pResult->codePointerOrStubLookup.constLookup = CreateConstLookupToSymbol(
-                        _compilation.NodeFactory.StringAllocator(targetMethod,
-                            new ModuleToken(_tokenContext, pResolvedToken.token), _signatureContext)
-                        );
-                }
-                else if (pResult->exactContextNeedsRuntimeLookup)
-                {
-                    // Nothing to do... The generic handle lookup gets embedded in to the codegen
-                    // during the jitting of the call.
-                    // (Note: The generic lookup in R2R is performed by a call to a helper at runtime, not by
-                    // codegen emitted at crossgen time)
-
-                    Debug.Assert(!forceUseRuntimeLookup);
-                    pResult->codePointerOrStubLookup.constLookup = CreateConstLookupToSymbol(
-                        _compilation.NodeFactory.MethodEntrypoint(targetMethod, constrainedType, method,
-                            new ModuleToken(_tokenContext, pResolvedToken.token), _signatureContext)
-                        );
-                }
-                else
-                {
-                    ISymbolNode instParam = null;
-
-                    if (targetMethod.RequiresInstMethodDescArg())
+                    if (targetMethod.IsIntrinsic)
                     {
-                        instParam = _compilation.SymbolNodeFactory.MethodGenericDictionary(concreteMethod,
-                            new ModuleToken(_tokenContext, pResolvedToken.token), _signatureContext);
-                    }
-                    else if (targetMethod.RequiresInstMethodTableArg() || referencingArrayAddressMethod)
-                    {
-                        // Ask for a constructed type symbol because we need the vtable to get to the dictionary
-                        instParam = _compilation.SymbolNodeFactory.ConstructedTypeSymbol(concreteMethod.OwningType, _signatureContext);
+                        // For multidim array Address method, we pretend the method requires a hidden instantiation argument
+                        // (even though it doesn't need one). We'll actually swap the method out for a differnt one with
+                        // a matching calling convention later. See ArrayMethod for a description.
+                        referencingArrayAddressMethod = targetMethod.IsArrayAddressMethod();
                     }
 
-                    if (instParam != null)
+                    MethodDesc concreteMethod = targetMethod;
+                    targetMethod = targetMethod.GetCanonMethodTarget(CanonicalFormKind.Specific);
+
+                    pResult->kind = CORINFO_CALL_KIND.CORINFO_CALL;
+
+                    // Compensate for always treating delegates as direct calls above
+                    if ((flags & CORINFO_CALLINFO_FLAGS.CORINFO_CALLINFO_LDFTN) != 0 && (flags & CORINFO_CALLINFO_FLAGS.CORINFO_CALLINFO_CALLVIRT) != 0 && !resolvedCallVirt)
                     {
-                        pResult->instParamLookup = CreateConstLookupToSymbol(instParam);
+                        pResult->kind = CORINFO_CALL_KIND.CORINFO_VIRTUALCALL_LDVIRTFTN;
                     }
 
-                    pResult->codePointerOrStubLookup.constLookup = CreateConstLookupToSymbol(
-                        _compilation.NodeFactory.MethodEntrypoint(targetMethod, constrainedType, method,
-                            new ModuleToken(_tokenContext, pResolvedToken.token), _signatureContext)
-                        );
+                    if (pResult->exactContextNeedsRuntimeLookup)
+                    {
+                        // Nothing to do... The generic handle lookup gets embedded in to the codegen
+                        // during the jitting of the call.
+                        // (Note: The generic lookup in R2R is performed by a call to a helper at runtime, not by
+                        // codegen emitted at crossgen time)
+
+                        Debug.Assert(!forceUseRuntimeLookup);
+                        pResult->codePointerOrStubLookup.constLookup = CreateConstLookupToSymbol(
+                            _compilation.NodeFactory.MethodEntrypoint(targetMethod, constrainedType, method,
+                                new ModuleToken(_tokenContext, pResolvedToken.token), _signatureContext)
+                            );
+                    }
+                    else
+                    {
+                        ISymbolNode instParam = null;
+
+                        if (targetMethod.RequiresInstMethodDescArg())
+                        {
+                            instParam = _compilation.SymbolNodeFactory.MethodGenericDictionary(concreteMethod,
+                                new ModuleToken(_tokenContext, pResolvedToken.token), _signatureContext);
+                        }
+                        else if (targetMethod.RequiresInstMethodTableArg() || referencingArrayAddressMethod)
+                        {
+                            // Ask for a constructed type symbol because we need the vtable to get to the dictionary
+                            instParam = _compilation.SymbolNodeFactory.ConstructedTypeSymbol(concreteMethod.OwningType, _signatureContext);
+                        }
+
+                        if (instParam != null)
+                        {
+                            pResult->instParamLookup = CreateConstLookupToSymbol(instParam);
+                        }
+
+                        if (pResult->kind == CORINFO_CALL_KIND.CORINFO_VIRTUALCALL_LDVIRTFTN)
+                        {
+                            pResult->codePointerOrStubLookup.constLookup = CreateConstLookupToSymbol(_compilation.NodeFactory.DynamicHelperCell(
+                                new MethodWithToken(targetMethod, new ModuleToken(_tokenContext, pResolvedToken.token)), _signatureContext));
+                        }
+                        else
+                        {
+                            pResult->codePointerOrStubLookup.constLookup = CreateConstLookupToSymbol(
+                            _compilation.NodeFactory.MethodEntrypoint(targetMethod, constrainedType, method,
+                                new ModuleToken(_tokenContext, pResolvedToken.token), _signatureContext)
+                            );
+                        }
+                    }
                 }
 
                 pResult->nullInstanceCheck = resolvedCallVirt;
@@ -976,26 +991,14 @@ namespace Internal.JitInterface
             {
                 // GVM Call Support
                 pResult->kind = CORINFO_CALL_KIND.CORINFO_VIRTUALCALL_LDVIRTFTN;
-                pResult->codePointerOrStubLookup.constLookup.accessType = InfoAccessType.IAT_VALUE;
                 pResult->nullInstanceCheck = true;
-
-                if (pResult->exactContextNeedsRuntimeLookup)
-                {
-                    ComputeLookup(ref pResolvedToken,
-                        GetRuntimeDeterminedObjectForToken(ref pResolvedToken),
-                        ReadyToRunHelperId.MethodHandle,
-                        ref pResult->codePointerOrStubLookup);
-                    Debug.Assert(pResult->codePointerOrStubLookup.lookupKind.needsRuntimeLookup);
-                }
-
-                // RyuJIT will assert if we report CORINFO_CALLCONV_PARAMTYPE for a result of a ldvirtftn
-                // We don't need an instantiation parameter, so let's just not report it. Might be nice to
-                // move that assert to some place later though.
-                targetIsFatFunctionPointer = true;
+                pResult->codePointerOrStubLookup.constLookup = CreateConstLookupToSymbol(
+                    _compilation.NodeFactory.DynamicHelperCell(
+                        new MethodWithToken(targetMethod, new ModuleToken(_tokenContext, pResolvedToken.token)), _signatureContext));
             }
             else
+            // In ReadyToRun, we always use the dispatch stub to call virtual methods
             {
-                // In ReadyToRun, we always use the dispatch stub to call virtual methods
                 pResult->kind = CORINFO_CALL_KIND.CORINFO_VIRTUALCALL_STUB;
 
                 if (pResult->exactContextNeedsRuntimeLookup)
@@ -1026,7 +1029,7 @@ namespace Internal.JitInterface
             pResult->classFlags = getClassAttribsInternal(targetMethod.OwningType);
 
             pResult->methodFlags = getMethodAttribsInternal(targetMethod);
-            Get_CORINFO_SIG_INFO(targetMethod, &pResult->sig, targetIsFatFunctionPointer);
+            Get_CORINFO_SIG_INFO(targetMethod, &pResult->sig);
 
             if ((flags & CORINFO_CALLINFO_FLAGS.CORINFO_CALLINFO_VERIFICATION) != 0)
             {

src/ILCompiler.RyuJit/src/JitInterface/CorInfoImpl.RyuJit.cs

@@ -1215,9 +1215,7 @@ namespace Internal.JitInterface
                 // move that assert to some place later though.
                 targetIsFatFunctionPointer = true;
             }
-            else
-            // In ReadyToRun, we always use the dispatch stub to call virtual methods
-            if ((flags & CORINFO_CALLINFO_FLAGS.CORINFO_CALLINFO_LDFTN) == 0
+            else if ((flags & CORINFO_CALLINFO_FLAGS.CORINFO_CALLINFO_LDFTN) == 0
                 && targetMethod.OwningType.IsInterface)
             {
                 pResult->kind = CORINFO_CALL_KIND.CORINFO_VIRTUALCALL_STUB;
@@ -1242,8 +1240,7 @@ namespace Internal.JitInterface
                         ));
                 }
             }
-            else
-            if ((flags & CORINFO_CALLINFO_FLAGS.CORINFO_CALLINFO_LDFTN) == 0
+            else if ((flags & CORINFO_CALLINFO_FLAGS.CORINFO_CALLINFO_LDFTN) == 0
                 && _compilation.HasFixedSlotVTable(targetMethod.OwningType))
             {
                 pResult->kind = CORINFO_CALL_KIND.CORINFO_VIRTUALCALL_VTABLE;

tests/src/Simple/ReadyToRunUnit/Program.cs

@@ -791,6 +791,45 @@ internal class Program
         return match;
     }
 
+    internal class ClassWithVirtual
+    {
+        public bool VirtualCalledFlag = false;
+
+        public virtual void Virtual()
+        {
+            Console.WriteLine("Virtual called");
+            VirtualCalledFlag = true;
+        }
+    }
+
+    public class BaseClass
+    {
+        public virtual int MyGvm<T>()
+        {
+            Console.WriteLine("MyGvm returning 100");
+            return 100;
+        }
+    }
+
+    // Test that ldvirtftn can load a virtual instance delegate method
+    public static bool VirtualDelegateLoadTest()
+    {
+        bool success = true;
+
+        var classWithVirtual = new ClassWithVirtual();
+
+        Action virtualMethod = classWithVirtual.Virtual;
+        virtualMethod();
+
+        success &= classWithVirtual.VirtualCalledFlag;
+
+        
+        var bc = new BaseClass();
+        success &= (bc.MyGvm<int>() == 100);
+
+        return success;
+    }
+
     public static int Main(string[] args)
     {
         if (args.Length > 0)
@@ -842,6 +881,7 @@ internal class Program
         RunTest("SharedGenericNonGcStaticTest", SharedGenericNonGcStaticTest());
         RunTest("SharedGenericTlsGcStaticTest", SharedGenericTlsGcStaticTest());
         RunTest("SharedGenericTlsNonGcStaticTest", SharedGenericTlsNonGcStaticTest());
+        RunTest("VirtualDelegateLoadTest", VirtualDelegateLoadTest());
 
         Console.WriteLine($@"{_passedTests.Count} tests pass:");
         foreach (string testName in _passedTests)

tests/src/Simple/ReadyToRunUnit/methodWhiteList.txt

@@ -4,3 +4,4 @@ Program+DisposeStruct..cctor()
 Program+DisposeClass.Dispose()
 Program+DisposeClass..cctor()
 ClassWithStatic..cctor()
+[ReadyToRunUnit.ni]Program+BaseClass.MyGvm()