[release/8.0] Fix a deadlock in NonGC + Profiler API (#91130)
authorgithub-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Fri, 25 Aug 2023 22:25:32 +0000 (15:25 -0700)
committerGitHub <noreply@github.com>
Fri, 25 Aug 2023 22:25:32 +0000 (15:25 -0700)
* Address Jan's feedback

* Fix Debug build

* Clean up

* Fix debug assert

* Update frozenobjectheap.cpp

* Address feedback

* fix build

* Update src/coreclr/vm/frozenobjectheap.cpp

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
* Address feedback

* Add gc lock for UpdateFrozenSegment

* Address feedback

* Oops, revert unrelated change

* Use preemptive for the whole method

* PublishFrozenObject has to be called in COOP

* Update src/coreclr/vm/frozenobjectheap.cpp

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
* Address feedback

* Add asserts

* Address feedback

* fix build

* Address feedback

* Update src/coreclr/vm/frozenobjectheap.cpp

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
---------

Co-authored-by: EgorBo <egorbo@gmail.com>
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
src/coreclr/gc/gc.cpp
src/coreclr/gc/gcee.cpp
src/coreclr/gc/gcpriv.h
src/coreclr/vm/appdomain.hpp
src/coreclr/vm/frozenobjectheap.cpp
src/coreclr/vm/frozenobjectheap.h
src/coreclr/vm/gchelpers.cpp
src/coreclr/vm/methodtable.cpp
src/coreclr/vm/profilingenumerators.cpp
src/coreclr/vm/proftoeeinterfaceimpl.cpp

index 02a9b8f..daeadfe 100644 (file)
@@ -9955,6 +9955,20 @@ BOOL gc_heap::insert_ro_segment (heap_segment* seg)
     return TRUE;
 }
 
+void gc_heap::update_ro_segment (heap_segment* seg, uint8_t* allocated, uint8_t* committed)
+{
+    enter_spin_lock (&gc_heap::gc_lock);
+
+    assert (use_frozen_segments_p);
+    assert (heap_segment_read_only_p (seg));
+    assert (allocated <= committed);
+    assert (committed <= heap_segment_reserved (seg));
+    heap_segment_allocated (seg) = allocated;
+    heap_segment_committed (seg) = committed;
+
+    leave_spin_lock (&gc_heap::gc_lock);
+}
+
 // No one is calling this function right now. If this is getting called we need
 // to take care of decommitting the mark array for it - we will need to remember
 // which portion of the mark array was committed and only decommit that.
index 6dbbfd6..32738da 100644 (file)
@@ -510,9 +510,12 @@ bool GCHeap::IsInFrozenSegment(Object *object)
 void GCHeap::UpdateFrozenSegment(segment_handle seg, uint8_t* allocated, uint8_t* committed)
 {
 #ifdef FEATURE_BASICFREEZE
-    heap_segment* heap_seg = reinterpret_cast<heap_segment*>(seg);
-    heap_segment_committed(heap_seg) = committed;
-    heap_segment_allocated(heap_seg) = allocated;
+#ifdef MULTIPLE_HEAPS
+    gc_heap* heap = gc_heap::g_heaps[0];
+#else
+    gc_heap* heap = pGenGCHeap;
+#endif //MULTIPLE_HEAPS
+    heap->update_ro_segment (reinterpret_cast<heap_segment*>(seg), allocated, committed);
 #endif // FEATURE_BASICFREEZE
 }
 
index da0085c..1a73add 100644 (file)
@@ -2380,6 +2380,7 @@ private:
 #ifdef FEATURE_BASICFREEZE
     PER_HEAP_METHOD BOOL insert_ro_segment (heap_segment* seg);
     PER_HEAP_METHOD void remove_ro_segment (heap_segment* seg);
+    PER_HEAP_METHOD void update_ro_segment (heap_segment* seg, uint8_t* allocated, uint8_t* committed);
 #endif //FEATURE_BASICFREEZE
     PER_HEAP_METHOD BOOL set_ro_segment_in_range (heap_segment* seg);
 #ifndef USE_REGIONS
index 2103ba6..7633fb9 100644 (file)
@@ -2451,12 +2451,24 @@ public:
     }
     static FrozenObjectHeapManager* GetFrozenObjectHeapManager()
     {
-        WRAPPER_NO_CONTRACT;
-        if (m_FrozenObjectHeapManager == NULL)
+        CONTRACTL
+        {
+            THROWS;
+            MODE_COOPERATIVE;
+        }
+        CONTRACTL_END;
+
+        if (VolatileLoad(&m_FrozenObjectHeapManager) == nullptr)
         {
             LazyInitFrozenObjectsHeap();
         }
-        return m_FrozenObjectHeapManager;
+        return VolatileLoad(&m_FrozenObjectHeapManager);
+    }
+    static FrozenObjectHeapManager* GetFrozenObjectHeapManagerNoThrow()
+    {
+        LIMITED_METHOD_CONTRACT;
+
+        return VolatileLoad(&m_FrozenObjectHeapManager);
     }
 #endif // DACCESS_COMPILE
 
index 4549215..41af231 100644 (file)
@@ -10,7 +10,8 @@
 #define FOH_COMMIT_SIZE (64 * 1024)
 
 FrozenObjectHeapManager::FrozenObjectHeapManager():
-    m_Crst(CrstFrozenObjectHeap, CRST_UNSAFE_COOPGC),
+    m_Crst(CrstFrozenObjectHeap, CRST_UNSAFE_ANYMODE),
+    m_SegmentRegistrationCrst(CrstFrozenObjectHeap),
     m_CurrentSegment(nullptr)
 {
 }
@@ -18,7 +19,9 @@ FrozenObjectHeapManager::FrozenObjectHeapManager():
 // Allocates an object of the give size (including header) on a frozen segment.
 // May return nullptr if object is too large (larger than FOH_COMMIT_SIZE)
 // in such cases caller is responsible to find a more appropriate heap to allocate it
-Object* FrozenObjectHeapManager::TryAllocateObject(PTR_MethodTable type, size_t objectSize, bool publish)
+
+Object* FrozenObjectHeapManager::TryAllocateObject(PTR_MethodTable type, size_t objectSize,
+    void(*initFunc)(Object*, void*), void* pParam)
 {
     CONTRACTL
     {
@@ -33,64 +36,83 @@ Object* FrozenObjectHeapManager::TryAllocateObject(PTR_MethodTable type, size_t
 #else // FEATURE_BASICFREEZE
 
     Object* obj = nullptr;
-    {
-        CrstHolder ch(&m_Crst);
-
-        _ASSERT(type != nullptr);
-        _ASSERT(FOH_COMMIT_SIZE >= MIN_OBJECT_SIZE);
-
-        // Currently we don't support frozen objects with special alignment requirements
-        // TODO: We should also give up on arrays of doubles on 32-bit platforms.
-        // (we currently never allocate them on frozen segments)
-    #ifdef FEATURE_64BIT_ALIGNMENT
-        if (type->RequiresAlign8())
-        {
-            // Align8 objects are not supported yet
-            return nullptr;
-        }
-    #endif
+    FrozenObjectSegment* curSeg = nullptr;
+    uint8_t* curSegmentCurrent = nullptr;
+    size_t curSegSizeCommitted = 0;
 
-        // NOTE: objectSize is expected be the full size including header
-        _ASSERT(objectSize >= MIN_OBJECT_SIZE);
-
-        if (objectSize > FOH_COMMIT_SIZE)
+    {
+        GCX_PREEMP();
         {
-            // The current design doesn't allow objects larger than FOH_COMMIT_SIZE and
-            // since FrozenObjectHeap is just an optimization, let's not fill it with huge objects.
-            return nullptr;
-        }
-
-        if (m_CurrentSegment == nullptr)
+            CrstHolder ch(&m_Crst);
+
+            _ASSERT(type != nullptr);
+            _ASSERT(FOH_COMMIT_SIZE >= MIN_OBJECT_SIZE);
+
+            // Currently we don't support frozen objects with special alignment requirements
+            // TODO: We should also give up on arrays of doubles on 32-bit platforms.
+            // (we currently never allocate them on frozen segments)
+#ifdef FEATURE_64BIT_ALIGNMENT
+            if (type->RequiresAlign8())
+            {
+                // Align8 objects are not supported yet
+                return nullptr;
+            }
+#endif
+
+            // NOTE: objectSize is expected be the full size including header
+            _ASSERT(objectSize >= MIN_OBJECT_SIZE);
+
+            if (objectSize > FOH_COMMIT_SIZE)
+            {
+                // The current design doesn't allow objects larger than FOH_COMMIT_SIZE and
+                // since FrozenObjectHeap is just an optimization, let's not fill it with huge objects.
+                return nullptr;
+            }
+
+            obj = m_CurrentSegment == nullptr ? nullptr : m_CurrentSegment->TryAllocateObject(type, objectSize);
+            // obj is nullptr if the current segment is full or hasn't been allocated yet
+            if (obj == nullptr)
+            {
+                size_t newSegmentSize = FOH_SEGMENT_DEFAULT_SIZE;
+                if (m_CurrentSegment != nullptr)
+                {
+                    // Double the reserved size to reduce the number of frozen segments in apps with lots of frozen objects
+                    // Use the same size in case if prevSegmentSize*2 operation overflows.
+                    const size_t prevSegmentSize = m_CurrentSegment->m_Size;
+                    newSegmentSize = max(prevSegmentSize, prevSegmentSize * 2);
+                }
+
+                m_CurrentSegment = new FrozenObjectSegment(newSegmentSize);
+                m_FrozenSegments.Append(m_CurrentSegment);
+
+                // Try again
+                obj = m_CurrentSegment->TryAllocateObject(type, objectSize);
+
+                // This time it's not expected to be null
+                _ASSERT(obj != nullptr);
+            }
+
+            if (initFunc != nullptr)
+            {
+                initFunc(obj, pParam);
+            }
+
+            curSeg = m_CurrentSegment;
+            curSegSizeCommitted = curSeg->m_SizeCommitted;
+            curSegmentCurrent = curSeg->m_pCurrent;
+        } // end of m_Crst lock
+
+        // Let GC know about the new segment or changes in it.
+        // We do it under a new lock because the main one (m_Crst) can be used by Profiler in a GC's thread
+        // and that might cause deadlocks since RegisterFrozenSegment may stuck on GC's lock.
         {
-            // Create the first segment on first allocation
-            m_CurrentSegment = new FrozenObjectSegment(FOH_SEGMENT_DEFAULT_SIZE);
-            m_FrozenSegments.Append(m_CurrentSegment);
-            _ASSERT(m_CurrentSegment != nullptr);
+            CrstHolder regLock(&m_SegmentRegistrationCrst);
+            curSeg->RegisterOrUpdate(curSegmentCurrent, curSegSizeCommitted);
         }
 
-        obj = m_CurrentSegment->TryAllocateObject(type, objectSize);
-
-        // The only case where it can be null is when the current segment is full and we need
-        // to create a new one
-        if (obj == nullptr)
-        {
-            // Double the reserved size to reduce the number of frozen segments in apps with lots of frozen objects
-            // Use the same size in case if prevSegmentSize*2 operation overflows.
-            size_t prevSegmentSize = m_CurrentSegment->GetSize();
-            m_CurrentSegment = new FrozenObjectSegment(max(prevSegmentSize, prevSegmentSize * 2));
-            m_FrozenSegments.Append(m_CurrentSegment);
-
-            // Try again
-            obj = m_CurrentSegment->TryAllocateObject(type, objectSize);
+    } // end of GCX_PREEMP
 
-            // This time it's not expected to be null
-            _ASSERT(obj != nullptr);
-        }
-    }
-    if (publish)
-    {
-        PublishFrozenObject(obj);
-    }
+    PublishFrozenObject(obj);
 
     return obj;
 #endif // !FEATURE_BASICFREEZE
@@ -101,10 +123,10 @@ Object* FrozenObjectHeapManager::TryAllocateObject(PTR_MethodTable type, size_t
 FrozenObjectSegment::FrozenObjectSegment(size_t sizeHint) :
     m_pStart(nullptr),
     m_pCurrent(nullptr),
+    m_pCurrentRegistered(nullptr),
     m_SizeCommitted(0),
     m_Size(sizeHint),
     m_SegmentHandle(nullptr)
-    COMMA_INDEBUG(m_ObjectsCount(0))
 {
     _ASSERT(m_Size > FOH_COMMIT_SIZE);
     _ASSERT(m_Size % FOH_COMMIT_SIZE == 0);
@@ -135,34 +157,59 @@ FrozenObjectSegment::FrozenObjectSegment(size_t sizeHint) :
         ThrowOutOfMemory();
     }
 
+    m_pStart = static_cast<uint8_t*>(committedAlloc);
+    m_pCurrent = m_pStart + sizeof(ObjHeader);
+    m_SizeCommitted = FOH_COMMIT_SIZE;
+
     // ClrVirtualAlloc is expected to be PageSize-aligned so we can expect
     // DATA_ALIGNMENT alignment as well
     _ASSERT(IS_ALIGNED(committedAlloc, DATA_ALIGNMENT));
+}
 
-    segment_info si;
-    si.pvMem = committedAlloc;
-    si.ibFirstObject = sizeof(ObjHeader);
-    si.ibAllocated = si.ibFirstObject;
-    si.ibCommit = FOH_COMMIT_SIZE;
-    si.ibReserved = m_Size;
-
-    m_SegmentHandle = GCHeapUtilities::GetGCHeap()->RegisterFrozenSegment(&si);
-    if (m_SegmentHandle == nullptr)
+void FrozenObjectSegment::RegisterOrUpdate(uint8_t* current, size_t sizeCommited)
+{
+    CONTRACTL
     {
-        ClrVirtualFree(alloc, 0, MEM_RELEASE);
-        ThrowOutOfMemory();
+        THROWS;
+        MODE_PREEMPTIVE;
     }
+    CONTRACTL_END
 
-    m_pStart = static_cast<uint8_t*>(committedAlloc);
-    m_pCurrent = m_pStart + sizeof(ObjHeader);
-    m_SizeCommitted = si.ibCommit;
-    INDEBUG(m_ObjectsCount = 0);
-    return;
+    if (m_pCurrentRegistered == nullptr)
+    {
+        segment_info si;
+        si.pvMem = m_pStart;
+        si.ibFirstObject = sizeof(ObjHeader);
+        si.ibAllocated = (size_t)current;
+        si.ibCommit = sizeCommited;
+        si.ibReserved = m_Size;
+
+        // NOTE: RegisterFrozenSegment may take a GC lock inside.
+        m_SegmentHandle = GCHeapUtilities::GetGCHeap()->RegisterFrozenSegment(&si);
+        if (m_SegmentHandle == nullptr)
+        {
+            ThrowOutOfMemory();
+        }
+        m_pCurrentRegistered = current;
+    }
+    else
+    {
+        if (current > m_pCurrentRegistered)
+        {
+            GCHeapUtilities::GetGCHeap()->UpdateFrozenSegment(
+                m_SegmentHandle, current, m_pStart + sizeCommited);
+            m_pCurrentRegistered = current;
+        }
+        else
+        {
+            // Some other thread already advanced it.
+        }
+    }
 }
 
 Object* FrozenObjectSegment::TryAllocateObject(PTR_MethodTable type, size_t objectSize)
 {
-    _ASSERT(m_pStart != nullptr && m_Size > 0 && m_SegmentHandle != nullptr); // Expected to be inited
+    _ASSERT((m_pStart != nullptr) && (m_Size > 0));
     _ASSERT(IS_ALIGNED(m_pCurrent, DATA_ALIGNMENT));
     _ASSERT(IS_ALIGNED(objectSize, DATA_ALIGNMENT));
     _ASSERT(objectSize <= FOH_COMMIT_SIZE);
@@ -194,16 +241,11 @@ Object* FrozenObjectSegment::TryAllocateObject(PTR_MethodTable type, size_t obje
         m_SizeCommitted += FOH_COMMIT_SIZE;
     }
 
-    INDEBUG(m_ObjectsCount++);
-
     Object* object = reinterpret_cast<Object*>(m_pCurrent);
     object->SetMethodTable(type);
 
     m_pCurrent += objectSize;
 
-    // Notify GC that we bumped the pointer and, probably, committed more memory in the reserved part
-    GCHeapUtilities::GetGCHeap()->UpdateFrozenSegment(m_SegmentHandle, m_pCurrent, m_pStart + m_SizeCommitted);
-
     return object;
 }
 
index d2c0bb6..e191731 100644 (file)
@@ -27,10 +27,12 @@ class FrozenObjectHeapManager
 {
 public:
     FrozenObjectHeapManager();
-    Object* TryAllocateObject(PTR_MethodTable type, size_t objectSize, bool publish = true);
+    Object* TryAllocateObject(PTR_MethodTable type, size_t objectSize,
+        void(*initFunc)(Object*,void*) = nullptr, void* pParam = nullptr);
 
 private:
     Crst m_Crst;
+    Crst m_SegmentRegistrationCrst;
     SArray<FrozenObjectSegment*> m_FrozenSegments;
     FrozenObjectSegment* m_CurrentSegment;
 
@@ -43,10 +45,7 @@ class FrozenObjectSegment
 public:
     FrozenObjectSegment(size_t sizeHint);
     Object* TryAllocateObject(PTR_MethodTable type, size_t objectSize);
-    size_t GetSize() const
-    {
-        return m_Size;
-    }
+    void RegisterOrUpdate(uint8_t* current, size_t sizeCommited);
 
 private:
     Object* GetFirstObject() const;
@@ -55,12 +54,20 @@ private:
     // Start of the reserved memory, the first object starts at "m_pStart + sizeof(ObjHeader)" (its pMT)
     uint8_t* m_pStart;
 
+    // NOTE: To handle potential race conditions, only m_[x]Registered fields should be accessed
+    // externally as they guarantee that GC is aware of the current state of the segment.
+
     // Pointer to the end of the current segment, ready to be used as a pMT for a new object
     // meaning that "m_pCurrent - sizeof(ObjHeader)" is the actual start of the new object (header).
     //
     // m_pCurrent <= m_SizeCommitted
     uint8_t* m_pCurrent;
 
+    // Last known value of m_pCurrent that GC is aware of.
+    //
+    // m_pCurrentRegistered <= m_pCurrent
+    uint8_t* m_pCurrentRegistered;
+
     // Memory committed in the current segment
     //
     // m_SizeCommitted <= m_pStart + FOH_SIZE_RESERVED
@@ -70,10 +77,10 @@ private:
     size_t m_Size;
 
     segment_handle m_SegmentHandle;
-    INDEBUG(size_t m_ObjectsCount);
 
     friend class ProfilerObjectEnum;
     friend class ProfToEEInterfaceImpl;
+    friend class FrozenObjectHeapManager;
 };
 
 #endif // _FROZENOBJECTHEAP_H
index e3c882f..8e8a464 100644 (file)
@@ -555,18 +555,18 @@ OBJECTREF TryAllocateFrozenSzArray(MethodTable* pArrayMT, INT32 cElements)
 #endif
 
     FrozenObjectHeapManager* foh = SystemDomain::GetFrozenObjectHeapManager();
-    ArrayBase* orArray = static_cast<ArrayBase*>(foh->TryAllocateObject(pArrayMT, PtrAlign(totalSize), /*publish*/ false));
+    ArrayBase* orArray = static_cast<ArrayBase*>(
+        foh->TryAllocateObject(pArrayMT, PtrAlign(totalSize), [](Object* obj, void* elemCntPtr){
+            // Initialize newly allocated object before publish
+            static_cast<ArrayBase*>(obj)->m_NumComponents = *static_cast<DWORD*>(elemCntPtr);
+        }, &cElements));
+
     if (orArray == nullptr)
     {
         // We failed to allocate on a frozen segment, fallback to AllocateSzArray
         // E.g. if the array is too big to fit on a frozen segment
         return NULL;
     }
-    orArray->m_NumComponents = cElements;
-
-    // Publish needs to be postponed in this case because we need to specify array length 
-    PublishObjectAndNotify(orArray, GC_ALLOC_NO_FLAGS);
-
     return ObjectToOBJECTREF(orArray);
 }
 
@@ -968,12 +968,15 @@ STRINGREF AllocateString(DWORD cchStringLength, bool preferFrozenHeap, bool* pIs
     if (preferFrozenHeap)
     {
         FrozenObjectHeapManager* foh = SystemDomain::GetFrozenObjectHeapManager();
-        orString = static_cast<StringObject*>(foh->TryAllocateObject(g_pStringClass, totalSize, /* publish = */false));
+
+        orString = static_cast<StringObject*>(foh->TryAllocateObject(
+            g_pStringClass, totalSize, [](Object* obj, void* pStrLen) {
+                // Initialize newly allocated object before publish
+                static_cast<StringObject*>(obj)->SetStringLength(*static_cast<DWORD*>(pStrLen));
+            }, &cchStringLength));
+
         if (orString != nullptr)
         {
-            orString->SetStringLength(cchStringLength);
-            // Publish needs to be postponed in this case because we need to specify string length 
-            PublishObjectAndNotify(orString, GC_ALLOC_NO_FLAGS);
             _ASSERTE(orString->GetBuffer()[cchStringLength] == W('\0'));
             orStringRef = ObjectToSTRINGREF(orString);
             *pIsFrozen = true;
@@ -1139,7 +1142,7 @@ OBJECTREF TryAllocateFrozenObject(MethodTable* pObjMT)
 #endif // FEATURE_64BIT_ALIGNMENT
 
     FrozenObjectHeapManager* foh = SystemDomain::GetFrozenObjectHeapManager();
-    Object* orObject = foh->TryAllocateObject(pObjMT, PtrAlign(pObjMT->GetBaseSize()), /*publish*/ true);
+    Object* orObject = foh->TryAllocateObject(pObjMT, PtrAlign(pObjMT->GetBaseSize()));
 
     return ObjectToOBJECTREF(orObject);
 }
index 1016a80..688bf29 100644 (file)
@@ -4187,8 +4187,9 @@ void MethodTable::AllocateRegularStaticBox(FieldDesc* pField, Object** boxedStat
         THROWS;
         GC_TRIGGERS;
         MODE_COOPERATIVE;
-        CONTRACTL_END;
     }
+    CONTRACTL_END
+
     _ASSERT(pField->IsStatic() && !pField->IsSpecialStatic() && pField->IsByValue());
 
     // Static fields are not pinned in collectible types so we need to protect the address
@@ -4222,8 +4223,8 @@ OBJECTREF MethodTable::AllocateStaticBox(MethodTable* pFieldMT, BOOL fPinned, OB
         THROWS;
         GC_TRIGGERS;
         MODE_COOPERATIVE;
-        CONTRACTL_END;
     }
+    CONTRACTL_END
 
     _ASSERTE(pFieldMT->IsValueType());
 
index 1d19a87..c55d40f 100644 (file)
@@ -103,7 +103,12 @@ BOOL ProfilerObjectEnum::Init()
     }
     CONTRACTL_END;
 
-    FrozenObjectHeapManager* foh = SystemDomain::GetFrozenObjectHeapManager();
+    FrozenObjectHeapManager* foh = SystemDomain::GetFrozenObjectHeapManagerNoThrow();
+    if (foh == nullptr)
+    {
+        return TRUE;
+    }
+
     CrstHolder ch(&foh->m_Crst);
 
     const unsigned segmentsCount = foh->m_FrozenSegments.GetCount();
@@ -113,7 +118,6 @@ BOOL ProfilerObjectEnum::Init()
         for (unsigned segmentIdx = 0; segmentIdx < segmentsCount; segmentIdx++)
         {
             const FrozenObjectSegment* segment = segments[segmentIdx];
-
             Object* currentObj = segment->GetFirstObject();
             while (currentObj != nullptr)
             {
index 1f2e4f2..11853ac 100644 (file)
@@ -7672,7 +7672,12 @@ HRESULT ProfToEEInterfaceImpl::GetNonGCHeapBounds(ULONG cObjectRanges,
         return E_INVALIDARG;
     }
 
-    FrozenObjectHeapManager* foh = SystemDomain::GetFrozenObjectHeapManager();
+    FrozenObjectHeapManager* foh = SystemDomain::GetFrozenObjectHeapManagerNoThrow();
+    if (foh == nullptr)
+    {
+        *pcObjectRanges = 0;
+        return S_OK;
+    }
     CrstHolder ch(&foh->m_Crst);
 
     const unsigned segmentsCount = foh->m_FrozenSegments.GetCount();