fix(token): make GreenTokenData a DST to widen retag past slice tail#212
Closed
phall1 wants to merge 4 commits into
Closed
fix(token): make GreenTokenData a DST to widen retag past slice tail#212phall1 wants to merge 4 commits into
phall1 wants to merge 4 commits into
Conversation
Comprehensive fixes for Miri UB under both stacked and tree borrows: Arc: - clone/drop/is_unique: access refcount via ptr::addr_of! on raw pointer instead of through &ArcInner reference (avoids provenance narrowing) - ThinArc::clone/drop: operate on refcount directly via raw pointer instead of going through with_arc → transient Arc Green types: - GreenNodeData wraps fat Repr (unsized HeaderSlice<H, [T]>) so &GreenNodeData has provenance covering the full slice - GreenNode/GreenToken::deref transmute from fat refs (correct provenance) - GreenNode/GreenToken::into_raw extract pointer from ThinArc directly instead of going through Deref → &Data → NonNull::from - GreenTokenData::text() uses raw pointer arithmetic for slice access - thin_to_thick made pub(crate), HeaderSlice fields made pub(crate) Cursor: - Cell<NonNull<GreenNodeData>> accessed via as_ptr().read() instead of get() to preserve allocation provenance through the Cell Status: ALL tests pass under -Zmiri-tree-borrows (4/4 non-mutable tests). Under stacked borrows, only the mutable tree path (clone_for_update) still fails due to Cell provenance limitations inherent to that model. Upstream: rust-analyzer#192, rust-analyzer#163, rust-analyzer#108
Make elided lifetimes explicit in return types where &self borrows: - GreenChild::as_ref() -> GreenElementRef<'_> - NodeData::green_siblings() -> slice::Iter<'_, GreenChild> These warnings caused CI failure with RUSTFLAGS="-D warnings".
Stacks on top of rust-analyzer#211. After rust-analyzer#211, `GreenNodeData` is a DST (`HeaderSlice<H, [GreenChild]>`) and `GreenNode::deref` returns a fat reference whose retag covers the whole slice. `GreenTokenData` was left as `HeaderSlice<H, [u8; 0]>`, so `GreenToken::deref` still hands out a narrow `&GreenTokenData` whose SharedReadOnly tag covers only `sizeof::<ReprThin>()`. Reaching the slice tail through that reference (via `ptr::addr_of!(self.data.slice)` + `from_raw_parts`, as rust-analyzer#211 currently does) is rejected by Stacked Borrows because the zero-size retag has no permission past the thin extent. Mirrors what rust-analyzer#211 did for `GreenNodeData`: change `GreenTokenData`'s inner field to `Repr = HeaderSlice<H, [u8]>`, route `GreenToken::deref` through `&self.ptr` (fat) + transmute, and have `into_raw`/`from_raw` carry/strip the slice metadata via `thin_to_thick` (same shape as the existing `GreenNode` impls). `text()` collapses back to `from_utf8_unchecked(self.data.slice())`. Repro: hashbrown rehash inside `NodeCache::token` calls `token_hash` -> `GreenTokenData::text` from a `&NoHash<GreenToken>` that has been narrowed by hashbrown's internal retag. Before this patch, that hits SB UB at `src/green/token.rs:104`: error: Undefined Behavior: trying to retag from <N> for SharedReadOnly permission at allocM[0x18], but that tag does not exist in the borrow stack for this location --> src/green/token.rs:104:25 let bytes = std::slice::from_raw_parts(slice_start, len); Caught by cyrs CI: - https://github.com/phall1/cyrs/actions/runs/25638947905 (job 75256239232) - https://github.com/phall1/cyrs/actions/runs/25639698954 (job 75257867300) Regression test: `tests/miri_node_cache_rehash.rs` forces hashbrown to grow the token map (200 distinct tokens) and asserts no SB violation under `-Zmiri-strict-provenance`. Fails on PR rust-analyzer#211 alone, passes after this patch. Refs rust-analyzer#163, rust-analyzer#192, rust-analyzer#108. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 10, 2026
phall1
pushed a commit
to phall1/cyrs
that referenced
this pull request
May 10, 2026
…#211) PR #211 alone doesn't fix the cyrs repro — promoted GreenNodeData to DST but left GreenTokenData narrow. Filed rust-analyzer/rowan#212 with the matching DST promotion for tokens (1 file, ~28 lines). Stacks on #211. Comments posted on PR #211 and issue #163. phall1/rowan:fix/sb-node-cache-rehash has both fixes stacked, ready for [patch.crates-io] use (cy-yrz). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Sorry for you investing time in this, but we'll rewrite rowan anyway as soon as we finish with the rust-analyzer assist migration, a rewrite that will make it much more performant and also likely to fix this issue (and even if not, the code this PR changes will be stale). So I'm inclined to close this PR, as well as #211. Also note that rowan should pass Tree Borrows, and that this specific rule from Stacked Borrows is unlikely to make it into the final aliasing model for Rust. |
Author
|
no worries! I'm happy to close this. I should be following rust analyzer more closely -- Where's the best spot to subscribe if you don't mind pointing me in the right direction? |
Contributor
|
#t-compiler/rust-analyzer > Future Rowan, rust-lang/rust-analyzer#15710, rust-lang/rust-analyzer#18285, we also have a GSoC project for that. |
phall1
pushed a commit
to phall1/cyrs
that referenced
this pull request
May 11, 2026
…itten ChayimFriedman2 commented on rust-analyzer/rowan#212: rewrite coming after RA assist migration, inclined to close PRs #211 and our #212. Also noted rowan passes Tree Borrows; the SB rule we're hitting is unlikely to be in Rust's final aliasing model = our miri failures are academic, not real UB. New strategy: maintain phall1/rowan fork as patch source. No more upstream PRs. cy-208 (cursor::free SB) becomes a fork-only patch. Track the eventual upstream rewrite via the rust-analyzer Zulip 'Future Rowan' thread + issues rust-lang/rust-analyzer#15710 and #18285 + the GSoC project. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
avrabe
added a commit
to pulseengine/rivet
that referenced
this pull request
Jun 10, 2026
…ri SB-sound Adopts phall1's unmerged rust-analyzer/rowan#212 into our fork as fix/miri-soundness-v3 (= v2 + `48a1b5e` GreenToken-as-DST + `9e7abd1` cursor SB fix). v2 was only sound under Tree Borrows; the residual thin-token SharedReadOnly zero-size-retag UB is now fixed, so rivet's rowan parsers are sound under BOTH Stacked Borrows and Tree Borrows. - Cargo.toml: pin branch v2 → v3; comment rewritten to the closed-unmerged / upstream-rewrite reality + the SB-soundness. - Cargo.lock: rowan git rev → 9e7abd1. - ci.yml: the scoped PR Miri job now gates under STACKED BORROWS (strictest; drop -Zmiri-tree-borrows) since v3 makes it sound. miri-full stays on Tree Borrows for the broader (incl. mutable-path) sweep. Confirmed: rivet-core builds against v3; native sexpr::/yaml_cst:: tests pass; rowan fork v3 builds + carries phall1's miri_node_cache_rehash / miri_cursor_free_sb regression tests; actionlint clean. Refs: REQ-215, #509 Trace: skip
avrabe
added a commit
to pulseengine/rivet
that referenced
this pull request
Jun 13, 2026
…45 min on one) (#521) * ci(miri): parallelize with cargo-nextest (process-per-test) to use idle cores Measured root cause of the 45-min Miri job + lean-mem contention: `cargo miri test` runs ONE Miri process — a single-threaded interpreter — pinning one core for 45 min while the rest of the (ample) box sits idle. The CPU/mem doesn't vanish into contention (peak 15 concurrent jobs, ~22s queues); Miri just can't use it, and it hogs the scarce lean-mem runner for 45 min so other lean-mem jobs queue behind it. Switch to `cargo miri nextest run`: each test runs in its own process, so the runner's cores are actually used and wall-time collapses toward the slowest single test. `--test-threads 4` bounds concurrent Miri processes under the 24G shadow-memory ceiling (conservative start; raise after a green run shows peak RSS). Same test selection: the libtest `--skip <m>` list is translated to nextest's `-E 'not (test(<m>) | ...)'` (verified locally: 721 run / 412 excluded, identical modules). Adds a resource-print step so the log shows the box's real nproc/free. Refs: REQ-215, #509 Trace: skip * ci(miri): use 24 of the runner's 32 cores (was 4) — box has 125 GiB, not 24 Measured the runner directly (the new resource-print step): 32 cores, 125 GiB RAM — the "lean-mem" label is a misnomer and memory is a non-constraint. At --test-threads 4 the parallel Miri job still timed out at 45 min (~678 tests × ~20s ÷ 4 ≈ 56 min); 28 of 32 cores sat idle. Raise to 24 threads: ~226 core-min ÷ 24 ≈ ~9-10 min, using ~72 GiB (well under 125). This is where the CPU/mem was "vanishing": into idle cores, because nothing was using them. Refs: REQ-215, #509 Trace: skip * ci(miri): scope PR gate to the unsafe/CST surface; full sweep nightly Correction to the earlier "24 threads thrash" claim: measured effective concurrency is 23x — parallelism works fine. The real problem is work VOLUME: the 678-test sweep is ~930 test-minutes (mean 104s/test, worst 557s) of mostly safe business logic (regex/glob/HTML) that Miri runs ~100-1000x slower than native and that has no `unsafe` to validate. No thread count fits that in a per-PR budget. So split it (the #498 nightly pattern): - PR/push `miri` → only the real UB surface: the SyntaxKind `transmute`s in the rowan CST parsers (sexpr + yaml_cst), 41 tests, ~3 min. This is what Miri is FOR. - nightly/manual `miri-full` → the full Miri-compatible sweep, 24 threads on the 125 GiB runner, 90-min budget, off the per-PR path. Refs: REQ-215, #509 Trace: skip * ci(miri): restore -Zmiri-tree-borrows on the scoped job (my regression) When I split Miri into scoped (PR) + full (nightly) I kept MIRIFLAGS on miri-full but dropped it from the scoped job, so it ran under default Stacked Borrows and surfaced rowan's thin-token SharedReadOnly zero-size-retag UB — which I mistakenly reported as a fresh finding. Research (subagent, source- grounded) confirms: rust-analyzer/rowan #210/#211/#212 are all closed UNMERGED (upstream is rewriting rowan, not patching); the maintainer holds that rowan should pass Tree Borrows and the failing Stacked Borrows rule is unlikely to survive Rust's final aliasing model. Our fork pin is sound under Tree Borrows, which the original job already used. Restore `MIRIFLAGS: -Zmiri-disable-isolation -Zmiri-tree-borrows` on the scoped job (matching miri-full and the pre-split job). Update the rowan pin comment in Cargo.toml to reflect the closed-unmerged/rewrite reality. Refs: REQ-215, #509 Trace: skip * build(rowan): bump fork pin to v3 (phall1 #212 token+cursor DST) → Miri SB-sound Adopts phall1's unmerged rust-analyzer/rowan#212 into our fork as fix/miri-soundness-v3 (= v2 + `48a1b5e` GreenToken-as-DST + `9e7abd1` cursor SB fix). v2 was only sound under Tree Borrows; the residual thin-token SharedReadOnly zero-size-retag UB is now fixed, so rivet's rowan parsers are sound under BOTH Stacked Borrows and Tree Borrows. - Cargo.toml: pin branch v2 → v3; comment rewritten to the closed-unmerged / upstream-rewrite reality + the SB-soundness. - Cargo.lock: rowan git rev → 9e7abd1. - ci.yml: the scoped PR Miri job now gates under STACKED BORROWS (strictest; drop -Zmiri-tree-borrows) since v3 makes it sound. miri-full stays on Tree Borrows for the broader (incl. mutable-path) sweep. Confirmed: rivet-core builds against v3; native sexpr::/yaml_cst:: tests pass; rowan fork v3 builds + carries phall1's miri_node_cache_rehash / miri_cursor_free_sb regression tests; actionlint clean. Refs: REQ-215, #509 Trace: skip
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
3 participants
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.
stacks on #211. tokens still need the same DST treatment you gave nodes there.
with #211 alone,
GreenToken::derefreturns a narrow&GreenTokenDatawhose retag covers onlysizeof::<ReprThin>().text()then widens past that withaddr_of!(self.data.slice) + from_raw_parts, which SB rejects:real-world path:
hashbrown::RawTablerehash inNodeCache::token->token_hash->text().caught in a cypher frontend project im doing.
what changes: same shape as your node fix in #211.
GreenTokenData::databecomesHeaderSlice<H, [u8]>,derefgoes through&self.ptr,into_raw/from_rawusethin_to_thick,text()collapses tofrom_utf8_unchecked(self.data.slice()). one file, ~28 lines.test:
tests/miri_node_cache_rehash.rsbuilds 200 distinct tokens to force a rehash. fails on #211 alone, passes here.cargo testandcargo +nightly miri test --test miri_node_cache_rehashboth green.one more SB hit out of scope here for the record:
ast::tests::ensure_mut_panic_on_create->ThinArc::clone->count_ptr.fetch_addderefs through narrow&GreenNodeDataand loses the count's provenance. separate fix.refs #163, #192, #108