From f4ee321b662068ebf819aec725c2a4fd0eee2bb6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Wed, 1 Nov 2023 05:52:10 +0900 Subject: [PATCH] Fix compilation of Synchronized method combined with generics (#94203) Fixes #77093. As I was looking at the bug, I realized it got miscategorized as native AOT, but the customer filed it for ReadyToRun. The bug is in both native AOT and ReadyToRun. I'm fixing it for both. We clearly didn't have any pre-existing test coverage, but as I was looking around the test tree, getclassfrommethodparam.cs was obviously testing this. For unexplained reasons, the Synchronized part got commented out. When I compared the file with the original in TFS at `$/DevDiv/FX/Feature/NetFXDev1/QA/CLR/testsrc/jit/Generics/Fields/getclassfrommethodparam.cs`, the commented out line is not commented out. So I'm fixing it. I've also added dedicated native aot test because this has dependency analysis implications that we need to be careful about. --- .../Runtime/Augments/TypeLoaderCallbacks.cs | 1 + .../SynchronizedMethodHelpers.cs | 13 +++++ .../TypeLoader/TypeLoaderEnvironment.cs | 6 +++ .../Compiler/MetadataManager.cs | 6 +++ .../IL/ILImporter.Scanner.cs | 8 +++ .../JitInterface/CorInfoImpl.ReadyToRun.cs | 2 + .../JitInterface/CorInfoImpl.RyuJit.cs | 5 ++ .../Fields/getclassfrommethodparam.cs | 3 +- .../SmokeTests/UnitTests/Generics.cs | 49 +++++++++++++++++++ 9 files changed, 91 insertions(+), 2 deletions(-) diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/Runtime/Augments/TypeLoaderCallbacks.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/Runtime/Augments/TypeLoaderCallbacks.cs index 99210e9db22c9..796b55c0bb311 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/Runtime/Augments/TypeLoaderCallbacks.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/Runtime/Augments/TypeLoaderCallbacks.cs @@ -14,6 +14,7 @@ namespace Internal.Runtime.Augments [CLSCompliant(false)] public abstract class TypeLoaderCallbacks { + public abstract bool TryGetOwningTypeForMethodDictionary(IntPtr dictionary, out RuntimeTypeHandle owningType); public abstract TypeManagerHandle GetModuleForMetadataReader(MetadataReader reader); public abstract bool TryGetConstructedGenericTypeForComponents(RuntimeTypeHandle genericTypeDefinitionHandle, RuntimeTypeHandle[] genericTypeArgumentHandles, out RuntimeTypeHandle runtimeTypeHandle); public abstract IntPtr GetThreadStaticGCDescForDynamicType(TypeManagerHandle handle, int index); diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/Runtime/CompilerHelpers/SynchronizedMethodHelpers.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/Runtime/CompilerHelpers/SynchronizedMethodHelpers.cs index 56701f033500b..1cb545db0c4dc 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/Runtime/CompilerHelpers/SynchronizedMethodHelpers.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/Runtime/CompilerHelpers/SynchronizedMethodHelpers.cs @@ -4,6 +4,10 @@ using System; using System.Threading; +using Internal.Runtime.Augments; + +using Debug = System.Diagnostics.Debug; + namespace Internal.Runtime.CompilerHelpers { /// @@ -73,5 +77,14 @@ private static unsafe RuntimeType GetStaticLockObject(MethodTable* pMT) { return Type.GetTypeFromMethodTable(pMT); } + + private static unsafe MethodTable* GetSyncFromClassHandle(MethodTable* pMT) => pMT; + + private static unsafe MethodTable* GetClassFromMethodParam(IntPtr pDictionary) + { + bool success = RuntimeAugments.TypeLoaderCallbacks.TryGetOwningTypeForMethodDictionary(pDictionary, out RuntimeTypeHandle th); + Debug.Assert(success); + return th.ToMethodTable(); + } } } diff --git a/src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/Runtime/TypeLoader/TypeLoaderEnvironment.cs b/src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/Runtime/TypeLoader/TypeLoaderEnvironment.cs index 1bd3cbd3672fa..37b6dc2b4d916 100644 --- a/src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/Runtime/TypeLoader/TypeLoaderEnvironment.cs +++ b/src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/Runtime/TypeLoader/TypeLoaderEnvironment.cs @@ -22,6 +22,12 @@ namespace Internal.Runtime.TypeLoader { internal class Callbacks : TypeLoaderCallbacks { + public override bool TryGetOwningTypeForMethodDictionary(IntPtr dictionary, out RuntimeTypeHandle owningType) + { + // PERF: computing NameAndSignature and the instantiation (that we discard) was useless + return TypeLoaderEnvironment.Instance.TryGetGenericMethodComponents(dictionary, out owningType, out _, out _); + } + public override TypeManagerHandle GetModuleForMetadataReader(MetadataReader reader) { return ModuleList.Instance.GetModuleForMetadataReader(reader); diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/MetadataManager.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/MetadataManager.cs index 98f2f238b8ca5..91d1e8c8c65f2 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/MetadataManager.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/MetadataManager.cs @@ -391,6 +391,12 @@ public void GetDependenciesDueToGenericDictionary(ref DependencyList dependencie dependencies ??= new DependencyList(); dependencies.Add(factory.GenericMethodsHashtableEntry(method), "Reflection visible dictionary"); } + + if (method.Signature.IsStatic && method.IsSynchronized) + { + dependencies ??= new DependencyList(); + dependencies.Add(factory.GenericMethodsHashtableEntry(method), "Will need to look up owning type from dictionary"); + } } public IEnumerable GetConditionalDependenciesDueToGenericDictionary(NodeFactory factory, MethodDesc method) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/IL/ILImporter.Scanner.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/IL/ILImporter.Scanner.cs index 4f669cc7fe8e8..bd4595f45137c 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/IL/ILImporter.Scanner.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/IL/ILImporter.Scanner.cs @@ -153,6 +153,14 @@ public DependencyList Import() { _dependencies.Add(_factory.NecessaryTypeSymbol(method.OwningType), reason); } + + if (_canonMethod.IsCanonicalMethod(CanonicalFormKind.Any)) + { + _dependencies.Add(_compilation.NodeFactory.MethodEntrypoint(_compilation.NodeFactory.TypeSystemContext.GetHelperEntryPoint("SynchronizedMethodHelpers", "GetSyncFromClassHandle")), reason); + + if (_canonMethod.RequiresInstMethodDescArg()) + _dependencies.Add(_compilation.NodeFactory.MethodEntrypoint(_compilation.NodeFactory.TypeSystemContext.GetHelperEntryPoint("SynchronizedMethodHelpers", "GetClassFromMethodParam")), reason); + } } else { 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 cb00ffa400dbf..befcf8a7d54dd 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs @@ -1257,6 +1257,8 @@ private ISymbolNode GetHelperFtnUncached(CorInfoHelpFunc ftnNum) case CorInfoHelpFunc.CORINFO_HELP_INITCLASS: case CorInfoHelpFunc.CORINFO_HELP_INITINSTCLASS: + case CorInfoHelpFunc.CORINFO_HELP_GETSYNCFROMCLASSHANDLE: + case CorInfoHelpFunc.CORINFO_HELP_GETCLASSFROMMETHODPARAM: case CorInfoHelpFunc.CORINFO_HELP_THROW_ARGUMENTEXCEPTION: case CorInfoHelpFunc.CORINFO_HELP_THROW_ARGUMENTOUTOFRANGEEXCEPTION: case CorInfoHelpFunc.CORINFO_HELP_THROW_PLATFORM_NOT_SUPPORTED: diff --git a/src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs b/src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs index 23146ce32d40c..e86b3502942f0 100644 --- a/src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs +++ b/src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs @@ -726,6 +726,11 @@ private ISymbolNode GetHelperFtnUncached(CorInfoHelpFunc ftnNum) id = ReadyToRunHelper.MonitorExitStatic; break; + case CorInfoHelpFunc.CORINFO_HELP_GETSYNCFROMCLASSHANDLE: + return _compilation.NodeFactory.MethodEntrypoint(_compilation.NodeFactory.TypeSystemContext.GetHelperEntryPoint("SynchronizedMethodHelpers", "GetSyncFromClassHandle")); + case CorInfoHelpFunc.CORINFO_HELP_GETCLASSFROMMETHODPARAM: + return _compilation.NodeFactory.MethodEntrypoint(_compilation.NodeFactory.TypeSystemContext.GetHelperEntryPoint("SynchronizedMethodHelpers", "GetClassFromMethodParam")); + case CorInfoHelpFunc.CORINFO_HELP_GVMLOOKUP_FOR_SLOT: id = ReadyToRunHelper.GVMLookupForSlot; break; diff --git a/src/tests/JIT/Generics/Fields/getclassfrommethodparam.cs b/src/tests/JIT/Generics/Fields/getclassfrommethodparam.cs index 93f6a2eb8df66..e3c12fdb4df1e 100644 --- a/src/tests/JIT/Generics/Fields/getclassfrommethodparam.cs +++ b/src/tests/JIT/Generics/Fields/getclassfrommethodparam.cs @@ -11,8 +11,7 @@ public class Foo { public static string Value; - // [MethodImpl(MethodImplOptions.Synchronized | MethodImplOptions.NoInlining)] - [MethodImpl(MethodImplOptions.NoInlining)] + [MethodImpl(MethodImplOptions.Synchronized | MethodImplOptions.NoInlining)] public static void Action(T value) { Value = value.ToString(); diff --git a/src/tests/nativeaot/SmokeTests/UnitTests/Generics.cs b/src/tests/nativeaot/SmokeTests/UnitTests/Generics.cs index 58a2b69cccc1e..bc5fe3e446d72 100644 --- a/src/tests/nativeaot/SmokeTests/UnitTests/Generics.cs +++ b/src/tests/nativeaot/SmokeTests/UnitTests/Generics.cs @@ -13,6 +13,7 @@ internal static int Run() TestDictionaryDependencyTracking.Run(); TestStaticBaseLookups.Run(); TestInitThisClass.Run(); + TestSynchronizedMethods.Run(); TestDelegateFatFunctionPointers.Run(); TestDelegateToCanonMethods.Run(); TestVirtualMethodUseTracking.Run(); @@ -602,6 +603,54 @@ public static void Run() } } + class TestSynchronizedMethods + { + static class Gen + { + [MethodImpl(MethodImplOptions.Synchronized)] + public static int Synchronize() => 42; + } + + static class NonGen + { + [MethodImpl(MethodImplOptions.Synchronized)] + public static int Synchronize() => 42; + } + + static class GenReflected + { + [MethodImpl(MethodImplOptions.Synchronized)] + public static int Synchronize() => 42; + } + + static class NonGenReflected + { + [MethodImpl(MethodImplOptions.Synchronized)] + public static int Synchronize() => 42; + } + + static Type s_genReflectedType = typeof(GenReflected<>); + static MethodInfo s_nonGenReflectedSynchronizeMethod = typeof(NonGenReflected).GetMethod("Synchronize"); + + public static void Run() + { + Gen.Synchronize(); + NonGen.Synchronize(); + Gen.Synchronize(); + NonGen.Synchronize(); + + { + var mi = (MethodInfo)s_genReflectedType.MakeGenericType(typeof(object)).GetMemberWithSameMetadataDefinitionAs(typeof(GenReflected<>).GetMethod("Synchronize")); + mi.Invoke(null, Array.Empty()); + } + + { + var mi = s_nonGenReflectedSynchronizeMethod.MakeGenericMethod(typeof(object)); + mi.Invoke(null, Array.Empty()); + } + } + } + /// /// Tests that lazily built vtables for canonically equivalent types have the same shape. ///