Skip to content

Commit

Permalink
Fix compilation of Synchronized method combined with generics (#94203)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
MichalStrehovsky committed Oct 31, 2023
1 parent c11e684 commit f4ee321
Show file tree
Hide file tree
Showing 9 changed files with 91 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@
using System;
using System.Threading;

using Internal.Runtime.Augments;

using Debug = System.Diagnostics.Debug;

namespace Internal.Runtime.CompilerHelpers
{
/// <summary>
Expand Down Expand Up @@ -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();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<CombinedDependencyListEntry> GetConditionalDependenciesDueToGenericDictionary(NodeFactory factory, MethodDesc method)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
3 changes: 1 addition & 2 deletions src/tests/JIT/Generics/Fields/getclassfrommethodparam.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,7 @@ public class Foo<F>
{
public static string Value;

// [MethodImpl(MethodImplOptions.Synchronized | MethodImplOptions.NoInlining)]
[MethodImpl(MethodImplOptions.NoInlining)]
[MethodImpl(MethodImplOptions.Synchronized | MethodImplOptions.NoInlining)]
public static void Action<T>(T value)
{
Value = value.ToString();
Expand Down
49 changes: 49 additions & 0 deletions src/tests/nativeaot/SmokeTests/UnitTests/Generics.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ internal static int Run()
TestDictionaryDependencyTracking.Run();
TestStaticBaseLookups.Run();
TestInitThisClass.Run();
TestSynchronizedMethods.Run();
TestDelegateFatFunctionPointers.Run();
TestDelegateToCanonMethods.Run();
TestVirtualMethodUseTracking.Run();
Expand Down Expand Up @@ -602,6 +603,54 @@ public static void Run()
}
}

class TestSynchronizedMethods
{
static class Gen<T>
{
[MethodImpl(MethodImplOptions.Synchronized)]
public static int Synchronize() => 42;
}

static class NonGen
{
[MethodImpl(MethodImplOptions.Synchronized)]
public static int Synchronize<T>() => 42;
}

static class GenReflected<T>
{
[MethodImpl(MethodImplOptions.Synchronized)]
public static int Synchronize() => 42;
}

static class NonGenReflected
{
[MethodImpl(MethodImplOptions.Synchronized)]
public static int Synchronize<T>() => 42;
}

static Type s_genReflectedType = typeof(GenReflected<>);
static MethodInfo s_nonGenReflectedSynchronizeMethod = typeof(NonGenReflected).GetMethod("Synchronize");

public static void Run()
{
Gen<object>.Synchronize();
NonGen.Synchronize<object>();
Gen<int>.Synchronize();
NonGen.Synchronize<int>();

{
var mi = (MethodInfo)s_genReflectedType.MakeGenericType(typeof(object)).GetMemberWithSameMetadataDefinitionAs(typeof(GenReflected<>).GetMethod("Synchronize"));
mi.Invoke(null, Array.Empty<object>());
}

{
var mi = s_nonGenReflectedSynchronizeMethod.MakeGenericMethod(typeof(object));
mi.Invoke(null, Array.Empty<object>());
}
}
}

/// <summary>
/// Tests that lazily built vtables for canonically equivalent types have the same shape.
/// </summary>
Expand Down

0 comments on commit f4ee321

Please sign in to comment.