Marge Bot pushed to branch master at Glasgow Haskell Compiler / GHC Commits: 4bd193fa by Sylvain Henry at 2026-08-13T10:17:00-04:00 RTS: fix LDV profiler's slop skipping (#27585) processHeapForDead was the one heap scanner not updated for the slop marker encoding introduced in #19048. It still assumed slop is zeroed: while (p < bd->free && !*p) p++; // skip slop so it stopped at the (StgWord)(-1) sentinel and passed it to processHeapClosureForDead. IS_FORWARDING_PTR(-1) holds, hence a garbage size was read out of LDVW and the scan ran off the block, tripping ASSERT(p == bd->free) on a debug RTS and silently corrupting the census otherwise. The loop was hand-copied in four places, so factor it out into skipSlop in ClosureMacros.h and use it in ProfHeap.c, Sanity.c, Printer.c and LdvProfile.c. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> - - - - - 11 changed files: - changelog.d/fix-heap-census-large-arrays-19048 - rts/LdvProfile.c - rts/PrimOps.cmm - rts/Printer.c - rts/ProfHeap.c - rts/include/rts/storage/ClosureMacros.h - rts/sm/Sanity.c - rts/sm/Storage.c - + testsuite/tests/rts/T27585.hs - + testsuite/tests/rts/T27585.stdout - testsuite/tests/rts/all.T Changes: ===================================== changelog.d/fix-heap-census-large-arrays-19048 ===================================== @@ -2,5 +2,5 @@ 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 +issues: #19048 #27585 +mrs: !15685 !16452 ===================================== rts/LdvProfile.c ===================================== @@ -177,8 +177,8 @@ processHeapForDead( bdescr *bd ) p = bd->start; while (p < bd->free) { p += processHeapClosureForDead((StgClosure *)p); - while (p < bd->free && !*p) // skip slop - p++; + // See Note [Skipping slop when scanning the heap] in ClosureMacros.h + p = skipSlop(p, bd->free); } ASSERT(p == bd->free); bd = bd->link; ===================================== rts/PrimOps.cmm ===================================== @@ -223,9 +223,8 @@ stg_isMutableByteArrayWeaklyPinnedzh ( gcptr mba ) /* 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. + * slop at address `slop_start`, we write an O(1) marker so that linear heap + * scans can skip over the slop without reading stale heap pointers. * * The marker scheme (let n = number of slop words): * @@ -239,8 +238,8 @@ stg_isMutableByteArrayWeaklyPinnedzh ( gcptr mba ) * * 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). + * to the next live closure. See Note [Skipping slop when scanning the heap] + * in ClosureMacros.h. */ // shrink size of MutableByteArray in-place ===================================== rts/Printer.c ===================================== @@ -1001,19 +1001,9 @@ 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 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; - } - } + // See Note [Skipping slop when scanning the heap] + // in ClosureMacros.h + r = skipSlop(r, bd->free); if (!LOOKS_LIKE_CLOSURE_PTR(r)) { debugBelch("%p found at %p, no closure at %p\n", p, q, r); ===================================== rts/ProfHeap.c ===================================== @@ -1317,37 +1317,8 @@ heapCensusBlock(Census *census, bdescr *bd) p += size; - /* 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; - } - } + /* See Note [Skipping slop when scanning the heap] in ClosureMacros.h */ + p = skipSlop(p, bd->free); } } @@ -1472,8 +1443,6 @@ heapCensusChain( Census *census, bdescr *bd ) // of the associated block descriptor, thus introducing slop at the end // of the object. This slop remains after GC, violating the assumption // of the loop below that all slop has been eliminated (#11627). - // The slop isn't always zeroed (e.g. in non-profiling mode, cf - // OVERWRITING_CLOSURE_OFS). // Consequently, we handle large ARR_WORDS objects as a special case. if (bd->flags & BF_LARGE) { StgPtr p = bd->start; ===================================== rts/include/rts/storage/ClosureMacros.h ===================================== @@ -634,6 +634,44 @@ INLINE_HEADER void writeSlopMarker(StgWord *slop, StgWord n) } } +// Note [Skipping slop when scanning the heap] +// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +// Every linear scan of the heap (heap census, LDV census, sanity checker, +// findPtr) has to step over slop between closures. Slop comes in two forms: +// +// 1. Zero words: alignment padding for large/pinned objects, zeroed +// explicitly (see MEMSET_SLOP_W in allocatePinned). +// +// 2. Slop markers: written by writeSlopMarker whenever a closure is +// overwritten by a smaller one. See Note [shrink-array slop marker] +// in PrimOps.cmm for the encoding. +// +// A closure can be shrunk repeatedly, leaving consecutive slop regions, so we +// must loop until reaching a word that can't be slop. Info pointers are never +// 0 or (StgWord)(-1), so such a word starts the next closure. +// +// All scanners must use skipSlop: if any one of them keeps its own copy of +// this loop it will go out of sync with the encoding (#27585). +INLINE_HEADER StgPtr skipSlop(StgPtr p, StgPtr end) +{ + while (p < end) { + if (!*p) { + // single-word slop region, or alignment padding + p++; + } else if (*p == (StgWord)(-1)) { + // Multi-word slop region: sentinel, count, then that many words. + // writeSlopMarker only writes the sentinel for n >= 2, so the count + // word is always within the region. We assert rather than bail out + // so that a corrupt heap is reported instead of silently skipped. + ASSERT(p + 1 < end); + p += 2 + *(p + 1); + } else { + break; + } + } + return p; +} + INLINE_HEADER void markImmutableSlop (StgClosure *p, uint32_t offset, /*< offset to start marking at, in words */ ===================================== rts/sm/Sanity.c ===================================== @@ -605,19 +605,9 @@ void checkHeapChain (bdescr *bd) ASSERT( size >= MIN_PAYLOAD_SIZE + sizeofW(StgHeader) ); p += size; - /* 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; - } - } + /* See Note [Skipping slop when scanning the heap] + in ClosureMacros.h */ + p = skipSlop(p, bd->free); } } } ===================================== rts/sm/Storage.c ===================================== @@ -1035,9 +1035,10 @@ accountAllocation(Capability *cap, W_ n) * leave slop behind depending on the size of the closure being * overwritten. See Note [marking slop when overwriting immutable closures]. * - * 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] + * To allow the heap profiler, the LDV profiler and the sanity checker to + * linearly scan over heap blocks, slop must be identifiable without reading + * stale heap pointers. + * See Note [Skipping slop when scanning the heap] in ClosureMacros.h * * Shrunk-array slop has a further, concurrent reader: the non-moving GC mark * thread scans SmallMutArrPtrs payloads while the mutator may be shrinking @@ -1207,7 +1208,7 @@ allocateMightFail (Capability *cap, W_ n) * When profiling we zero the space used for alignment. This allows us to * traverse pinned blocks in the heap profiler. * - * See Note [skipping slop in the heap profiler] + * See Note [Skipping slop when scanning the heap] in ClosureMacros.h */ #define MEMSET_SLOP_W(p, val, len_w) memset(p, val, (len_w) * sizeof(W_)) ===================================== testsuite/tests/rts/T27585.hs ===================================== @@ -0,0 +1,61 @@ +{-# LANGUAGE MagicHash, UnboxedTuples, BlockArguments #-} +module Main where + +import GHC.Exts +import GHC.IO (IO(..)) +import System.Mem (performMajorGC) + +-- Lifted wrapper so SmallMutableArray# can be passed around. +data MArr = MArr (SmallMutableArray# RealWorld Integer) + +-- Variant of T19048 for the LDV (biographical) profiler, +RTS -hb (#27585). +-- +-- The array is promoted to the oldest generation *before* shrinking, so the +-- shrink-array slop marker is written into an old-generation block. The next +-- major GC then runs LdvCensusForDead, whose linear heap scan +-- (processHeapForDead in rts/LdvProfile.c) must skip the slop correctly. +-- +-- The array must stay below LARGE_OBJECT_THRESHOLD (409 words): large objects +-- live on the large_objects chain, which the census does not scan linearly. +main :: IO () +main = do + ma <- newArr + fillArr ma 299 + -- Two major GCs promote the array to the oldest generation. + performMajorGC + performMajorGC + -- Shrink: writes the slop marker over slots [10..299], in place, in an + -- old-generation block. + shrinkArr ma + n <- getSize ma + putStrLn $ "size after shrink = " ++ show n + -- With -hb active, LdvCensusForDead scans the old blocks containing the + -- slop marker. + performMajorGC + x <- readElem ma 0 + putStrLn $ "arr[0] = " ++ show x + putStrLn "survived" + +newArr :: IO MArr +newArr = IO \s -> case newSmallArray# 300# (0 :: Integer) s of + (# s', ma #) -> (# s', MArr ma #) + +-- Overwrite every slot with a distinct Integer so each holds a unique, +-- definitely non-zero heap pointer. +fillArr :: MArr -> Int -> IO () +fillArr _ (-1) = pure () +fillArr arr@(MArr ma) n@(I# n#) = do + IO \s -> case writeSmallArray# ma n# (fromIntegral n :: Integer) s of + s' -> (# s', () #) + fillArr arr (n - 1) + +shrinkArr :: MArr -> IO () +shrinkArr (MArr ma) = IO \s -> + case shrinkSmallMutableArray# ma 10# s of s' -> (# s', () #) + +getSize :: MArr -> IO Int +getSize (MArr ma) = IO \s -> + case getSizeofSmallMutableArray# ma s of (# s', n# #) -> (# s', I# n# #) + +readElem :: MArr -> Int -> IO Integer +readElem (MArr ma) (I# i#) = IO \s -> readSmallArray# ma i# s ===================================== testsuite/tests/rts/T27585.stdout ===================================== @@ -0,0 +1,3 @@ +size after shrink = 10 +arr[0] = 0 +survived ===================================== testsuite/tests/rts/all.T ===================================== @@ -710,3 +710,13 @@ test('T19048', , extra_run_opts('+RTS -hT -i0 -RTS') ], compile_and_run, ['-O -rtsopts']) + +test('T27585', + [ omit_ghci + , no_check_hp + , js_skip + , when(have_profiling(), extra_ways(['prof_hb'])) + , only_ways(['prof_hb']) + , extra_run_opts('+RTS -i0 -RTS') + ], + compile_and_run, ['-O -rtsopts']) View it on GitLab: https://gitlab.haskell.org/ghc/ghc/-/commit/4bd193fa999df14d06eabb6119ca87ef... -- View it on GitLab: https://gitlab.haskell.org/ghc/ghc/-/commit/4bd193fa999df14d06eabb6119ca87ef... 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