Marge Bot pushed to branch master at Glasgow Haskell Compiler / GHC Commits: 46d4f963 by Sylvain Henry at 2026-07-29T06:38:40-04:00 RTS: correctly mark slop bytes when shrinking large arrays (#19048) Correctly mark slop bytes even when profiling is off so that heap census doesn't traverse garbage-collected closures. - - - - - 17 changed files: - + changelog.d/fix-heap-census-large-arrays-19048 - rts/Apply.cmm - rts/ZeroSlop.c → rts/MarkSlop.c - rts/PrimOps.cmm - rts/Printer.c - rts/ProfHeap.c - rts/RtsFlags.c - rts/ThreadPaused.c - rts/include/Cmm.h - rts/include/rts/storage/ClosureMacros.h - rts/rts.cabal - rts/sm/NonMovingMark.c - rts/sm/Sanity.c - rts/sm/Storage.c - + testsuite/tests/rts/T19048.hs - + testsuite/tests/rts/T19048.stdout - testsuite/tests/rts/all.T Changes: ===================================== changelog.d/fix-heap-census-large-arrays-19048 ===================================== @@ -0,0 +1,6 @@ +section: rts +synopsis: Correctly mark slop bytes when shrinking large arrays. + Heap census no longer traverses garbage-collected closures when profiling + is off. +issues: #19048 +mrs: !15685 ===================================== rts/Apply.cmm ===================================== @@ -736,8 +736,8 @@ for: R1 = StgAP_STACK_fun(ap); // Because of eager blackholing the closure no longer has correct size so - // threadPaused() can't correctly zero the slop, so we do it here. See #15571 - // and Note [zeroing slop when overwriting closures]. + // threadPaused() can't correctly mark the slop, so we do it here. See #15571 + // and Note [marking slop when overwriting immutable closures]. OVERWRITING_CLOSURE_SIZE(ap, BYTES_TO_WDS(SIZEOF_StgThunkHeader) + 2 + Words); ENTER_R1(); ===================================== rts/ZeroSlop.c → rts/MarkSlop.c ===================================== @@ -2,7 +2,7 @@ * * (c) The GHC Team, 1998-2012 * - * Utilities for zeroing slop callable from Cmm + * Utilities for marking slop callable from Cmm * * N.B. If you are in C you should rather using the inlineable utilities * (e.g. overwritingClosure) defined in ClosureMacros.h. @@ -11,14 +11,14 @@ #include "Rts.h" -void stg_overwritingClosure (StgClosure *p) +void stg_writeSlopMarker (StgWord *slop, StgWord n) { - overwritingClosure(p); + writeSlopMarker(slop, n); } -void stg_overwritingMutableClosureOfs (StgClosure *p, uint32_t offset) +void stg_overwritingClosure (StgClosure *p) { - overwritingMutableClosureOfs(p, offset); + overwritingClosure(p); } void stg_overwritingClosureSize (StgClosure *p, uint32_t size /* in words */) ===================================== rts/PrimOps.cmm ===================================== @@ -216,10 +216,33 @@ stg_isMutableByteArrayWeaklyPinnedzh ( gcptr mba ) * point if it's an older generation block, the mutator won't * allocate into those blocks anyway. * - * If check fails, fall back to the conservative code path: just zero the slop + * If check fails, fall back to the conservative code path: just mark the slop * and return when shrinking, or allocate a new array when growing. */ +/* Note [shrink-array slop marker] + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + * When shrinkSmallMutableArray# or shrinkMutableByteArray# creates n words of + * slop at address `slop_start`, we write an O(1) marker so that the heap + * census (heapCensus in ProfHeap.c) and the sanity checker (checkHeapChain in + * Sanity.c) can skip over the slop without reading stale heap pointers. + * + * The marker scheme (let n = number of slop words): + * + * n == 0 nothing written (no slop) + * n == 1 slop[0] = 0 + * n >= 2 slop[0] = (StgWord)(-1) -- sentinel (0xFFFF...FFFF) + * slop[1] = n - 2 -- words remaining after the two-word header + * + * The sentinel value (StgWord)(-1) is safe because info-table pointers always + * live in the text segment; 0xFFFF...FFFF is never a valid info pointer. + * + * An array may be shrunk multiple times, leaving consecutive slop regions. + * Traversal code must therefore loop over all slop regions before advancing + * to the next live closure. See the while-loops in heapCensusBlock (ProfHeap.c) + * and checkHeapChain (Sanity.c). + */ + // shrink size of MutableByteArray in-place stg_shrinkMutableByteArrayzh ( gcptr mba, W_ new_size ) // MutableByteArray# s -> Int# -> State# s -> State# s @@ -236,7 +259,11 @@ stg_shrinkMutableByteArrayzh ( gcptr mba, W_ new_size ) bd = Bdescr(mba); if (bdescr_free(bd) != mba + WDS(old_wds) || (bd != StgRegTable_rCurrentAlloc(BaseReg) && bd != Capability_pinned_object_block(MyCapability()))) { - OVERWRITING_CLOSURE_MUTABLE(mba, new_wds); + /* fall-back: not at end of nursery block; write slop marker. + See Note [shrink-array slop marker] */ + W_ n; + n = old_wds - new_wds; + foreign "C" stg_writeSlopMarker(mba + WDS(new_wds) "ptr", n); StgArrBytes_bytes(mba) = new_size; // No need to call PROF_HEADER_CREATE. See Note [LDV profiling and resizing arrays] return (); @@ -321,8 +348,16 @@ again: } } - OVERWRITING_CLOSURE_MUTABLE(mba, (BYTES_TO_WDS(SIZEOF_StgSmallMutArrPtrs) + - new_size)); + /* Write slop marker. See Note [shrink-array slop marker] */ + W_ old_size_sma, n_sma, slop_start; + old_size_sma = StgSmallMutArrPtrs_ptrs(mba); + n_sma = old_size_sma - new_size; + slop_start = mba + SIZEOF_StgSmallMutArrPtrs + WDS(new_size); + foreign "C" stg_writeSlopMarker(slop_start "ptr", n_sma); + + // No ordering needed: the concurrent mark thread bounds the slop with the + // marker's own release/acquire (Note [Slop marker memory ordering]), not + // with this store, and reading the new size it never scans the slop. StgSmallMutArrPtrs_ptrs(mba) = new_size; // No need to call PROF_HEADER_CREATE. See Note [LDV profiling and resizing arrays] ===================================== rts/Printer.c ===================================== @@ -1001,8 +1001,19 @@ findPtrBlocks (StgPtr p, bdescr *bd, StgPtr arr[], int arr_size, int i) if (UNTAG_CONST_CLOSURE((StgClosure*)*q) == (const StgClosure *)p) { if (i < arr_size) { for (r = bd->start; r < bd->free; r = end) { - // skip over zeroed-out slop - while (*r == 0) r++; + // skip over marked slop; loop because an array + // may have been shrunk multiple times. + // See Note [shrink-array slop marker] in PrimOps.cmm. + while (r < bd->free) { + if (!*r) { + r++; + } else if (*r == (StgWord)(-1)) { + StgWord skip = *(r + 1); + r += 2 + skip; + } else { + break; + } + } if (!LOOKS_LIKE_CLOSURE_PTR(r)) { debugBelch("%p found at %p, no closure at %p\n", p, q, r); ===================================== rts/ProfHeap.c ===================================== @@ -1317,20 +1317,37 @@ heapCensusBlock(Census *census, bdescr *bd) p += size; - /* skip over slop, see Note [slop on the heap] */ - while (p < bd->free && !*p) p++; - /* Note [skipping slop in the heap profiler] - * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - * We make sure to zero slop that can remain after a major GC so - * here we can assume any slop words we see until the block's free - * pointer are zero. Since info pointers are always nonzero we can - * use this to scan for the next valid heap closure. - * - * Note that not all types of slop are relevant here, only the ones - * that can remain after major GC. So essentially just large objects - * and pinned objects. All other closures will have been packed nice - * and tight into fresh blocks. - */ + /* skip over slop (zero words from large/pinned objects, or + shrink-array slop markers); loop because an array may have been + shrunk multiple times, leaving consecutive slop regions. + See Note [slop on the heap] and Note [shrink-array slop marker] + in PrimOps.cmm. + + Note [skipping slop in the heap profiler] + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + Slop left behind after major GC comes in two forms: + + 1. Zero words: alignment padding for large/pinned objects. + We zero these explicitly (see MEMSET_SLOP_W in allocatePinned). + + 2. Shrink-array slop markers: written by stg_shrinkMutableByteArrayzh + and stg_shrinkSmallMutableArrayzh in all build modes. A single-word + slop region is represented as a zero word; a multi-word region begins + with the sentinel (StgWord)(-1) followed by a count of additional + words. See Note [shrink-array slop marker] in PrimOps.cmm. + + Because an array can be shrunk multiple times, we loop until we + see a word that looks like a valid info pointer. */ + while (p < bd->free) { + if (!*p) { + p++; + } else if (*p == (StgWord)(-1)) { + StgWord skip = *(p + 1); + p += 2 + skip; + } else { + break; + } + } } } ===================================== rts/RtsFlags.c ===================================== @@ -2026,7 +2026,7 @@ static void normaliseRtsOpts (void) #if !defined(PROFILING) && !defined(DEBUG) // The mark-region collector is incompatible with heap census unless - // we zero slop of blackhole'd thunks, which doesn't happen in the + // we mark slop of blackhole'd thunks, which doesn't happen in the // vanilla way. See #9666. if (RtsFlags.ProfFlags.doHeapProfile && RtsFlags.GcFlags.sweep) { barf("The mark-region collector can only be used with profiling\n" ===================================== rts/ThreadPaused.c ===================================== @@ -383,7 +383,7 @@ threadPaused(Capability *cap, StgTSO *tso) } } - // zero out the slop so that the sanity checker can tell + // mark the slop so that the sanity checker can tell // where the next closure is. N.B. We mustn't do this until we have // pushed the free variables to the update remembered set above. OVERWRITING_CLOSURE_SIZE(bh, closure_sizeW_(bh, INFO_PTR_TO_STRUCT(bh_info))); ===================================== rts/include/Cmm.h ===================================== @@ -659,15 +659,9 @@ #if defined(PROFILING) || defined(DEBUG) #define OVERWRITING_CLOSURE_SIZE(c, size) foreign "C" stg_overwritingClosureSize(c "ptr", size) #define OVERWRITING_CLOSURE(c) foreign "C" stg_overwritingClosure(c "ptr") -#define OVERWRITING_CLOSURE_MUTABLE(c, off) foreign "C" stg_overwritingMutableClosureOfs(c "ptr", off) #else #define OVERWRITING_CLOSURE_SIZE(c, size) /* nothing */ #define OVERWRITING_CLOSURE(c) /* nothing */ -/* This is used to zero slop after shrunk arrays. It is important that we do - * this whenever profiling is enabled as described in Note [slop on the heap] - * in Storage.c. */ -#define OVERWRITING_CLOSURE_MUTABLE(c, off) \ - if (TO_W_(RtsFlags_ProfFlags_doHeapProfile(RtsFlags)) != 0) { foreign "C" stg_overwritingMutableClosureOfs(c "ptr", off); } #endif #define IS_STACK_CLEAN(stack) \ ===================================== rts/include/rts/storage/ClosureMacros.h ===================================== @@ -533,29 +533,34 @@ EXTERN_INLINE StgWord8 *mutArrPtrsCard (StgMutArrPtrs *a, W_ n) */ /* - Note [zeroing slop when overwriting closures] - ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - When we overwrite a closure in the heap with a smaller one, in some scenarios - we need to write zero words into "slop"; the memory that is left - unoccupied. See Note [slop on the heap] + Note [marking slop when overwriting immutable closures] + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + When we overwrite a closure in the heap with a smaller one, we need to mark + the "slop" -- the memory that is left unoccupied -- so that the heap can be + linearly scanned. See Note [slop on the heap] - Zeroing slop is required for: + For mutable closures (e.g. shrinking arrays), slop is always marked + unconditionally via writeSlopMarker, in all build modes. + See Note [shrink-array slop marker] in PrimOps.cmm. + + For immutable closures (e.g. thunks overwritten with indirections), slop + marking is only needed for: - full-heap sanity checks (DEBUG, and +RTS -DS), - - LDV profiling (PROFILING, and +RTS -hb) and + - LDV profiling (PROFILING, and +RTS -hb) - However we can get into trouble if we're zeroing slop for ordinarily - immutable closures when using multiple threads, since there is nothing - preventing another thread from still being in the process of reading the - memory we're about to zero. + However we can get into trouble if we're marking slop for immutable closures + when using multiple threads, since there is nothing preventing another thread + from still being in the process of reading the memory we're about to + overwrite. - Thus, with the THREADED RTS and +RTS -N2 or greater we must not zero + Thus, with the THREADED RTS and +RTS -N2 or greater we must not mark immutable closure's slop. Similarly, the concurrent GC's mark thread - may race when a mutator during slop-zeroing. Consequently, we also disable - zeroing when the non-moving GC is in use. + may race with a mutator during slop marking. Consequently, we also disable + marking of immutable closures when the non-moving GC is in use. - Hence, an immutable closure's slop is zeroed when either: + Hence, an immutable closure's slop is marked when either: - PROFILING && era > 0 (LDV is on) && !nonmoving-gc-enabled or - !THREADED && DEBUG @@ -575,15 +580,11 @@ EXTERN_INLINE StgWord8 *mutArrPtrsCard (StgMutArrPtrs *a, W_ n) overwritingClosure(c) #define OVERWRITING_CLOSURE_SIZE(c, size) \ overwritingClosureSize(c, size) -#define OVERWRITING_CLOSURE_MUTABLE(c, off) \ - overwritingMutableClosureOfs(c, off) #else #define OVERWRITING_CLOSURE(c) \ do { (void) sizeof(c); } while(0) #define OVERWRITING_CLOSURE_SIZE(c, size) \ do { (void) sizeof(c); (void) sizeof(size); } while(0) -#define OVERWRITING_CLOSURE_MUTABLE(c, off) \ - do { (void) sizeof(c); (void) sizeof(off); } while(0) #endif #if defined(PROFILING) @@ -591,16 +592,57 @@ void LDV_recordDead (const StgClosure *c, uint32_t size); RTS_PRIVATE bool isInherentlyUsed ( StgHalfWord closure_type ); #endif +// Note [Slop marker memory ordering] +// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +// The non-moving GC mark thread reads SmallMutArrPtrs payload elements +// concurrently with the mutator, which may shrink the array via +// stg_shrinkSmallMutableArrayzh. Shrinking writes a slop marker over the +// vacated elements (see Note [shrink-array slop marker] in PrimOps.cmm): +// +// n == 1: slop[0] = 0 +// n >= 2: slop[0] = -1 (sentinel) +// slop[1] = n-2 (count of further slop words) +// +// The mark thread reads elements with ACQUIRE_LOAD and, at each position i>0, +// re-reads element i-1 after reading i to detect a concurrently written -1 +// sentinel (see rts/sm/NonMovingMark.c). For this check to be sound the +// store of -1 to slop[0] must be visible to the reader when it observes the +// skip count at slop[1]. This is guaranteed by the release-acquire pairing: +// the RELEASE_STORE of n-2 to slop[1] ensures that the prior RELAXED_STORE +// of -1 to slop[0] is visible to any thread that performs an ACQUIRE_LOAD of +// slop[1] and sees the skip count. +// +// The re-read of element i-1 is only performed when the value c just read at +// position i could plausibly be a skip count, i.e. when (StgWord)c < n-i +// (a skip count at position i must satisfy i + skip <= n-1, so skip < n-i). +// Values at or above that bound are valid closure pointers, so no re-read is +// needed. In practice closure pointers are word-aligned kernel addresses and +// far exceed any plausible skip count, so this eliminates virtually all of +// the redundant re-reads. +// +// The non-moving GC only runs in the threaded RTS, where RELEASE_STORE and +// ACQUIRE_LOAD are both __atomic_* operations. The _ALWAYS variants are not +// needed here. +INLINE_HEADER void writeSlopMarker(StgWord *slop, StgWord n) +{ + // See Note [Slop marker memory ordering] + if (n == 1) { + RELAXED_STORE(&slop[0], (StgWord)0); + } else if (n >= 2) { + RELAXED_STORE(&slop[0], (StgWord)(-1)); + RELEASE_STORE(&slop[1], n - 2); + } +} + INLINE_HEADER void -zeroSlop (StgClosure *p, - uint32_t offset, /*< offset to start zeroing at, in words */ - uint32_t size, /*< total closure size, in words */ - bool known_mutable /*< is this a closure who's slop we can always zero? */ +markImmutableSlop (StgClosure *p, + uint32_t offset, /*< offset to start marking at, in words */ + uint32_t size /*< total closure size, in words */ ) { - // see Note [zeroing slop when overwriting closures], also #8402 + // see Note [marking slop when overwriting immutable closures], also #8402 - const bool want_to_zero_immutable_slop = false + const bool want_to_mark = false // Sanity checking (-DS) is enabled || RTS_DEREF(RtsFlags).DebugFlags.sanity #if defined(PROFILING) @@ -609,44 +651,23 @@ zeroSlop (StgClosure *p, #endif ; - const bool can_zero_immutable_slop = + const bool can_mark = // Only if we're running single threaded. getNumCapabilities() == 1 && !RTS_DEREF(RtsFlags).GcFlags.useNonmoving; // see #23170 - const bool zero_slop_immutable = - want_to_zero_immutable_slop && can_zero_immutable_slop; - - const bool zero_slop_mutable = -#if defined(PROFILING) - // Always zero mutable closure slop when profiling. We do this to cover - // the case of shrinking mutable arrays in pinned blocks for the heap - // profiler, see Note [skipping slop in the heap profiler] - // - // TODO: We could make this check more specific and only zero if the - // object is in a BF_PINNED bdescr here. Update Note [slop on the heap] - // and [zeroing slop when overwriting closures] if you change this. - true -#else - zero_slop_immutable -#endif - ; - - const bool zero_slop = - // If we're not sure this is a mutable closure treat it like an - // immutable one. - known_mutable ? zero_slop_mutable : zero_slop_immutable; - - if(!zero_slop) + if(!(want_to_mark && can_mark)) return; - for (uint32_t i = offset; i < size; i++) { - ((StgWord *)p)[i] = 0; - } + // Write a slop marker so that the heap profiler and sanity checker can skip + // over the slop without reading stale heap pointers. + // See Note [shrink-array slop marker] in PrimOps.cmm for the encoding. + writeSlopMarker((StgWord *)p + offset, size - offset); } // N.B. the stg_* variants of the utilities below are only for calling from // Cmm. The INLINE_HEADER functions should be used when in C. +void stg_writeSlopMarker (StgWord *slop, StgWord n); void stg_overwritingClosure (StgClosure *p); INLINE_HEADER void overwritingClosure (StgClosure *p) { @@ -655,31 +676,10 @@ INLINE_HEADER void overwritingClosure (StgClosure *p) if(era > 0 && !isInherentlyUsed(get_itbl(p)->type)) LDV_recordDead(p, size); #endif - zeroSlop(p, sizeofW(StgThunkHeader), size, /*known_mutable=*/false); + markImmutableSlop(p, sizeofW(StgThunkHeader), size); } -// Version of 'overwritingClosure' which overwrites only a suffix of a -// closure. The offset is expressed in words relative to 'p' and shall -// be less than or equal to closure_sizeW(p), and usually at least as -// large as the respective thunk header. -void stg_overwritingMutableClosureOfs (StgClosure *p, uint32_t offset); -INLINE_HEADER void overwritingMutableClosureOfs (StgClosure *p, uint32_t offset) -{ - // Since overwritingClosureOfs is only ever called by: - // - // - shrinkMutableByteArray# (ARR_WORDS) and - // - // - shrinkSmallMutableArray# (SMALL_MUT_ARR_PTRS) - // - // we can safely omit the Ldv_recordDead call. Since these closures are - // considered inherently used we don't need to track their destruction. -#if defined(PROFILING) - ASSERT(isInherentlyUsed(get_itbl(p)->type) == true); -#endif - zeroSlop(p, offset, closure_sizeW(p), /*known_mutable=*/true); -} - // Version of 'overwritingClosure' which takes closure size as argument. void stg_overwritingClosureSize (StgClosure *p, uint32_t size /* in words */); INLINE_HEADER void overwritingClosureSize (StgClosure *p, uint32_t size) @@ -691,5 +691,5 @@ INLINE_HEADER void overwritingClosureSize (StgClosure *p, uint32_t size) if(era > 0) LDV_recordDead(p, size); #endif - zeroSlop(p, sizeofW(StgThunkHeader), size, /*known_mutable=*/false); + markImmutableSlop(p, sizeofW(StgThunkHeader), size); } ===================================== rts/rts.cabal ===================================== @@ -473,7 +473,7 @@ library TSANUtils.c WSDeque.c Weak.c - ZeroSlop.c + MarkSlop.c eventlog/EventLog.c eventlog/EventLogWriter.c hooks/FlagDefaults.c ===================================== rts/sm/NonMovingMark.c ===================================== @@ -1663,9 +1663,42 @@ mark_closure (MarkQueue *queue, const StgClosure *p0, StgClosure **origin) case SMALL_MUT_ARR_PTRS_FROZEN_CLEAN: case SMALL_MUT_ARR_PTRS_FROZEN_DIRTY: { StgSmallMutArrPtrs *arr = (StgSmallMutArrPtrs *) p; - for (StgWord i = 0; i < arr->ptrs; i++) { - StgClosure **field = &arr->payload[i]; - markQueuePushClosure(queue, ACQUIRE_LOAD(field), field); + StgWord n = arr->ptrs; + if (n == 0) break; + + for (StgWord i = 0; i < n; i++) { + StgClosure *c = ACQUIRE_LOAD(&arr->payload[i]); + // If NULL or -1, we know the rest is slop + if (c == NULL || c == (StgClosure *)(-1)) break; + // A valid skip count at position i must satisfy + // i + skip <= n-1 (sentinel at i-1, count at i, skip more words) + // i.e. skip < n-i. If c is out of that range it cannot be a skip + // count, so we must have read a valid closure pointer. + bool maybe_slop_count = (StgWord)c < n - i; + if (maybe_slop_count && i != 0) { + // Otherwise re-read the previous element: the mutator may have + // written -1 there after we last saw it, making the current + // word the skip count rather than a valid closure pointer. + // + // The ACQUIRE_LOAD of payload[i] above synchronizes with the + // RELEASE_STORE in writeSlopMarker, so a RELAXED_LOAD suffices + // here; see Note [Slop marker memory ordering] in + // rts/include/rts/storage/ClosureMacros.h. + if (RELAXED_LOAD(&arr->payload[i-1]) == (StgClosure *)(-1)) break; + } + + // Track origin so indirections reached through array elements get + // short-cut (see Note [Origin references in the nonmoving + // collector] in NonMovingMark.h), but only when c cannot be a skip + // count, i.e. c >= n-i. + // The collapse rewrites the cell with a CAS that fires only if it + // still holds c; restricting to c >= n-i guarantees c differs from + // any skip count the mutator could write at this cell while + // concurrently shrinking the array, so the CAS can never clobber a + // slop marker. Real heap addresses are far above n, so in practice + // every element is still short-cut. + StgClosure **origin = maybe_slop_count ? NULL : &arr->payload[i]; + markQueuePushClosure(queue, c, origin); } break; } ===================================== rts/sm/Sanity.c ===================================== @@ -605,9 +605,19 @@ void checkHeapChain (bdescr *bd) ASSERT( size >= MIN_PAYLOAD_SIZE + sizeofW(StgHeader) ); p += size; - /* skip over slop, see Note [slop on the heap] */ - while (p < bd->free && - (*p < 0x1000 || !LOOKS_LIKE_INFO_PTR(*p))) { p++; } + /* skip slop; loop because an array may have been shrunk + multiple times. See Note [slop on the heap] in Storage.c + and Note [shrink-array slop marker] in PrimOps.cmm. */ + while (p < bd->free) { + if (!*p) { + p++; + } else if (*p == (StgWord)(-1)) { + StgWord skip = *(p + 1); + p += 2 + skip; + } else { + break; + } + } } } } @@ -1013,9 +1023,9 @@ static void checkGeneration (generation *gen, // ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ // heap sanity checking doesn't work with SMP for two reasons: // - // * We can't zero the slop. However, we can sanity-check the heap after a + // * We can't mark the slop. However, we can sanity-check the heap after a // major gc, because there is no slop. See also Updates.h and Note - // [zeroing slop when overwriting closures]. + // [marking slop when overwriting immutable closures]. // // * The nonmoving collector may be mutating its large object lists, // unless we were in fact called by the nonmoving collector. ===================================== rts/sm/Storage.c ===================================== @@ -1033,29 +1033,30 @@ accountAllocation(Capability *cap, W_ n) * * During GC the RTS overwrites closures with forwarding pointers, this can * leave slop behind depending on the size of the closure being - * overwritten. See Note [zeroing slop when overwriting closures]. + * overwritten. See Note [marking slop when overwriting immutable closures]. * - * Under various ways we actually zero slop so we can linearly scan over blocks - * of closures. This trick is used by the sanity checking code and the heap - * profiler, see Note [skipping slop in the heap profiler]. + * To allow the heap profiler and sanity checker to linearly scan over heap + * blocks, slop must be identifiable without reading stale heap pointers. + * See Note [skipping slop in the heap profiler] * - * In general we zero: + * Shrunk-array slop has a further, concurrent reader: the non-moving GC mark + * thread scans SmallMutArrPtrs payloads while the mutator may be shrinking + * them, so it must identify the slop with the right memory ordering. See Note + * [Slop marker memory ordering] in ClosureMacros.h. * + * For pinned/large-object alignment slop we use explicit zeroing: * - Pinned object alignment slop, see MEMSET_SLOP_W in allocatePinned. * - Large object alignment slop, see MEMSET_SLOP_W in allocatePinned. - * - Shrunk array slop, see OVERWRITING_CLOSURE_MUTABLE. * - * Note that this is necessary even in the vanilla (e.g. non-profiling) RTS - * since the user may trigger a heap census via +RTS -hT, which can be used - * even when not linking against the profiled RTS. Failing to zero slop - * due to array shrinking has resulted in a few nasty bugs (#17572, #9666). - * However, since array shrink may result in large amounts of slop (unlike - * alignment), we take care to only zero such slop when heap profiling or DEBUG - * are enabled. + * For shrunk-array slop we write an O(1) marker in all build modes. + * See Note [shrink-array slop marker] in PrimOps.cmm for the encoding. + * This replaces the old approach of zeroing the entire slop region, which was a + * no-op in vanilla (non-profiling, non-debug) builds and caused heap-census + * crashes (#19048, #17572, #9666). * - * When performing LDV profiling or using a (single threaded) debug RTS we zero - * slop even when overwriting immutable closures, see Note [zeroing slop when - * overwriting closures]. + * When performing LDV profiling or using a (single threaded) debug RTS we mark + * slop even when overwriting immutable closures, see Note [marking slop when + * overwriting immutable closures]. */ /* ===================================== testsuite/tests/rts/T19048.hs ===================================== @@ -0,0 +1,49 @@ +{-# LANGUAGE MagicHash, UnboxedTuples, BlockArguments #-} +module Main where + +import GHC.Exts +import GHC.ST (ST(..), runST) +import System.Mem (performMajorGC) + +-- Lifted wrapper so SmallArray# can appear in non-unlifted positions. +data SmallArr a = SmallArr (SmallArray# a) + +main :: IO () +main = do + let arr = buildArr + let n = case arr of SmallArr a -> I# (sizeofSmallArray# a) + putStrLn $ "size after shrink = " ++ show n + -- With +RTS -hT -i0 this triggers heapCensus. + -- The census advances past the 10 live elements, then hits the 490 + -- stale heap pointers in the slop and crashes. + performMajorGC + -- Keep arr alive across the GC by reading from it after. + let v = case arr of SmallArr a -> case indexSmallArray# a 0# of (# x #) -> x + putStrLn $ "arr[0] = " ++ show (v :: Integer) + putStrLn "survived" + +-- Allocate 500 slots, write a DISTINCT Integer to every slot, then +-- shrink to 10. Slots [10..499] become slop: they still hold live, +-- non-zero heap pointers in the raw memory, but the ptrs header field +-- says there are only 10 elements. zeroSlop is a no-op in non-PROFILING +-- builds (even when -hT is active), so the slop is never cleared. +buildArr :: SmallArr Integer +buildArr = runST $ ST \s0 -> + -- All 500 slots start with a non-null initial value. + case newSmallArray# 500# (0 :: Integer) s0 of { (# s1, ma #) -> + -- Overwrite every slot with a distinct Integer so each holds a + -- unique heap pointer (no sharing, definitely non-zero). + case fill 499 ma s1 of { s2 -> + -- Shrink: ptrs = 10, but physical slots [10..499] are NOT zeroed. + case shrinkSmallMutableArray# ma 10# s2 of { s3 -> + case unsafeFreezeSmallArray# ma s3 of { (# s4, a #) -> + (# s4, SmallArr a #) }}}} + +-- Fill slots [0..n] each with a distinct Integer value (n, n-1, ..., 0). +-- 'Integer' guarantees a genuine heap object for every value. +fill :: Int -> SmallMutableArray# s Integer -> State# s -> State# s +fill 0 ma s = writeSmallArray# ma 0# (0 :: Integer) s +fill n ma s = + let I# n# = n + in case writeSmallArray# ma n# (fromIntegral n :: Integer) s of + s' -> fill (n - 1) ma s' ===================================== testsuite/tests/rts/T19048.stdout ===================================== @@ -0,0 +1,3 @@ +size after shrink = 10 +arr[0] = 0 +survived ===================================== testsuite/tests/rts/all.T ===================================== @@ -702,3 +702,11 @@ test('T27123', [when(have_profiling(), extra_ways(['prof']))], compile_and_run, test('T27434', extra_ways(['compacting_gc']), compile_and_run, ['']) + +test('T19048', + [ omit_ghci + , no_check_hp + , js_skip + , extra_run_opts('+RTS -hT -i0 -RTS') + ], + compile_and_run, ['-O -rtsopts']) View it on GitLab: https://gitlab.haskell.org/ghc/ghc/-/commit/46d4f9636d6e5d724b294da12a28d0ac... -- View it on GitLab: https://gitlab.haskell.org/ghc/ghc/-/commit/46d4f9636d6e5d724b294da12a28d0ac... You're receiving this email because of your account on gitlab.haskell.org. Manage all notifications: https://gitlab.haskell.org/-/profile/notifications | Help: https://gitlab.haskell.org/help