Skip to content

fix(codegen): keep single-unit globals in their own linkage so zero-init caches stay in __bss (#9610) - #9626

Closed
proggeramlug wants to merge 1 commit into
PerryTS:mainfrom
proggeramlug:fix/9610-ic-globals-bss
Closed

fix(codegen): keep single-unit globals in their own linkage so zero-init caches stay in __bss (#9610)#9626
proggeramlug wants to merge 1 commit into
PerryTS:mainfrom
proggeramlug:fix/9610-ic-globals-bss

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Summary

Codegen-unit splitting (#5391) promoted every generated global to linkonce_odr so the linker could fold a global that more than one unit defines. Almost every global is defined by exactly one unit, and on Mach-O the promotion is not free: linkonce_odr is weak-for-linker, and LLVM's Mach-O section picker routes weak-for-linker globals to the coalesced data section without ever asking whether the initializer is zero. A zeroinitializer global that belongs in zerofill __DATA,__bss therefore lands in file-backed __DATA,__data, and its zeros are written into the binary.

Per-site inline caches are [12 x i64] zeroinitializer, one per property-access site, each referenced by exactly one function and so emitted into exactly one unit. That is what fills __data in a large build.

This PR applies the promotion only where a link would actually see a global defined twice, and only to definitions that already carry local linkage — a strong external definition keeps linkonce_odr, so ld64 still coalesces two modules' same-named globals rather than reporting a duplicate symbol. Nothing else about the split policy changes: every referencing unit still carries its own definition, which is what -dead_strip needs.

Zero runtime behaviour change; ELF and COFF were never affected (their BSS classification ignores linkage, and the non-Mach-O path already gives each global one strong owner).

The mechanism

clang -c on one .ll with the same [12 x i64] zeroinitializer global at five linkages (LLVM 22.1.8):

linkage macOS (size -m) Linux (readelf -S)
private, internal __DATA,__bss (zerofill) .bss
linkonce_odr, weak_odr __DATA,__data — file-backed .bss
external __DATA,__common (zerofill) .bss

So the bug is Mach-O only, and it is triggered by the split, not by the cache emission: an unsplit build already puts the caches in __bss.

Changes

  • crates/perry-codegen/src/module.rscodegen_unit_parts now carries global definitions in their original linkage and applies promote_global_for_units per unit, gated on defining_unit_count[gi] > 1 || !has_local_linkage(def).
  • New has_local_linkage helper, and the doc block for promote_global_for_units moved onto the function it describes (it was sitting above global_symbol_name).
  • New test mach_o_split_promotes_only_globals_two_units_define.

Related issue

Fixes #9610

Test plan

Placement, before/after compilers built from this branch (macOS arm64, --no-link, sections of the merged unit object):

__data __bss
before, 1 unit (no split) 0 57,640
before, 4 units 57,680 0
before, 8 units 57,696 0
after, 4 units 40 57,600
after, 8 units 40 57,600

A split build now places caches exactly as an unsplit build does. The residual 40 B is one global that two units really do share and so is still (correctly) linkonce_odr.

Scale of the problem on a real artifact — the cc binary in my tree (2026-08-24 build): __data = 22,233,000 B, of which 16,285 of 2,779,125 words are nonzero — 99.41% literal zeros. #9610's census measures 25,159,016 B / 99.5% on a newer build.

Linked binaries, same before/after pair (macOS arm64, 200-function probe, 8-way split vs no split):

build file bytes __data __bss (zerofill)
before, 1 unit (no split) 15,760,456 21,672 610,352
before, 8 units 15,793,480 79,400 552,688
after, 8 units 15,760,456 21,736 610,288

The split moved 57,728 B out of zerofill into the file; after the fix an 8-unit build is byte-for-byte the same size as the unsplit one. before, 1 unit and after, 1 unit are identical (15,760,456), so the two compilers differ only on the split path.

Behaviour, Linux x86_64 — one program (classes, any-typed property access, Map/Set iteration, JSON.stringify) compiled at 1/2/4/8 units, all four binaries produce byte-identical output:

units=1 rc=0 size=9421936 out=4360.7406 140 1365 17 [3,4]
units=2 rc=0 size=9421752 out=4360.7406 140 1365 17 [3,4]
units=4 rc=0 size=9421784 out=4360.7406 140 1365 17 [3,4]
units=8 rc=0 size=9421816 out=4360.7406 140 1365 17 [3,4]

Real-world split, Linuxtests/test_next_app_route_dylib.sh (the #7174 Next.js bundle case) compiles the whole 104-module route through the unit splitter, including a 36-unit split of app-page.runtime.prod.js and an 8-unit split of chunks/2.js. Codegen and the ld -r merges complete cleanly on this branch. A run I invoked with a non-default --profile release tripped the fixture's own provider-linker symbol check (provider link selected no app ABI symbols) — downstream of codegen, in the shim that swaps libperry_runtime.rlib for a dylib; re-running it in its default configuration.

Unit tests: cargo test -p perry-codegen — 1397 lib + 51 integration tests pass. The new test fails on the unfixed lowering (reverting just the gate):

assertion `left == right` failed: @perry_ic_m__0 is referenced by one function,
so exactly one unit defines it — in its original local linkage, which is what
keeps it in __bss
  left: 0
 right: 1
  • cargo build --release clean
  • Added a #[test] in the affected crate
  • cargo test --workspace — ran -p perry-codegen plus the next-app-route dylib end-to-end

https://claude.ai/code/session_019iAdropXK3d2gyuGexbKMv

Summary by CodeRabbit

  • Bug Fixes
    • Improved code generation for macOS binaries by preserving appropriate linkage for shared global and string definitions.
    • Fixed handling of single-unit zero-initialized globals so they remain in the expected zero-fill storage section.
    • Prevented unnecessary symbol promotion when definitions are used by only one generated code unit.

…nit caches stay in __bss

Splitting a module into codegen units promoted every generated global to
`linkonce_odr` so the linker could fold a global that more than one unit
defines. Most globals are defined by exactly one unit, and the promotion is
not free on Mach-O: `linkonce_odr` is weak-for-linker, and
`TargetLoweringObjectFileMachO::SelectSectionForGlobal` routes every
weak-for-linker global to the coalesced data section before it looks at
whether the initializer is zero. A `zeroinitializer` global that would have
gone to zerofill `__DATA,__bss` therefore lands in file-backed
`__DATA,__data` and its zeros are written into the binary.

Promote only the globals a link would actually see defined twice. ELF and
COFF are unaffected: their BSS classification ignores linkage, and the
non-Mach-O path already gives each global a single strong owner.

Fixes PerryTS#9610

Claude-Session: https://claude.ai/code/session_019iAdropXK3d2gyuGexbKMv
@coderabbitai

coderabbitai Bot commented Sep 3, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: 2240ef88-2ad4-46f0-a149-3a4485148f51

📥 Commits

Reviewing files that changed from the base of the PR and between 0952a64 and 6ee7790.

📒 Files selected for processing (1)
  • crates/perry-codegen/src/module.rs

Included review availability: Your plan provides up to 8 included reviews per hour; 1 remains after this review.


📝 Walkthrough

Walkthrough

Changes

Mach-O global linkage

Layer / File(s) Summary
Conditional global promotion
crates/perry-codegen/src/module.rs
codegen_unit_parts counts defining units and promotes globals only when required. Single-unit local zeroinitializer globals retain local linkage and remain eligible for Mach-O __bss.
Mach-O linkage regression test
crates/perry-codegen/src/module.rs
The test verifies linkage for single-unit local globals, shared globals, and strong external definitions.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 6ee77

Mach-O single-unit zero-initialized globals retain local linkage and can be emitted in BSS, reducing file-backed binary data while preserving coalescing for shared or externally visible globals. No current merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the codegen linkage change and its Mach-O BSS impact.
Description check ✅ Passed The description includes all required sections, explains the implementation, links issue #9610, and provides detailed test results. It also clearly records that the full workspace test command was not…
Linked Issues check ✅ Passed The changes satisfy the coding objectives in [#9610]. Single-unit zero-initialized globals retain local linkage for Mach-O BSS placement, while multi-unit and externally linked definitions retain requ…
Out of Scope Changes check ✅ Passed All changes are related to [#9610]: conditional global promotion, linkage detection, documentation relocation, and a Mach-O regression test. No unrelated code changes are identified.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 1 files.
Full details: Description check

Explanation

The description includes all required sections, explains the implementation, links issue #9610, and provides detailed test results. It also clearly records that the full workspace test command was not run.

Full details: Linked Issues check

Explanation

The changes satisfy the coding objectives in [#9610]. Single-unit zero-initialized globals retain local linkage for Mach-O BSS placement, while multi-unit and externally linked definitions retain required promotion behavior. The PR adds a regression test and reports matching data/BSS changes with unchanged runtime behavior.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Follow-up on the one open item in the test plan — tests/test_next_app_route_dylib.sh, re-run in its default configuration (my earlier --profile release override was what tripped the fixture's provider-linker symbol check; that check passes on the default profile).

On this branch the gate compiles all 104 route modules, performs six multi-unit splits — including a 36-unit split of app-page.runtime.prod.js and an 8-unit split of chunks/2.jsld -r-merges each of them, and writes the app dylib:

655:Wrote shared library: /tmp/perry-next-app-route.pZPYXW/providers/next-app-route.so

It then stops at runtime:

provider host exited during cold start 1
dlopen failed: .../providers/libperry_runtime.so: cannot allocate memory in static TLS block

That failure is pre-existing on this host, not from this PR. I ran the identical gate on origin/main (666481e) on the same machine: same six multi-unit merges, app dylib written at line 664, and the same dlopen error. libperry_runtime.so is a pure-Rust cargo artifact — perry's codegen never runs for it — and this diff touches one file in perry-codegen, so it cannot reach that library. Raising glibc.rtld.optional_static_tls did not help either; it looks like a glibc static-TLS-surplus limit on this box rather than something the gate can control.

So: the splitting path this PR changes is exercised end-to-end on the real Next.js bundle and produces a linkable dylib, byte-for-byte as far as origin/main gets on the same host.

https://claude.ai/code/session_019iAdropXK3d2gyuGexbKMv

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Landed via merge train #9638 (rebase-merge, authorship preserved).

@proggeramlug
proggeramlug deleted the fix/9610-ic-globals-bss branch September 3, 2026 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant