runtime: C-ABI FFI stage 1 — dlopen, typed calls, ptr/CString on pinned buffers (#6562)#6580
Conversation
…ed buffers (PerryTS#6562) bun:ffi-shaped module: dlopen(path, table) -> { symbols, close() } with typed per-symbol call stubs, FFIType (Bun-exact numeric values + aliases), ptr(view[, off]) on non-moving buffer storage, CString, suffix. i64/u64 returns are always BigInt; i64_fast/u64_fast number-within-safe-range; pointers are JS numbers. Stage>=2 exports (toArrayBuffer, JSCallback, CFunction, linkSymbols, viewSource, read, toBuffer, FFIType.function) are declared and throw descriptive stage-1 errors. Call stubs are register-image trampolines (no libffi, no link-line changes): scalar args pack densely per register class through one maximal extern "C" signature; correct on SysV x86-64 + AAPCS64 for <=8 int-class + <=8 float-class args, enforced at dlopen time. f32 args travel as bit images; narrow returns truncate to declared width. Pointer lifetime contract: buffer bytes never move (old-arena, TENURED, movable:false, no growth path), so ptr() is stable for the object's lifetime; views resolve through the view registry to the backing bytes; close() poisons symbols instead of leaving a dangling handle. Documented in bun_ffi/mod.rs. Wiring uses the existing native-module machinery: NATIVE_MODULES/ RUNTIME_ONLY_MODULES + manifest (docs regenerated), NmBucket::BunFfi + js_nm_install_bun_ffi (declared in runtime_decls), callable-export check/arity, constants path for FFIType/suffix (cached object is a registered GC root), module keys. Tests: e2e tier 1 (purpose-built C dylib, every FFIType incl. BigInt boundaries, mixed-register ABI, pinned-buffer round trips both ways), tier 1b (error surfaces), tier 2 (real bun-pty 0.4.10 dylib: spawn sh through the pty, echo round-trip, resize, kill; skips offline) + 9 trampoline/marshalling unit tests in perry-runtime.
📝 WalkthroughWalkthroughAdds stage-1 ChangesBun FFI support
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
# Conflicts: # crates/perry-codegen/src/nm_install.rs # crates/perry-codegen/src/runtime_decls/objects.rs # crates/perry-runtime/src/lib.rs # crates/perry-runtime/src/object/native_module_dispatch.rs # crates/perry-runtime/src/object/native_module_dispatch/dispatch_a_c.rs # crates/perry-runtime/src/object/native_module_registry.rs # docs/api/perry.d.ts # docs/src/api/reference.md
proggeramlug
added
the
ready
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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.
Inline comments:
In `@crates/perry-runtime/src/bun_ffi/call.rs`:
- Around line 83-105: Replace the transmuted fixed-argument function-pointer
calls in call_int, call_f64, and call_f32 with a real trampoline or
exact-signature shim matching each callee’s prototype. Preserve the existing
integer and floating-point argument forwarding and return types, while ensuring
every invocation uses an ABI-compatible function signature.
In `@crates/perry-runtime/src/bun_ffi/dlopen.rs`:
- Around line 413-425: Make the symbol-loading flow around open_library
transactional: validate all inputs before opening the library, resolve symbols
into temporary records and names without mutating LIBS or SYMS, and close the
handle on every post-open validation or resolution failure. Only after all
symbols resolve successfully should the implementation append the LibRecord and
commit the accumulated SYMS entries; preserve existing error propagation and
successful lookup behavior.
- Around line 462-473: Update the args validation in the surrounding FFI
symbol-processing function before the ArrayHeader cast: validate that args_value
has the concrete array value tag, not merely a nonzero pointer. For non-array
values, throw the existing ERR_INVALID_ARG_TYPE error; only then call
js_nanbox_get_pointer and use js_array_length/js_array_get, preserving the
existing undefined/null handling and NaN-boxing validation.
- Around line 605-656: Update cstring_value so the value_buffer_span branch
preserves the source buffer length instead of discarding it, then bound both
explicit-length reads and call::read_cstring_value NUL scans to that span. Keep
numeric-pointer handling unchanged, and ensure Buffer inputs cannot read beyond
managed storage.
In `@crates/perry-runtime/src/bun_ffi/types.rs`:
- Around line 169-196: Replace the global FFI_TYPE_OBJECT_CACHE with the
existing runtime-thread-local cache mechanism used by gc_init, and update
ffi_type_object_value() to load and store the cached object through that TLS
slot. Remove or adapt scan_ffi_type_cache_mut so each thread’s scanner visits
its own cached pointer, ensuring no heap object or initialization state is
shared across runtime threads.
In `@crates/perry/tests/bun_ffi_stage1.rs`:
- Around line 280-281: Duplicate the critical ABI, lifecycle, and error-contract
assertions from tier1_every_ffi_type_against_test_dylib and the referenced tests
into cargo-test-visible unit tests outside crates/perry/tests, while retaining
the existing end-to-end integration suite. Ensure the new unit-test coverage
exercises the same FFI behavior and runs in the repository’s normal every-PR
cargo test target.
- Around line 509-523: Update the PTY roundtrip polling logic around the marker
checks so terminal input echo alone cannot satisfy completion or the final
assertion. Require a distinct shell-output token, or track marker occurrences
and require at least a second occurrence, then use that same criterion for the
loop break and roundtrip validation.
- Line 95: Update the C fixture function ffi_i32_add1 so adding one to INT32_MAX
cannot invoke signed overflow while preserving the expected wraparound result.
Use unsigned arithmetic or explicitly guard the maximum value within this
function.
In `@docs/api/perry.d.ts`:
- Around line 280-298: Mark the stage-2/3 exports in the public declarations
around CFunction, CString, JSCallback, dlopen, linkSymbols, ptr, read,
toArrayBuffer, toBuffer, and viewSource with JSDoc indicating they are
unsupported in stage 1 and throw ERR_NOT_IMPLEMENTED. In
docs/src/api/reference.md lines 347-356, label the corresponding methods as
unsupported in stage 1; no other API entries need changes.
In `@docs/src/api/reference.md`:
- Line 21: Update the bun:ffi index link in the API reference contents to target
the generated heading fragment `#bunffi` instead of `#bun-ffi`, preserving the
existing link text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: faf4cd34-22e5-422b-95ed-2f6efd8ec63c
📒 Files selected for processing (20)
crates/perry-api-manifest/src/entries.rscrates/perry-api-manifest/src/entries/part_1.rscrates/perry-codegen/src/nm_install.rscrates/perry-codegen/src/runtime_decls/objects.rscrates/perry-runtime/src/bun_ffi/call.rscrates/perry-runtime/src/bun_ffi/dlopen.rscrates/perry-runtime/src/bun_ffi/mod.rscrates/perry-runtime/src/bun_ffi/types.rscrates/perry-runtime/src/gc/mod.rscrates/perry-runtime/src/lib.rscrates/perry-runtime/src/object/native_module/callable_export_check.rscrates/perry-runtime/src/object/native_module/callable_exports.rscrates/perry-runtime/src/object/native_module/constants.rscrates/perry-runtime/src/object/native_module/module_keys.rscrates/perry-runtime/src/object/native_module_dispatch.rscrates/perry-runtime/src/object/native_module_dispatch/dispatch_a_c.rscrates/perry-runtime/src/object/native_module_registry.rscrates/perry/tests/bun_ffi_stage1.rsdocs/api/perry.d.tsdocs/src/api/reference.md
| #[test] | ||
| fn tier1_every_ffi_type_against_test_dylib() { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Keep critical FFI acceptance coverage in an every-PR test target.
These tests live under crates/perry/tests, so the repository’s normal PR coverage may miss regressions across the entire new API layer. Retain the end-to-end suite, but duplicate the critical ABI, lifecycle, and error-contract checks in cargo-test-visible unit tests.
As per coding guidelines, “Prefer acceptance coverage in cargo-test-visible unit tests because integration suites under crates/*/tests/*.rs do not run on every PR.” <coding_guidelines>
Also applies to: 390-391, 535-536
🤖 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/tests/bun_ffi_stage1.rs` around lines 280 - 281, Duplicate the
critical ABI, lifecycle, and error-contract assertions from
tier1_every_ffi_type_against_test_dylib and the referenced tests into
cargo-test-visible unit tests outside crates/perry/tests, while retaining the
existing end-to-end integration suite. Ensure the new unit-test coverage
exercises the same FFI behavior and runs in the repository’s normal every-PR
cargo test target.
Source: Coding guidelines
Findings from the PR review, each fixed: 1. call.rs — replace the fixed 16-arg over-call trampoline (UB per Rust's ABI model, and it wrote surplus x86-64 stack slots into the callee's frame) with EXACT-ARITY shims: dispatch on the marshalled (n_int, n_float) and transmute to a signature with precisely that many usize + f64 params (9x9 macro-generated shims per return class). The zero-deps register-image design is unchanged; libffi noted as the eventual fully-blessed alternative. Wide-register int passing / f32 bit-image / narrow-return truncation documented as residual assumptions. 2. dlopen.rs — make symbol preparation transactional: validate + resolve the whole table into locals first (prepare_symbols, never throws); on any error dlclose the fresh handle and throw at one site, so repeated malformed dlopen calls can't grow LIBS/SYMS/leaked-name state. 3. dlopen.rs — verify the `args` value is genuinely an Array (js_array_is_array) before reading it as an ArrayHeader. 4. dlopen.rs — bound CString reads: keep the source buffer's span and clamp both the explicit-length slice and the NUL scan to its managed storage (raw numeric pointers stay caller-trusted, as in Bun). 5. types.rs — FFIType cache is now thread-local with per-thread rooting (perry's arena/GC is per-thread) instead of a process-global atomic. 6. tests — C fixture uses unsigned internal arithmetic (no signed-overflow UB at INT32_MAX + 1); wraparound expectation preserved. 7. tests — pty round-trip asserts on a shell-EVALUATED marker (echo FFI_$((40+2))_OK -> FFI_42_OK) the terminal echo can't contain, so input echo can't satisfy the check before the shell runs. 8. tests — duplicate the ABI, error-contract, and type-parse checks as cargo-visible perry-runtime unit tests (9 -> 22) so every-PR CI covers them without the compiled-binary e2e target. 9. docs — stage>=2 exports (toArrayBuffer/JSCallback/CFunction/linkSymbols/ viewSource/read/toBuffer) marked `.stub_note` at the manifest source so the generated .d.ts/reference.md say "not yet implemented, throws"; stub_inventory drift-guard updated (+PerryTS#6562 cluster, +bun:ffi allowlist). 10. docs — anchor() now matches mdbook's normalize_id (drops punctuation instead of turning it into `-`), so the `bun:ffi` TOC link targets `#bunffi` (also fixes slashed/util.types module anchors). Regenerated. Also merged origin/main (bun globals PerryTS#6560, node-pty PerryTS#6563, wasm PerryTS#6579): union-resolved the additive conflicts (bun + bun:ffi + node-pty buckets, NM_BUCKET_COUNT 40). Tests: perry-runtime bun_ffi 22/22; api-manifest incl. stub_inventory 4/4; e2e bun_ffi_stage1 3/3 (tier 1 exact-arity ABI, tier 1b errors, tier 2 real bun-pty shell round-trip); fmt clean; api docs regenerated. # Conflicts: # crates/perry-runtime/src/object/native_module_dispatch.rs # crates/perry-runtime/src/object/native_module_registry.rs # docs/api/perry.d.ts # docs/src/api/reference.md
|
Addressed all 10 CodeRabbit findings in
Validation (macOS arm64): |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/perry-api-manifest/tests/stub_inventory.rs (1)
96-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMirror these
bun:ffiassertions in an always-run unit test.This integration suite does not run on every PR, so regressions in the new count and allowlist can escape normal CI. Keep this inventory test, but add equivalent unit coverage near the manifest entries.
As per coding guidelines, acceptance coverage should prefer cargo-test-visible unit tests because
crates/*/tests/*.rssuites do not run on every PR.Also applies to: 127-128
🤖 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-api-manifest/tests/stub_inventory.rs` around lines 96 - 100, add an always-run unit test near the bun:ffi manifest entries that asserts the same seven-symbol count and allowlist represented by the “#6562” inventory entry. Keep the existing stub_inventory integration coverage unchanged, and ensure the unit test validates both the declared count and the specific symbols to prevent regressions.Source: Coding guidelines
🤖 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.
Inline comments:
In `@crates/perry-runtime/src/bun_ffi/call.rs`:
- Around line 598-624: Gate every test invoking raw::call_* with the existing
production target predicate, applying it consistently from exact_arity_zero_args
through the register_image_reaches_eight_int_args and
register_image_reaches_eight_float_args tests and the additional affected tests.
Keep the test bodies unchanged; only ensure unsupported targets skip these
native-call tests instead of executing unreachable! stubs.
---
Nitpick comments:
In `@crates/perry-api-manifest/tests/stub_inventory.rs`:
- Around line 96-100: add an always-run unit test near the bun:ffi manifest
entries that asserts the same seven-symbol count and allowlist represented by
the “#6562” inventory entry. Keep the existing stub_inventory integration
coverage unchanged, and ensure the unit test validates both the declared count
and the specific symbols to prevent regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 415cdba2-5708-4222-9da1-7f7974982030
📒 Files selected for processing (9)
crates/perry-api-manifest/src/emit.rscrates/perry-api-manifest/src/entries/part_1.rscrates/perry-api-manifest/tests/stub_inventory.rscrates/perry-runtime/src/bun_ffi/call.rscrates/perry-runtime/src/bun_ffi/dlopen.rscrates/perry-runtime/src/bun_ffi/types.rscrates/perry/tests/bun_ffi_stage1.rsdocs/api/perry.d.tsdocs/src/api/reference.md
🚧 Files skipped from review as they are similar to previous changes (5)
- crates/perry-api-manifest/src/entries/part_1.rs
- docs/api/perry.d.ts
- docs/src/api/reference.md
- crates/perry-runtime/src/bun_ffi/dlopen.rs
- crates/perry/tests/bun_ffi_stage1.rs
| #[test] | ||
| fn register_image_reaches_eight_int_args() { | ||
| let image = image_from(&[1, 2, 3, 4, 5, 6, 7, 8], &[]); | ||
| let r = unsafe { | ||
| raw::call_int( | ||
| sum8_i32 as usize, | ||
| image.n_int, | ||
| &image.ints, | ||
| image.n_float, | ||
| &image.floats, | ||
| ) | ||
| }; | ||
| assert_eq!(r as i64, 36); | ||
| } | ||
|
|
||
| #[test] | ||
| fn register_image_reaches_eight_float_args() { | ||
| let image = image_from(&[], &[0.5, 1.5, 2.5, 3.5, 4.5, 5.5, 6.5, 7.5]); | ||
| let r = unsafe { | ||
| raw::call_f64( | ||
| dsum8 as usize, | ||
| image.n_int, | ||
| &image.ints, | ||
| image.n_float, | ||
| &image.floats, | ||
| ) | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Gate native-call tests to supported targets.
On unsupported targets, these tests call the raw stubs that always execute unreachable!, making cargo test fail even though runtime FFI is intentionally unavailable. Apply the production target predicate to every test invoking raw::call_*.
Proposed direction
+#[cfg(all(unix, any(target_arch = "x86_64", target_arch = "aarch64")))]
#[test]
fn register_image_reaches_eight_int_args() {Apply the same attribute through exact_arity_zero_args.
Also applies to: 634-741
🤖 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-runtime/src/bun_ffi/call.rs` around lines 598 - 624, Gate every
test invoking raw::call_* with the existing production target predicate,
applying it consistently from exact_arity_zero_args through the
register_image_reaches_eight_int_args and
register_image_reaches_eight_float_args tests and the additional affected tests.
Keep the test bodies unchanged; only ensure unsupported targets skip these
native-call tests instead of executing unreachable! stubs.
1 participant
Stage 1 of #6562: a
bun:ffi-shaped C-ABI FFI layer, proven end-to-end against bun-pty's prebuiltlibrust_ptydylib (spawn a real shell, write/read round-trip through the pty, resize, kill).Surface (stage 1)
dlopen(path, symbolTable)→{ symbols, close() }, entries{ args: FFIType[], returns: FFIType }(string aliases accepted,returnsdefaults tovoid,argsto[]— Bun-compatible).FFIType— numeric values and key set mirror Bun's runtime object exactly (char=0 … function=17, incl. the numeric self-keys"0"–"17"and aliasesint32_t/c_int/pointer/usize/…).ptr(view[, byteOffset])→ number address of a Buffer / TypedArray / ArrayBuffer / DataView's bytes.CString(ptr[, byteOffset[, byteLength]])→ decoded string (stage-1 divergence: primitive string, not aStringsubclass with.ptr; NULL →null).suffix("dylib"/"so"/"dll").i64_fast/u64_fastreturn number within safe-integer range, BigInt beyond — per Bun's documented mapping. Pointers are plain JS numbers (null →null).toArrayBuffer,toBuffer,JSCallback,CFunction,linkSymbols,viewSource,read, andFFIType.function— each throws a descriptiveERR_NOT_IMPLEMENTED/ dlopen-timeTypeErrorinstead ofundefined is not a function, so feature probes fail actionably. The dispatch, type-slot, and signature-record plumbing is shaped so stages 2/3 only lift rejections.The pointer-lifetime / pinning contract (the part that must not be wrong)
perry's GC relocates objects, but buffer byte storage never moves: Buffer / TypedArray / ArrayBuffer / DataView allocations go straight to the non-moving old arena, born TENURED,
movable: false, bytes inline after the header (made unconditional in the 2026-07-09 audit precisely so raw pointers can be handed to native code), and there is no in-place growth path for buffers (arrays grow via forwarding stubs — #6228 — buffers never do). Therefore:ptr(view)is stable for the lifetime of the JS object; it is invalidated by the object becoming unreachable (old-arena blocks are recycled) or by detach (transfer). The caller must keep the buffer referenced while native code holds the pointer — the same contract Bun documents.ptr()does not root.subarray,new Uint8Array(ab, off, len)) resolve throughbuffer::view::resolve_data_ptrto the ultimate backing bytes (buffer+len native param: Uint8Array view over an ArrayBuffer resolves to the wrong data pointer (silent corruption; standalone arrays fine) #6515), so native code always sees the true data; a native write through a view pointer is not propagated into the view's local copy (pre-existing Improve node:buffer slice/subarray shared backing-store parity #1205 registry design), so pass base buffers to native code that writes — which is what real consumers (bun-pty, opentui) do. Documented inbun_ffi/mod.rs.cstringargs) are Rust-owned.close()dlcloses and poisons the library's symbols — later calls throwERR_INVALID_STATEinstead of jumping through a dangling handle (stricter than Bun, which leaves this UB).Call stubs: register-image trampoline, not libffi
All stage-1 FFITypes are scalars, so instead of adding libffi to every platform link line, calls go through one maximal
extern "C" fn(usize×8, f64×8) -> {u64|f64|f32}signature with arguments packed densely per register class. On both supported ABIs (SysV x86-64, AAPCS64 incl. Apple arm64) integer-class and float-class args are assigned to their register files independently and in order, so the callee's narrower scalar prototype reads exactly the registers (and, on x86-64, the two stack slots for int args 7–8) that were populated. f32 args are passed as their bit-image in the low 32 bits of a vector register; narrow int/bool returns are truncated to declared width. Limits enforced atdlopentime (never at call time): ≤ 8 integer-class + ≤ 8 float-class args, ≤ 16 total, no variadics, unix x86_64/aarch64 only — anything else throws a descriptive error. Rationale + ABI analysis inbun_ffi/call.rs; the register-image claims are pinned by Rust-level unit tests calling real narrowedextern "C"callees (mixed int/float order, 8-int, 8-double, f32 bit-image, u64 full-width, narrow-return truncation).Wiring
Follows the existing native-module machinery end to end (no new registries):
NATIVE_MODULES/RUNTIME_ONLY_MODULES+ manifest entries (api-docs regenerated),NmBucket::BunFfidispatch bucket +js_nm_install_bun_ffi(declared inruntime_decls/objects.rsso dynamic-import emission links), callable-export check/arity for bound value reads,FFIType/suffixvia the constants path (cachedFFITypeobject is a registered mutable GC root), namespaceObject.keyslist. Symbol call stubs are runtime closures (one capture = symbol-registry index) over shared per-arity thunks, dispatching through the standard closure-call ABI.Tests
crates/perry/tests/bun_ffi_stage1.rs(e2e, compiled binaries):cc) exercising every FFIType: all int widths/signs incl. wrap-at-width, i64/u64 BigInt boundary values (INT64_MIN/INT64_MAX/UINT64_MAX), f32/f64, bool, mixed int/float register assignment, 8-int/8-double register limits,ptrround-trips in both directions (JS→native reads, native→JS writes into a pinned buffer), cstring both directions + UTF-8 + NULL,CString, use-after-close.FFIType.functiondlopen rejection,JSCallbackstage-1 throw, string-as-pointer rejection (Bun's exact hint).terminal.ts: spawnshthrough the pty, echo round-trip, resize, kill, close. Resolves the dylib viaBUN_PTY_LIBornpm pack bun-pty@0.4.10; skips with a note when unavailable (offline CI).bun_ffi::callunit tests for the trampoline/marshalling (run viacargo test -p perry-runtime bun_ffi -- --test-threads=1).Test evidence (macOS arm64, re-run after rebasing onto main 2230ec5):
cargo test -p perry --test bun_ffi_stage1 -- --test-threads=1→ 3/3 ok (tier 2 fetched bun-pty 0.4.10 vianpm packand drove the real dylib: spawn/round-trip/resize/kill);cargo test -p perry-runtime bun_ffi -- --test-threads=1→ 9/9 ok;cargo test -p perry-runtime native_module -- --test-threads=1→ 3/3 ok (no regressions);cargo fmt --all -- --checkclean; api docs regenerated for the drift gate.Not in this PR (explicitly)
toArrayBufferexternal backing stores (stage 2) andJSCallbacknative→JS trampolines (stage 3) — see the bring-up plan in runtime: C-ABI FFI layer (bun:ffi shape) — dlopen, typed calls, pinned buffers, toArrayBuffer, JSCallback #6562.require("bun:ffi")default-export shape (ESM named imports are the shape bun-pty/opentui actually use).Closes nothing; part of #6562 (stage 1 of 4).
Summary by CodeRabbit
bun:ffimodule, including typed native calls,dlopen/symbol access, pointer helpers,CString, andFFITypeplus platform library suffix.bun:ffi(including anchor consistency).bun-ptylibrary.