Add direct symbol references back - #764
Merged
Merged
Conversation
filmor
force-pushed
the
direct-symbol-references
branch
6 times, most recently
from
September 2, 2026 09:39
fa07f82 to
cd697f6
Compare
filmor
force-pushed
the
direct-symbol-references
branch
from
September 3, 2026 10:16
cd697f6 to
207b958
Compare
We don't actually require it anywhere else (though it works on Linux just as well). Also changing the `SysIOVec` type to the (transparent) safe Rust representation `IoSliceMut` allows us to drop the `libc` dependency on other systems.
There was a problem hiding this comment.
🔵 Needs a closer look
SysIOVec is currently changed to a Rust std type that is not FFI/ABI-compatible with the NIF C SysIOVec layout, breaking #[repr(C)] structs and NIF functions that use it.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
rustler/src/sys/functions.rs:5
- The comment says
DYN_NIF_CALLBACKSis "defined in the generatedapi.*.rs", but the generated files referenceDYN_NIF_CALLBACKSand the actualstatic mut DYN_NIF_CALLBACKSis defined here insys/functions.rs. This mismatch makes the safety/initialization story harder to follow.
rustler/src/sys/nif_filler.rs:10 NoopNifFilleris compiled on all targets but is only used on non-macOS builds (sincenew()is#[cfg(not(target_os = "macos"))]). On macOS this will be unused and can triggerdead_codewarnings; considercfg-gating the type/impl to the same condition asnew().
rustler/src/sys/types.rs:352
SysIOVecis part of the NIF C ABI (used insideErlNifIOQueue/ErlNifIOVecand returned fromenif_ioq_peek). Replacing it withstd::io::IoSliceMut<'static>makes these#[repr(C)]structs no longer FFI-safe becauseIoSliceMuthas no C layout guarantee and does not match the platformiovec/WSABUFlayout expected by the VM.
- Files reviewed: 34/34 changed files
- Comments generated: 0 new
- Review effort level: Lite
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This undoes parts of #648. We keep dynamic symbol lookup on Windows (because that is just how it works on this platform) and macOS (because this allows our users to skip messing with linker flags).
The rationale is a forked/vendored copy of rustler that I found, where the dev ran into
dlopen(NULL)not being implemented at all in Musl static builds. That means we have to have the "static" linking in place if we want to support that platform.My initial idea of making the
.socallable didn't quite work out, and with the new symbol generation setup, we have a relatively easy way to makecargo-rustler(if we ever implement it fully) expose the necessary symbols to the .so at load time.