From 69b91f94cef1ae22f435fd132672b60ab5c8bd74 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Wed, 5 Aug 2020 12:54:45 -0700 Subject: [PATCH] Add support for reporting byrefs to RVA static fields of collectible assemblies (#40346) - Keep track of all RVA static field locations - For assemblies loaded from PE files, use a range that is the entire PE range - For assemblies dynamically created, use piecemeal ranges for each individual RVA static field - Report byref references via the GcReportLoaderAllocator mechanism in PromoteCarefully - Add a test to cover this scenario, and thread statics - disable test on Mono, as it doesn't pass there yet --- src/coreclr/src/vm/assembly.cpp | 13 ++ src/coreclr/src/vm/commodule.cpp | 6 + src/coreclr/src/vm/loaderallocator.cpp | 43 +++++++ src/coreclr/src/vm/loaderallocator.hpp | 13 +- src/coreclr/src/vm/lockedrangelist.h | 61 +++++++++ src/coreclr/src/vm/siginfo.cpp | 10 ++ src/coreclr/src/vm/stubmgr.h | 48 +------ src/coreclr/tests/issues.targets | 3 + .../ByRefLocals/ByRefLocals.cs | 142 +++++++++++++++++++++ .../ByRefLocals/ByRefLocals.csproj | 13 ++ .../ByRefLocals/SpanAccessor.cs | 9 ++ .../ByRefLocals/SpanAccessor.csproj | 11 ++ .../CollectibleAssemblies/ByRefLocals/Unloaded.cs | 26 ++++ .../ByRefLocals/Unloaded.csproj | 12 ++ 14 files changed, 362 insertions(+), 48 deletions(-) create mode 100644 src/coreclr/src/vm/lockedrangelist.h create mode 100644 src/tests/Loader/CollectibleAssemblies/ByRefLocals/ByRefLocals.cs create mode 100644 src/tests/Loader/CollectibleAssemblies/ByRefLocals/ByRefLocals.csproj create mode 100644 src/tests/Loader/CollectibleAssemblies/ByRefLocals/SpanAccessor.cs create mode 100644 src/tests/Loader/CollectibleAssemblies/ByRefLocals/SpanAccessor.csproj create mode 100644 src/tests/Loader/CollectibleAssemblies/ByRefLocals/Unloaded.cs create mode 100644 src/tests/Loader/CollectibleAssemblies/ByRefLocals/Unloaded.csproj diff --git a/src/coreclr/src/vm/assembly.cpp b/src/coreclr/src/vm/assembly.cpp index 8bfb3d2..4934ae6 100644 --- a/src/coreclr/src/vm/assembly.cpp +++ b/src/coreclr/src/vm/assembly.cpp @@ -190,6 +190,19 @@ void Assembly::Init(AllocMemTracker *pamTracker, LoaderAllocator *pLoaderAllocat // loading it entirely. //CacheFriendAssemblyInfo(); +#ifndef CROSSGEN_COMPILE + if (IsCollectible()) + { + COUNT_T size; + BYTE *start = (BYTE*)m_pManifest->GetFile()->GetLoadedImageContents(&size); + if (start != NULL) + { + GCX_COOP(); + LoaderAllocator::AssociateMemoryWithLoaderAllocator(start, start + size, m_pLoaderAllocator); + } + } +#endif + { CANNOTTHROWCOMPLUSEXCEPTION(); FAULT_FORBID(); diff --git a/src/coreclr/src/vm/commodule.cpp b/src/coreclr/src/vm/commodule.cpp index a1f2e27..7ca511e 100644 --- a/src/coreclr/src/vm/commodule.cpp +++ b/src/coreclr/src/vm/commodule.cpp @@ -643,6 +643,12 @@ void QCALLTYPE COMModule::SetFieldRVAContent(QCall::ModuleHandle pModule, INT32 if (pContent != NULL) memcpy(pvBlob, pContent, length); + if (pReflectionModule->IsCollectible()) + { + GCX_COOP(); + LoaderAllocator::AssociateMemoryWithLoaderAllocator((BYTE*)pvBlob, ((BYTE*)pvBlob) + length, pReflectionModule->GetLoaderAllocator()); + } + // set FieldRVA into metadata. Note that this is not final RVA in the image if save to disk. We will do another round of fix up upon save. IfFailThrow( pRCW->GetEmitter()->SetFieldRVA(tkField, dwRVA) ); diff --git a/src/coreclr/src/vm/loaderallocator.cpp b/src/coreclr/src/vm/loaderallocator.cpp index 71f4a81..4215b8c 100644 --- a/src/coreclr/src/vm/loaderallocator.cpp +++ b/src/coreclr/src/vm/loaderallocator.cpp @@ -662,6 +662,11 @@ BOOL QCALLTYPE LoaderAllocator::Destroy(QCall::LoaderAllocatorHandle pLoaderAllo STRESS_LOG1(LF_CLASSLOADER, LL_INFO100, "Begin LoaderAllocator::Destroy for loader allocator %p\n", reinterpret_cast(static_cast(pLoaderAllocator))); LoaderAllocatorID *pID = pLoaderAllocator->Id(); + { + GCX_COOP(); + LoaderAllocator::RemoveMemoryToLoaderAllocatorAssociation(pLoaderAllocator); + } + // This will probably change for shared code unloading _ASSERTE(pID->GetType() == LAT_Assembly); @@ -1984,6 +1989,44 @@ UMEntryThunkCache *LoaderAllocator::GetUMEntryThunkCache() return m_pUMEntryThunkCache; } +/* static */ +void LoaderAllocator::RemoveMemoryToLoaderAllocatorAssociation(LoaderAllocator* pLoaderAllocator) +{ + CONTRACTL { + THROWS; + MODE_COOPERATIVE; + } CONTRACTL_END; + + GlobalLoaderAllocator* pGlobalAllocator = (GlobalLoaderAllocator*)SystemDomain::GetGlobalLoaderAllocator(); + pGlobalAllocator->m_memoryAssociations.RemoveRanges(pLoaderAllocator); +} + +/* static */ +void LoaderAllocator::AssociateMemoryWithLoaderAllocator(BYTE *start, const BYTE *end, LoaderAllocator* pLoaderAllocator) +{ + CONTRACTL { + THROWS; + MODE_COOPERATIVE; + } CONTRACTL_END; + + GlobalLoaderAllocator* pGlobalAllocator = (GlobalLoaderAllocator*)SystemDomain::GetGlobalLoaderAllocator(); + pGlobalAllocator->m_memoryAssociations.AddRange(start, end, pLoaderAllocator); +} + +/* static */ +PTR_LoaderAllocator LoaderAllocator::GetAssociatedLoaderAllocator_Unsafe(TADDR ptr) +{ + LIMITED_METHOD_CONTRACT; + + GlobalLoaderAllocator* pGlobalAllocator = (GlobalLoaderAllocator*)SystemDomain::GetGlobalLoaderAllocator(); + LoaderAllocator* pLoaderAllocator; + if (pGlobalAllocator->m_memoryAssociations.IsInRangeWorker_Unlocked(ptr, reinterpret_cast(&pLoaderAllocator))) + { + return pLoaderAllocator; + } + return NULL; +} + #endif // !CROSSGEN_COMPILE #ifdef FEATURE_COMINTEROP diff --git a/src/coreclr/src/vm/loaderallocator.hpp b/src/coreclr/src/vm/loaderallocator.hpp index 70b7cd8..6028496 100644 --- a/src/coreclr/src/vm/loaderallocator.hpp +++ b/src/coreclr/src/vm/loaderallocator.hpp @@ -23,6 +23,7 @@ class FuncPtrStubs; #include "methoddescbackpatchinfo.h" #include "crossloaderallocatorhash.h" #include "onstackreplacement.h" +#include "lockedrangelist.h" #define VPTRU_LoaderAllocator 0x3200 @@ -405,6 +406,13 @@ public: virtual LoaderAllocatorID* Id() =0; BOOL IsCollectible() { WRAPPER_NO_CONTRACT; return m_IsCollectible; } + // This function may only be called while the runtime is suspended + // As it does not lock around access to a RangeList + static PTR_LoaderAllocator GetAssociatedLoaderAllocator_Unsafe(TADDR ptr); + + static void AssociateMemoryWithLoaderAllocator(BYTE *start, const BYTE *end, LoaderAllocator* pLoaderAllocator); + static void RemoveMemoryToLoaderAllocatorAssociation(LoaderAllocator* pLoaderAllocator); + #ifdef DACCESS_COMPILE void EnumMemoryRegions(CLRDataEnumMemoryFlags flags); #endif @@ -627,12 +635,16 @@ typedef VPTR(LoaderAllocator) PTR_LoaderAllocator; class GlobalLoaderAllocator : public LoaderAllocator { + friend class LoaderAllocator; VPTR_VTABLE_CLASS(GlobalLoaderAllocator, LoaderAllocator) VPTR_UNIQUE(VPTRU_LoaderAllocator+1) DAC_ALIGNAS(LoaderAllocator) // Align the first member to the alignment of the base class BYTE m_ExecutableHeapInstance[sizeof(LoaderHeap)]; + // Associate memory regions with loader allocator objects + LockedRangeList m_memoryAssociations; + protected: LoaderAllocatorID m_Id; @@ -712,7 +724,6 @@ private: typedef VPTR(AssemblyLoaderAllocator) PTR_AssemblyLoaderAllocator; - #include "loaderallocator.inl" #endif // __LoaderAllocator_h__ diff --git a/src/coreclr/src/vm/lockedrangelist.h b/src/coreclr/src/vm/lockedrangelist.h new file mode 100644 index 0000000..9289b58 --- /dev/null +++ b/src/coreclr/src/vm/lockedrangelist.h @@ -0,0 +1,61 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +#ifndef __LockedRangeList_h__ +#define __LockedRangeList_h__ + +// ------------------------------------------------------- +// This just wraps the RangeList methods in a read or +// write lock depending on the operation. +// ------------------------------------------------------- + +class LockedRangeList : public RangeList +{ + public: + VPTR_VTABLE_CLASS(LockedRangeList, RangeList) + + LockedRangeList() : RangeList(), m_RangeListRWLock(COOPERATIVE_OR_PREEMPTIVE, LOCK_TYPE_DEFAULT) + { + LIMITED_METHOD_CONTRACT; + } + + ~LockedRangeList() + { + LIMITED_METHOD_CONTRACT; + } + + BOOL IsInRangeWorker_Unlocked(TADDR address, TADDR *pID = NULL) + { + WRAPPER_NO_CONTRACT; + SUPPORTS_DAC; + return RangeList::IsInRangeWorker(address, pID); + } + + protected: + + virtual BOOL AddRangeWorker(const BYTE *start, const BYTE *end, void *id) + { + WRAPPER_NO_CONTRACT; + SimpleWriteLockHolder lh(&m_RangeListRWLock); + return RangeList::AddRangeWorker(start,end,id); + } + + virtual void RemoveRangesWorker(void *id, const BYTE *start = NULL, const BYTE *end = NULL) + { + WRAPPER_NO_CONTRACT; + SimpleWriteLockHolder lh(&m_RangeListRWLock); + RangeList::RemoveRangesWorker(id,start,end); + } + + virtual BOOL IsInRangeWorker(TADDR address, TADDR *pID = NULL) + { + WRAPPER_NO_CONTRACT; + SUPPORTS_DAC; + SimpleReadLockHolder lh(&m_RangeListRWLock); + return RangeList::IsInRangeWorker(address, pID); + } + + SimpleRWLock m_RangeListRWLock; +}; + +#endif // __LockedRangeList_h__ \ No newline at end of file diff --git a/src/coreclr/src/vm/siginfo.cpp b/src/coreclr/src/vm/siginfo.cpp index 1d3fbbd..5cdeb7a 100644 --- a/src/coreclr/src/vm/siginfo.cpp +++ b/src/coreclr/src/vm/siginfo.cpp @@ -4900,6 +4900,16 @@ void PromoteCarefully(promote_func fn, return; } +#ifndef CROSSGEN_COMPILE + if (sc->promotion) + { + LoaderAllocator*pLoaderAllocator = LoaderAllocator::GetAssociatedLoaderAllocator_Unsafe(PTR_TO_TADDR(*ppObj)); + if (pLoaderAllocator != NULL) + { + GcReportLoaderAllocator(fn, sc, pLoaderAllocator); + } + } +#endif // CROSSGEN_COMPILE #endif // !defined(DACCESS_COMPILE) (*fn) (ppObj, sc, flags); diff --git a/src/coreclr/src/vm/stubmgr.h b/src/coreclr/src/vm/stubmgr.h index d36db9f..471f456 100644 --- a/src/coreclr/src/vm/stubmgr.h +++ b/src/coreclr/src/vm/stubmgr.h @@ -47,6 +47,7 @@ #define __stubmgr_h__ #include "simplerwlock.hpp" +#include "lockedrangelist.h" // When 'TraceStub' returns, it gives the address of where the 'target' is for a stub' // TraceType indicates what this 'target' is @@ -339,53 +340,6 @@ private: #endif // !CROSSGEN_COMPILE }; -// ------------------------------------------------------- -// This just wraps the RangeList methods in a read or -// write lock depending on the operation. -// ------------------------------------------------------- - -class LockedRangeList : public RangeList -{ - public: - VPTR_VTABLE_CLASS(LockedRangeList, RangeList) - - LockedRangeList() : RangeList(), m_RangeListRWLock(COOPERATIVE_OR_PREEMPTIVE, LOCK_TYPE_DEFAULT) - { - LIMITED_METHOD_CONTRACT; - } - - ~LockedRangeList() - { - LIMITED_METHOD_CONTRACT; - } - - protected: - - virtual BOOL AddRangeWorker(const BYTE *start, const BYTE *end, void *id) - { - WRAPPER_NO_CONTRACT; - SimpleWriteLockHolder lh(&m_RangeListRWLock); - return RangeList::AddRangeWorker(start,end,id); - } - - virtual void RemoveRangesWorker(void *id, const BYTE *start = NULL, const BYTE *end = NULL) - { - WRAPPER_NO_CONTRACT; - SimpleWriteLockHolder lh(&m_RangeListRWLock); - RangeList::RemoveRangesWorker(id,start,end); - } - - virtual BOOL IsInRangeWorker(TADDR address, TADDR *pID = NULL) - { - WRAPPER_NO_CONTRACT; - SUPPORTS_DAC; - SimpleReadLockHolder lh(&m_RangeListRWLock); - return RangeList::IsInRangeWorker(address, pID); - } - - SimpleRWLock m_RangeListRWLock; -}; - #ifndef CROSSGEN_COMPILE //----------------------------------------------------------- diff --git a/src/coreclr/tests/issues.targets b/src/coreclr/tests/issues.targets index f15a2b5..edfe25e 100644 --- a/src/coreclr/tests/issues.targets +++ b/src/coreclr/tests/issues.targets @@ -961,6 +961,9 @@ + + https://github.com/dotnet/runtime/issues/40394 + Handling for Copy constructors isn't present in mono interop diff --git a/src/tests/Loader/CollectibleAssemblies/ByRefLocals/ByRefLocals.cs b/src/tests/Loader/CollectibleAssemblies/ByRefLocals/ByRefLocals.cs new file mode 100644 index 0000000..834bc47 --- /dev/null +++ b/src/tests/Loader/CollectibleAssemblies/ByRefLocals/ByRefLocals.cs @@ -0,0 +1,142 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.IO; +using System.Reflection; +using System.Reflection.Emit; +using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; +using System.Runtime.Loader; + +class Program +{ + static int Main(string[] args) + { + var holdResult = HoldAssembliesAliveThroughByRefFields(out GCHandle gch1, out GCHandle gch2); + if (holdResult != 100) + return holdResult; + + // At this point, nothing should keep the collectible assembly alive + // Loop for a bit forcing the GC to run, and then it should be freed + for (int i = 0; i < 10; i++) + { + GC.Collect(2); + GC.WaitForPendingFinalizers(); + } + + if (gch1.Target != null) + { + return 3; + } + if (gch2.Target != null) + { + return 4; + } + + return 100; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int HoldAssembliesAliveThroughByRefFields(out GCHandle gch1, out GCHandle gch2) + { + // ThreadStatic lifetime check. Here we don't require the actual assembly to remain loaded, but we do require the field to remain accessible + var spanThreadStatic = LoadAssemblyThreadStatic(out GCHandle gchThreadStatic); + GC.Collect(2); + GC.WaitForPendingFinalizers(); + GC.Collect(2); + GC.WaitForPendingFinalizers(); + GC.Collect(2); + GC.WaitForPendingFinalizers(); + + var span1 = LoadAssembly(out gch1); + GC.Collect(2); + GC.WaitForPendingFinalizers(); + GC.Collect(2); + GC.WaitForPendingFinalizers(); + GC.Collect(2); + GC.WaitForPendingFinalizers(); + var span2 = CreateAssemblyDynamically(out gch2); + for (int i = 0; i < 10; i++) + { + Console.WriteLine(span1[0]); + Console.WriteLine(span2[0]); + Console.WriteLine(spanThreadStatic[0]); + GC.Collect(2); + GC.WaitForPendingFinalizers(); + if (gch1.Target == null) + { + return 1; + } + if (gch2.Target == null) + { + return 2; + } + if (spanThreadStatic[0] != 7) + { + Console.WriteLine($"spanThreadStatic[0] = {spanThreadStatic[0]}"); + return 5; + } + } + + return 100; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static ReadOnlySpan LoadAssembly(out GCHandle gchToAssembly) + { + var alc = new AssemblyLoadContext("test", isCollectible: true); + var a = alc.LoadFromAssemblyPath(Path.Combine(Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location), "Unloaded.dll")); + gchToAssembly = GCHandle.Alloc(a, GCHandleType.WeakTrackResurrection); + + var spanAccessor = (IReturnSpan)Activator.CreateInstance(a.GetType("SpanAccessor")); + + alc.Unload(); + + return spanAccessor.GetSpan(); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static ReadOnlySpan LoadAssemblyThreadStatic(out GCHandle gchToAssembly) + { + var alc = new AssemblyLoadContext("test", isCollectible: true); + var a = alc.LoadFromAssemblyPath(Path.Combine(Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location), "Unloaded.dll")); + gchToAssembly = GCHandle.Alloc(a, GCHandleType.WeakTrackResurrection); + + var spanAccessor = (IReturnSpan)Activator.CreateInstance(a.GetType("ThreadStaticSpanAccessor")); + + alc.Unload(); + + return spanAccessor.GetSpan(); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static ReadOnlySpan CreateAssemblyDynamically(out GCHandle gchToAssembly) + { + AssemblyBuilder ab = + AssemblyBuilder.DefineDynamicAssembly( + new AssemblyName("tempAssembly"), + AssemblyBuilderAccess.RunAndCollect); + ModuleBuilder modb = ab.DefineDynamicModule("tempAssembly.dll"); + + var byRefAccessField = modb.DefineInitializedData("RawBytes", new byte[] {1,2,3,4,5}, FieldAttributes.Public | FieldAttributes.Static); + modb.CreateGlobalFunctions(); + + TypeBuilder tb = modb.DefineType("GetSpanType", TypeAttributes.Class, typeof(object), new Type[]{typeof(IReturnSpan)}); + var mb = tb.DefineMethod("GetSpan", MethodAttributes.Public | MethodAttributes.HideBySig | MethodAttributes.Virtual, typeof(ReadOnlySpan), new Type[]{}); + ILGenerator myMethodIL = mb.GetILGenerator(); + myMethodIL.Emit(OpCodes.Ldsflda, byRefAccessField); + myMethodIL.Emit(OpCodes.Ldc_I4_4); + myMethodIL.Emit(OpCodes.Newobj, typeof(ReadOnlySpan).GetConstructor(new Type[]{typeof(void*), typeof(int)})); + myMethodIL.Emit(OpCodes.Ret); + + var getSpanType = tb.CreateType(); + + gchToAssembly = GCHandle.Alloc(getSpanType, GCHandleType.WeakTrackResurrection); + + var spanAccessor = (IReturnSpan)Activator.CreateInstance(getSpanType); + + return spanAccessor.GetSpan(); + } + +} \ No newline at end of file diff --git a/src/tests/Loader/CollectibleAssemblies/ByRefLocals/ByRefLocals.csproj b/src/tests/Loader/CollectibleAssemblies/ByRefLocals/ByRefLocals.csproj new file mode 100644 index 0000000..7cde272 --- /dev/null +++ b/src/tests/Loader/CollectibleAssemblies/ByRefLocals/ByRefLocals.csproj @@ -0,0 +1,13 @@ + + + true + Exe + BuildAndRun + 0 + + + + + + + \ No newline at end of file diff --git a/src/tests/Loader/CollectibleAssemblies/ByRefLocals/SpanAccessor.cs b/src/tests/Loader/CollectibleAssemblies/ByRefLocals/SpanAccessor.cs new file mode 100644 index 0000000..c05b505 --- /dev/null +++ b/src/tests/Loader/CollectibleAssemblies/ByRefLocals/SpanAccessor.cs @@ -0,0 +1,9 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; + +public interface IReturnSpan +{ + ReadOnlySpan GetSpan(); +} \ No newline at end of file diff --git a/src/tests/Loader/CollectibleAssemblies/ByRefLocals/SpanAccessor.csproj b/src/tests/Loader/CollectibleAssemblies/ByRefLocals/SpanAccessor.csproj new file mode 100644 index 0000000..88e99d9 --- /dev/null +++ b/src/tests/Loader/CollectibleAssemblies/ByRefLocals/SpanAccessor.csproj @@ -0,0 +1,11 @@ + + + true + Library + BuildOnly + 0 + + + + + \ No newline at end of file diff --git a/src/tests/Loader/CollectibleAssemblies/ByRefLocals/Unloaded.cs b/src/tests/Loader/CollectibleAssemblies/ByRefLocals/Unloaded.cs new file mode 100644 index 0000000..3f2615e --- /dev/null +++ b/src/tests/Loader/CollectibleAssemblies/ByRefLocals/Unloaded.cs @@ -0,0 +1,26 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Runtime.CompilerServices; + +public class SpanAccessor : IReturnSpan +{ + public static ReadOnlySpan RawData => new byte[] { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 }; + + public ReadOnlySpan GetSpan() + { + return RawData; + } +} + +public class ThreadStaticSpanAccessor : IReturnSpan +{ + [ThreadStatic] + public static byte ThreadStaticByte = 7; + + public unsafe ReadOnlySpan GetSpan() + { + return new ReadOnlySpan(Unsafe.AsPointer(ref ThreadStaticByte), 1); + } +} \ No newline at end of file diff --git a/src/tests/Loader/CollectibleAssemblies/ByRefLocals/Unloaded.csproj b/src/tests/Loader/CollectibleAssemblies/ByRefLocals/Unloaded.csproj new file mode 100644 index 0000000..6d13ecf --- /dev/null +++ b/src/tests/Loader/CollectibleAssemblies/ByRefLocals/Unloaded.csproj @@ -0,0 +1,12 @@ + + + true + Library + BuildOnly + 0 + + + + + + \ No newline at end of file -- 2.7.4