From f08fd125233e12cc12db094dfbd6aba0df1ce759 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Carvalho?= Date: Fri, 14 Aug 2026 16:58:59 +0100 Subject: [PATCH 1/4] Follow the chunk chain across a subsegment's holes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit glslang/win-kexp#103: 106,516 VS chunk headers refused on one live 26100 walk, ~196 per extent, and every single one of them failing the same predicate — the chunk runs past the end of its subsegment. One predicate at 100% looked like a predicate that was wrong, and the bound it tests was the suspect. The bound is right, and provably so. `RtlpHpVsSubsegmentCreate` writes `Size = (bytes - first) >> 4` into the subsegment and puts the first chunk at `+ first`, so `region.address + region.size` is exactly where the chunks end. The refusals were not corruption either: against a garbage `Sizes` word that bound is the *only* predicate with any selectivity, since a zero size and a size below the headers are one value each in 65,536. Their expected count across 106,516 refusals is under two apiece. Zero of each is what a correct check does, not evidence against it. What was actually wrong sits one level up. A VS chunk is variable-size, so it can only be found from the end of the chunk before it — but `walk_region` hands `walk_vs` one *committed extent* at a time, and `RtlpHpVsSubsegmentCommitPages` commits and decommits page ranges anywhere inside a subsegment, tracking them in `_HEAP_VS_SUBSEGMENT.CommitBitmap`. Holes are the allocator's steady state. Every extent after one was being decoded from the extent's own first sixteen-byte boundary — a guess — and the refusals were the walk scanning sixteen bytes at a time to find its way back. The refusals were the harmless half: whatever the scan landed on that decoded plausibly went into `spans` as an allocation, tag and all. So follow the chain instead. A decommitted range is always the interior of a free chunk, because the allocator has to keep its own headers readable, which means the chunk before a hole records a size that reaches past it and names the header on the far side. `walk_vs` now takes that address and returns the next one; when it cannot be placed, the extent is skipped rather than guessed at, and `unplaced_bytes` says how much that cost — the number that keeps the refusal honest, since declining to decode buys correctness with coverage. A regression test reproduces the live signature in miniature: a fixture with no corruption and a correct bound produced 128 refusals in one extent, all of them "runs past the end of its subsegment". Also cross-checks the bound against the subsegment's own `Size`, which was read for the signature and thrown away. Reported rather than preferred: it is corroboration from a second source, and trusting it over the descriptor would let one misread field truncate every VS region on a build we resolve wrongly. --- src/pool/index.rs | 3 + src/pool/query.rs | 4 + src/pool/render.rs | 2 + src/pool/snapshot.rs | 309 ++++++++++++++++++++++++++++++++++++++++-- src/pool_extension.rs | 1 + 5 files changed, 310 insertions(+), 9 deletions(-) diff --git a/src/pool/index.rs b/src/pool/index.rs index ed38241..717f088 100644 --- a/src/pool/index.rs +++ b/src/pool/index.rs @@ -39,6 +39,8 @@ pub(crate) struct PoolIndex { pub stalls: WalkStalls, /// Carried through from [`PoolSnapshot::refused_chunks`]. pub refused_chunks: u64, + /// Carried through from [`PoolSnapshot::unplaced_bytes`]. + pub unplaced_bytes: u64, row_postings: HashMap>, span_rows: Vec, } @@ -91,6 +93,7 @@ impl PoolIndex { budget_expired: snapshot.budget_expired, stalls: snapshot.stalls, refused_chunks: snapshot.refused_chunks, + unplaced_bytes: snapshot.unplaced_bytes, row_postings, span_rows, } diff --git a/src/pool/query.rs b/src/pool/query.rs index 88a40a0..b265889 100644 --- a/src/pool/query.rs +++ b/src/pool/query.rs @@ -241,6 +241,9 @@ pub struct PoolSnapshotReport { /// Chunk headers the walk refused and resynchronised past; see /// [`crate::pool::PoolSnapshot::refused_chunks`] for why this is not read off a diagnostic. pub refused_chunks: u64, + /// Committed VS bytes the walk declined to decode because it could not place a chunk + /// boundary in them; see [`crate::pool::PoolSnapshot::unplaced_bytes`]. + pub unplaced_bytes: u64, } /// How much of the pool a walk covered. @@ -319,6 +322,7 @@ fn report_of(index: &PoolIndex) -> PoolSnapshotReport { diagnostics: index.diagnostics.clone(), stalls: index.stalls, refused_chunks: index.refused_chunks, + unplaced_bytes: index.unplaced_bytes, } } diff --git a/src/pool/render.rs b/src/pool/render.rs index 1a7131b..8a72e7b 100644 --- a/src/pool/render.rs +++ b/src/pool/render.rs @@ -414,6 +414,7 @@ mod tests { budget_expired: false, stalls: Default::default(), refused_chunks: 0, + unplaced_bytes: 0, diagnostics: PoolDiagnostics::from_iter([ "per-session paged heaps are not included".to_string() ]), @@ -508,6 +509,7 @@ mod tests { budget_expired: false, stalls: Default::default(), refused_chunks: 0, + unplaced_bytes: 0, diagnostics: PoolDiagnostics::default(), }); let dml_chunks = render_pool_map( diff --git a/src/pool/snapshot.rs b/src/pool/snapshot.rs index 67e8a0d..2bf80d1 100644 --- a/src/pool/snapshot.rs +++ b/src/pool/snapshot.rs @@ -1198,6 +1198,26 @@ fn discover_segment_context( region_address += first as u64; region_size -= first; block_size = 0; + // The bound every VS chunk is checked against is `region_address + region_size`, + // derived from the page-range descriptor. The subsegment carries the same number + // itself, and it is the one the allocator laid the chunks out to: + // `RtlpHpVsSubsegmentCreate` writes `Size = (bytes - first) >> 4` and puts the + // first chunk at `+ first`, so the two must agree exactly. Read and thrown away + // until now — glslang/win-kexp#103 asked whether the descriptor-derived bound was + // systematically short, and this is the check that answers it on any target + // rather than by argument. + // + // Reported, not preferred: `declared` is corroboration from a second source, and + // trusting it over the descriptor would let one misread `Size` truncate every VS + // region on a build whose field we resolve wrongly. A disagreement here is the + // signal to go and look. + let declared_size = usize::from(declared) * 16; + if declared_size != region_size { + discovery.diagnostics.push(format!( + "VS subsegment {address:#x} declares {declared_size:#x} of chunks where \ + its page range leaves room for {region_size:#x}" + )); + } } let descriptor_node = metadata_address + offset as u64 + tree_node_offset as u64; // Two independent ways to be free, and either is enough: the range sits in the @@ -1633,6 +1653,16 @@ pub(crate) struct PoolSnapshot { /// Only `walk_vs` feeds it today; LFH subsegments are refused during discovery, one per /// subsegment, where the count and the message already agree. pub refused_chunks: u64, + /// Committed bytes of a VS subsegment the walk declined to decode, because it could not say + /// where a chunk began in them. + /// + /// The cost of [`SnapshotWalker::walk_vs`] refusing to guess, and the number that keeps that + /// refusal honest: what it buys is that nothing fabricated enters `spans`, and what it costs + /// is coverage, which is invisible unless it is sized. A walk that reports no refusals *and* + /// no unplaced bytes decoded every committed byte of every subsegment it reached; one that + /// reports a large figure here has lost the chunk chain somewhere, and the chain is the only + /// thing that can find a variable-size header. + pub unplaced_bytes: u64, } /// What valid-region queries that could not advance cost the walk, and what stepping over them @@ -1916,6 +1946,12 @@ impl<'a, M: PoolMemory> SnapshotWalker<'a, M> { let mut cursor = region.address; let mut consecutive_stalls = 0u32; let mut stalled_here = false; + // Where the next VS chunk header is, carried across the committed extents of one + // subsegment. `walk_lfh` and `walk_page_ranges` need no such thing — their slots are a + // fixed size, so they index off the region base and are correct wherever an extent starts. + // A VS chunk is only findable from the end of the one before it, which is a fact about + // the region and not about the extent, so it has to live out here. + let mut vs_chunk = Some(region.address); while cursor < requested_end { check_budget(self.memory)?; let remaining = requested_end.saturating_sub(cursor).min(usize::MAX as u64) as usize; @@ -2036,7 +2072,9 @@ impl<'a, M: PoolMemory> SnapshotWalker<'a, M> { } match region.backend { PoolBackend::Lfh => self.walk_lfh(region, valid_base, &bytes, snapshot), - PoolBackend::Vs => self.walk_vs(region, valid_base, &bytes, snapshot), + PoolBackend::Vs => { + vs_chunk = self.walk_vs(region, valid_base, &bytes, vs_chunk, snapshot); + } PoolBackend::Segment => self.walk_page_ranges(region, valid_base, &bytes, snapshot), PoolBackend::Large => return Ok(()), } @@ -2291,7 +2329,29 @@ impl<'a, M: PoolMemory> SnapshotWalker<'a, M> { } } - /// Decodes one committed extent of a VS subsegment, chunk by chunk. + /// Decodes one committed extent of a VS subsegment, chunk by chunk, and says where the + /// chunk after the last one it read begins. + /// + /// **A VS extent can only be decoded from a known chunk boundary.** Chunks vary in size, so + /// unlike [`Self::walk_lfh`] and [`Self::walk_page_ranges`] — whose slots are a fixed size, + /// making them correct at whatever offset into the region an extent happens to start — the + /// only way to know where a header is, is to have walked the one before it. `walk_region` + /// hands this one *committed extent* at a time, and a subsegment is routinely committed in + /// pieces: `RtlpHpVsSubsegmentCommitPages` commits and decommits page ranges anywhere inside + /// the subsegment and records them in `_HEAP_VS_SUBSEGMENT.CommitBitmap`, so holes between + /// committed extents are the allocator's steady state and not damage. + /// + /// Starting each extent at its own first sixteen-byte boundary — which is what this did — is + /// therefore a guess on every extent after a hole, and where glslang/win-kexp#103's 106,516 + /// refusals came from: ~196 per extent, one per sixteen bytes, until the scan wandered onto + /// a word that decoded plausibly. The refusals were the harmless half. Whatever the scan + /// wandered onto was pushed into `spans` as an allocation, tag and all. + /// + /// The chain is what crosses the hole. A decommitted range is always the *interior* of a + /// free chunk — the allocator has to keep its own headers readable — so the chunk before a + /// hole records a size that reaches past it and the next header lands in the next committed + /// extent, at an address this walk already knows. `expected` carries that address between + /// extents. `None` means the walk lost it, and a walk that has lost it does not guess again. /// /// A refused header costs more than itself: the walk no longer knows where the next one /// starts, so it advances sixteen bytes and tries again, and every header after it in the @@ -2299,9 +2359,41 @@ impl<'a, M: PoolMemory> SnapshotWalker<'a, M> { /// One header rewritten while we read it and one systematically misdecoded field therefore /// look identical from a count — which is why what is reported here is the count of /// **chunks** refused rather than of extents that contained a refusal, the failing - /// predicate in its own words, and the `Sizes` word as read. - fn walk_vs(&self, region: &PoolRegion, base: u64, bytes: &[u8], snapshot: &mut PoolSnapshot) { - let mut offset = ((16 - (base as usize & 0xf)) & 0xf).min(bytes.len()); + /// predicate in its own words, and the `Sizes` word as read. It is also why a refusal ends + /// the chain for the whole region: every offset after it rests on that guess. + fn walk_vs( + &self, + region: &PoolRegion, + base: u64, + bytes: &[u8], + expected: Option, + snapshot: &mut PoolSnapshot, + ) -> Option { + let extent_end = base.saturating_add(bytes.len() as u64); + let Some(next) = expected.filter(|next| *next >= base) else { + // Either an earlier extent of this region lost the chain, or the header it pointed + // at fell inside the hole just crossed. Both come to the same thing: nothing in + // these bytes can be placed. Sized and not merely counted, because what this costs + // is coverage — and the alternative, decoding from a guess, costs correctness. + snapshot.unplaced_bytes = snapshot.unplaced_bytes.saturating_add(bytes.len() as u64); + snapshot.diagnostics.push(format!( + "VS extent at {base:#x} does not begin on a chunk boundary; {:#x} bytes not decoded", + bytes.len() + )); + snapshot.complete = false; + return None; + }; + if next >= extent_end { + // A chunk that began before this extent covers all of it — an ordinary large free + // chunk with its interior decommitted. Nothing to decode and nothing lost: the + // expectation still names a header further on. + return Some(next); + } + let mut offset = (next - base) as usize; + // Where the *next* extent resumes, kept in step with `offset` so every way out of the + // loop below leaves it pointing at a header rather than at wherever the bytes ran out. + let mut resume = next; + let mut lost = false; let mut chunks = 0usize; let header_bytes = region.vs_header_size + region.pool_header.size; let subsegment_end = region.address.saturating_add(region.size as u64); @@ -2339,6 +2431,11 @@ impl<'a, M: PoolMemory> SnapshotWalker<'a, M> { refused += 1; resync_from.get_or_insert(header_address); previous_chunk = None; + // The sixteen-byte scan below may find its way back onto real headers + // inside this extent, but it cannot *know* that it has — so whatever it + // ends on is not an address to hand the next extent. Sticky, because a + // chunk decoded after the scan resumes is exactly the guess in question. + lost = true; snapshot.complete = false; offset = offset.saturating_add(16); continue; @@ -2363,6 +2460,15 @@ impl<'a, M: PoolMemory> SnapshotWalker<'a, M> { } let chunk_size = chunk.size; if offset.saturating_add(chunk_size) > bytes.len() { + // The chunk reaches past the committed extent, which after the bound check in + // `decode_vs_chunk` can only mean a hole ahead of it inside the subsegment — + // so this is the ordinary free chunk with a decommitted interior, not a walk + // running out of bytes. It carries the chain over the hole, which is the whole + // reason the expectation is returned rather than recomputed per extent. + // + // Still incomplete, and for the reason `complete` exists: no span is emitted for + // this chunk, so the snapshot omits it however well the walk understands it. + resume = header_address.saturating_add(chunk_size as u64); snapshot.complete = false; break; } @@ -2373,6 +2479,11 @@ impl<'a, M: PoolMemory> SnapshotWalker<'a, M> { let Some(header) = adjust_page_end_header(candidate, region.pool_header.size as u64) else { + // Where this chunk's *pool* header sits could not be worked out, so no span + // is emitted for it. Its size decoded and passed the bound check, though, so + // the chunk chain is not what was lost here and the next extent can still be + // placed. + resume = header_address.saturating_add(chunk_size as u64); snapshot.complete = false; break; }; @@ -2410,6 +2521,7 @@ impl<'a, M: PoolMemory> SnapshotWalker<'a, M> { snapshot.spans.push(span); previous_chunk = Some(chunk_size); offset += chunk_size; + resume = base + offset as u64; chunks += 1; } if let Some(from) = resync_from { @@ -2430,6 +2542,7 @@ impl<'a, M: PoolMemory> SnapshotWalker<'a, M> { .diagnostics .push(format!("VS traversal limit reached at {base:#x}")); } + (!lost).then_some(resume) } fn walk_page_ranges( @@ -2691,7 +2804,7 @@ mod tests { vs.pool_header = no_pool_header; let mut vs_bytes = vs_extent(&[(0x40, 0)]); vs_bytes[0x10..0x14].copy_from_slice(b"VS!!"); - walker.walk_vs(&vs, vs.address, &vs_bytes, &mut snapshot); + walker.walk_vs(&vs, vs.address, &vs_bytes, Some(vs.address), &mut snapshot); let mut page = lfh_region(0x3000, 0x20); page.size = 0x20; @@ -3171,7 +3284,7 @@ mod tests { complete: true, ..PoolSnapshot::default() }; - walker.walk_vs(®ion, VS_BASE, bytes, &mut snapshot); + walker.walk_vs(®ion, VS_BASE, bytes, Some(VS_BASE), &mut snapshot); snapshot } @@ -3467,6 +3580,131 @@ mod tests { ); } + /// A VS subsegment with a hole in it, walked in two committed extents. + /// + /// `bytes` tiles the whole region with chunks the way the allocator does; `holes` names the + /// pages `valid_region` will report as uncommitted. That is what + /// `RtlpHpVsSubsegmentCommitPages` produces on a real target — it commits and decommits page + /// ranges anywhere inside the subsegment and tracks them in `_HEAP_VS_SUBSEGMENT.CommitBitmap` + /// — and it is the shape the old walk had no way to survive. + fn walk_vs_with_holes(chunks: &[(usize, usize)], holes: &[u64]) -> PoolSnapshot { + let bytes = vs_extent(chunks); + let region = vs_region(bytes.len()); + let mut memory = HoleyMemory::new(VS_BASE, bytes); + memory.holes.extend(holes.iter().copied()); + walk_holey(&memory, ®ion) + } + + /// glslang/win-kexp#103: 106,516 VS chunk headers refused on one live 26100 walk, ~196 per + /// extent, every one of them failing the same subsegment-bound check — which read as a bound + /// that was wrong. It was not. Nothing was wrong with any of the three predicates: the walk + /// was handing them a *guess*, because it started every committed extent at that extent's own + /// first sixteen-byte boundary, and only the first extent of a subsegment begins on a chunk. + /// + /// Here the chunk before the hole is 0x2800 bytes and lands the next header at 0x5800, in the + /// middle of the page the walk resumes on. Starting at 0x5000 costs 128 refusals to scan back + /// onto it; following the chain costs none. + #[test] + fn test_a_vs_extent_after_a_hole_resumes_on_the_chunk_the_chain_names() { + let snapshot = walk_vs_with_holes( + &[ + (0x800, 0), + (0x800, 0x800), + // Spans the decommitted page, as the free chunk whose interior was decommitted + // always does, and ends part-way into the page after it. + (0x2800, 0x800), + (0x800, 0x2800), + ], + &[VS_BASE + 0x2000], + ); + + assert_eq!( + snapshot.refused_chunks, + 0, + "the chain names the header, so nothing has to be scanned for: {:?}", + snapshot.diagnostics.examples() + ); + assert_eq!(snapshot.unplaced_bytes, 0); + let allocated: Vec<_> = snapshot + .spans + .iter() + .filter(|span| span.state == PoolState::Allocated) + .map(|span| span.header_address) + .collect(); + // The chunk that spans the hole is not among them: its header was read and understood, + // but the walk emits no span for a chunk it could not read to the end of. + assert_eq!( + allocated, + [VS_BASE + 0x10, VS_BASE + 0x810, VS_BASE + 0x3810], + "the chunk on the far side of the hole is found, and nothing else is invented" + ); + } + + /// The other half of the same rule, and the price of it. When the header the chain names + /// falls *inside* the hole, the walk has no way to know where a chunk begins in the extent + /// after it — so it decodes none of it and says how much that cost. The extent here holds two + /// perfectly good chunks; declining them loses coverage, and decoding from a guess would put + /// whatever a garbage word decoded to into `spans` as an allocation, which is worse. + #[test] + fn test_a_vs_extent_whose_chunk_boundary_fell_in_the_hole_is_not_guessed_at() { + let snapshot = walk_vs_with_holes( + &[ + (0x1000, 0), + (0x1000, 0x1000), + (0x1000, 0x1000), + (0x1000, 0x1000), + ], + &[VS_BASE + 0x2000], + ); + + assert_eq!(snapshot.refused_chunks, 0); + assert_eq!( + snapshot.unplaced_bytes, 0x1000, + "the whole undecodable extent is sized, not just noted" + ); + assert!( + snapshot + .diagnostics + .examples() + .iter() + .any(|message| message.contains("does not begin on a chunk boundary")), + "{:?}", + snapshot.diagnostics.examples() + ); + let allocated: Vec<_> = snapshot + .spans + .iter() + .filter(|span| span.state == PoolState::Allocated) + .map(|span| span.header_address) + .collect(); + assert_eq!( + allocated, + [VS_BASE + 0x10, VS_BASE + 0x1010], + "nothing from the extent the walk could not place reaches the snapshot" + ); + } + + /// An extent entirely inside one chunk. Nothing to decode there and nothing lost: the + /// expectation still names a header further on, so this must not be billed as coverage the + /// walk gave up — which is the difference between a hole in a large free chunk (ordinary) and + /// a chain the walk dropped (not). + #[test] + fn test_an_extent_inside_a_single_chunk_costs_nothing() { + let snapshot = walk_vs_with_holes(&[(0x1000, 0), (0x3000, 0x1000)], &[VS_BASE + 0x2000]); + + assert_eq!(snapshot.unplaced_bytes, 0); + assert_eq!(snapshot.refused_chunks, 0); + assert!( + !snapshot + .diagnostics + .examples() + .iter() + .any(|message| message.contains("does not begin on a chunk boundary")), + "{:?}", + snapshot.diagnostics.examples() + ); + } + /// The reason the `break` was there in the first place: the advance has to be /// unconditional or a query that keeps answering the same way spins forever. Unconditional /// *and* bounded — a region that is dead all the way down costs a fixed number of queries, @@ -4016,7 +4254,16 @@ mod tests { put(bytes, address + 8, tag); } + /// The fixture, with its VS subsegment declaring the chunk area its page range actually + /// leaves — `(2 pages - sizeof(_HEAP_VS_SUBSEGMENT rounded)) / 16`, which is what + /// `RtlpHpVsSubsegmentCreate` writes. Kept in step deliberately: the walk now cross-checks + /// the two, so a fixture that disagreed would put a complaint in every other test's output + /// and hide the one case that should raise it. fn synthetic_memory() -> SyntheticMemory { + synthetic_memory_declaring((0x2000 - 0xfe0) / 16) + } + + fn synthetic_memory_declaring(declared: u16) -> SyntheticMemory { let mut bytes = Writes::default(); fill(&mut bytes, STATE, 0x200); put_u32(&mut bytes, STATE, 1); @@ -4082,8 +4329,8 @@ mod tests { let vs = SEGMENT + 0x3000; fill(&mut bytes, vs, 0x2000); - put_u16(&mut bytes, vs, 0x8000 | (0x2bed ^ 2)); - put_u16(&mut bytes, vs + 2, 2); + put_u16(&mut bytes, vs, 0x8000 | (0x2bed ^ declared)); + put_u16(&mut bytes, vs + 2, declared); let first_chunk = vs + 0xfe0; let cached_chunk = first_chunk + 0x40; let free_chunk = cached_chunk + 0x40; @@ -4180,6 +4427,50 @@ mod tests { } } + /// glslang/win-kexp#103 asked whether the bound every VS chunk is checked against — + /// `region.address + region.size`, from the page-range descriptor — is systematically short, + /// since on a live 26100 walk 106,516 refusals all failed that one check and no other. + /// `RtlpHpVsSubsegmentCreate` writes the same quantity into the subsegment itself as + /// `Size = (bytes - first) >> 4`, so the two are the same number from two sources and the + /// walk can say so on any target instead of anyone arguing it. Both directions, because a + /// check that never fires and a check that always fires read identically from a live run. + #[test] + fn test_a_vs_subsegment_is_measured_against_its_own_declared_size() { + let quiet = walk_synthetic(synthetic_memory()); + let complaint = "declares"; + assert!( + !quiet + .diagnostics + .examples() + .iter() + .any(|message| message.contains(complaint)), + "a subsegment whose two sizes agree draws no complaint: {:?}", + quiet.diagnostics.examples() + ); + + let short = walk_synthetic(synthetic_memory_declaring((0x2000 - 0xfe0) / 16 - 1)); + assert!( + short + .diagnostics + .examples() + .iter() + .any(|message| message.contains(complaint)), + "{:?}", + short.diagnostics.examples() + ); + } + + fn walk_synthetic(memory: SyntheticMemory) -> PoolSnapshot { + let layout = synthetic_layout(); + SnapshotWalker { + memory: &memory, + layout: &layout, + traversal_limit: 1024, + } + .walk(None) + .unwrap() + } + #[test] fn test_pool_snapshot_walks_all_backends() { let memory = synthetic_memory(); diff --git a/src/pool_extension.rs b/src/pool_extension.rs index ae33872..ed9557f 100644 --- a/src/pool_extension.rs +++ b/src/pool_extension.rs @@ -169,6 +169,7 @@ fn command_poolmap(engine: &DebugEngine, args: &str) -> Result<(), String> { budget_expired: filtered.budget_expired, stalls: filtered.stalls, refused_chunks: filtered.refused_chunks, + unplaced_bytes: filtered.unplaced_bytes, spans: filtered.spans, diagnostics: filtered.diagnostics, }; From 9b9ed908df7155ec0874e3022c3015a97dc16de3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Carvalho?= Date: Fri, 14 Aug 2026 17:00:53 +0100 Subject: [PATCH 2/4] Say what the engine answered when a query will not advance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit glslang/win-kexp#104 asked whether `recovered_bytes: 0` — across 1,619 stalls and 6.6MB stepped over on a live 26100 walk — meant nothing was behind those stalls, or meant nothing was being counted. It is the former. `stalled_here` latches for the rest of the region and every later successful read adds to the figure, and `test_a_stalled_query_costs_a_page_not_the_region` already pins it at two pages for a stall with committed memory behind it. So the coverage half of #94 recovers nothing on that target, and the doc now says so rather than leaving a zero for someone to read as a broken counter. Kept, because the cost is bounded at eight queries per dead region and what it replaced was losing every committed page behind one bad one. What is missing to decide it properly is why the query stalls at all, and the walk knew that and threw it away: two different answers arrive at this branch — a region reported *behind* the cursor, and a zero-length region reported ahead of it — and they need opposite fixes. The engine's own answer now travels with the diagnostic, so one live run can tell them apart. Also unmangles the message, which has been emitting twenty-two spaces mid-sentence since a source line was wrapped without a continuation. --- src/pool/snapshot.rs | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/src/pool/snapshot.rs b/src/pool/snapshot.rs index 2bf80d1..6984f41 100644 --- a/src/pool/snapshot.rs +++ b/src/pool/snapshot.rs @@ -1677,6 +1677,17 @@ pub(crate) struct PoolSnapshot { /// `recovered_bytes` is what the change is judged by: committed memory read *after* a stall in /// the same region, which is precisely the coverage a walk that gave up at the first stall /// reported as nothing at all. +/// +/// **On live 26100 it has measured zero** — 1,619 stalls, 6,627,520 bytes stepped over, nothing +/// read behind any of them (glslang/win-kexp#104). That is the number saying what it says, not a +/// counter that was never wired up: `stalled_here` latches for the rest of the region and any +/// later extent adds to `recovered_bytes`, which +/// [`SnapshotWalker::walk_region`]'s own test pins at two pages. So on that target every stall +/// sits at the end of its region's readable content, and the page-stepping buys nothing there. +/// It is kept because it is bounded — at most [`MAX_CONSECUTIVE_STALLS`] queries per dead region +/// — and because the failure it replaced was losing every committed page behind one bad one. The +/// diagnostic now carries the engine's own answer at each stall, which is what a later run needs +/// to decide whether stepping can be replaced by stopping. #[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] pub struct WalkStalls { /// How many times a query could not advance and the walk stepped over a page. @@ -2009,8 +2020,16 @@ impl<'a, M: PoolMemory> SnapshotWalker<'a, M> { // without end, which is the reason this used to abandon the region outright. let page_end = (valid_base & !(PAGE_SIZE - 1)).saturating_add(PAGE_SIZE); let skip = page_end.min(requested_end) - valid_base; + // The engine's own answer travels with the complaint, because the two shapes + // that arrive here need opposite fixes and nothing else can tell them apart: a + // region reported *behind* the cursor (`reported_base` below `valid_base`) means + // the query is answering about memory already walked, while a zero-length region + // reported ahead of it means the engine found something and could not size it. + // `PoolDiagnostics` folds numbers into `#`, so this stays one shape however many + // times it fires, and the verbatim sample carries the real values. snapshot.diagnostics.push(format!( - "valid-region query made no progress at {valid_base:#x}; stepping over the rest of the page" + "valid-region query made no progress at {valid_base:#x}: the engine answered \ + {reported_base:#x}+{reported_size:#x}; stepping over the rest of the page" )); self.unreadable(region, valid_base, skip, snapshot); snapshot.stalls.pages += 1; From c380dbb6c420e2ed84d1e85b527b9f2e7be9bce7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Carvalho?= Date: Fri, 14 Aug 2026 17:07:45 +0100 Subject: [PATCH 3/4] Carry the unplaced-bytes count on the heap report too The user segment heap goes through the same `walk_vs`, so it can lose the chunk chain the same way. `refused_headers` travels there already; this is the other half of the same measurement. --- src/heap.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/heap.rs b/src/heap.rs index 39cac91..278c01c 100644 --- a/src/heap.rs +++ b/src/heap.rs @@ -159,6 +159,9 @@ pub struct HeapWalkReport { pub diagnostic_count: usize, pub unreadable_gaps: usize, pub refused_headers: u64, + /// Committed VS bytes the walk declined to decode because it could not place a chunk + /// boundary in them; see [`crate::pool::PoolSnapshot::unplaced_bytes`]. + pub unplaced_bytes: u64, pub stalls: WalkStalls, } @@ -669,6 +672,7 @@ fn from_pool_snapshot( .filter(|allocation| allocation.state == HeapState::Unreadable) .count(), refused_headers: snapshot.refused_chunks, + unplaced_bytes: snapshot.unplaced_bytes, stalls: snapshot.stalls, }; let diagnostics = HeapDiagnosticReport { From 7d694cacbd31f18677c70b042a693c6ab209895f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Carvalho?= Date: Fri, 14 Aug 2026 17:27:28 +0100 Subject: [PATCH 4/4] Size a VS region from the subsegment, not from the range that holds it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Measured on a live 26100 kernel, which is what settles glslang/win-kexp#103. The chunk-chain fix in the previous commit was real but was the smaller half: refusals fell 106,516 -> 76,398, not to zero, and the remainder had the same signature as before — one predicate, ~248 per affected extent. 248 * 16 is a page, and that is exactly what it turned out to be. The page range holding a VS subsegment is one unit larger than the subsegment: 053 flags=0f UnitSize=11 UnitOffset=00 a VS range, 17 pages 054..063 UnitSize=00 UnitOffset=01..10 its 16 continuation units 064 flags=03 UnitSize=01 and the next range begins while the subsegment at that address declared Size 0xffd — 0xffd0 bytes of chunks, plus `first`, is 0x10000. Sixteen pages of subsegment in a seventeen page range, at both the 0x10000 and 0x20000 sizes, on 1,388 subsegments. `UnitSize` is read correctly; it simply is not the subsegment's size. So the walk was decoding a page that holds no chunks, and refusing a header every sixteen bytes across whatever part of it happened to be committed. The bound was wrong after all — too large rather than too small, which is why the refusals looked like corruption at the tail of every affected extent. `nt!RtlpHpVsSubsegmentInitialize` writes `Size = (bytes - first) >> 4` and lays the first chunk at `+ first`, so the subsegment's own `Size` is the chunk area. Preferred over the descriptor rather than compared with it, because it is already checked: a subsegment is only accepted when `Signature ^ Size` is 0x2bed, so a `Size` resolved from the wrong offset cannot reach the walk. Clamped so it can only ever shrink the region, and a declaration that is zero or larger than the range keeps the descriptor's bound and says so. The diagnostic added in the previous commit fired 1,388 times and is what found this; it now fires only for a declaration that cannot be the chunk area, since one spare page is the ordinary shape and a line per subsegment is noise. --- src/pool/snapshot.rs | 142 +++++++++++++++++++++++++++++++++---------- 1 file changed, 110 insertions(+), 32 deletions(-) diff --git a/src/pool/snapshot.rs b/src/pool/snapshot.rs index 6984f41..f63f49f 100644 --- a/src/pool/snapshot.rs +++ b/src/pool/snapshot.rs @@ -1198,25 +1198,44 @@ fn discover_segment_context( region_address += first as u64; region_size -= first; block_size = 0; - // The bound every VS chunk is checked against is `region_address + region_size`, - // derived from the page-range descriptor. The subsegment carries the same number - // itself, and it is the one the allocator laid the chunks out to: - // `RtlpHpVsSubsegmentCreate` writes `Size = (bytes - first) >> 4` and puts the - // first chunk at `+ first`, so the two must agree exactly. Read and thrown away - // until now — glslang/win-kexp#103 asked whether the descriptor-derived bound was - // systematically short, and this is the check that answers it on any target - // rather than by argument. + // **The page range is not the subsegment, and is routinely larger.** The chunk + // area is what `nt!RtlpHpVsSubsegmentInitialize` lays out — `Size = (bytes - + // first) >> 4` sixteen-byte units starting at `+ first` — and the descriptor + // sizes the *range that holds it*, which on 26100 is one unit more: // - // Reported, not preferred: `declared` is corroboration from a second source, and - // trusting it over the descriptor would let one misread `Size` truncate every VS - // region on a build whose field we resolve wrongly. A disagreement here is the - // signal to go and look. + // ```text + // 053 flags=0f UnitSize=11 UnitOffset=00 <- a VS range: 17 pages + // 054..063 UnitSize=00 UnitOffset=01..10 <- its 16 continuation units + // 064 flags=03 UnitSize=01 <- and the next range begins + // ``` + // + // while the subsegment at 0x53000 declared Size 0xffd — 0xffd0 bytes of chunks, + // plus `first`, is 0x10000. One spare page, every time, at both 0x10000 and + // 0x20000 subsegment sizes. + // + // Bounding by the range is what raised glslang/win-kexp#103: where that spare + // page happened to be committed, the walk decoded it, and since nothing there is + // a chunk it refused a header every sixteen bytes to the end — 0x1000/16 = 256 + // per subsegment, against 248 measured. The remaining refusals on a live walk + // were, to a rounding error, exactly this page. + // + // `declared` is preferred over the descriptor rather than merely compared with + // it, because it is the number the chunks were laid out to *and* it is checked: + // this subsegment is only accepted at all if `Signature ^ Size == 0x2bed`, so a + // `Size` we resolved from the wrong offset cannot reach here. Clamped anyway — + // it may only ever shrink the region, never point the walk outside the range it + // was given. let declared_size = usize::from(declared) * 16; - if declared_size != region_size { + if declared_size == 0 || declared_size > region_size { + // Not the ordinary spare page: the subsegment is claiming no chunks at all, + // or more than its range can hold. Its signature checked out, so this is + // worth a line rather than a silent clamp. discovery.diagnostics.push(format!( "VS subsegment {address:#x} declares {declared_size:#x} of chunks where \ its page range leaves room for {region_size:#x}" )); + } else { + region_size = declared_size; } } let descriptor_node = metadata_address + offset as u64 + tree_node_offset as u64; @@ -2361,10 +2380,15 @@ impl<'a, M: PoolMemory> SnapshotWalker<'a, M> { /// committed extents are the allocator's steady state and not damage. /// /// Starting each extent at its own first sixteen-byte boundary — which is what this did — is - /// therefore a guess on every extent after a hole, and where glslang/win-kexp#103's 106,516 - /// refusals came from: ~196 per extent, one per sixteen bytes, until the scan wandered onto - /// a word that decoded plausibly. The refusals were the harmless half. Whatever the scan - /// wandered onto was pushed into `spans` as an allocation, tag and all. + /// therefore a guess on every extent after a hole, and one of the two sources of + /// glslang/win-kexp#103's 106,516 refusals: one per sixteen bytes, until the scan wandered + /// onto a word that decoded plausibly. The refusals were the harmless half. Whatever the + /// scan wandered onto was pushed into `spans` as an allocation, tag and all. + /// + /// It was the smaller half. A live 26100 walk with this fixed still refused 76,398 headers, + /// and the rest were the *bound*: the page range holding a VS subsegment is a unit larger + /// than the subsegment, so the walk was decoding a page that holds no chunks. See + /// [`discover_segment_context`], which now sizes the region from the subsegment. /// /// The chain is what crosses the hole. A decommitted range is always the *interior* of a /// free chunk — the allocator has to keep its own headers readable — so the chunk before a @@ -4446,37 +4470,91 @@ mod tests { } } - /// glslang/win-kexp#103 asked whether the bound every VS chunk is checked against — - /// `region.address + region.size`, from the page-range descriptor — is systematically short, - /// since on a live 26100 walk 106,516 refusals all failed that one check and no other. - /// `RtlpHpVsSubsegmentCreate` writes the same quantity into the subsegment itself as - /// `Size = (bytes - first) >> 4`, so the two are the same number from two sources and the - /// walk can say so on any target instead of anyone arguing it. Both directions, because a - /// check that never fires and a check that always fires read identically from a live run. + /// glslang/win-kexp#103, settled on a live 26100 kernel: the page range holding a VS + /// subsegment is **larger than the subsegment**, by one unit every time, so a walk bounded by + /// the descriptor decodes a page that holds no chunks and refuses a header every sixteen + /// bytes across it. The subsegment's own `Size` is the chunk area, and it is only reachable + /// through a signature check that would have rejected a misread — so it is preferred, and + /// clamped so it can never point the walk outside the range it was given. #[test] - fn test_a_vs_subsegment_is_measured_against_its_own_declared_size() { - let quiet = walk_synthetic(synthetic_memory()); + fn test_a_vs_subsegment_is_bounded_by_its_own_declared_size() { let complaint = "declares"; + let quiet = walk_synthetic(synthetic_memory()); assert!( !quiet .diagnostics .examples() .iter() .any(|message| message.contains(complaint)), - "a subsegment whose two sizes agree draws no complaint: {:?}", + "a subsegment sized like the real ones draws no complaint: {:?}", quiet.diagnostics.examples() ); - - let short = walk_synthetic(synthetic_memory_declaring((0x2000 - 0xfe0) / 16 - 1)); + let all = vs_chunks(&quiet); + + // The live shape: chunks end before the range does. What sits past the declared end is + // not part of this subsegment and must not be decoded — which is the entire finding. + let bounded = walk_synthetic(synthetic_memory_declaring(8)); + // `first` is the fixture's `_HEAP_VS_SUBSEGMENT` size rounded up, and eight units past it + // is where the chunk area now ends — well short of the page range, exactly as a real one + // does. Nothing at or beyond that address is part of this subsegment. + let declared_end = SEGMENT + 0x3000 + 0xfe0 + 8 * 16; + assert!( + vs_chunks(&bounded) < all, + "bounding the subsegment has to cost the chunks past its end: {:?}", + bounded.diagnostics.examples() + ); + assert!( + snapshot_vs_spans(&bounded).all(|span| span.header_address < declared_end), + "nothing past {declared_end:#x} may be decoded: {:?}", + snapshot_vs_spans(&bounded) + .map(|span| span.header_address) + .collect::>() + ); + assert!( + snapshot_vs_spans(&quiet).any(|span| span.header_address >= declared_end), + "and the check is only worth anything if the wider bound did reach past it" + ); assert!( - short + !bounded .diagnostics .examples() .iter() .any(|message| message.contains(complaint)), - "{:?}", - short.diagnostics.examples() + "and that is the ordinary shape, not something to complain about once per \ + subsegment: {:?}", + bounded.diagnostics.examples() ); + + // Neither of these can be the chunk area, and both would be silent damage if clamped + // without a word: no chunks at all, and more than the range can hold. + for declared in [0, 0x200] { + let bogus = walk_synthetic(synthetic_memory_declaring(declared)); + assert!( + bogus + .diagnostics + .examples() + .iter() + .any(|message| message.contains(complaint)), + "declared {declared:#x}: {:?}", + bogus.diagnostics.examples() + ); + assert_eq!( + vs_chunks(&bogus), + all, + "an unusable declaration falls back to the descriptor's bound" + ); + } + } + + fn snapshot_vs_spans(snapshot: &PoolSnapshot) -> impl Iterator { + snapshot + .spans + .iter() + .filter(|span| span.backend == PoolBackend::Vs && span.state != PoolState::Unreadable) + } + + fn vs_chunks(snapshot: &PoolSnapshot) -> usize { + snapshot_vs_spans(snapshot).count() } fn walk_synthetic(memory: SyntheticMemory) -> PoolSnapshot {