Add plugin ABI shims & Windows link/export fixes#5480
Conversation
Emit a plugin ABI shim when building a dylib and ensure host runtime symbols and CRT are exported/resolved on Windows. - crates/perry-codegen: move detection of exported `activate`/`deactivate` earlier and emit a plugin ABI shim for dylib entry modules. The shim provides `perry_plugin_abi_version` and `plugin_activate`/`plugin_deactivate` wrappers that unbox the NaN-boxed API handle and call the user-provided functions (or return failure if absent). - crates/perry/src/commands/compile.rs: when linking MSVC DLLs, add `/defaultlib:libcmt` so the static C runtime is pulled in and CRT symbols referenced by the auto-generated DllMain are resolved (prevents ERROR_DLL_INIT_FAILED at LoadLibrary time). - crates/perry/src/commands/compile/link/build_and_run.rs: when generating the host .def file for the plugin host, run `llvm-nm` over the runtime and stdlib .lib archives to enumerate all `js_*`, `perry_*`, and `PERRY_*` symbols and write them to the .def. This ensures the plugin host exports the full symbol surface that plugins will resolve via GetProcAddress/dlsym (avoids many unresolved symbols causing DLL init failures). - crates/perry/src/commands/compile/link/mod.rs: update the PLUGIN_HOST_SYMBOLS list to reflect current host API names (rename/replace several entries, add new convenience names, and explicitly include `PERRY_CLASS_FIELD_INLINE_GUARD_DISABLED` so it's always exported even if not picked up by nm heuristics). Overall: these changes fix plugin load/link failures on Windows by exporting required runtime symbols and ensuring the plugin ABI entrypoints exist and match the host's expectations.
📝 WalkthroughWalkthroughThe PR fixes plugin dylib support across codegen and linking. In codegen, ChangesPlugin ABI Shim, Symbol Exports, and Windows Dylib Linking
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/perry/src/commands/compile/link/build_and_run.rs (3)
484-518: 💤 Low valueInconsistent indentation inside the success block.
Line 486 (
for line in text.lines()) appears to be outdented relative to line 485, making it visually ambiguous whether the loop is inside or outside theif out.status.success()block. Rust's braces govern scope so this compiles correctly, but the visual inconsistency hurts readability during review.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry/src/commands/compile/link/build_and_run.rs` around lines 484 - 518, The for loop starting with `for line in text.lines()` is not properly indented relative to its parent `if out.status.success()` block. Increase the indentation of the entire for loop and all its contents (including the comments and the code inside the loop body) by one indentation level so they align consistently as a child block of the if statement. This will make the scope relationship clear and improve code readability.
510-522: 💤 Low valuePotential duplicate symbols in the .def file.
PLUGIN_HOST_SYMBOLSis written first (lines 455-456), then all enumeratedperry_*symbols are written again (lines 521-522). SincePLUGIN_HOST_SYMBOLScontainsperry_plugin_*names that match theperry_*filter, duplicates will appear in the .def file. Whilelink.exetolerates duplicate EXPORTS entries, this also causes the log count at line 526 to overstate the unique symbol count.Consider filtering
all_symsagainstPLUGIN_HOST_SYMBOLSbefore writing, or using a single unified set from the start.One approach: deduplicate before writing
+ let base_set: std::collections::HashSet<&str> = + PLUGIN_HOST_SYMBOLS.iter().copied().collect(); for s in &all_syms { + if base_set.contains(s.as_str()) { + continue; + } let _ = writeln!(def_file, " {}", s); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry/src/commands/compile/link/build_and_run.rs` around lines 510 - 522, The issue is that symbols from PLUGIN_HOST_SYMBOLS (which contain perry_plugin_* entries) are written to the def_file first, then all enumerated perry_* symbols from all_syms are written again, causing duplicates because all_syms also collects symbols matching the perry_* pattern. Before writing all_syms to the def_file in the loop starting at line 521, filter out any symbols that are already in PLUGIN_HOST_SYMBOLS to eliminate duplicates and ensure the subsequent log count accurately reflects the actual unique symbol count.
472-529: ⚡ Quick winConsider warning when
llvm-nmis unavailable and the fallback is used.When
find_llvm_tool("llvm-nm")returnsNone, the code silently falls back to exporting onlyPLUGIN_HOST_SYMBOLS(~50 entries) instead of the full ~2900 symbol surface. Plugins will then fail to load withERROR_DLL_INIT_FAILEDdue to unresolvedjs_gc_*,js_array_*, etc., and users will have no indication that the reduced export set is the root cause.Adding an
eprintln!warning (or downgrading the summary log) when the enumeration is skipped would help users diagnose this failure mode.Suggested diagnostic
eprintln!( "[perry] plugin-host .def: {} symbols ({} base + enumerated)", all_syms.len() + PLUGIN_HOST_SYMBOLS.len(), PLUGIN_HOST_SYMBOLS.len() ); + } else { + eprintln!( + "[perry] plugin-host .def: llvm-nm not found; using {} base symbols only \ + (plugins may fail to load if they reference js_*/perry_* FFI symbols)", + PLUGIN_HOST_SYMBOLS.len() + ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry/src/commands/compile/link/build_and_run.rs` around lines 472 - 529, The code silently falls back to exporting only PLUGIN_HOST_SYMBOLS when find_llvm_tool("llvm-nm") returns None, which can cause plugin loading failures with no diagnostic message. Add an eprintln! warning outside the if let Some(nm_path) block (or as a complementary else/fallback case) to alert users when llvm-nm is unavailable and the symbol enumeration is being skipped, ensuring the warning message clearly indicates that only the base symbols are being exported instead of the full ~2900 symbol surface.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/perry/src/commands/compile/link/build_and_run.rs`:
- Around line 484-518: The for loop starting with `for line in text.lines()` is
not properly indented relative to its parent `if out.status.success()` block.
Increase the indentation of the entire for loop and all its contents (including
the comments and the code inside the loop body) by one indentation level so they
align consistently as a child block of the if statement. This will make the
scope relationship clear and improve code readability.
- Around line 510-522: The issue is that symbols from PLUGIN_HOST_SYMBOLS (which
contain perry_plugin_* entries) are written to the def_file first, then all
enumerated perry_* symbols from all_syms are written again, causing duplicates
because all_syms also collects symbols matching the perry_* pattern. Before
writing all_syms to the def_file in the loop starting at line 521, filter out
any symbols that are already in PLUGIN_HOST_SYMBOLS to eliminate duplicates and
ensure the subsequent log count accurately reflects the actual unique symbol
count.
- Around line 472-529: The code silently falls back to exporting only
PLUGIN_HOST_SYMBOLS when find_llvm_tool("llvm-nm") returns None, which can cause
plugin loading failures with no diagnostic message. Add an eprintln! warning
outside the if let Some(nm_path) block (or as a complementary else/fallback
case) to alert users when llvm-nm is unavailable and the symbol enumeration is
being skipped, ensuring the warning message clearly indicates that only the base
symbols are being exported instead of the full ~2900 symbol surface.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5f252ee0-b9cb-4c09-b207-9a89653bfb88
📒 Files selected for processing (4)
crates/perry-codegen/src/codegen/entry.rscrates/perry/src/commands/compile.rscrates/perry/src/commands/compile/link/build_and_run.rscrates/perry/src/commands/compile/link/mod.rs
1 participant
Summary
Fix three Windows-plugin regressions introduced in #5409 so a
--output-type dylibplugin actually loads and itsplugin_activateC entry point is reachable from the host atLoadLibraryWtime.Changes
crates/perry-codegen/src/codegen/entry.rs— moved theperry_plugin_abi_version/plugin_activate/plugin_deactivateABI shim out of the non-entry} else {branch ofcompile_module_entry(where it was dead code and the symbols never reached the .o file) and into the entry-module branch, where it actually fires. Also added the missing%prefix on theapi_handleparameter name so the emitted LLVM IR is well-formed (define i64 @plugin_activate(i64 %api_handle)instead of(i64 api_handle)).crates/perry/src/commands/compile/link/mod.rs—PLUGIN_HOST_SYMBOLShad 7 wrong names that don't exist incrates/perry-runtime/src/plugin.rs(perry_plugin_abi_version,perry_plugin_emit_event_bus,perry_plugin_lookup_symbol,perry_plugin_plugin_count,perry_plugin_subscribe_event,perry_plugin_unsubscribe_event,perry_plugin_last_load_error). Replaced with the real exported names (perry_plugin_count,perry_plugin_emit_event,perry_plugin_on,perry_plugin_off,perry_plugin_log,perry_plugin_set_config,perry_plugin_discover,perry_plugin_emit, plus missingperry_plugin_set_config/perry_plugin_discover/perry_plugin_init) and addedPERRY_CLASS_FIELD_INLINE_GUARD_DISABLED(a#[no_mangle] pub staticreferenced from compiled plugin code).crates/perry/src/commands/compile.rs— added/defaultlib:libcmtto the dyliblld-linkcommand so the MSVC CRT's auto-emittedDllMain+_fltusedresolve inside the plugin DLL. Without this,LoadLibraryWreturnsERROR_DLL_INIT_FAILED(Win32 error 1114) even when everyjs_*/perry_*symbol is otherwise present in the host.crates/perry/src/commands/compile/link/build_and_run.rs— when building a Windows plugin host, enumerate the auto-built runtime + stdlib .lib archives withllvm-nm(via the existingfind_llvm_tooldiscovery:PERRY_LLVM_NMenv → rust sysroot →where/which) and emit everyjs_*/perry_*/PERRY_*symbol into the host's/DEF:response file. The staticPLUGIN_HOST_SYMBOLSlist only covered ~50 entries; the host actually needs to export ~2900 to make everyGetProcAddressfrom the plugin DLL resolve. Falls back to the static list only whenllvm-nmisn't on PATH.Related issue
Refs #5409 (the original Windows plugin support commit that shipped with the regressions above).
Test plan
Confirmed on this branch:
plugin_activate/plugin_deactivate/perry_plugin_abi_versioneach get a real RVA (0x1E70/0x2000/0x1E80) — pre-fix all three were the forwarder-string sentinel0x80000000, which is what caused the segfault onGetProcAddress-then-call.Host .def now contains ~2900
js_*/perry_*exports (was ~50), and the host's host.exe grows from 9.3 MB → 13.8 MB accordingly.Host's
loadPluginreaches theplugin_activatecall (confirmed via temporaryeprintln!tracing inperry_plugin_load— removed before commit). Beyond that point the plugin's auto-emittedperry_module_initstill segfaults due to a Windows-specific TLS-init-order issue (the DLL's TLS block isn't initialized on theLoadLibraryWthread before the auto-emitted constructor runsjs_gc_init/js_shadow_frame_push). That's a separate, larger problem and is out of scope for this PR — documented inexamples/plugin-demo/README.md.cargo build --releasecleancargo test --workspace --exclude perry-ui-ios --exclude perry-ui-tvos --exclude perry-ui-watchos --exclude perry-ui-gtk4 --exclude perry-ui-android --exclude perry-ui-windowspasses — not run in this session; should pass since the changes are codegen + link-only and don't touch runtime semantics(if user-facing) Added
examples/plugin-demo/{greeter,host,host_test,greeter_min,greeter_empty}.ts+build.bat/run.bat/README.mdas a working Windows host + plugin example (the existingtests/test_plugin_lifecycle.shalready covers the macOS/Linux path)(if CLI / stdlib / runtime API changed) Updated
docs/src/— no public API change, so no doc update needed(if touching a platform UI backend) Built
-p perry-ui-windowsindirectly via the host link on Windows (link.exeis invoked bybuild_and_run.rswith the new/DEF://defaultlib:libcmtflags)cargo fmtandcargo clippyclean for the four touched files (only the same pre-existingunused import: lower_pattern_binding_intowarning that already exists onmain)Screenshots / output
Before (perry.exe built before #5409's shim was reachable, on a stale-cache build of the plugin):
After (this PR, with the shim correctly placed in the entry branch +
llvm-nmenumeration of the runtime/stdlib exports +/defaultlib:libcmtin the dylib link):Host link log (new
[perry]line) confirms the enumeration happened:Checklist
feat:/fix:/docs:/chore:prefix convention used in the log