Always pass LoaderAllocator when creating Assembly (dotnet#107643)

We are currently conditionally passing the `LoaderAllocator` to initialize the `Assembly` based on whether it is collectible and then having the initialization explicitly get the global one if it was null. When we create the `Assembly`, we already have the appropriate loader allocator (global loader allocator for non-collectible, corresponding assembly loader allocator for collectible), so just create the `Assembly` with it.
jtschuster · Sep 17, 2024 · 78ac1bf · 78ac1bf
1 parent 482a6b1
commit 78ac1bf
Show file tree

Hide file tree

Showing 3 changed files with 37 additions and 46 deletions.
diff --git a/src/coreclr/vm/assembly.cpp b/src/coreclr/vm/assembly.cpp
@@ -117,28 +117,35 @@ void Assembly::Initialize()
 // It cannot do any allocations or operations that might fail. Those operations should be done
 // in Assembly::Init()
 //----------------------------------------------------------------------------------------------
-Assembly::Assembly(PEAssembly* pPEAssembly, DebuggerAssemblyControlFlags debuggerFlags, BOOL fIsCollectible) :
-    m_pClassLoader(NULL),
-    m_pEntryPoint(NULL),
-    m_pModule(NULL),
-    m_pPEAssembly(clr::SafeAddRef(pPEAssembly)),
-    m_pFriendAssemblyDescriptor(NULL),
-    m_isDynamic(false),
+Assembly::Assembly(PEAssembly* pPEAssembly, DebuggerAssemblyControlFlags debuggerFlags, LoaderAllocator *pLoaderAllocator)
+    : m_pClassLoader(NULL)
+    , m_pEntryPoint(NULL)
+    , m_pModule(NULL)
+    , m_pPEAssembly(clr::SafeAddRef(pPEAssembly))
+    , m_pFriendAssemblyDescriptor(NULL)
+    , m_isDynamic(false)
 #ifdef FEATURE_COLLECTIBLE_TYPES
-    m_isCollectible(fIsCollectible),
+    , m_isCollectible{pLoaderAllocator->IsCollectible()}
 #endif
-    m_pLoaderAllocator(NULL),
+    , m_pLoaderAllocator{pLoaderAllocator}
 #ifdef FEATURE_COMINTEROP
-    m_pITypeLib(NULL),
+    , m_pITypeLib(NULL)
 #endif // FEATURE_COMINTEROP
 #ifdef FEATURE_COMINTEROP
-    m_InteropAttributeStatus(INTEROP_ATTRIBUTE_UNSET),
+    , m_InteropAttributeStatus(INTEROP_ATTRIBUTE_UNSET)
 #endif
-    m_debuggerFlags(debuggerFlags),
-    m_fTerminated(FALSE),
-    m_hExposedObject{}
+    , m_debuggerFlags(debuggerFlags)
+    , m_fTerminated(FALSE)
+    , m_hExposedObject{}
 {
-    STANDARD_VM_CONTRACT;
+    CONTRACTL
+    {
+        STANDARD_VM_CHECK;
+        PRECONDITION(pPEAssembly != NULL);
+        PRECONDITION(pLoaderAllocator != NULL);
+        PRECONDITION(pLoaderAllocator->IsCollectible() || pLoaderAllocator == SystemDomain::GetGlobalLoaderAllocator());
+    }
+    CONTRACTL_END
 }
 
 // This name needs to stay in sync with AssemblyBuilder.ManifestModuleName
@@ -151,32 +158,10 @@ Assembly::Assembly(PEAssembly* pPEAssembly, DebuggerAssemblyControlFlags debugge
 // and the assembly is safely destructable. Whether this function throws or succeeds,
 // it must leave the Assembly in a safely destructable state.
 //----------------------------------------------------------------------------------------------
-void Assembly::Init(AllocMemTracker *pamTracker, LoaderAllocator *pLoaderAllocator)
+void Assembly::Init(AllocMemTracker *pamTracker)
 {
     STANDARD_VM_CONTRACT;
 
-    if (IsSystem())
-    {
-        _ASSERTE(pLoaderAllocator == NULL); // pLoaderAllocator may only be non-null for collectible types
-        m_pLoaderAllocator = SystemDomain::GetGlobalLoaderAllocator();
-    }
-    else
-    {
-        if (!IsCollectible())
-        {
-            // pLoaderAllocator will only be non-null for reflection emit assemblies
-            _ASSERTE((pLoaderAllocator == NULL) || (pLoaderAllocator == AppDomain::GetCurrentDomain()->GetLoaderAllocator()));
-            m_pLoaderAllocator = AppDomain::GetCurrentDomain()->GetLoaderAllocator();
-        }
-        else
-        {
-            _ASSERTE(pLoaderAllocator != NULL); // ppLoaderAllocator must be non-null for collectible assemblies
-
-            m_pLoaderAllocator = pLoaderAllocator;
-        }
-    }
-    _ASSERTE(m_pLoaderAllocator != NULL);
-
     m_pClassLoader = new ClassLoader(this);
     m_pClassLoader->Init(pamTracker);
 
@@ -321,13 +306,19 @@ void Assembly::Terminate( BOOL signalProfiler )
 Assembly * Assembly::Create(
     PEAssembly *                 pPEAssembly,
     DebuggerAssemblyControlFlags debuggerFlags,
-    BOOL                         fIsCollectible,
     AllocMemTracker *            pamTracker,
     LoaderAllocator *            pLoaderAllocator)
 {
-    STANDARD_VM_CONTRACT;
+    CONTRACTL
+    {
+        STANDARD_VM_CHECK;
+        PRECONDITION(pPEAssembly != NULL);
+        PRECONDITION(pLoaderAllocator != NULL);
+        PRECONDITION(pLoaderAllocator->IsCollectible() || pLoaderAllocator == SystemDomain::GetGlobalLoaderAllocator());
+    }
+    CONTRACTL_END
 
-    NewHolder<Assembly> pAssembly (new Assembly(pPEAssembly, debuggerFlags, fIsCollectible));
+    NewHolder<Assembly> pAssembly (new Assembly(pPEAssembly, debuggerFlags, pLoaderAllocator));
 
 #ifdef PROFILING_SUPPORTED
     {
@@ -341,7 +332,7 @@ Assembly * Assembly::Create(
     EX_TRY
 #endif
     {
-        pAssembly->Init(pamTracker, pLoaderAllocator);
+        pAssembly->Init(pamTracker);
     }
 #ifdef PROFILING_SUPPORTED
     EX_HOOK

diff --git a/src/coreclr/vm/assembly.hpp b/src/coreclr/vm/assembly.hpp
@@ -55,14 +55,14 @@ class Assembly
     friend class ClrDataAccess;
 
 private:
-    Assembly(PEAssembly *pPEAssembly, DebuggerAssemblyControlFlags debuggerFlags, BOOL fIsCollectible);
-    void Init(AllocMemTracker *pamTracker, LoaderAllocator *pLoaderAllocator);
+    Assembly(PEAssembly *pPEAssembly, DebuggerAssemblyControlFlags debuggerFlags, LoaderAllocator* pLoaderAllocator);
+    void Init(AllocMemTracker *pamTracker);
 
 public:
     void StartUnload();
     void Terminate( BOOL signalProfiler = TRUE );
 
-    static Assembly *Create(PEAssembly *pPEAssembly, DebuggerAssemblyControlFlags debuggerFlags, BOOL fIsCollectible, AllocMemTracker *pamTracker, LoaderAllocator *pLoaderAllocator);
+    static Assembly *Create(PEAssembly *pPEAssembly, DebuggerAssemblyControlFlags debuggerFlags, AllocMemTracker *pamTracker, LoaderAllocator *pLoaderAllocator);
     static void Initialize();
 
     BOOL IsSystem() { WRAPPER_NO_CONTRACT; return m_pPEAssembly->IsSystem(); }

diff --git a/src/coreclr/vm/domainassembly.cpp b/src/coreclr/vm/domainassembly.cpp
@@ -55,7 +55,7 @@ DomainAssembly::DomainAssembly(PEAssembly* pPEAssembly, LoaderAllocator* pLoader
     SetupDebuggingConfig();
 
     // Create the Assembly
-    NewHolder<Assembly> assembly = Assembly::Create(GetPEAssembly(), GetDebuggerInfoBits(), IsCollectible(), memTracker, IsCollectible() ? GetLoaderAllocator() : NULL);
+    NewHolder<Assembly> assembly = Assembly::Create(pPEAssembly, GetDebuggerInfoBits(), memTracker, pLoaderAllocator);
 
     m_pAssembly = assembly.Extract();
     m_pModule = m_pAssembly->GetModule();