-
-
Notifications
You must be signed in to change notification settings - Fork 159
fix(codegen): module-unique names for promoted string constants and the null guard (ELF multi-unit link) #8942
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| ### Fixed | ||
|
|
||
| - **Codegen units: `add_string_constant` globals and the null-guard global are now module-unique, so a multi-module Linux link of split modules no longer fails with `multiple definition of .str.N`** (#8942). Every `LlModule` numbered its anonymous rodata constants from `.str.0`, which is fine while they stay `private`. Codegen-unit splitting (#5391/#7174) promotes them so sibling units can reference them, and on every non-Mach-O target the owning unit's copy is a plain strong definition with default visibility (`make_unique_owner_global` — whose doc said "COFF" but whose branch runs for ELF too). Two modules large enough to split therefore both exported `.str.375` with different contents; GNU ld refused a Next.js App Route bundle's `--output-type dylib` link with 2,188 `multiple definition` errors (`app-page.runtime.prod.js` ↔ `route.js`, `jsonwebtoken/index.js` ↔ `route.js`). macOS was not clean either, only quiet: Mach-O keeps the replicated `linkonce_odr` copies (`weak external automatically hidden`) and ld64 coalesces weak definitions by name, so one module's bytes silently stood in for another's whenever a `.str.N` index was shared — `fn.name`, class names and `Function.prototype.toString` came out wrong, reproduced on macOS with a three-module program and `PERRY_CODEGEN_UNITS=2`. `compile_module` now installs the module symbol prefix on the `LlModule` (`set_symbol_prefix`) and `add_string_constant` mints `@<prefix>_.str.N`, mirroring what `strings.rs` already did for `<prefix>_.str.N.bytes`. The unprefixed `perry_null_guard_zero` (the safe-dereference target of `safe_load_i32_from_ptr`) takes the identical promotion path and is renamed `perry_null_guard_zero_<prefix>`, injected per function through the `RegCounter` cell so no call site changes. Unique names rather than `linkonce_odr hidden` because a COMDAT is folded by name and would reproduce ld64's silent merge on ELF; the #7174 one-definition-per-module layout is unchanged. Fixtures that never set a prefix keep the bare names. Verified on macOS end to end (split output byte-identical to Node after, miscompiled before) and on ELF by retargeting the dumped units to `x86_64-unknown-linux-gnu` under the owner policy and linking with `ld.lld` (10 duplicate symbols before, 0 after); a real Linux build is the remaining proof. `crates/perry-codegen/src/module.rs`, `block.rs`, `function.rs`, `codegen/mod.rs`. | ||
| - **`--output-type dylib` on Linux now links `-lm -lpthread -ldl`, like the executable link always has** (#8942). The `cc -shared` plugin link in `run_pipeline.rs` never carried the system libraries: a plugin resolves every `perry_*`/`js_*` symbol from the host at `dlopen` time, so the omission was invisible until a real-app ELF dylib link — the same Next.js route bundle, right after the string-constant collisions were fixed — failed with `undefined reference to 'floor'` / `'log10'` from `perry_closure_*` functions, where LLVM had lowered `llvm.floor`/`llvm.log10` to libm calls. New `link/linux_dylib_libs.rs::push_unix_dylib_output` appends the libraries AFTER the objects (GNU ld only resolves what precedes a `-l`, and Ubuntu's default `--as-needed` drops an earlier one) and then `-o`; unit tests pin the order and that macOS (`-lSystem` already carries libm) gets no extra flags. `crates/perry/src/commands/compile/run_pipeline.rs`, `crates/perry/src/commands/compile/link/linux_dylib_libs.rs`, `link/mod.rs`. | ||
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Hyphenate the compound modifier.
Change
end to endtoend-to-endbeforesplit output.🧰 Tools
🪛 LanguageTool
[grammar] ~3-~3: Use a hyphen to join words.
Context: ...ep the bare names. Verified on macOS end to end (split output byte-identical to Node...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Source: Linters/SAST tools