From 896728dafd176c4d2e9da369478408a6e3f8e43a Mon Sep 17 00:00:00 2001 From: Juan Hoyos Date: Wed, 31 Aug 2022 16:02:29 -0700 Subject: [PATCH] Fix recursive issues for large Regions GC heap enumeration (#3333) * Fix recursive issues for large Regions GC heap enumeration * Ensure check interrupt in other calls of GC algorithms --- src/SOS/Strike/sos.cpp | 36 ++++++++++++++++++++++++++++-------- src/SOS/Strike/sos.h | 23 +++++++++++++++++++++-- src/SOS/Strike/strike.cpp | 30 ++++++++++++------------------ 3 files changed, 61 insertions(+), 28 deletions(-) diff --git a/src/SOS/Strike/sos.cpp b/src/SOS/Strike/sos.cpp index 5764a86dd4..4040947155 100644 --- a/src/SOS/Strike/sos.cpp +++ b/src/SOS/Strike/sos.cpp @@ -586,11 +586,13 @@ namespace sos mCurrObj = mStart < TO_TADDR(mSegment.mem) ? TO_TADDR(mSegment.mem) : mStart; mSegmentEnd = TO_TADDR(mSegment.highAllocMark); - CheckSegmentRange(); + TryAlignToObjectInRange(); } - bool ObjectIterator::NextSegment() + bool ObjectIterator::TryMoveNextSegment() { + CheckInterrupt(); + if (mCurrHeap >= mNumHeaps) { return false; @@ -648,16 +650,30 @@ namespace sos mLastObj = 0; mCurrObj = mStart < TO_TADDR(mSegment.mem) ? TO_TADDR(mSegment.mem) : mStart; mSegmentEnd = TO_TADDR(mSegment.highAllocMark); - return CheckSegmentRange(); + return true; } - bool ObjectIterator::CheckSegmentRange() + bool ObjectIterator::TryMoveToObjectInNextSegmentInRange() { - CheckInterrupt(); + if (TryMoveNextSegment()) + { + return TryAlignToObjectInRange(); + } + return false; + } + + bool ObjectIterator::TryAlignToObjectInRange() + { + CheckInterrupt(); while (!MemOverlap(mStart, mEnd, TO_TADDR(mSegment.mem), mSegmentEnd)) - if (!NextSegment()) + { + CheckInterrupt(); + if (!TryMoveNextSegment()) + { return false; + } + } // At this point we know that the current segment contains objects in // the correct range. However, there's no telling if the user gave us @@ -724,7 +740,7 @@ namespace sos } catch(const sos::Exception &) { - NextSegment(); + TryMoveToObjectInNextSegmentInRange(); } } @@ -742,6 +758,8 @@ namespace sos void ObjectIterator::MoveToNextObject() { + CheckInterrupt(); + // Object::GetSize can be unaligned, so we must align it ourselves. size_t size = (bLarge || bPinned) ? AlignLarge(mCurrObj.GetSize()) : Align(mCurrObj.GetSize()); @@ -773,7 +791,9 @@ namespace sos } if (mCurrObj > mEnd || mCurrObj >= mSegmentEnd) - NextSegment(); + { + TryMoveToObjectInNextSegmentInRange(); + } } SyncBlkIterator::SyncBlkIterator() diff --git a/src/SOS/Strike/sos.h b/src/SOS/Strike/sos.h index 2a58703c85..e4335daa28 100644 --- a/src/SOS/Strike/sos.h +++ b/src/SOS/Strike/sos.h @@ -627,8 +627,27 @@ namespace sos void BuildError(__out_ecount(count) char *out, size_t count, const char *format, ...) const; void AssertSanity() const; - bool NextSegment(); - bool CheckSegmentRange(); + + /* + This function moves to the next segment/region without checking any restrictions + on the range. Returns true if it was able to move to a new segment/region. + */ + bool TryMoveNextSegment(); + + /* + Aligns the iterator to the object that falls in the requested range, moving to + the next segment/region as necessary. The iterator state doesn't change if the + current object already lies in the requested range. Returns true if aligning + to such an object was possible. + */ + bool TryAlignToObjectInRange(); + + /* + Moves to the next segment/region that contains an object in the requested + range and align it to such object. This operation always moves the iterator. + Returns false if no such move was possible. + */ + bool TryMoveToObjectInNextSegmentInRange(); void MoveToNextObject(); private: diff --git a/src/SOS/Strike/strike.cpp b/src/SOS/Strike/strike.cpp index efd0124967..75b646d7c5 100644 --- a/src/SOS/Strike/strike.cpp +++ b/src/SOS/Strike/strike.cpp @@ -11045,6 +11045,7 @@ DECLARE_API(GCWhere) } } + DWORD heapIdx = 0; if (!IsServerBuild()) { DacpGcHeapDetails heapDetails; @@ -11054,13 +11055,7 @@ DECLARE_API(GCWhere) return Status; } - if (GCObjInHeap(taddrObj, heapDetails, trngSeg, gen, allocCtx, bLarge)) - { - ExtOut("Address " WIN64_8SPACES " Gen Heap segment " WIN64_8SPACES " begin " WIN64_8SPACES " allocated " WIN64_8SPACES " size\n"); - ExtOut("%p %d %2d %p %p %p 0x%x(%d)\n", - SOS_PTR(taddrObj), gen, 0, SOS_PTR(trngSeg.segAddr), SOS_PTR(trngSeg.start), SOS_PTR(trngSeg.end), size, size); - bFound = TRUE; - } + bFound = GCObjInHeap(taddrObj, heapDetails, trngSeg, gen, allocCtx, bLarge); } else { @@ -11086,24 +11081,17 @@ DECLARE_API(GCWhere) return Status; } - for (DWORD n = 0; n < dwNHeaps; n ++) + for (heapIdx = 0; heapIdx < dwNHeaps && !bFound; heapIdx++) { DacpGcHeapDetails dacHeapDetails; - if (dacHeapDetails.Request(g_sos, heapAddrs[n]) != S_OK) + if (dacHeapDetails.Request(g_sos, heapAddrs[heapIdx]) != S_OK) { ExtOut("Error requesting details\n"); return Status; } - GCHeapDetails heapDetails(dacHeapDetails, heapAddrs[n]); - if (GCObjInHeap(taddrObj, heapDetails, trngSeg, gen, allocCtx, bLarge)) - { - ExtOut("Address " WIN64_8SPACES " Gen Heap segment " WIN64_8SPACES " begin " WIN64_8SPACES " allocated" WIN64_8SPACES " size\n"); - ExtOut("%p %d %2d %p %p %p 0x%x(%d)\n", - SOS_PTR(taddrObj), gen, n, SOS_PTR(trngSeg.segAddr), SOS_PTR(trngSeg.start), SOS_PTR(trngSeg.end), size, size); - bFound = TRUE; - break; - } + GCHeapDetails heapDetails(dacHeapDetails, heapAddrs[heapIdx]); + bFound = GCObjInHeap(taddrObj, heapDetails, trngSeg, gen, allocCtx, bLarge); } } @@ -11111,6 +11099,12 @@ DECLARE_API(GCWhere) { ExtOut("Address %#p not found in the managed heap.\n", SOS_PTR(taddrObj)); } + else + { + ExtOut("Address " WIN64_8SPACES " Gen Heap segment " WIN64_8SPACES " begin " WIN64_8SPACES " allocated" WIN64_8SPACES " size\n"); + ExtOut("%p %d %2d %p %p %p 0x%x(%d)\n", + SOS_PTR(taddrObj), gen, heapIdx, SOS_PTR(trngSeg.segAddr), SOS_PTR(trngSeg.start), SOS_PTR(trngSeg.end), size, size); + } return Status; }