Trace grace subtrees on the legacy heap, and gate heap integrity in CI - #5471
Merged
Conversation
The grace-subtree pass added in #5442 covers BiBOP pages only. The legacy sweep grants a fresh (gcMark == -1) legacy object exactly the same one-cycle grace -- codenameOneGCSweep promotes it to the current epoch instead of freeing it -- but nothing traced its subtree, so an older object reachable ONLY through such an object was freed while it was still referenced. That is the same defect #5442 fixed, on the other heap. Everything above CN1_BIBOP_MAX_OBJECT lands there (the retained large byte[] blocks and Hashtable bucket arrays of issue 5425), as does every allocation the adaptive survivor-heavy bypass diverts off the page heap, and every matured survivor, whose table entry is what the sweep consults. Measured with the audit half added below: ~65,800 untraced fresh legacy objects per cycle on a bypass-heavy workload, with children reachable only through them. Mirror the page walk over allObjectsInHeap. Only entries already migrated into the table can be fresh at that point -- pending allocations are not swept until the mark that migrates them, and migration happens with the owning thread paused, upstream of this pass -- so the pass is exact. Cost is one extra walk of an array the sweep already walks in full, and only fresh entries are traced: StormAB and LoadLoop wall time and RSS are unchanged and LargeArrayLoad still collects in 5 cycles. CN1_DISABLE_LEGACY_GRACE is the A/B escape hatch, mirroring CN1_DISABLE_SATB. Add the gate that would have caught both halves. Checksums are structurally blind to this failure: when a sweep frees memory a survivor still references, nothing diverges at the point of the bug -- the dangling reference reads whatever object recycled the slot, so the damage surfaces later, elsewhere, as corrupted data. That is how #5436's regression reached a user as "non word" dictionary entries and an impossible NPE instead of as a failing test. -DCN1_GC_VERIFY makes the invariant observable by destroying the plausible replacement: - POISON every reclaimed page slot and legacy block. This includes the O(1) all-dead page reclaim, which is where nearly all page memory is actually reclaimed and which normally drops a page without writing a single slot, leaving every dead object with an intact-looking header -- the reason a dangling read in this VM finds plausible data rather than crashing. - QUARANTINE freed legacy blocks in a ring instead of returning them to the C allocator, so a poisoned block stays mapped and recognizable. - VERIFY after every sweep: walk each survivor through its own generated mark function with the collector in verify mode, classifying every reference field against the page registry, the live-extent index and the quarantine set. A field pointing into reclaimed memory is reported with the holder's class, the victim's class and the field's mark call site, then aborts at the cycle that created it. The gate holds CURRENT-EPOCH survivors to the invariant, where a dangling field is unambiguous: the sweep either marked the object reachable (marking traces children) or promoted it by the grace rule (tracing the subtree was the grace pass's job). References it cannot place are skipped, so a violation is never a false alarm. CN1_GC_VERIFY_AGING extends it to previous-epoch survivors as a census rather than a gate. run-gc-verify.sh runs it over nine drivers and then re-injects the #5442 defect (CN1_GC_FAULT=nograce disables the grace pass) and REQUIRES the verifier to catch it -- a gate nobody has watched fail is not a gate, and a build where the verification silently compiled out would otherwise report a permanent, meaningless pass. GcHeapIntegrityIntegrationTest is the CI twin and asserts both halves. Two supporting pieces, both born from the same investigation: - LegacyGrace is the legacy-path twin of GraceAudit, plus the [GRACE-AUDIT-LEGACY] half of -DCN1_GRACE_AUDIT. Writing it exposed why drivers in this area come back green while the defect is present: the hazard has to be built with no mark in flight (during a mark the SATB barriers cover the very reference move under test) and the driver has to scrub its own native stack afterwards, because the conservative root scan marks whatever a returned frame's leftover word still points at. Both are documented at the driver and in the README. - CN1_GC_TRACE_MARK names the mark pass that keeps a class alive, which is how that retention was identified; CN1_GC_VERIFY_CENSUS reports whether a driver's hazard set actually ages out instead of being pinned. Validation: gauntlet GREEN in both stop modes, run-gc-verify GREEN over all nine drivers with the fault self-test firing, GraceAudit clean under -DCN1_GRACE_AUDIT, and the new integration test green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends ParparVM’s concurrent GC correctness guarantees by (1) tracing grace subtrees for legacy-heap objects (mirroring the existing BiBOP/page-heap grace-subtree tracing), and (2) adding a QA-only heap integrity verifier (-DCN1_GC_VERIFY) plus an integration-test gate that exercises and self-validates the verifier in CI.
Changes:
- Add legacy-heap grace-subtree tracing during mark to prevent sweeping objects reachable only through graced legacy objects.
- Introduce
-DCN1_GC_VERIFYQA mode: poison/quarantine freed memory and post-sweep verification that survivors don’t reference reclaimed memory (with fault-injection support). - Add new benchmark drivers/scripts and a JUnit integration test (
GcHeapIntegrityIntegrationTest) that runs the verified build and asserts the verifier can also catch a re-injected known defect.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| vm/tests/src/test/resources/com/codename1/tools/translator/GcVerifyApp.java | New workload app used by the heap-integrity integration test to exercise GC hazard shapes. |
| vm/tests/src/test/java/com/codename1/tools/translator/GcHeapIntegrityIntegrationTest.java | New CI gate test that builds/runs ParparVM with -DCN1_GC_VERIFY and includes a fault-injection self-test. |
| vm/ByteCodeTranslator/src/cn1_globals.m | Implements legacy grace-subtree tracing and the CN1_GC_VERIFY poison/quarantine/verify infrastructure (plus fault injection and audit hooks). |
| vm/benchmarks/src/com/bench/LegacyGrace.java | New benchmark driver to reproduce and audit the legacy-heap grace hazard under -DCN1_GRACE_AUDIT. |
| vm/benchmarks/run-gc-verify.sh | New script to run the verifier gate across multiple drivers and enforce a fault-injection self-test. |
| vm/benchmarks/README.md | Documentation updates covering LegacyGrace and the -DCN1_GC_VERIFY gate and diagnostics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Both halves of the gate are decided by environment variables, and anyone debugging the collector has exactly those exported: CN1_GC_VERIFY_SOFT downgrades the abort the faulted half asserts on, and CN1_GC_FAULT injects the defect the clean half asserts is absent. An inherited knob would invert a result rather than fail loudly. Drop CN1_* from the child environment in the integration test and unset the knobs at the top of run-gc-verify.sh, so both start from a known state and see only what they set themselves. Verified by running each with CN1_GC_FAULT=nograce and CN1_GC_VERIFY_SOFT=1 exported: the test passes and the script reports GC-VERIFY GREEN. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Cloudflare Preview
|
cn1GcVerifyChild read __builtin_return_address(0) itself, which resolves to the return address in gcMarkObject -- one frame too deep, and in a different object file from the generated mark function the label claimed it pointed into. Anyone following a violation back to a field would have landed in the collector rather than at the field read. Capture the address in gcMarkObject instead, where this frame's return address IS the instruction inside the generated mark function (or gcMarkArrayObject for an element), and pass it down. Report it as an offset from the holder's own mark function so the line is self-verifying without symbols and survives ASLR: markSite= 0x102fa66c0 = markFn+36 (the field read, inside the holder's mark function) A small positive offset means the frame is right; add it to the mark function's symbol to reach the exact field. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Compared 146 screenshots: 146 matched. Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 146 screenshots: 146 matched. Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 147 screenshots: 147 matched. |
Collaborator
Author
|
Compared 147 screenshots: 147 matched. |
Collaborator
Author
|
Compared 148 screenshots: 148 matched. Benchmark Results
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 181 screenshots: 181 matched. |
Collaborator
Author
|
Compared 146 screenshots: 146 matched. Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 217 screenshots: 217 matched. |
Collaborator
Author
|
Compared 143 screenshots: 143 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 144 screenshots: 144 matched. |
Collaborator
Author
|
Compared 149 screenshots: 149 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
…est on Linux Three problems, all the same shape: a check that reports success without having checked anything. 1. The verifier never reported whether it RAN. MtStress, MapTorture and SbTorture exit before any sweep completes, so their "clean" results in run-gc-verify.sh meant only that nothing was ever verified -- exactly the hollow-gate failure the fault self-test exists to prevent, sitting inside the same script. Count completed passes, print them at exit, and fail any driver reporting zero. Those three now end with one collection over the heap they built (printing nothing, so the gauntlet's byte-identical comparison is unaffected) and contribute real coverage. 2. The CI self-test failed on Linux: the fault was injected and the workload completed, but no dangling reference appeared, so the test declared the gate inert. Reproduced in a linux/amd64 container -- the platform is fine (GraceAudit detects the fault there), the app's hazard was too weak. Its during-mark phase now follows the shape that provably breaks a missing grace pass: refill, kick a mark, then allocate dropped fresh nodes in sleep-separated slices so they land across the mark rather than racing past it, then go quiet for a full cycle so the untraced children age past the free threshold. Verified in the container: clean 0 violations / 23 passes, faulted aborts with 20 reports, three runs, no variance. 3. The integration test now also requires a nonzero pass count, so a workload that stops driving collection fails instead of passing silently. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
✅ ByteCodeTranslator Quality ReportTest & Coverage
Benchmark Results
Static Analysis
Generated automatically by the PR CI workflow. |
MapTorture, MtStress and SbTorture predate the copyright gate and carried no header at all. The gate only inspects files a PR adds or modifies, so giving them a trailing collection in the previous commit is what pulled them into scope. Header text copied verbatim from LegacyGrace.java, added in this PR and already passing. Comment-only: scripts/check-copyright-headers.sh passes over the branch, and the gauntlet still matches the host JVM byte for byte. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The comment described an earlier design in which the function handed the evicted block back for the caller to release. It returns a boolean and frees the displaced block itself, and the call site in codenameOneGcFree already relies on that boolean -- so the comment was the only thing out of date, and the one part a future caller would have read first. State what the return value means instead: TRUE when the block was quarantined and must NOT be freed, FALSE when the quarantine could not be allocated and the caller should free normally. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
cn1GcFaultNoGrace was initialized only by cn1GcVerifyHeap, which runs after the sweep. The first cycle of every faulted run therefore traced grace subtrees normally, and a workload completing a single GC cycle never had the defect injected at all -- the self-test would have reported a gate that "cannot fail" purely because nothing ever faulted it. Resolve the switch at its first use in the mark instead; cn1GcFaultInit is idempotent and GC-thread only, so the call in cn1GcVerifyHeap stays as a harmless second one. Observable: [GC-FAULT] now prints before the first verify pass rather than after a sweep. Checking that turned up a matching gap in GcStress, which reported 1 verify pass on one run and 0 on the next: its churn triggers collections, but whether the last one reaches its sweep before the process exits is a race, so the new vacuity check would have flaked in CI. It gets the same trailing collection as the other drivers (prints nothing, so the gauntlet comparison is unaffected). Two consecutive gate runs now report identical pass counts across all nine drivers, and the gauntlet stays byte-identical to the host JVM. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The hazard comment still described the driver's first shape -- 256 parents of ~800 bytes -- after it was scaled to KEEP=10000 arrays of 65 references. The number matters rather than being decoration: the window only works while nothing in it starts a collection, so a reader checking that property was being handed the wrong figure by an order of magnitude. Give the real one (about 560 bytes each, 5.6 MB total, against the 24 MB allocation-volume trigger) and say which constants would break the window if raised. Same for the other precondition, that no mark is in flight, since both are equally easy to lose when editing the driver. Comment-only; LegacyGrace still reports 120 clean verify passes with the fault self-test firing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
GcStress is the fourth torture in this tree without a header, and the trailing collection added for the vacuity check pulled it into the copyright gate's scope. Missed on the first pass because the earlier commit added headers to the three drivers touched at that point, and GcStress was edited after that check ran. Audited every source file this branch touches rather than fixing one report at a time: all eight now carry the header and scripts/check-copyright-headers.sh passes over the full branch range. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
shai-almog
added a commit
that referenced
this pull request
Jul 26, 2026
…#5474) * Stop the O(1) page reclaim from freeing slots the per-slot walk keeps Follow-up to #5471, which flagged kept objects referencing reclaimed memory on the issue-5425 workload and left the cause open. The O(1) whole-page reclaim tests gcLastMarkedEpoch != V, but the per-slot walk it claims to reproduce ("byte-identical outcome") frees on m < V-1 -- so a slot marked at V-1 SURVIVES the walk while the shortcut drops the entire page holding it. Measured on LargeArrayLoad, the final issue-5425 shape: 26,924 slots freed a full cycle early in one run. The other drivers report zero, which fits: it needs a large retained survivor set sharing pages with garbage, which is exactly what that workload builds. That is what produced the dangling pairs. The legacy sweep ages on the same m < V-1 rule, so a matured Hashtable.Entry kept at V-1 was left pointing at a page-resident byte[] payload the shortcut had already reclaimed -- the reporter's dictionary shape precisely. Test gcLastMarkedEpoch < V-1 instead. Both bounds are then exact and neither implies the other: gcLastMarkedEpoch covers slots marked since the last full walk, gcGraceEpoch covers slots the sweep itself promoted out of grace. earlyFreed goes 26,924 -> 0 across the suite, and it is free: LoadLoop and StormAB wall time and RSS unchanged, LargeArrayLoad still 5 cycles, peak RSS 93.1 vs 92.6 MB. On the resurrection half of that report, the measurement says the hazard is real but essentially never fires: across all nine drivers, 1 resurrection total and 0 of them holding a dangling reference. For the conservative scan to revive an object a stale stack word must point at it, and such a word generally keeps marking it EVERY cycle -- retention, not resurrection. The audit stays in as a gate so that stops being an assumption. Three things keep this honest: - resurrected / resurrectedDangling / earlyFreed are reported at exit, and run-gc-verify.sh fails on a nonzero earlyFreed or resurrectedDangling. - CN1_GC_FAULT=earlyfree restores the old bound, and a second self-test requires the gate to reject it (26,924 slots), so the new check cannot go inert the way an unexercised assertion does. - CN1_GC_VERIFY_AGING is documented as PERTURBING: it doubles the post-sweep walk and the extra GC-thread time moves page ageing by orders of magnitude (early-freed 0 vs 205,958 from one binary). The 30k-60k figures quoted in #5471 came from that mode and overstated the problem; every number here is from the default configuration. Validation: gauntlet GREEN in both stop modes, run-gc-verify GREEN over nine drivers with both self-tests firing, GcHeapIntegrityIntegrationTest green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Fix the stale precondition comment and cache the debug env lookup The bullet list above the O(1) page decision still documented gcLastMarkedEpoch != V -- the exact bound this PR replaces -- so the file stated both the old rule and the new one a few lines apart, with the wrong one first. The bullet now carries the real criterion and says why != V is insufficient; the paragraph below no longer restates it. getenv("CN1_GC_DEBUG_EARLY") also ran once per early-freed slot, and the earlyfree self-test drives 26,924 of them through that loop. Cached in a static, like the other QA env reads in this file. Gate re-run: clean, with both self-tests firing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Name the phase in the dangling-reference report The resurrection audit runs at the end of the mark but drives the same per-field reporter as the post-sweep gate, so anything it found announced itself as happening "after sweep" -- pointing a reader at the wrong phase of the collector while they try to reconstruct what freed the memory. Carry the calling context in the report instead of suppressing the per-field detail, which is the part worth having: the holder and victim classes and the mark site are what map a resurrected object's dangling field back to source. The audit also now says which resolver snapshot it classifies against (the mark's, correct there because the memory it looks for was reclaimed by earlier cycles). Post-sweep reports still read "after sweep"; verified against the nograce self-test, and the gate stays green with both self-tests firing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #5442 (issue #5425). Two parts: the grace-subtree guarantee was only implemented on one of the two heaps, and nothing in the suite asserted the guarantee itself.
The gap
codenameOneGCSweepgrants a fresh (gcMark == -1) legacy object exactly the BiBOP grace rule -- it promotes the object to the current epoch instead of freeing it:but the grace-subtree pass added in #5442 walks the page registry only. An older object reachable ONLY through a graced legacy object was therefore freed while still referenced -- the same defect, on the other heap.
That heap is not a corner case for this issue. Everything above
CN1_BIBOP_MAX_OBJECTlives there (the retained largebyte[]blocks andHashtablebucket arrays of the reporter's dictionary), as does every allocation the adaptive survivor-heavy bypass diverts off the page heap, and every matured survivor. Measured with the new audit half on a bypass-heavy workload:Fix: mirror the page walk over
allObjectsInHeap. Only entries already migrated into the table can be fresh at that point (pending allocations are not swept until the mark that migrates them, and migration happens with the owning thread paused, upstream of the pass), so the pass is exact.CN1_DISABLE_LEGACY_GRACEis the A/B escape hatch, mirroringCN1_DISABLE_SATB.Why no test caught either half
Checksums are structurally blind to this failure. When a sweep frees memory a survivor still references, nothing diverges at the point of the bug -- the dangling reference reads whatever object recycled the slot, so the damage surfaces later, elsewhere, as corrupted data. That is how #5436's regression reached a user as "non word" dictionary entries and an impossible NPE rather than as a red test.
Three mechanisms make it worse, all measured while building the driver for this PR:
GraceAuditalso grades the grace pass against itself (it marks what it finds, so the audit build masks the defect it detects on later cycles) and covers only BiBOP.The gate:
-DCN1_GC_VERIFYMakes the invariant -- no object the sweep kept may reference memory the sweep reclaimed -- directly observable, by destroying the plausible replacement:
A violation aborts at the cycle that created it, naming the holder, the victim and the field:
The gate holds current-epoch survivors to the invariant, where a dangling field is unambiguous: the sweep either marked the object reachable (marking traces children) or promoted it by the grace rule (tracing the subtree was the grace pass's job). References it cannot place are skipped, so a violation is never a false alarm.
CN1_GC_VERIFY_AGINGextends it to previous-epoch survivors as a census rather than a gate.The self-test is the point of the script.
run-gc-verify.shfinishes by re-injecting the exact defect #5442 fixed (CN1_GC_FAULT=nograce) and requires the verifier to catch it -- a gate nobody has watched fail is not a gate, and a build where the verification silently compiled out would otherwise report a permanent, meaningless pass.GcHeapIntegrityIntegrationTestis the CI twin and asserts both halves.Validation
run-gauntlet.sh, both stop modesrun-gc-verify.sh(9 drivers + fault self-test)GcHeapIntegrityIntegrationTestGraceAuditunder-DCN1_GRACE_AUDITdoomedChildren=0LargeArrayLoadcollection cyclesKnown, deliberately not gated
With
CN1_GC_VERIFY_AGING=1,LargeArrayLoad(the final issue-5425 shape) reports ~30k-60k references per cycle from previous-epoch survivors into memory the sweep already reclaimed --Hashtable.Entryholders pointing at freedbyte[]payloads. The two heaps age out of the one-cycle safety margin on different schedules: the O(1) page path drops a page before the legacy table gives up on its entries.Strictly, unreachable objects are entitled to dangle, so this is not gated. But this collector resurrects unreachable objects routinely (mechanism 2 above), and resurrecting one of those entries makes the drain follow a field into recycled memory. Filed here as the next thing to chase rather than fixed blind.
🤖 Generated with Claude Code