Let the grace passes mark objects the resolve guard was discarding (issue 5425) - #5477
Open
shai-almog wants to merge 3 commits into
Open
Let the grace passes mark objects the resolve guard was discarding (issue 5425)#5477shai-almog wants to merge 3 commits into
shai-almog wants to merge 3 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7686f97bbf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses ParparVM/iOS heap corruption in the concurrent GC by adding a “late-grace re-mark” at the end of codenameOneGCMark() to ensure fresh objects that would otherwise be sweep grace-promoted are traced (so their subtrees aren’t reclaimed). It also makes conservative-root snapshot invalidation available outside CN1_GC_VERIFY and updates comments to reflect the corrected safety model around grace vs subtree tracing.
Changes:
- Add a late-grace re-mark pass that refreshes conservative snapshots and traces still-fresh legacy + BiBOP objects before sweep.
- Make
cn1ConsSnapEpochReset()available outsideCN1_GC_VERIFYso snapshot rebuilds aren’t silently epoch-gated away. - Clarify/update comments around sweep grace-promotion and
gcMarkObject()’s conservative resolve guard behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
✅ ByteCodeTranslator Quality ReportTest & Coverage
Benchmark Results
Static Analysis
Generated automatically by the PR CI workflow. |
Contributor
Cloudflare Preview
|
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
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 146 screenshots: 146 matched. Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 181 screenshots: 181 matched. |
Collaborator
Author
|
Compared 217 screenshots: 217 matched. |
Collaborator
Author
|
Compared 148 screenshots: 148 matched. Benchmark Results
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 149 screenshots: 149 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 144 screenshots: 144 matched. |
…ssue 5425)
The reporter's dictionary load corrupts the heap on iOS: garbled words, an
"invalid block type" from the inflater, an impossible NPE. Reproduced by running
his Dtest.java classes verbatim over the dictionary.res attached to the issue,
with a second thread allocating alongside the loader. Under -DCN1_GC_VERIFY the
unfixed VM aborts every run with thousands of dangling references; without it,
SIGSEGV inside Hashtable.rehash / get.
Per-object mark history named the holder of every dangling reference: a fresh
legacy object stamped live by codenameOneGCSweep's grace branch with
lastTraced == 0 -- never traced in its life. The cause is one guard:
if(cn1ConservativeResolve((void*)obj) != obj && !cn1GcImmortalObjContains(obj))
return;
It exists to reject CONSERVATIVELY DERIVED pointers -- arbitrary machine words
off a native stack that may not be objects at all. But it rejects every pointer
it cannot resolve, and resolution is against a page/extent snapshot built at MARK
START, so an object allocated later can never resolve. When the mid-mark grace
passes walk allObjectsInHeap and the page registry and hand such an object over,
the mark is silently discarded. The sweep then grace-promotes it (mark == -1 ->
currentGcMarkValue) WITHOUT tracing, in the same index-ordered walk that frees
dead objects, so an older object reachable only through it is reclaimed at a
lower index.
Those two walks hold AUTHORITATIVE references: they read the object out of a
registry, not off a stack. They need no proof. cn1GcTrustedRoots marks that
window so gcMarkObject skips the guard for them, and the existing passes then
mark what they were already trying to mark. Nothing new runs; a pass that was
throwing its results away stops doing so.
Also corrects the comments asserting a fresh object is safe because grace keeps
it. Grace keeps the OBJECT; it does not trace the SUBTREE. That premise appears
at the guard, at the snapshot-rebuild note and at the sweep's promotion site, and
is what this defect was hiding behind.
Do NOT set cn1GcTrustedRoots around anything marking from a stack scan or
register file -- that is the case the guard exists for.
Validation: reproducer clean 3/3 over 166 collection cycles and 800M verified
references (baseline fails 100% with ~7000 violations); run-gc-verify.sh GREEN
over all nine drivers with both fault self-tests firing; geomean 1.22x vs 1.21x
on master, objectAllocation 3.85x vs 3.46x -- within run variance, all checksums
bit-identical to the host JVM.
An earlier revision of this branch instead added a late-grace re-mark pass to
compensate for the guard. It was correct but cost 2.1x on objectAllocation and
+8% geomean, and review found two further defects in it (a suffix scan unsound
against tombstone reuse, and an O(heap) pass widening the unbarriered
mark-to-sweep window). Fixing the guard makes all of that unnecessary.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
shai-almog
force-pushed
the
fix-5425-late-grace-remark
branch
from
July 27, 2026 15:33
f68878d to
8e9f717
Compare
shai-almog
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e9f717ca6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
cn1GcTrustedRoots spanned gcMarkDrain() in the legacy grace pass. The drain follows child words out of arbitrary generated mark functions -- including those of DEAD objects kept alive by a conservative native-stack false positive, whose reference fields can dangle into freed or unmapped memory. Bypassing cn1ConservativeResolve for those is exactly the SIGSEGV the guard exists to prevent, so the change reopened the crash it was meant to be safe around. Trust covers only the registry walk, which is where the authoritative references are. Every fresh entry is stamped and queued by then, so the drain needs none. The BiBOP half already cleared per-page before its drain; the legacy half now matches. Also from review: - Acquire-load the mark word in the legacy grace walk before dereferencing the header. The mark word is the publication point (see gcMarkObject); a plain read can let a weak-memory target observe the object without the preceding parentClsReference store. Pre-existing, but the resolve guard had been backstopping it for this population. - Revert gcAuditSnapshot to CN1_GRACE_AUDIT-only. Promoting it was leftover from a narrowing that no longer exists: an O(#pages) walk plus atomics in every build for a field only read under that flag, with a stale comment referring to a pass that was deleted. Re-validated: reproducer clean 3/3 over 149 collection cycles and 732M verified references; run-gc-verify.sh GREEN over all nine drivers with both fault self-tests firing; GcStress checksum unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Compared 143 screenshots: 143 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
cn1GcTrustedRoots was a process-wide global. Marking can run as a worker pool (gcMarkDrainParallel), so a concurrent worker would also bypass the conservative-resolve guard while a grace pass held the flag -- on pointers that are NOT authoritative, which is the precise case the guard exists for. It is safe today only because gcMarkResolveThreadCount() returns 1, and the comment there says that serial default is a temporary isolation experiment: the hazard would arm itself the moment parallel marking is restored, and would look unrelated when it fired. Make it __thread. Manual set/unset also is not nesting-safe -- the BiBOP half toggles it per page inside a loop, so an early return or a future nested trusted scope would leak a trusted window into unrelated marking. CN1_GC_TRUSTED_BEGIN/END save and restore the previous value instead of clearing to zero. Re-validated: reproducer clean 2/2 over 102 collection cycles and 492M verified references; run-gc-verify.sh GREEN over all nine drivers with both fault self-tests firing; GcStress checksum unchanged, including a -DCN1_GC_MARK_THREADS=4 build -- the parallel configuration this is about. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
vm/ByteCodeTranslator/src/cn1_globals.m:1079
CN1_GC_TRUSTED_BEGIN/ENDcurrently declaresint __cn1TrustSavedin the surrounding scope. Identifiers beginning with__are reserved, and the plain declaration also makes the macros easy to misuse (e.g., multiple BEGIN/END pairs in the same scope) despite the nearby comment mentioning nesting/early-return safety.
Wrap the BEGIN/END pair in a single do { ... } while(0) statement and use a non-reserved local name so the saved value is scoped to the pair and can’t collide with other uses.
// restore, so nesting and any future early return cannot leak a trusted window
// into unrelated marking.
static __thread int cn1GcTrustedRoots = 0;
#define CN1_GC_TRUSTED_BEGIN() int __cn1TrustSaved = cn1GcTrustedRoots; cn1GcTrustedRoots = 1
#define CN1_GC_TRUSTED_END() cn1GcTrustedRoots = __cn1TrustSaved
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.
Fixes #5425.
The failure
The reporter's dictionary load corrupts the heap on iOS: garbled words, an
invalid block typefrom the inflater, an impossible NPE. Reproduced by running hisDtest.javaclasses verbatim over thedictionary.resattached to the issue, built through the ParparVM clean target, with a second thread allocating alongside the loader. Under-DCN1_GC_VERIFYthe unfixed VM aborts every run with thousands of dangling references; without the verifier, SIGSEGV insideHashtable.rehash/get.Root cause — one guard, applied wider than its purpose
Per-object mark history named the holder of every dangling reference: a fresh legacy object stamped live by
codenameOneGCSweep's grace branch withlastTraced == 0— never traced in its life. The cause:That guard exists to reject conservatively derived pointers — arbitrary machine words off a native stack that may not be objects at all, where dereferencing would crash. But it rejects every pointer it cannot resolve, and resolution is against a page/extent snapshot built at mark start — so an object allocated later can never resolve.
When the mid-mark grace passes (#5442 BiBOP, #5471 legacy) walk
allObjectsInHeapand the page registry and hand such an object over, the mark is silently discarded. The sweep then grace-promotes it (mark == -1→currentGcMarkValue) without tracing, in the same index-ordered walk that frees dead objects — so an older object reachable only through it is reclaimed at a lower index.Hashtable.rehashproduces exactly this shape: a freshEntry[]holding the only reference to older entries.The fix
Those two walks hold authoritative references — they read the object out of a registry, not off a stack — so they need no proof.
cn1GcTrustedRootsmarks that window andgcMarkObjectskips the guard for it. The existing passes then mark what they were already trying to mark.Nothing new runs. A pass that was throwing its results away stops doing so.
It also corrects the comments asserting a fresh object is safe because grace keeps it. Grace keeps the object; it does not trace the subtree. That premise appears at the guard, at the snapshot-rebuild note and at the sweep's promotion site, and is what this defect was hiding behind.
cn1GcTrustedRootsaround anything marking from a stack scan or register file — that is the case the guard exists for.Validation
run-gc-verify.shHistory of this branch
An earlier revision added a late-grace re-mark pass to compensate for the guard rather than fix it. It was correct but cost 2.1x on objectAllocation and +8% geomean, and review found two further defects in it: a suffix scan unsound against
placeObjectInHeapCollection's tombstone reuse, and an O(heap) pass widening the unbarriered mark-to-sweep window. Both are moot now — there is no extra pass. Thanks to both reviewers; the slot-reuse catch is what forced the full-table scan that exposed the cost that sent me back to the guard.Caveats
No regression test. Eight attempts at a synthetic gate driver, none reproduced (best: 4 verify passes / 138M refs / 0 violations with the fix off, against 40–56 passes on the real workload). Deleted rather than shipped, per this repo's own standard that a gate nobody has watched fail is not a gate. The working reproducer is the reporter's
Dtest.javaplus hisdictionary.res, two mutator threads, no pacing. Pinning this down deserves a follow-up.macOS arm64 desktop only. Nothing verified on device, which is where the reported symptom lives. Worth having the reporter build this branch.
Separate issues surfaced during the investigation
gcMarkResolveThreadCounthardcodesint n = 1, forcing serial marking, with a comment admitting a second parked ordering hole.cn1BibopPacingCapsizes backpressure off machine RAM, so peak RSS tracks allocation rate rather than live set: 34 GB against a ~150 MB live set. On a device that is a jetsam, and a plausible part of the reporter's "IOS reports the app crashed".cn1FusedInstallPrimArraycreates permanently unregistered, never-marked objects — safe today only because they are leaves.🤖 Generated with Claude Code