From de6fea8e9ea8d7b9dd31c514dd16453e9cb90787 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Carvalho?= Date: Thu, 24 Sep 2026 17:08:57 +0100 Subject: [PATCH] docs(heap): `Unreadable` is the absence of an answer, not a claim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The doc comments this state landed with said it is "memory the walk could not read, and which the target *has*". That is true of the cases the memory manager confirms committed and false of every case where no commitment query could be made at all — a kernel walk, a dump recording no memory information, a query that failed — which is precisely the half `committed_run`'s `None` arm exists for. `Uncommitted` carries a positive answer; `Unreadable` carries the absence of one. Saying otherwise turns *we could not tell* into a fact about the target, which is the one thing this walker is built not to do. Corrected at both definition sites and in `docs/unknown-not-absent.md`, whose new section is where the distinction is stated. Prose only; no behaviour changes and 435 tests pass unchanged. Raised by review on glslang/windbg-mcp#382. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01MUhLUt9rB6zd42Y25btB3h --- CHANGELOG.md | 4 +++- docs/unknown-not-absent.md | 9 ++++++++- src/heap.rs | 10 ++++++---- src/pool.rs | 14 +++++++++++--- 4 files changed, 28 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a58c242..629e646 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,7 +42,9 @@ All notable changes to this project are documented here. The format follows Only a positive `MEM_RESERVE`/`MEM_FREE` excuses a gap — a failed query, a run that cannot advance, an unnamed state and a source that cannot be asked are each conservative, so the excuse is never granted by an absence of evidence. The kernel pool walk is not asked at all - and is unchanged. + and is unchanged. Which also fixes what `Unreadable` *means*: it is now everything that could + not be established as empty, including every case nothing could be asked about, so it is the + conservative bucket rather than a claim that the target has the memory. A free chunk whose middle the allocator decommitted is the same question reached another way: it runs past the committed extent it starts in, and `walk_vs` emitted no span for it and diff --git a/docs/unknown-not-absent.md b/docs/unknown-not-absent.md index 3d91b3c..6699a19 100644 --- a/docs/unknown-not-absent.md +++ b/docs/unknown-not-absent.md @@ -87,6 +87,12 @@ Three states hide behind that one observation, and the failed read does not sepa | Committed and paged out, or absent from a dump | The target has this memory and the walk did not see it. A real gap. | | Committed and present | Not a gap at all — it read. | +`Uncommitted` is the first row and **only** the first row, because only it can be established. The +other two, and every case where no commitment query could be made at all — a kernel walk, a dump +recording no memory information, a query that failed — stay `Unreadable`. So `Unreadable` is the +conservative bucket rather than a claim that the target has the memory, and prose about it that +says *memory the process has* is overstating exactly the half that was not measured. + The allocator's own records can answer the first distinction: a page range descriptor's `CommittedPageCount`, a VS subsegment's `CommitBitmap`, an LFH subsegment's commit state at `CommitStateOffset`. That is three structures, each of which moves between builds, to learn @@ -112,7 +118,8 @@ runs past the committed extent it starts in, and `walk_vs` emitted no span for i geometry and state, both of which are known there — the header was read, the size came out of it and passed the subsegment bound, the state comes from the free tree — and the only thing missing is the chunk's *contents*, which no span carries. So where the tail holds nothing the -chunk is reported; where it is memory the process has, it is not, and now the walk says so. +chunk is reported; where that could not be established — memory the process has, or memory +nothing could be asked about — it is not, and now the walk says so. Measured on a live 26200 process (`sihost`, four Segment Heaps, 19,448 chunks, 2026-09-24): all 48 gaps were `MEM_RESERVE`, both controls — an allocated chunk and a free one — were diff --git a/src/heap.rs b/src/heap.rs index f38d22e..cf61d68 100644 --- a/src/heap.rs +++ b/src/heap.rs @@ -112,8 +112,9 @@ pub enum HeapState { Allocated, ReusableFree, CachedFree, - /// Memory the walk could not read that the process does have; see - /// [`PoolState::Unreadable`]. + /// A span the walk could not read and nothing established was empty — which includes both + /// memory the process does have and memory nothing could be asked about. The conservative + /// bucket rather than a claim; see [`PoolState::Unreadable`]. Unreadable, /// Address space in a heap region with no pages behind it; see /// [`PoolState::Uncommitted`]. Not a gap in the walk's coverage — a reserved subsegment @@ -182,8 +183,9 @@ pub struct HeapWalkReport { pub total_chunks: usize, pub allocated_chunks: usize, pub diagnostic_count: usize, - /// Spans the walk could not read and the process **does** have; each one clears - /// [`WalkCoverage::complete`]. + /// Spans the walk could not read and nothing established were empty; each one clears + /// [`WalkCoverage::complete`]. See [`HeapState::Unreadable`] for why that is not the same as + /// memory the process has. pub unreadable_gaps: usize, /// Spans with no pages behind them — reserved subsegment tails and the like, which the /// memory manager confirmed rather than the walk assumed. diff --git a/src/pool.rs b/src/pool.rs index ba5491a..8ce9e3e 100644 --- a/src/pool.rs +++ b/src/pool.rs @@ -57,10 +57,18 @@ pub enum PoolState { Allocated, ReusableFree, CachedFree, - /// Memory the walk could not read, and which the target *has*: a page that is committed - /// and paged out, one missing from a dump, or one the debugger refused. Something may have - /// been there, so this is a hole in the walk's coverage and clears + /// A span the walk could not read and **nothing established was empty**. Something may have + /// been there, so it is a hole in the walk's coverage and clears /// [`query::WalkCoverage::complete`]. + /// + /// **It is the conservative bucket, not a claim that the target has the memory.** Two very + /// different things land here: a page the memory manager confirms is committed — paged out, + /// missing from a dump, refused by the debugger — and a page no commitment query could be + /// made about at all, which is every kernel walk and any target + /// [`snapshot::PoolMemory::committed_run`] answers `None` for. Only + /// [`Self::Uncommitted`] carries a positive answer; this one carries the absence of one, and + /// reading it as confirmed memory would turn *we could not tell* into a fact about the + /// target. Unreadable, /// Address space inside a region with **no pages behind it** — reserved, or committed and /// since released — as the target's memory manager says, not as the walk inferred from