fix(codegen): keep emitted rodata symbols module-unique - #8054
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
💤 Files with no reviewable changes (4)
📝 WalkthroughWalkthroughThe change replaces the per-function typed-parse counter with the module-wide ChangesTyped-parse rodata symbol uniqueness
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: ⚪ Minimal · up to This change makes emitted read-only-data symbols unique within a module and adds regression coverage for the collision case; no actionable merge-blocking risk remains after normal checks and review. Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Independent audit of exact head I audited the complete source diff and a clean landing merge onto current Independent isolated optimized regression on the landing tree: The exact product oracle also compiled and ran the real parse-churn subject with correct output, exit 0, seven copying minors, and a current-format non-in-place minor reporting Disposition: merge exact head; close #8048; keep #7966/#8049 open. |
|
Independent audit of exact head : mergeable.\n\nI audited the complete source diff and a clean landing merge onto current (). The old counter reset for each emitted function body; the replacement starts from , advances for every rodata/IC site, and writes the end value back before the next body. The typed-parse, V8, and Node-submodule symbol prefixes remain disjoint. There are no version/Cargo/package changes, no unresolved review threads, and plus pass.\n\nIndependent isolated optimized regression on the landing tree:\n\n\n\nThe exact product oracle also compiled and ran the real parse-churn subject with correct output, exit 0, seven copying minors, and a current-format non-in-place minor reporting . The wrapper's red result is separately explained by #8049: its stale parser misreads that nonzero line as zero. I did not use CI status as evidence for or against this product fix.\n\nDisposition: merge exact head; close #8048; keep #7966/#8049 open. |
Summary
$spec_i32plus boxed-body collision that redefined@perry_typed_parse_keys_0Evidence
Scheduled failure: gc-parse-churn run 31737943472 / job 94574017539, head
3381e1b89048592cfc91f3d4a43349c6e7c16015.Before this change, both emitted bodies restarted
FnCtx::typed_parse_counterat zero and appended the same private global to one LLVM module. The regression now emits both bodies, requires two parse-schema globals, and asserts their symbol names are distinct.Local verification (isolated target
/Users/amlug/cargo-targets/red-gc-gates-sub):cargo test --profile perry-dev -p perry-codegen --test native_proof_regressions typed_parse_rodata_names_are_unique_across_spec_and_boxed_bodies -- --exact— 1 passedBLOB_BYTES 245781,PARSED_LENGTH 4000,CHURN_TOUCH 240000,MISMATCHES 0, exit 0, and a current-format non-in-place minor withcopied_objects=183cargo fmt --all -- --checkgit diff --checkScope note: the shipped #8054 wrapper alone still cannot print
PARSE_CHURN_LAYOUT_GATE_OK, because its liveness parser only recognizes the obsolete copy-minor log format. The end-to-end wrapper success I observed used the separate #8049 parser fix in the committed audit tree. That parser change is intentionally excluded from this PR; the raw compile/output/exit and nonzero current-format collector evidence above validate this product fix without claiming the independent wrapper bug is fixed here.The release archives and compiler were built together and their mtimes were verified before the end-to-end run.
Closes #8048.
Refs #7966.