From 3e7ad90234bdfd96f8e85d1658e5e622a22c392b Mon Sep 17 00:00:00 2001 From: Tomas Date: Sat, 20 Feb 2021 22:56:24 +0100 Subject: [PATCH 1/3] Fix handling of array constructors in Crossgen2 Fixes: https://github.com/dotnet/runtime/issues/48204 --- .../ReadyToRunCodegenNodeFactory.cs | 6 +++++- .../JitInterface/CorInfoImpl.ReadyToRun.cs | 17 ++++++++++++++--- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs index 209a5a1af2116e..65fa2a6c67ab6c 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs @@ -358,7 +358,11 @@ private IMethodNode CreateMethodEntrypoint(TypeAndMethod key) MethodWithToken method = key.Method; bool isInstantiatingStub = key.IsInstantiatingStub; bool isPrecodeImportRequired = key.IsPrecodeImportRequired; - MethodDesc compilableMethod = method.Method.GetCanonMethodTarget(CanonicalFormKind.Specific); + MethodDesc compilableMethod = method.Method; + if (!compilableMethod.OwningType.IsArray) + { + compilableMethod = compilableMethod.GetCanonMethodTarget(CanonicalFormKind.Specific); + } MethodWithGCInfo methodWithGCInfo = null; if (CompilationModuleGroup.ContainsMethodBody(compilableMethod, false)) diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs index 794320a5b9d024..54bab9aa19e944 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs @@ -1334,7 +1334,7 @@ private void ceeInfoGetCallInfo( // Its basic meaning is that shared generic methods always need instantiating // stubs as the shared generic code needs the method dictionary parameter that cannot // be provided by other means. - useInstantiatingStub = originalMethod.GetCanonMethodTarget(CanonicalFormKind.Specific).RequiresInstMethodDescArg(); + useInstantiatingStub = originalMethod.OwningType.IsArray || originalMethod.GetCanonMethodTarget(CanonicalFormKind.Specific).RequiresInstMethodDescArg(); callerMethod = HandleToObject(callerHandle); @@ -1534,7 +1534,7 @@ private void ceeInfoGetCallInfo( } methodToCall = targetMethod; - MethodDesc canonMethod = targetMethod.GetCanonMethodTarget(CanonicalFormKind.Specific); + MethodDesc canonMethod = (targetMethod.OwningType.IsArray ? null : targetMethod.GetCanonMethodTarget(CanonicalFormKind.Specific)); if (directCall) { @@ -1572,7 +1572,18 @@ private void ceeInfoGetCallInfo( const CORINFO_CALLINFO_FLAGS LdVirtFtnMask = CORINFO_CALLINFO_FLAGS.CORINFO_CALLINFO_LDFTN | CORINFO_CALLINFO_FLAGS.CORINFO_CALLINFO_CALLVIRT; bool unresolvedLdVirtFtn = ((flags & LdVirtFtnMask) == LdVirtFtnMask) && !resolvedCallVirt; - if ((pResult->exactContextNeedsRuntimeLookup && useInstantiatingStub && (!allowInstParam || resolvedConstraint)) || forceUseRuntimeLookup) + if (targetMethod.OwningType.IsArray && targetMethod.IsConstructor) + { + // Constructors on arrays are special and don't actually have entrypoints. + // That would be fine by itself and wouldn't need special casing. But + // constructors on SzArray have a weird property that causes them not to have canonical forms. + // int[][] has a .ctor(int32,int32) to construct the jagged array in one go, but its canonical + // form of __Canon[] doesn't have the two-parameter constructor. The canonical form would need + // to have an unlimited number of constructors to cover stuff like "int[][][][][][]..." + pResult->kind = CORINFO_CALL_KIND.CORINFO_CALL; + pResult->codePointerOrStubLookup.constLookup = default; + } + else if ((pResult->exactContextNeedsRuntimeLookup && useInstantiatingStub && (!allowInstParam || resolvedConstraint)) || forceUseRuntimeLookup) { if (unresolvedLdVirtFtn) { From 9ddf83480fbcef527ca3e58703ca3a8d19eb858f Mon Sep 17 00:00:00 2001 From: Tomas Date: Sun, 21 Feb 2021 01:23:36 +0100 Subject: [PATCH 2/3] Ramping up on the method descriptor handle in CoreCLR --- .../JitInterface/CorInfoImpl.ReadyToRun.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs index 54bab9aa19e944..6ab912edbde537 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs @@ -1548,7 +1548,7 @@ private void ceeInfoGetCallInfo( bool allowInstParam = (flags & CORINFO_CALLINFO_FLAGS.CORINFO_CALLINFO_ALLOWINSTPARAM) != 0; - if (!allowInstParam && canonMethod.RequiresInstArg()) + if (!allowInstParam && canonMethod != null && canonMethod.RequiresInstArg()) { useInstantiatingStub = true; } @@ -1608,7 +1608,7 @@ private void ceeInfoGetCallInfo( if (allowInstParam) { useInstantiatingStub = false; - methodToCall = canonMethod; + methodToCall = canonMethod ?? methodToCall; } pResult->kind = CORINFO_CALL_KIND.CORINFO_CALL; From 652ca4d709848aace53fb1ccf3c9bc1fbb3bb0ec Mon Sep 17 00:00:00 2001 From: Tomas Date: Sun, 21 Feb 2021 22:02:03 +0100 Subject: [PATCH 3/3] Improve the change based on Michal's PR feedback Thanks to Michal's feedback I've been able to better grasp the problem and fix deficiencies of my first commit. I have moved setting of the constLookup to the "zapinfo version of getCallInfo", I have modified the code to avoid canonicalization only for array constructors and I rolled back the change to CreateMethodEntrypoint that is no longer necessary after the "zapinfo" fix. Thanks Tomas --- .../ReadyToRunCodegenNodeFactory.cs | 6 +---- .../JitInterface/CorInfoImpl.ReadyToRun.cs | 25 ++++++++++++------- 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs index 65fa2a6c67ab6c..209a5a1af2116e 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs @@ -358,11 +358,7 @@ private IMethodNode CreateMethodEntrypoint(TypeAndMethod key) MethodWithToken method = key.Method; bool isInstantiatingStub = key.IsInstantiatingStub; bool isPrecodeImportRequired = key.IsPrecodeImportRequired; - MethodDesc compilableMethod = method.Method; - if (!compilableMethod.OwningType.IsArray) - { - compilableMethod = compilableMethod.GetCanonMethodTarget(CanonicalFormKind.Specific); - } + MethodDesc compilableMethod = method.Method.GetCanonMethodTarget(CanonicalFormKind.Specific); MethodWithGCInfo methodWithGCInfo = null; if (CompilationModuleGroup.ContainsMethodBody(compilableMethod, false)) diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs index 6ab912edbde537..a1cf8b89c13d65 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs @@ -1534,7 +1534,8 @@ private void ceeInfoGetCallInfo( } methodToCall = targetMethod; - MethodDesc canonMethod = (targetMethod.OwningType.IsArray ? null : targetMethod.GetCanonMethodTarget(CanonicalFormKind.Specific)); + bool isArrayConstructor = targetMethod.OwningType.IsArray && targetMethod.IsConstructor; + MethodDesc canonMethod = (isArrayConstructor ? null : targetMethod.GetCanonMethodTarget(CanonicalFormKind.Specific)); if (directCall) { @@ -1572,7 +1573,7 @@ private void ceeInfoGetCallInfo( const CORINFO_CALLINFO_FLAGS LdVirtFtnMask = CORINFO_CALLINFO_FLAGS.CORINFO_CALLINFO_LDFTN | CORINFO_CALLINFO_FLAGS.CORINFO_CALLINFO_CALLVIRT; bool unresolvedLdVirtFtn = ((flags & LdVirtFtnMask) == LdVirtFtnMask) && !resolvedCallVirt; - if (targetMethod.OwningType.IsArray && targetMethod.IsConstructor) + if (isArrayConstructor) { // Constructors on arrays are special and don't actually have entrypoints. // That would be fine by itself and wouldn't need special casing. But @@ -1581,7 +1582,6 @@ private void ceeInfoGetCallInfo( // form of __Canon[] doesn't have the two-parameter constructor. The canonical form would need // to have an unlimited number of constructors to cover stuff like "int[][][][][][]..." pResult->kind = CORINFO_CALL_KIND.CORINFO_CALL; - pResult->codePointerOrStubLookup.constLookup = default; } else if ((pResult->exactContextNeedsRuntimeLookup && useInstantiatingStub && (!allowInstParam || resolvedConstraint)) || forceUseRuntimeLookup) { @@ -1835,12 +1835,19 @@ private void getCallInfo(ref CORINFO_RESOLVED_TOKEN pResolvedToken, CORINFO_RESO nonUnboxingMethod = rawPinvoke.Target; } - // READYTORUN: FUTURE: Direct calls if possible - pResult->codePointerOrStubLookup.constLookup = CreateConstLookupToSymbol( - _compilation.NodeFactory.MethodEntrypoint( - ComputeMethodWithToken(nonUnboxingMethod, ref pResolvedToken, constrainedType, unboxing: isUnboxingStub), - isInstantiatingStub: useInstantiatingStub, - isPrecodeImportRequired: (flags & CORINFO_CALLINFO_FLAGS.CORINFO_CALLINFO_LDFTN) != 0)); + if (methodToCall.OwningType.IsArray && methodToCall.IsConstructor) + { + pResult->codePointerOrStubLookup.constLookup = default; + } + else + { + // READYTORUN: FUTURE: Direct calls if possible + pResult->codePointerOrStubLookup.constLookup = CreateConstLookupToSymbol( + _compilation.NodeFactory.MethodEntrypoint( + ComputeMethodWithToken(nonUnboxingMethod, ref pResolvedToken, constrainedType, unboxing: isUnboxingStub), + isInstantiatingStub: useInstantiatingStub, + isPrecodeImportRequired: (flags & CORINFO_CALLINFO_FLAGS.CORINFO_CALLINFO_LDFTN) != 0)); + } // If the abi of the method isn't stable, this will cause a usage of the RequiresRuntimeJitSymbol, which will trigger a RequiresRuntimeJitException UpdateConstLookupWithRequiresRuntimeJitSymbolIfNeeded(ref pResult->codePointerOrStubLookup.constLookup, targetMethod);